On Wed, 2 May 2001, Oliver Neukum wrote:
> as was recently discussed, sleep_on and derivates, which have a little race
> problem are used 21 times in drivers/usb
and 12 more in drivers/usb/serial ;-)
> The attached patch should fix one such place in printer.
Don't want to be pedantic, but there are some more issues:
- current state should be set to TASK_INTERRUPTIBLE for each loop
traversing. schedule() always returns RUNNABLE, so we would only
sleep once.
- memory barriers required to make sure we are really on the wait_queue
before testing whether to sleep - otherwise there would be a race in
case the interrupt/wakeup is executed on other CPU
- better use set_current_state() - it provides the memory barrier when
placed between add_wait_queue() and sleep/nosleep decision
- sleep/nosleep decision must be atomic. If this is enforced by a spinlock
we may use __set_current_state() (which does not serialize).
- not only lpusb_read() uses sleep_on(), similar for lpusb_write() but
with timeout
the updated patch below (vs. 2.4.4, -ac should be ok) addresses these
issues.
> It even compiles.
and nothing appears to be broken - at least for my HL1250 (2 EP).
> 20 more to go. People let's kill them all.
Good idea anyway!
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 Wed May 2 22:36:41 2001
@@ -331,22 +331,42 @@
{
struct usblp *usblp = file->private_data;
int timeout, err = 0, writecount = 0;
+ unsigned long flags;
+ int status;
+ 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) {
- if (signal_pending(current))
- return writecount ? writecount : -EINTR;
+ /* if wakeup arrives here we won't sleep! */
+ __set_current_state(TASK_INTERRUPTIBLE);
- timeout = interruptible_sleep_on_timeout(&usblp->wait,
timeout);
+ /* spinlock for atomic access and serialization */
+ spin_lock_irqsave(&usblp->writeurb.lock, flags);
+ status = usblp->writeurb.status;
+ spin_unlock_irqrestore(&usblp->writeurb.lock, flags);
+
+ if (status != -EINPROGRESS)
+ break;
+ if (signal_pending(current)) {
+ remove_wait_queue(&usblp->wait, &wait);
+ set_current_state(TASK_RUNNING);
+ return writecount ? writecount : -EINTR;
+ }
+ timeout = schedule_timeout(timeout);
}
+ remove_wait_queue(&usblp->wait, &wait);
+ set_current_state(TASK_RUNNING); /* provides barrier */
}
if (!usblp->dev)
@@ -388,6 +408,9 @@
static ssize_t usblp_read(struct file *file, char *buffer, size_t count, loff_t *ppos)
{
struct usblp *usblp = file->private_data;
+ unsigned long flags;
+ int status;
+ DECLARE_WAITQUEUE(wait,current);
if (!usblp->bidir)
return -EINVAL;
@@ -397,11 +420,23 @@
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- while (usblp->readurb.status == -EINPROGRESS) {
- if (signal_pending(current))
+ add_wait_queue(&usblp->wait, &wait);
+ for (;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&usblp->readurb.lock, flags);
+ status = usblp->readurb.status;
+ spin_unlock_irqrestore(&usblp->readurb.lock, flags);
+ if (status != -EINPROGRESS)
+ break;
+ if (signal_pending(current)) {
+ remove_wait_queue(&usblp->wait, &wait);
+ set_current_state(TASK_RUNNING);
return -EINTR;
- interruptible_sleep_on(&usblp->wait);
+ }
+ schedule();
}
+ remove_wait_queue(&usblp->wait, &wait);
+ __set_current_state(TASK_RUNNING);
}
if (!usblp->dev)
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
