Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> On 2018-06-20 11:35:05 [-0400], Alan Stern wrote:
> > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > index 4cdd515a4385..c3a8bd586121 100644
> > > --- a/include/linux/usb.h
> > > +++ b/include/linux/usb.h
> > > @@ -1657,7 +1657,8 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
> > >   *	the endpoint descriptor's bInterval value.
> > >   *
> > >   * Initializes a interrupt urb with the proper information needed to submit
> > > - * it to a device.
> > > + * it to a device. This function can also be used to initialize an isochronous
> > > + * urb.
> > 
> > No, no!  This function can _help_ initialize an isochronous URB, but
> > that's all.  It would be better to create an explicit 
> > usb_fill_isoc_urb() routine, and even that would have to be incomplete.
> 
> Yes, incomplete. I read incomplete as some additional fields have to be
> set which are not set yet. But for FS you write…

There's also the cognitive dissonance of using a routine named 
"usb_fill_int_urb" to initialize an isochronous URB.  That should set 
off alarm bells in the mind of anyone reading the code, no matter what 
the documentation says.

> > There are two problems with using usb_fill_int_urb() to initialize an 
> > isochronous URB:
> > 
> > 	The calculation of the interval value is wrong for full-speed
> > 	devices.
> 
> Why wrong?

Because the interval value in the FS endpoint descriptor uses a
logarithmic encoding, whereas the value stored in the URB uses a linear
encoding.  Simply copying one value to the other will store an
incorrect number, except in the lucky cases where the interval is
either 1 or 2.

Alan Stern

> > 	The routine does not set urb->number_of_packets or
> > 	urb->iso_frame_desc[].
> 
> Yes.
> 
> > Alan Stern
> 
> Sebastian




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux