Re: [PATCH 05/23] USB: set the correct interval for interrupt URBs

On Mon, 23 Apr 2007, Pete Zaitcev wrote:

> On Fri,  9 Mar 2007 20:04:26 -0800, Greg Kroah-Hartman <[email protected]> wrote:
> 
> > From: Alan Stern <[email protected]>
> > 
> > This patch (as862) fixes a couple of bugs in the way usbcore handles
> > intervals for interrupt URBs.  usb_interrupt_msg (and usb_bulk_msg for
> > backward compatibility) don't set the interval correctly for
> > high-speed devices.  proc_do_submiturb() doesn't set it correctly when
> > a bulk URB is submitted to an interrupt endpoint.
> > 
> > Signed-off-by: Alan Stern <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> 
> Woops, looks like I missed this. Konrad Rzeszutek of IBM alerted me to
> this fix.
> 
> This looks good:
> 
> > --- a/drivers/usb/core/devio.c
> > @@ -1053,7 +1048,11 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
> >  	as->urb->setup_packet = (unsigned char*)dr;
> >  	as->urb->start_frame = uurb->start_frame;
> >  	as->urb->number_of_packets = uurb->number_of_packets;
> > -	as->urb->interval = interval;
> > +	if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
> > +			ps->dev->speed == USB_SPEED_HIGH)
> > +		as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
> > +	else
> > +		as->urb->interval = ep->desc.bInterval;
> >          as->urb->context = as;
> 
> ... because it's how we should calculate the interval, and we assign
> the field in URB, so fine. But this looks bogus:
> 
> > +++ b/drivers/usb/core/message.c
> > @@ -221,10 +221,15 @@ int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe,
> >  
> >  	if ((ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
> >  			USB_ENDPOINT_XFER_INT) {
> > +		int interval;
> > +
> > +		if (usb_dev->speed == USB_SPEED_HIGH)
> > +			interval = 1 << min(15, ep->desc.bInterval - 1);
> > +		else
> > +			interval = ep->desc.bInterval;
> >  		pipe = (pipe & ~(3 << 30)) | (PIPE_INTERRUPT << 30);
> >  		usb_fill_int_urb(urb, usb_dev, pipe, data, len,
> > -				usb_api_blocking_completion, NULL,
> > -				ep->desc.bInterval);
> > +				usb_api_blocking_completion, NULL, interval);
> 
> This is because usb_fill_int_urb does this computation already.

So it does.

> Looks like someone (Jeff Layton maybe?) forwarded you a patch for RHEL 4,
> which is based on 2.6.9, and so it needs the last segment.
> 
> Alan, am I right?

I can't remember the details any more, but somebody definitely alerted me 
to one aspect of the problem.  Not Jeff Layton.  Probably it was the usbfs 
part, and I introduced the mistake in usb_bulk_msg() all by myself.  :-(

A patch to revert the bad portion follows shortly.

Alan Stern


-------------------------------------------------------------------------
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