On Fri, Mar 30, 2007 at 08:58:38AM +0200, Oliver Neukum wrote:
> Am Freitag, 30. M?rz 2007 07:07 schrieb Greg KH:
> > On Thu, Mar 29, 2007 at 02:25:34PM +0200, Oliver Neukum wrote:
> > > @@ -332,12 +330,12 @@
> > >
> > > dbg("%s - port %d", __FUNCTION__, port->number);
> > >
> > > - port->write_urb_busy = 0;
> > > if (urb->status) {
> > > dbg("%s - nonzero write bulk status received: %d",
> > > __FUNCTION__, urb->status);
> > > return;
> > > }
> > > -
> > > + smp_mb(); /* we are busy until we have read our status */
> >
> > I don't understand why this is necessary, we shouldn't have to put this
> > all over our drivers, if so, something is really wrong.
>
> OK, this is subtle. The completion handlers are called without locks held,
> eg. this from ehci-q.c:
>
> /* complete() can reenter this HCD */
> spin_unlock (&ehci->lock);
> usb_hcd_giveback_urb (ehci_to_hcd(ehci), urb);
> spin_lock (&ehci->lock);
>
> The comment gives you the reason and is right. The locks must be dropped.
> Therefore, if the urb is resubmitted while the completion handler is running,
> usbcore will fully process this urb. This means that the status will be set
> to -EINPROGRESS.
> So the status is valid only until the urb is resubmitted. The driver
> in question may resubmit the urb in the write path if !port->write_urb_busy.
Ok, that makes sense.
> We must consider clearing a busy flag equivalent to resubmitting the
> urb ourselves. In a completion handler that resubmits the urb you do that
> resubmission last in the handler, don't you?
Yes.
> As an added complication, the resubmission would happen on another cpu.
> Therefore the order on the bus must be correct, hence the "smp_mb()".
This part I still don't understand.
What are we doing a memory barier for after the completion handler is
finished here? If we set the write_urb_busy flag to 0 after we check
the status, we should be safe, as that urb will not be able to be
resubmitted until that point in time. Or should we be grabbing the lock
for the port when we change the status instead? That would make more
sense I think.
thanks,
greg k-h
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
