On Thu, 3 May 2001, Oliver Neukum wrote:
> > - current state should be set to TASK_INTERRUPTIBLE for each loop
> > traversing. schedule() always returns RUNNABLE, so we would only
> > sleep once.
>
> How could the condition be true, once we are woken up ?
Waiting on several events on the same waitqueue. For example, both
lpusb_read() and lpusb_write() use lpusb->wait. So whenever an IN or OUT
transfer gets completed _both_ wake up. So we have to loop and test again
and sleep if it wasn't for us. Otherwise we would not even need a loop.
> But there is a bug, pending signals are not check for after schedule, but
> before.
Oops - right. If loop is exited due to timeout or status!=EINPROGRESS we
don't test for pending signals again. Test for pending signals could be
placed after the loop with a simple break in the loop.
> > - sleep/nosleep decision must be atomic. If this is enforced by a spinlock
> > we may use __set_current_state() (which does not serialize).
>
> What for ? There just must not be a false negative, which the order adding to
> queue and checking will ensure.
> Please explain.
Ok, I took drivers/usb/audio.c as a reference, which is implemented with a
spinlock. I'm not absolutely sure whether it's really required. Using the
urb->lock is probably somewhat misleading - what we need for sure is a
mb(). More protection (like spin_lock_irqsave) might be required in case
some pointers might get changed in interrupt f.e. - I agree reading
urb->status should be save (really for all arch's?). And it will never
change to become EINPROGRESS suddenly. OTOH the spinlock is quite cheap
here and makes clear this loop is somewhat fragile when modified.
> Either you trust gcc to write status in one write or you don't. To ensure
> what you want it would have to be atomic_t. A spinlock won't help.
Well, a block of code enclosed in spinlock with irq-disabled is pretty
atomic wrt. to SMP or interrupt races I believe. I'm not talking
about races due to PCI-BM access to some memory location. This does not
happen for urb->status.
> This patch is better, but would you think about the spinlocks ?
ok, since we have the same test in the enclosing if-statement unprotected
I think we should protect them both or none of it. At least for lpusb I
don't see something brakes without - so I've dropped them.
However I do prefer keeping the test for pending signals just before
going to sleep so we can bail out until then. Missing test after loop
exit added behind the loop.
Updated patch below.
Martin
----------------------
--- linux-2.4.4/drivers/usb.orig/printer.c Sat Apr 14 13:44:41 2001
+++ linux-2.4.4/drivers/usb/printer.c Thu May 3 16:51:22 2001
@@ -331,22 +331,30 @@
{
struct usblp *usblp = file->private_data;
int timeout, err = 0, writecount = 0;
+ DECLARE_WAITQUEUE(wait,current);
while (writecount < count) {
+ /* FIXME: explicitly atomic required? */
if (usblp->writeurb.status == -EINPROGRESS) {
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
+ add_wait_queue(&usblp->wait, &wait);
timeout = USBLP_WRITE_TIMEOUT;
- while (timeout && usblp->writeurb.status == -EINPROGRESS) {
-
+ while (timeout) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (usblp->writeurb.status != -EINPROGRESS)
+ break;
if (signal_pending(current))
- return writecount ? writecount : -EINTR;
-
- timeout = interruptible_sleep_on_timeout(&usblp->wait,
timeout);
+ break;
+ timeout = schedule_timeout(timeout);
}
+ remove_wait_queue(&usblp->wait, &wait);
+ set_current_state(TASK_RUNNING); /* provides barrier */
+ if (signal_pending(current))
+ return writecount ? writecount : -EINTR;
}
if (!usblp->dev)
@@ -388,6 +396,7 @@
static ssize_t usblp_read(struct file *file, char *buffer, size_t count, loff_t *ppos)
{
struct usblp *usblp = file->private_data;
+ DECLARE_WAITQUEUE(wait,current);
if (!usblp->bidir)
return -EINVAL;
@@ -397,11 +406,19 @@
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- while (usblp->readurb.status == -EINPROGRESS) {
+ add_wait_queue(&usblp->wait, &wait);
+ for (;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (usblp->readurb.status != -EINPROGRESS)
+ break;
if (signal_pending(current))
- return -EINTR;
- interruptible_sleep_on(&usblp->wait);
+ break;
+ schedule();
}
+ remove_wait_queue(&usblp->wait, &wait);
+ set_current_state(TASK_RUNNING);
+ if (signal_pending(current))
+ return -EINTR;
}
if (!usblp->dev)
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel