[PATCH] usb: core: allow zero packet flag for interrupt urbs

[PATCH] usb: core: allow zero packet flag for interrupt urbs

[Amit Virdi]

Section 4.4.7.2 of the USB3.0 spec says:
	A zero-length data payload is a valid transfer and may be useful for
	some implementations.

So, extend the logic of allowing URB_ZERO_PACKET to interrupt urbs too.
Otherwise, the kernel throws error of BOGUS transfer flags.

Signed-off-by: Amit Virdi <amit.virdi@st.com>
---
 drivers/usb/core/urb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 991386c..a136246 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -460,6 +460,10 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	case USB_ENDPOINT_XFER_CONTROL:
 		allowed |= URB_NO_FSBR;	/* only affects UHCI */
 		/* FALLTHROUGH */
+	case USB_ENDPOINT_XFER_INT:
+		if (is_out)
+			allowed |= URB_ZERO_PACKET;
+		/* FALLTHROUGH */
 	default:			/* all non-iso endpoints */
 		if (!is_out)
 			allowed |= URB_SHORT_NOT_OK;
-- 
1.8.0

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Hans de Goede]

Hi,

On 07/17/2014 10:47 AM, Amit Virdi wrote:
> Section 4.4.7.2 of the USB3.0 spec says:
> 	A zero-length data payload is a valid transfer and may be useful for
> 	some implementations.
> 
> So, extend the logic of allowing URB_ZERO_PACKET to interrupt urbs too.
> Otherwise, the kernel throws error of BOGUS transfer flags.
> 
> Signed-off-by: Amit Virdi <amit.virdi@st.com>

Seems sensible to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/usb/core/urb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 991386c..a136246 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -460,6 +460,10 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  	case USB_ENDPOINT_XFER_CONTROL:
>  		allowed |= URB_NO_FSBR;	/* only affects UHCI */
>  		/* FALLTHROUGH */
> +	case USB_ENDPOINT_XFER_INT:
> +		if (is_out)
> +			allowed |= URB_ZERO_PACKET;
> +		/* FALLTHROUGH */
>  	default:			/* all non-iso endpoints */
>  		if (!is_out)
>  			allowed |= URB_SHORT_NOT_OK;
> 

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Alan Stern]

On Thu, 17 Jul 2014, Amit Virdi wrote:

> Section 4.4.7.2 of the USB3.0 spec says:
> 	A zero-length data payload is a valid transfer and may be useful for
> 	some implementations.
> 
> So, extend the logic of allowing URB_ZERO_PACKET to interrupt urbs too.
> Otherwise, the kernel throws error of BOGUS transfer flags.
> 
> Signed-off-by: Amit Virdi <amit.virdi@st.com>
> ---
>  drivers/usb/core/urb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 991386c..a136246 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -460,6 +460,10 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  	case USB_ENDPOINT_XFER_CONTROL:
>  		allowed |= URB_NO_FSBR;	/* only affects UHCI */
>  		/* FALLTHROUGH */
> +	case USB_ENDPOINT_XFER_INT:
> +		if (is_out)
> +			allowed |= URB_ZERO_PACKET;
> +		/* FALLTHROUGH */
>  	default:			/* all non-iso endpoints */
>  		if (!is_out)
>  			allowed |= URB_SHORT_NOT_OK;

I can't say this is actually wrong, but have you ever encountered a 
situation where this would be needed?  How often does anyone need to do 
a multi-packet transfer over an interrupt endpoint?

Alan Stern

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Steve Calfee]

On Thu, Jul 17, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> I can't say this is actually wrong, but have you ever encountered a
> situation where this would be needed?  How often does anyone need to do
> a multi-packet transfer over an interrupt endpoint?
>
Hi Alan,

I did some testing with multi interrupt transfers some time ago. You
can get allocated a guaranteed 3x1024 time slot per uframe for an
interval of your choice on usb 2.0. So zlps are needed to send exactly
1024 or 2048 bytes in that time slot. Also, interrupt transfers are
crc checked and resent (as long as uframe time is still available). So
you get guaranteed timing like iso sends, and guaranteed data like
bulk sends. This could be valuable for time sensitive apps with known
data capacities.

Microsoft only started supporting multi-interrupt transfers in Windows
Vista and beyond. Winxp refuses to die, so probably no one has built
an actual hardware device for consumers that uses multi-interrupt
xfers up to now.

Regards, Steve

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Alan Stern]

On Thu, 17 Jul 2014, Steve Calfee wrote:

> On Thu, Jul 17, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > I can't say this is actually wrong, but have you ever encountered a
> > situation where this would be needed?  How often does anyone need to do
> > a multi-packet transfer over an interrupt endpoint?
> >
> Hi Alan,
> 
> I did some testing with multi interrupt transfers some time ago. You
> can get allocated a guaranteed 3x1024 time slot per uframe for an
> interval of your choice on usb 2.0. So zlps are needed to send exactly
> 1024 or 2048 bytes in that time slot.

That's true, but misleading.  Use of zero-length packets is relatively
independent of the time slot divisions.  zlps are necessary to mark the
end of a multi-packet transfer; they aren't necessary if all you want
to do is send less than 3072 bytes during a single 3x1024 time slot.

Consider also that the URB_ZERO_PACKET flag will give you at most one
zlp.  That won't be good enough if you want to send exactly 1024 bytes
in the time slot; you would need two zlps.  Besides, why would you want
to limit the amount of data that gets sent during the time slot?  If
you have more than 1024 or 2048 bytes, why not send all of it?

(For that matter, what happens if you want to send 2000 bytes during 
the time slot?  The first packet will contain 1024 bytes, the second 
will contain the remaining 976, and there won't be a zlp even if 
URB_ZERO_PACKET is set.  Instead, the third packet will contain 
whatever data has been queued next.  I was a little surprised to see 
that section 4.10.3 in the EHCI spec _doesn't_ say the controller must 
leave the Exercute Transaction state when a short packet is
encountered -- even though not doing so would violate the USB-2.0 
spec.)

Conversely, if you have only 1024 bytes to send, you don't need to do
anything special.  Just send them.  There won't be any zlp during the
time slot, but if no data is queued then nothing will be sent after
those 1024 bytes.

> Also, interrupt transfers are
> crc checked and resent (as long as uframe time is still available). So
> you get guaranteed timing like iso sends, and guaranteed data like
> bulk sends. This could be valuable for time sensitive apps with known
> data capacities.
> 
> Microsoft only started supporting multi-interrupt transfers in Windows
> Vista and beyond. Winxp refuses to die, so probably no one has built
> an actual hardware device for consumers that uses multi-interrupt
> xfers up to now.

Well, I don't object to the patch.  It just seems odd, because it
implies that someone is using an interrupt endpoint like a bulk
endpoint.

Amit, what is the reason for this patch?  Is it purely theoretical?  
Or is it actually needed for some real purpose?

Alan Stern

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Steve Calfee]

On Thu, Jul 17, 2014 at 12:32 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 17 Jul 2014, Steve Calfee wrote:
>
>> Hi Alan,
>>
>> I did some testing with multi interrupt transfers some time ago. You
>> can get allocated a guaranteed 3x1024 time slot per uframe for an
>> interval of your choice on usb 2.0. So zlps are needed to send exactly
>> 1024 or 2048 bytes in that time slot.
>
> That's true, but misleading.  Use of zero-length packets is relatively
> independent of the time slot divisions.  zlps are necessary to mark the
> end of a multi-packet transfer; they aren't necessary if all you want
> to do is send less than 3072 bytes during a single 3x1024 time slot.
>
> Consider also that the URB_ZERO_PACKET flag will give you at most one
> zlp.  That won't be good enough if you want to send exactly 1024 bytes
> in the time slot; you would need two zlps.  Besides, why would you want
> to limit the amount of data that gets sent during the time slot?  If
> you have more than 1024 or 2048 bytes, why not send all of it?
>
> (For that matter, what happens if you want to send 2000 bytes during
> the time slot?  The first packet will contain 1024 bytes, the second
> will contain the remaining 976, and there won't be a zlp even if
> URB_ZERO_PACKET is set.  Instead, the third packet will contain
> whatever data has been queued next.  I was a little surprised to see
> that section 4.10.3 in the EHCI spec _doesn't_ say the controller must
> leave the Exercute Transaction state when a short packet is
> encountered -- even though not doing so would violate the USB-2.0
> spec.)
>
> Conversely, if you have only 1024 bytes to send, you don't need to do
> anything special.  Just send them.  There won't be any zlp during the
> time slot, but if no data is queued then nothing will be sent after
> those 1024 bytes.
>
Hi Alan,

It has been a few years since I was doing this, but here is my
understanding. If a device descriptor says x bytes should be reserved
(1 to 3072), the host will allocate that much bandwidth. If the sender
sends some number of bytes between 1 and 3072 (must be less than or
equal to x allocation), everything is ok the receiver will know when
the sender is done. But (as with bulk xfers) if exactly 1024 or 2048
bytes is sent, the receiver will not know the xfer is done. So in
those cases, just as in bulk, the sender needs to send a ZLP.

The receiver will request up to 3 int xfers (depending on x>2048 or x
>1024), and will stop issuing requests  (on a host sender) on a short
transfer or a full xfer.

Timeslots are guaranteed reservations, but they are not automatically
always used. Unused bandwidth in a uframe (after reserved uses what it
wants) will be used as needed by bulk or control transfers.

Regards, Steve

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Alan Stern]

On Thu, 17 Jul 2014, Steve Calfee wrote:

> Hi Alan,
> 
> It has been a few years since I was doing this, but here is my
> understanding. If a device descriptor says x bytes should be reserved
> (1 to 3072), the host will allocate that much bandwidth. If the sender
> sends some number of bytes between 1 and 3072 (must be less than or
> equal to x allocation), everything is ok the receiver will know when
> the sender is done. But (as with bulk xfers) if exactly 1024 or 2048
> bytes is sent, the receiver will not know the xfer is done. So in
> those cases, just as in bulk, the sender needs to send a ZLP.

You're getting mixed up over two different ideas here: a transfer and
the data sent during a particular microframe.  They aren't the same.

Thus, the receiver does know when a microframe's worth of data is
finished, even if exactly 1024 or 2048 bytes were sent, because the
receiver knows when the microframe ends (it gets an SOF packet).

Conversely, if a short transfer's length is a multiple of the maxpacket
size then without a zlp the receiver won't know when the transfer is
finished -- even if the transfer ends up occupying 3072 bytes.  For 
example, suppose the receiver expects the transfer to be 4096 bytes, 
but the sender has only 3072 bytes of data to send.

> The receiver will request up to 3 int xfers (depending on x>2048 or x
> >1024), and will stop issuing requests  (on a host sender) on a short
> transfer or a full xfer.

In USB, receivers do not issue requests.  The host initiates all 
transfers.

> Timeslots are guaranteed reservations, but they are not automatically
> always used. Unused bandwidth in a uframe (after reserved uses what it
> wants) will be used as needed by bulk or control transfers.

Correct.

Alan Stern

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Amit Virdi]

On 7/17/2014 8:25 PM, Alan Stern wrote:
> I can't say this is actually wrong, but have you ever encountered a
> situation where this would be needed?  How often does anyone need to do
> a multi-packet transfer over an interrupt endpoint?

Honestly, I haven't found any such real device yet. I did this change 
while I was going through the code when adding support for interrupt 
transfers in gadget zero. I'm a no expert, but the spec says it should 
be supported so this code should be added.

However, I messed up a little in this patch. It should have been
---
         case USB_ENDPOINT_XFER_ISOC:
                 allowed |= URB_ISO_ASAP;
                 break;
+       case USB_ENDPOINT_XFER_INT:
+               if (is_out)
+                       allowed |= URB_ZERO_PACKET;
+               else
+                       allowed |= URB_SHORT_NOT_OK;
+               break;
         }
         allowed &= urb->transfer_flags;

---
Otherwise, it sets zero packet flag for control out transfers too.

I'll send a V2 with this change if you agree to setting of the zero 
packet flag for interrupt transfers.

Regards
Amit Virdi

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Alan Stern]

On Fri, 18 Jul 2014, Amit Virdi wrote:

> On 7/17/2014 8:25 PM, Alan Stern wrote:
> > I can't say this is actually wrong, but have you ever encountered a
> > situation where this would be needed?  How often does anyone need to do
> > a multi-packet transfer over an interrupt endpoint?
> 
> Honestly, I haven't found any such real device yet. I did this change 
> while I was going through the code when adding support for interrupt 
> transfers in gadget zero. I'm a no expert, but the spec says it should 
> be supported so this code should be added.

I just remembered, the HID spec requires that all reports (except the
longest) end with a short packet.  And since output reports are allowed
to be sent over an Interrupt-OUT endpoint, we have to accept the
URB_ZERO_PACKET flag for interrupt URBs.

> However, I messed up a little in this patch. It should have been
> ---
>          case USB_ENDPOINT_XFER_ISOC:
>                  allowed |= URB_ISO_ASAP;
>                  break;
> +       case USB_ENDPOINT_XFER_INT:
> +               if (is_out)
> +                       allowed |= URB_ZERO_PACKET;
> +               else
> +                       allowed |= URB_SHORT_NOT_OK;
> +               break;
>          }
>          allowed &= urb->transfer_flags;
> 
> ---
> Otherwise, it sets zero packet flag for control out transfers too.
> 
> I'll send a V2 with this change if you agree to setting of the zero 
> packet flag for interrupt transfers.

How about this change instead?

 			URB_FREE_BUFFER);
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
+	case USB_ENDPOINT_XFER_INT:
 		if (is_out)
 			allowed |= URB_ZERO_PACKET;
 		/* FALLTHROUGH */

Alan Stern

Re: [PATCH] usb: core: allow zero packet flag for interrupt urbs

[Amit Virdi]

On 7/18/2014 8:09 PM, Alan Stern wrote:
> On Fri, 18 Jul 2014, Amit Virdi wrote:
>
>> On 7/17/2014 8:25 PM, Alan Stern wrote:
>>> I can't say this is actually wrong, but have you ever encountered a
>>> situation where this would be needed?  How often does anyone need to do
>>> a multi-packet transfer over an interrupt endpoint?
>>
>> Honestly, I haven't found any such real device yet. I did this change
>> while I was going through the code when adding support for interrupt
>> transfers in gadget zero. I'm a no expert, but the spec says it should
>> be supported so this code should be added.
>
> I just remembered, the HID spec requires that all reports (except the
> longest) end with a short packet.  And since output reports are allowed
> to be sent over an Interrupt-OUT endpoint, we have to accept the
> URB_ZERO_PACKET flag for interrupt URBs.
>

Ok, great!

>> However, I messed up a little in this patch. It should have been
>> ---
>>           case USB_ENDPOINT_XFER_ISOC:
>>                   allowed |= URB_ISO_ASAP;
>>                   break;
>> +       case USB_ENDPOINT_XFER_INT:
>> +               if (is_out)
>> +                       allowed |= URB_ZERO_PACKET;
>> +               else
>> +                       allowed |= URB_SHORT_NOT_OK;
>> +               break;
>>           }
>>           allowed &= urb->transfer_flags;
>>
>> ---
>> Otherwise, it sets zero packet flag for control out transfers too.
>>
>> I'll send a V2 with this change if you agree to setting of the zero
>> packet flag for interrupt transfers.
>
> How about this change instead?
>
>   			URB_FREE_BUFFER);
>   	switch (xfertype) {
>   	case USB_ENDPOINT_XFER_BULK:
> +	case USB_ENDPOINT_XFER_INT:

Looks better.
I'll submit a V2 with this change.

Regards
Amit Virdi

[PATCH V2] usb: core: allow zero packet flag for interrupt urbs

[Amit Virdi]

Section 4.4.7.2 "Interrupt Transfer Bandwidth Requirements" of the USB3.0 spec
says:
	A zero-length data payload is a valid transfer and may be useful for
	some implementations.

So, extend the logic of allowing URB_ZERO_PACKET to interrupt urbs too.
Otherwise, the kernel throws warning of BOGUS transfer flags.

Signed-off-by: Amit Virdi <amit.virdi@st.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/core/urb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 991386ceb4ec..c9e8ee81b6b7 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -454,6 +454,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 			URB_FREE_BUFFER);
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
+	case USB_ENDPOINT_XFER_INT:
 		if (is_out)
 			allowed |= URB_ZERO_PACKET;
 		/* FALLTHROUGH */
-- 
1.8.0

Re: [PATCH V2] usb: core: allow zero packet flag for interrupt urbs

[Alan Stern]

On Mon, 21 Jul 2014, Amit Virdi wrote:

> Section 4.4.7.2 "Interrupt Transfer Bandwidth Requirements" of the USB3.0 spec
> says:
> 	A zero-length data payload is a valid transfer and may be useful for
> 	some implementations.
> 
> So, extend the logic of allowing URB_ZERO_PACKET to interrupt urbs too.
> Otherwise, the kernel throws warning of BOGUS transfer flags.
> 
> Signed-off-by: Amit Virdi <amit.virdi@st.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/core/urb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 991386ceb4ec..c9e8ee81b6b7 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -454,6 +454,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  			URB_FREE_BUFFER);
>  	switch (xfertype) {
>  	case USB_ENDPOINT_XFER_BULK:
> +	case USB_ENDPOINT_XFER_INT:
>  		if (is_out)
>  			allowed |= URB_ZERO_PACKET;
>  		/* FALLTHROUGH */

Acked-by: Alan Stern <stern@rowland.harvard.edu>