On Fri, Mar 30, 2007 at 11:17:28AM +0200, Oliver Neukum wrote:
> Am Freitag, 30. M?rz 2007 11:03 schrieb Greg KH:
> > On Fri, Mar 30, 2007 at 10:45:26AM +0200, Oliver Neukum wrote:
> > > Am Freitag, 30. M?rz 2007 10:39 schrieb Greg KH:
> > > > > To solve this with locking would require both CPU A and CPU B to take
> > > > > the lock, which means that the urb would have to be submitted with
> > > > > GFP_ATOMIC.
> > > >
> > > > Can't this all be solved with a simple:
> > > >
> > > > ????????get port spinlock
> > > > ????????a = urb->status
> > > > ????????write_busy = 0
> > > > ????????unlock spinlock
> > > >
> > > > In the callback, and in the write path:
> > > >
> > > > ????????get port spinlock
> > > > ????????if write_busy
> > > > ????????????????unlock
> > > > ????????????????exit
> > > > ????????write_busy = 1
> > > > ????????unlock port spinlock
> > > > ????????submit_urb
> > > >
> > >
> > > Oops. You are right. That would work.
> > > If you think that's simpler, yes it can be done.
> >
> > Yes, I think that would be both easier to understand, and easier to
> > prove correct :)
>
> However it would not always work out so nicely.
> Have a look at this code from usblp:
>
> timeout = USBLP_WRITE_TIMEOUT;
>
> rv = wait_event_interruptible_timeout(usblp->wait,
> usblp->wcomplete || !usblp->present , timeout);
> if (rv < 0)
> return writecount ? writecount : -EINTR;
Well, usblp incorrectly (in my opinion) tries to return the status of
every urb to userspace. I'm still not quite sure why it needs to do
that, and we can't just throw a ton of urbs at the device and then walk
away. It's not like userspace does much if we report an error on the
exact packet that fails, vs a packet later on, right?
> and
>
> static void usblp_bulk_write(struct urb *urb)
> {
> struct usblp *usblp = urb->context;
>
> if (unlikely(!usblp || !usblp->dev || !usblp->used))
> return;
> if (unlikely(!usblp->present))
> goto unplug;
> if (unlikely(urb->status))
> warn("usblp%d: nonzero read/write bulk status received: %d",
> usblp->minor, urb->status);
> usblp->wcomplete = 1;
> unplug:
> wake_up_interruptible(&usblp->wait);
> }
>
> It has the same issue. The correct code is:
>
> if (unlikely(urb->status))
> warn("usblp%d: nonzero read/write bulk status received: %d",
> usblp->minor, urb->status);
> smp_mb();
> usblp->wcomplete = 1;
> unplug:
> wake_up_interruptible(&usblp->wait);
>
> This can'be solved with a simple spinlock, as wait_event() can sleep.
> Neither is simply returning 0 an option. It would mean EOF.
> Really somebody went to some effort to find a lockless solution and
> did well, he just didn't figure out of order execution units. You'd destroy
> that.
I think it really comes down to the problem of checking the urb status
flag.
I _REALLY_ want to get rid of that field and not have drivers us it at
all. I would like to pass the value of the status flag to the urb
callback directly, and only have the status field be used by the host
controllers.
The last time I looked into implementing this, I think the UHCI driver
had some 'issues' with how it would work, or maybe it was EHCI, sorry it
was a few years ago.
However, if we are able to fix that, and never let drivers touch that
field, then the large majority of these kinds of problems will go away,
right?
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
