| Message ID | 20180620152007.xapqkv4ww2hnmvkq@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote: > Laurent suggested that the kerneldoc documentation could state that > usb_fill_int_urb() can also be used for the initialisation of an > isochronous urb. The USB documentation in > Documentation/driver-api/usb/URB.rst already mentions this, some drivers > do so and there is no explicit usb_fill_iso_urb(). > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > On 2018-06-20 17:14:53 [+0300], Laurent Pinchart wrote: > > > So you simply asking that the kerneldoc of usb_fill_int_urb() is > > > extended to mention isoc, too? > > > > That would be nice I think. > > here it is. > > include/linux/usb.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > 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. 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. The routine does not set urb->number_of_packets or urb->iso_frame_desc[]. Alan Stern > * > * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic > * encoding of the endpoint interval, and express polling intervals in >
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 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? > The routine does not set urb->number_of_packets or > urb->iso_frame_desc[]. Yes. > Alan Stern Sebastian
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
On 2018-06-20 12:21:33 [-0400], Alan Stern wrote: > > > 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. Hmmm. Now that I looked into USB 2.0 specification it really says logarithmic encoding for ISOC endpoints on every speed and not just HS. And INTR endpoints have logarithmic encoding only for HS. I remembered it differently… But based on this, we should really introduce usb_fill_iso_urb() which handles this correctly. Also the documentation should be updated because the suggestion for ubs_fill_intr_urb() is misleading (especially if you have wrong memory about the encoding on FS). > Alan Stern Sebastian
On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote: > On 2018-06-20 12:21:33 [-0400], Alan Stern wrote: > > > > 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. > > Hmmm. Now that I looked into USB 2.0 specification it really says > logarithmic encoding for ISOC endpoints on every speed and not just HS. > And INTR endpoints have logarithmic encoding only for HS. I remembered > it differently… > > But based on this, we should really introduce usb_fill_iso_urb() which > handles this correctly. Also the documentation should be updated because > the suggestion for ubs_fill_intr_urb() is misleading (especially if you > have wrong memory about the encoding on FS). That sounds like a good thing to do. Alan Stern
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. * * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic * encoding of the endpoint interval, and express polling intervals in