Re: usblp: add dynamic URBs, fix races

Am Freitag, 22. Juni 2007 schrieb Pete Zaitcev:
> On Fri, 22 Jun 2007 10:52:18 +0200, Oliver Neukum <[email protected]> wrote:
> 
> > > +	if (urb->status < 0)
> > > +		usblp->rstatus = urb->status;
> > > +	else
> > > +		usblp->rstatus = urb->actual_length;
> > >  	usblp->rcomplete = 1;
> > > -unplug:
> > > -	wake_up_interruptible(&usblp->wait);
> > > +	wake_up(&usblp->rwait);
> > 
> > Why no longer _interruptible ?
> 
> Because I want to wake anything that gets into this wait list.
> Currently, every waiter is interruptible, but I don't want to keep
> track. It's the same thing as using spin_lock_irqsave().

I see.

> > > +	spin_lock(&usblp->lock);
> > > +	if (urb->status < 0)
> > > +		usblp->wstatus = urb->status;
> > > +	else
> > > +		usblp->wstatus = urb->actual_length;
> > 
> > This raises the question why an error would mean that no payload
> > has been transfered.
> 
> In the context of usblp it's pretty much the same thing. If the
> stream is corrupt, you might as well abort the whole job: it's going
> to be illegible. There's no reason to return the actual_length to
> the application.

Are you sure that no printing app will merge print jobs?

> > > +	/* XXX Use usb_setup_bulk_urb when available. Talk to Marcel. */
> > > +	kfree(urb->transfer_buffer);
> > > +	urb->transfer_buffer = NULL;	/* Not refcounted, so to be safe... */
> > > +	usb_free_urb(urb);
> > 
> > If you want to be safe, do it without a race.
> 
> What race?

CPU A						CPU B
kfree(urb->transfer_buffer); 
							if (urb->transfer_buffer) something();
urb->transfer_buffer = NULL;

If you want to do this I'd suggest:

t = urb->transfer_buffer;
urb->transfer_buffer = NULL;
kfree(t);

Generally, I would oppose such nullings. There's no way to do
this cleanly.

> > >  static void usblp_unlink_urbs(struct usblp *usblp)
> > >  {
> > > -	usb_kill_urb(usblp->writeurb);
> > > -	if (usblp->bidir)
> > > -		usb_kill_urb(usblp->readurb);
> > > +	usb_kill_anchored_urbs(&usblp->urbs);
> > >  }
> > 
> > What good does encapsulation here?
> 
> Believe it or not, this thought has crossed my mind.
> I just wanted to preserve the area in case something else
> would be needed. Observe that the usb-skeleton waits for the URBs
> to complete. We do NOT do it on purpose, but this is unusual.

OK.
 
> > >  static unsigned int usblp_poll(struct file *file, struct poll_table_struct *wait)
> > >  {
> > > +	int ret;
> > > +	unsigned long flags;
> > 
> > We are called with interrupts on.
> 
> We discussed it a few times before. I guess my use of spin_lock in
> callbacks wasn't enough to appease you, eh?

No, because here it bothers me for another reason.
It is just a few cycles. But it makes the code harder to maintain.
The next one to touch this will look at it, scratch his head and
use GFP_ATOMIC just in case or do some funny stuff just in
case somebody might call this function with a spinlock held.

Needlessly coding for interrupts off is a bad thing. You should make
it very clear whether something can be called with interrupts off or
not. Agnosticism is a vice.

On a related note I find kerneldoc deficient in that regard. Every
more or less obvious parameter is documented but there's no
tag to describe which locks must, may or may not be held.

> > > +	spin_lock_irqsave(&usblp->lock, flags);
> > > +	ret = ((!usblp->bidir || !usblp->rcomplete) ? 0 : POLLIN  | POLLRDNORM)
> > >   			       | (!usblp->wcomplete ? 0 : POLLOUT | POLLWRNORM);
> > > +	spin_unlock_irqrestore(&usblp->lock, flags);
> > > +	return ret;
> > >  }
> > 
> > Shouldn't disconnect cause an error to be reported?
> 
> Good question. Old driver didn't do it. Since URBs fail on disconnect,
> it caused a momentary spin of the application and a EIO from next write.

Which is obviously legitimate.

> If you have good contacts with CUPS people or other application writers,
> it may help to ask them if they want anything. It's easy to add.

I'll ask.
 
> N.B., it may be useless, depending who schedules earlier. If the writer
> wakes up before the khubd does, everything would happen just as it
> always did (e.g. a quick loop-around and a EIO).
> 
> > > +	if (mutex_lock_interruptible(&usblp->wmut)) {
> > > +		rv = -EINTR;
> > 
> > Why not -ERESTARTSYS ?
> 
> Hmm. Yes, the syscall is restartable at this point. I guess I inherited
> it from the old driver, where the loop was unified, so it didn't try
> to distinguish the cases when something was written or nothing was written,
> and returned -EINTR always. On the other hand, what's the harm?

No harm. I just wondered whether there's a reason which would have
meant auditing other drivers for it.

	Regards
		Oliver

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel