Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1367093 > unrolled thread

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe Balbi <balbi@kernel.org> - 2016-03-30 13:00 +0200
    Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Michal Nazarewicz <mina86@mina86.com> - 2016-03-30 14:40 +0200
      Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller  than wMaxPacketSize Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-04-01 11:30 +0200
        Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe Balbi <balbi@kernel.org> - 2016-04-01 12:30 +0200
          Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller  than wMaxPacketSize Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-04-01 17:00 +0200

#1367093 — Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1367173

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»

[toc] | [prev] | [next] | [standalone]

#1369124 — Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1369166

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1369401 — Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web