Andrea Arcangeli (arcangeli@mbox.queen.it)
Thu, 16 Apr 1998 02:05:34 +0200 (CEST)
On Wed, 15 Apr 1998, Tim Waugh wrote:
>The first (from Phil) fixes parport_pc module parameters, and enhances lp
>module parameters so that you can force old behaviour with
>"parport=none,0" (instead of "-1,0").
I have not looked at that patch since sure will work fine ;-).
>The second makes parport_yield_blocking useful. Having recently learned
>that I only have a hazy grasp of the implications of calling "schedule()",
>would someone like to give me a clue about whether it should be done in
>parport_yield (the non-blocking one) as well?
Tim, the second patch is buggy.
The bug is that you exchanged lp_schedule() with lp_yield(). lp_schedule()
is called so since it _must_ schedule() in _some_ way inside it.
lp_yield() can schedule() only if need_resched is 1.
As second it' s not smart to schedule() if _then_ we will have to
sleep_on() in parport_claim_or_block().
Note also that the only object of parport_yiled_blocking() (at least this
is my point of view) is to give a change to other pardevice to own
parport, only to avoid parport starvation to other pardevice. It shouldn'
t automatically means that must schedule() other process to run, for
example if there aren' t other pardevices running we can safe return
without schedule().
I' d like to leave separate the need to schedule() other process and don'
t allow parport starvation.
lp_yiled() instead take care of all kind of starvation, of other
process and other waiter pardevices but don' t schedule() for sure.
lp_schedule() instead will sure schedule. It' s also a bit smart and
release parport before schedule() if there are other waiters and lp owned
parport for more that timeslice. Note that the process timeout before
lp_schedule() mustn' t be set very high since sleeping for a lot of time
will cause starvation to other pardevice, this is reason we release
parport and recall schedule() by hand in lp_error() since we are sure we
will sleep for a lot of time since the printer is not yet online while lp
was polled or a pardevice has waken_up lp from the interrupt.
Looking at parport_share.c I see that we right spin_lock_irqsave() before
insert the pardevice in the waiters list (after the "blocked" label in
parport_claim()). This because we must be sure that the parport owner (and
it maybe the only other pardevice running) has not yet released parport.
With the current implementation it can happen that while we are adding the
pardevice to the waiters list (at the end of parport_claim()) another CPU
could run parport_release(). The other CPU could run "port->cad = NULL;"
while the first CPU has just passed the test:
spin_lock_irqsave (&port->lock, flags);
if (port->cad == NULL) {
/* The port got released in the meantime. */
spin_unlock_irqrestore (&port->lock, flags);
goto try_again;
}
With the old cli() all was right since the other CPU was blocked at this
check time.
This _should_ be the needed patch (against 2.1.96):
--- linux/drivers/misc/parport_share.c Wed Apr 15 12:49:49 1998
+++ /tmp/parport_share.c Thu Apr 16 02:01:33 1998
@@ -398,6 +398,7 @@
{
struct parport *port = dev->port;
struct pardevice *pd;
+ unsigned long flags;
/* Make sure that dev is the current device */
if (port->cad != dev) {
@@ -405,7 +406,9 @@
"when not owner\n", port->name, dev->name);
return;
}
+ spin_lock_irqsave(&port->lock, flags);
port->cad = NULL;
+ spin_lock_irqrestore(&port->lock, flags);
/* Save control registers */
port->ops->save_state(port, dev->state);
Andrea[s] Arcangeli
-- To unsubscribe, send mail to: linux-parport-request@torque.net --
-- with the single word "unsubscribe" in the body of the message. --
This archive was generated by hypermail 2.0b3 on Wed 30 Dec 1998 - 10:17:37 EST