USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

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.
  *
  * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic
  * encoding of the endpoint interval, and express polling intervals in

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

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
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html