linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
@ 2016-03-09 19:39 Felipe F. Tonello
  2016-03-09 22:43 ` Steve Calfee
  2016-03-11 23:07 ` Michal Nazarewicz
  0 siblings, 2 replies; 14+ messages in thread
From: Felipe F. Tonello @ 2016-03-09 19:39 UTC (permalink / raw)
  To: linux-usb; +Cc: linux-kernel, Felipe Balbi, Michal Nazarewicz, Robert Baldyga

buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
devices.

That caused the OUT endpoint to freeze if the host send any data packet of
length greater than 256 bytes.

This is an example dump of what happended on that enpoint:
HOST:   [DATA][Length=260][...]
DEVICE: [NAK]
HOST:   [PING]
DEVICE: [NAK]
HOST:   [PING]
DEVICE: [NAK]
...
HOST:   [PING]
DEVICE: [NAK]

This patch fixes this problem by setting the minimum usb_request's buffer size
for the OUT endpoint as its wMaxPacketSize.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 8475e3dc82d4..826ba641f29d 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	/* allocate a bunch of read buffers and queue them all at once. */
 	for (i = 0; i < midi->qlen && err == 0; i++) {
 		struct usb_request *req =
-			midi_alloc_ep_req(midi->out_ep, midi->buflen);
+			midi_alloc_ep_req(midi->out_ep,
+				max_t(unsigned, midi->buflen,
+					bulk_out_desc.wMaxPacketSize));
 		if (req == NULL)
 			return -ENOMEM;
 
-- 
2.7.2

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-09 19:39 [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe F. Tonello
@ 2016-03-09 22:43 ` Steve Calfee
  2016-03-10  9:23   ` Felipe Ferreri Tonello
  2016-03-11  8:44   ` Clemens Ladisch
  2016-03-11 23:07 ` Michal Nazarewicz
  1 sibling, 2 replies; 14+ messages in thread
From: Steve Calfee @ 2016-03-09 22:43 UTC (permalink / raw)
  To: Felipe F. Tonello
  Cc: USB list, Kernel development list, Felipe Balbi,
	Michal Nazarewicz, Robert Baldyga

On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> wrote:
> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
> devices.
>
> That caused the OUT endpoint to freeze if the host send any data packet of
> length greater than 256 bytes.
>
> This is an example dump of what happended on that enpoint:
> HOST:   [DATA][Length=260][...]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> ...
> HOST:   [PING]
> DEVICE: [NAK]
>
> This patch fixes this problem by setting the minimum usb_request's buffer size
> for the OUT endpoint as its wMaxPacketSize.
>
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 8475e3dc82d4..826ba641f29d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>         /* allocate a bunch of read buffers and queue them all at once. */
>         for (i = 0; i < midi->qlen && err == 0; i++) {
>                 struct usb_request *req =
> -                       midi_alloc_ep_req(midi->out_ep, midi->buflen);
> +                       midi_alloc_ep_req(midi->out_ep,
> +                               max_t(unsigned, midi->buflen,
> +                                       bulk_out_desc.wMaxPacketSize));
>                 if (req == NULL)
>                         return -ENOMEM;
>
Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?

Regards, Steve

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-09 22:43 ` Steve Calfee
@ 2016-03-10  9:23   ` Felipe Ferreri Tonello
  2016-03-11  8:44   ` Clemens Ladisch
  1 sibling, 0 replies; 14+ messages in thread
From: Felipe Ferreri Tonello @ 2016-03-10  9:23 UTC (permalink / raw)
  To: Steve Calfee
  Cc: USB list, Kernel development list, Felipe Balbi,
	Michal Nazarewicz, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

Hi Steve,

On 09/03/16 22:43, Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>         /* allocate a bunch of read buffers and queue them all at once. */
>>         for (i = 0; i < midi->qlen && err == 0; i++) {
>>                 struct usb_request *req =
>> -                       midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +                       midi_alloc_ep_req(midi->out_ep,
>> +                               max_t(unsigned, midi->buflen,
>> +                                       bulk_out_desc.wMaxPacketSize));
>>                 if (req == NULL)
>>                         return -ENOMEM;
>>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?
> 

No, because of the *max_t(unsigned, midi->buflen,
bulk_out_desc.wMaxPacketSize)*.

Maybe that's not the most clear indentation but I had to break it in
order to avoid passing 80 columns.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-09 22:43 ` Steve Calfee
  2016-03-10  9:23   ` Felipe Ferreri Tonello
@ 2016-03-11  8:44   ` Clemens Ladisch
  1 sibling, 0 replies; 14+ messages in thread
From: Clemens Ladisch @ 2016-03-11  8:44 UTC (permalink / raw)
  To: Steve Calfee, Felipe F. Tonello
  Cc: USB list, Kernel development list, Felipe Balbi,
	Michal Nazarewicz, Robert Baldyga

Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> wrote:
>> [...]
>> This patch fixes this problem by setting the minimum usb_request's buffer size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>                 struct usb_request *req =
>> -                       midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +                       midi_alloc_ep_req(midi->out_ep,
>> +                               max_t(unsigned, midi->buflen,
>> +                                       bulk_out_desc.wMaxPacketSize));
>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?

No.  f_midi_handle_out_data() uses only the request buffer.


Regards,
Clemens

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-09 19:39 [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe F. Tonello
  2016-03-09 22:43 ` Steve Calfee
@ 2016-03-11 23:07 ` Michal Nazarewicz
  2016-03-14 16:00   ` Felipe Ferreri Tonello
  2016-03-30 10:51   ` Felipe Balbi
  1 sibling, 2 replies; 14+ messages in thread
From: Michal Nazarewicz @ 2016-03-11 23:07 UTC (permalink / raw)
  To: Felipe F. Tonello, linux-usb; +Cc: linux-kernel, Felipe Balbi, Robert Baldyga

On Wed, Mar 09 2016, Felipe F. Tonello wrote:
> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
> devices.
>
> That caused the OUT endpoint to freeze if the host send any data packet of
> length greater than 256 bytes.
>
> This is an example dump of what happended on that enpoint:
> HOST:   [DATA][Length=260][...]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> ...
> HOST:   [PING]
> DEVICE: [NAK]
>
> This patch fixes this problem by setting the minimum usb_request's buffer size
> for the OUT endpoint as its wMaxPacketSize.
>
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

But see comment below:

> ---
>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 8475e3dc82d4..826ba641f29d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	/* allocate a bunch of read buffers and queue them all at once. */
>  	for (i = 0; i < midi->qlen && err == 0; i++) {
>  		struct usb_request *req =
> -			midi_alloc_ep_req(midi->out_ep, midi->buflen);
> +			midi_alloc_ep_req(midi->out_ep,
> +				max_t(unsigned, midi->buflen,
> +					bulk_out_desc.wMaxPacketSize));

Or, just:

+			midi_alloc_ep_req(midi->out_ep,
+					  bulk_out_desc.wMaxPacketSize);

Packet cannot be greater than wMaxPacketSize so there is no need to
allocate more (if buflen > wMaxPacketSize).

>  		if (req == NULL)
>  			return -ENOMEM;
>  

I’m also wondering whether it would be beneficial to get rid of buflen
all together and use wMaxPacketSie for in endpoints as well?  Is that
feasible?

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-11 23:07 ` Michal Nazarewicz
@ 2016-03-14 16:00   ` Felipe Ferreri Tonello
  2016-03-15  9:48     ` Clemens Ladisch
  2016-03-30 10:51   ` Felipe Balbi
  1 sibling, 1 reply; 14+ messages in thread
From: Felipe Ferreri Tonello @ 2016-03-14 16:00 UTC (permalink / raw)
  To: Michal Nazarewicz, linux-usb; +Cc: linux-kernel, Felipe Balbi, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

Hi Michal,

On 11/03/16 23:07, Michal Nazarewicz wrote:
> On Wed, Mar 09 2016, Felipe F. Tonello wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
> But see comment below:
> 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>  	/* allocate a bunch of read buffers and queue them all at once. */
>>  	for (i = 0; i < midi->qlen && err == 0; i++) {
>>  		struct usb_request *req =
>> -			midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +			midi_alloc_ep_req(midi->out_ep,
>> +				max_t(unsigned, midi->buflen,
>> +					bulk_out_desc.wMaxPacketSize));
> 
> Or, just:
> 
> +			midi_alloc_ep_req(midi->out_ep,
> +					  bulk_out_desc.wMaxPacketSize);
> 
> Packet cannot be greater than wMaxPacketSize so there is no need to
> allocate more (if buflen > wMaxPacketSize).

I didn't know that was a constraint. If so, I agree with you.

> 
>>  		if (req == NULL)
>>  			return -ENOMEM;
>>  
> 
> I’m also wondering whether it would be beneficial to get rid of buflen
> all together and use wMaxPacketSie for in endpoints as well?  Is that
> feasible?

Yes, we could just remove the buflen parameter.

The only scenario where I can see buflen been "useful" is if the user
wants to have a smaller buffer size for the OUT endpoint. Should we
support this case or not?

I can rework this patch for any case we agree on.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-14 16:00   ` Felipe Ferreri Tonello
@ 2016-03-15  9:48     ` Clemens Ladisch
  0 siblings, 0 replies; 14+ messages in thread
From: Clemens Ladisch @ 2016-03-15  9:48 UTC (permalink / raw)
  To: Felipe Ferreri Tonello, Michal Nazarewicz, linux-usb
  Cc: linux-kernel, Felipe Balbi, Robert Baldyga

Felipe Ferreri Tonello wrote:
> On 11/03/16 23:07, Michal Nazarewicz wrote:
>> I’m also wondering whether it would be beneficial to get rid of buflen
>> all together and use wMaxPacketSie for in endpoints as well?  Is that
>> feasible?
>
> Yes, we could just remove the buflen parameter.
>
> The only scenario where I can see buflen been "useful" is if the user
> wants to have a smaller buffer size for the OUT endpoint. Should we
> support this case or not?

Splitting data into multiple packets would not make sense from
a performance perspective.  The only possible reason would be to work
around a (theoretical) bug in some host software that does not handle
larger buffers, but there aren't that many host implementations, and I
am not aware of any with such a bug.


Regards,
Clemens

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-11 23:07 ` Michal Nazarewicz
  2016-03-14 16:00   ` Felipe Ferreri Tonello
@ 2016-03-30 10:51   ` Felipe Balbi
  2016-03-30 12:33     ` Michal Nazarewicz
  1 sibling, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2016-03-30 10:51 UTC (permalink / raw)
  To: Michal Nazarewicz, Felipe F. Tonello, linux-usb
  Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Michal Nazarewicz <mina86@mina86.com> writes:
> [ text/plain ]
> On Wed, Mar 09 2016, Felipe F. Tonello wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>
> But see comment below:
>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>  	/* allocate a bunch of read buffers and queue them all at once. */
>>  	for (i = 0; i < midi->qlen && err == 0; i++) {
>>  		struct usb_request *req =
>> -			midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +			midi_alloc_ep_req(midi->out_ep,
>> +				max_t(unsigned, midi->buflen,
>> +					bulk_out_desc.wMaxPacketSize));
>
> Or, just:
>
> +			midi_alloc_ep_req(midi->out_ep,
> +					  bulk_out_desc.wMaxPacketSize);
>
> Packet cannot be greater than wMaxPacketSize so there is no need to
> allocate more (if buflen > wMaxPacketSize).

a USB packet, right. that's correct. But a struct usb_request can point
to whatever size buffer it wants and UDC is required to split that into
wMaxPacketSize transfers.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-30 10:51   ` Felipe Balbi
@ 2016-03-30 12:33     ` Michal Nazarewicz
  2016-04-01  9:25       ` Felipe Ferreri Tonello
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Nazarewicz @ 2016-03-30 12:33 UTC (permalink / raw)
  To: Felipe Balbi, Felipe F. Tonello, linux-usb; +Cc: linux-kernel, Robert Baldyga

On Wed, Mar 30 2016, Felipe Balbi wrote:
> a USB packet, right. that's correct. But a struct usb_request can
> point to whatever size buffer it wants and UDC is required to split
> that into wMaxPacketSize transfers.

D’oh.  Of course.  Disregard all my comments on the patch (except for
Ack).

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-03-30 12:33     ` Michal Nazarewicz
@ 2016-04-01  9:25       ` Felipe Ferreri Tonello
  2016-04-01 10:22         ` Felipe Balbi
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Ferreri Tonello @ 2016-04-01  9:25 UTC (permalink / raw)
  To: Michal Nazarewicz, Felipe Balbi, linux-usb; +Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

Hi Balbi and Mina,

On 30/03/16 13:33, Michal Nazarewicz wrote:
> On Wed, Mar 30 2016, Felipe Balbi wrote:
>> a USB packet, right. that's correct. But a struct usb_request can
>> point to whatever size buffer it wants and UDC is required to split
>> that into wMaxPacketSize transfers.
> 
> D’oh.  Of course.  Disregard all my comments on the patch (except for
> Ack).
> 

I didn't really get it. Does that mean that if buflen is multiple of
wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
one usb_request and call complete() or it will always call complete() on
each [DATA] packet, thus not requiring buflen at all?

Does that mean that we can still use buflen and this patch is still
valid? (besides the endianess issue that was addressed on v2)

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-04-01  9:25       ` Felipe Ferreri Tonello
@ 2016-04-01 10:22         ` Felipe Balbi
  2016-04-01 14:52           ` Felipe Ferreri Tonello
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2016-04-01 10:22 UTC (permalink / raw)
  To: Felipe Ferreri Tonello, Michal Nazarewicz, linux-usb
  Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
> Hi Balbi and Mina,
>
> On 30/03/16 13:33, Michal Nazarewicz wrote:
>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>> a USB packet, right. that's correct. But a struct usb_request can
>>> point to whatever size buffer it wants and UDC is required to split
>>> that into wMaxPacketSize transfers.
>> 
>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>> Ack).
>> 
>
> I didn't really get it. Does that mean that if buflen is multiple of
> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
> one usb_request and call complete() or it will always call complete() on
> each [DATA] packet, thus not requiring buflen at all?
>
> Does that mean that we can still use buflen and this patch is still
> valid? (besides the endianess issue that was addressed on v2)

if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
e.g. 256 bytes, the UDC controller is required to do whatever it needs
to do to transfer 2048 bytes (or less if there's a short packet).

You don't need to break these 2048 bytes into several requests yourself,
the UDC is required to do that for the gadget.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-04-01 10:22         ` Felipe Balbi
@ 2016-04-01 14:52           ` Felipe Ferreri Tonello
  2016-04-04 10:46             ` Felipe Balbi
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Ferreri Tonello @ 2016-04-01 14:52 UTC (permalink / raw)
  To: Felipe Balbi, Michal Nazarewicz, linux-usb; +Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

Hi Balbi,

On 01/04/16 11:22, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> Hi Balbi and Mina,
>>
>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>> point to whatever size buffer it wants and UDC is required to split
>>>> that into wMaxPacketSize transfers.
>>>
>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>> Ack).
>>>
>>
>> I didn't really get it. Does that mean that if buflen is multiple of
>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>> one usb_request and call complete() or it will always call complete() on
>> each [DATA] packet, thus not requiring buflen at all?
>>
>> Does that mean that we can still use buflen and this patch is still
>> valid? (besides the endianess issue that was addressed on v2)
> 
> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
> e.g. 256 bytes, the UDC controller is required to do whatever it needs
> to do to transfer 2048 bytes (or less if there's a short packet).
> 
> You don't need to break these 2048 bytes into several requests yourself,
> the UDC is required to do that for the gadget.

Right, what about OUT endpoints?

So that means that buflen is still usable, at least on IN endpoints.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-04-01 14:52           ` Felipe Ferreri Tonello
@ 2016-04-04 10:46             ` Felipe Balbi
  2016-04-04 11:33               ` Felipe Ferreri Tonello
  0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2016-04-04 10:46 UTC (permalink / raw)
  To: Felipe Ferreri Tonello, Michal Nazarewicz, linux-usb
  Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>>> point to whatever size buffer it wants and UDC is required to split
>>>>> that into wMaxPacketSize transfers.
>>>>
>>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>>> Ack).
>>>>
>>>
>>> I didn't really get it. Does that mean that if buflen is multiple of
>>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>>> one usb_request and call complete() or it will always call complete() on
>>> each [DATA] packet, thus not requiring buflen at all?
>>>
>>> Does that mean that we can still use buflen and this patch is still
>>> valid? (besides the endianess issue that was addressed on v2)
>> 
>> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
>> e.g. 256 bytes, the UDC controller is required to do whatever it needs
>> to do to transfer 2048 bytes (or less if there's a short packet).
>> 
>> You don't need to break these 2048 bytes into several requests yourself,
>> the UDC is required to do that for the gadget.
>
> Right, what about OUT endpoints?

also applicable

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
  2016-04-04 10:46             ` Felipe Balbi
@ 2016-04-04 11:33               ` Felipe Ferreri Tonello
  0 siblings, 0 replies; 14+ messages in thread
From: Felipe Ferreri Tonello @ 2016-04-04 11:33 UTC (permalink / raw)
  To: Felipe Balbi, Michal Nazarewicz, linux-usb; +Cc: linux-kernel, Robert Baldyga

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Hi Balbi,

On 04/04/16 11:46, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>>>> point to whatever size buffer it wants and UDC is required to split
>>>>>> that into wMaxPacketSize transfers.
>>>>>
>>>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>>>> Ack).
>>>>>
>>>>
>>>> I didn't really get it. Does that mean that if buflen is multiple of
>>>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>>>> one usb_request and call complete() or it will always call complete() on
>>>> each [DATA] packet, thus not requiring buflen at all?
>>>>
>>>> Does that mean that we can still use buflen and this patch is still
>>>> valid? (besides the endianess issue that was addressed on v2)
>>>
>>> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
>>> e.g. 256 bytes, the UDC controller is required to do whatever it needs
>>> to do to transfer 2048 bytes (or less if there's a short packet).
>>>
>>> You don't need to break these 2048 bytes into several requests yourself,
>>> the UDC is required to do that for the gadget.
>>
>> Right, what about OUT endpoints?
> 
> also applicable
> 

Ok, so I will make few tests here and resend a v3 probably with buflen
still.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-04-04 11:30 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-09 19:39 [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe F. Tonello
2016-03-09 22:43 ` Steve Calfee
2016-03-10  9:23   ` Felipe Ferreri Tonello
2016-03-11  8:44   ` Clemens Ladisch
2016-03-11 23:07 ` Michal Nazarewicz
2016-03-14 16:00   ` Felipe Ferreri Tonello
2016-03-15  9:48     ` Clemens Ladisch
2016-03-30 10:51   ` Felipe Balbi
2016-03-30 12:33     ` Michal Nazarewicz
2016-04-01  9:25       ` Felipe Ferreri Tonello
2016-04-01 10:22         ` Felipe Balbi
2016-04-01 14:52           ` Felipe Ferreri Tonello
2016-04-04 10:46             ` Felipe Balbi
2016-04-04 11:33               ` Felipe Ferreri Tonello

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).