Re: [PATCH]generic usb serial driver

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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel