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;
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.
Regards
Oliver
-------------------------------------------------------------------------
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
