Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize

[Felipe Balbi <balbi@kernel.org>]

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


Hi again,

Felipe Balbi <balbi@kernel.org> writes:
> Michal Nazarewicz <mina86@mina86.com> writes:
>> On Fri, Jul 08 2016, Baolin Wang wrote:
>>> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@mina86.com> wrote:
>>>> On Thu, Jul 07 2016, Baolin Wang wrote:
>>>>> Some gadget device (such as dwc3 gadget) requires quirk_ep_out_aligned_size
>>>>> attribute, which means it need to align the request buffer's size to an ep's
>>>>> maxpacketsize.
>>>>>
>>>>> Thus we add usb_ep_align_maybe() function to check if it is need to align
>>>>> the request buffer's size to an ep's maxpacketsize.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>
>>>> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>>>>
>>>>> ---
>>>>>  drivers/usb/gadget/function/f_midi.c |   18 +++++++++++-------
>>>>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>>> index 58fc199..2e3f11e 100644
>>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>>> @@ -328,7 +328,7 @@ static int f_midi_start_ep(struct f_midi *midi,
>>>>>  static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>>>  {
>>>>>       struct f_midi *midi = func_to_midi(f);
>>>>> -     unsigned i;
>>>>> +     unsigned i, length;
>>>>>       int err;
>>>>>
>>>>>       /* we only set alt for MIDIStreaming interface */
>>>>> @@ -345,9 +345,11 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>>>
>>>>>       /* pre-allocate write usb requests to use on f_midi_transmit. */
>>>>>       while (kfifo_avail(&midi->in_req_fifo)) {
>>>>> -             struct usb_request *req =
>>>>> -                     midi_alloc_ep_req(midi->in_ep, midi->buflen);
>>>>> +             struct usb_request *req;
>>>>>
>>>>> +             length = usb_ep_align_maybe(midi->gadget, midi->in_ep,
>>>>> +                                         midi->buflen);
>>>>> +             req = midi_alloc_ep_req(midi->in_ep, length);
>>>>>               if (req == NULL)
>>>>>                       return -ENOMEM;
>>>>>
>>>>> @@ -359,10 +361,12 @@ 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,
>>>>> -                             max_t(unsigned, midi->buflen,
>>>>> -                                     bulk_out_desc.wMaxPacketSize));
>>>>> +             struct usb_request *req;
>>>>> +
>>>>> +             length = usb_ep_align_maybe(midi->gadget, midi->out_ep,
>>>>> +                                         midi->buflen);
>>>>> +             req = midi_alloc_ep_req(midi->out_ep,
>>>>> +                     max_t(unsigned, length, bulk_out_desc.wMaxPacketSize));
>>>>
>>>> Perhaps:
>>>>
>>>> +               length = midi->buflen < bulk_out_desc.wMaxPacketSize
>>>> +                       ? bulk_out_desc.wMaxPacketSize
>>>> +                       : usb_ep_align_maybe(midi->gadget, midi->out_ep,
>>>> +                                            midi->buflen);
>>>> +               req = midi_alloc_ep_req(midi->out_ep, length);
>>>>
>>>> I find it somewhat cleaner.  Up to you.
>>>
>>> But if the gadget does not requires 'quirk_ep_out_aligned_size', then
>>> we also can keep midi->buflen length although midi->buflen <
>>> bulk_out_desc.wMaxPacketSize, right? Thanks for your comment.
>>
>> I don’t know.  That’s not what the original code was doing.  The
>> original code was using:
>>
>>     max_t(unsigned, midi->buflen, bulk_out_desc.wMaxPacketSize));
>>
>> for some reason.>
>
> My take on this is that it's calling max_t() to try and align to
> wMaxPacketSize. We can see from original commit what was the intent:
>
> commit 03d27ade4941076b34c823d63d91dc895731a595
> Author: Felipe F. Tonello <eu@felipetonello.com>
> Date:   Wed Mar 9 19:39:30 2016 +0000
>
>     usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
>     
>     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.
>     
>     Acked-by: Michal Nazarewicz <mina86@mina86.com>
>     Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde99b03..9ad51dcab982 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -360,7 +360,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;
>
> Seems to me usb_ep_align_maybe() would cover this case just as well. But
> then, Felipe's UDC driver seems to need quirk_ep_out_aligned_size. Felipe?

another way to look at this is that buflen < wMaxPacketSize for bulk
endpoints is kinda weird, so we might just go ahead and allocate buflen
aligned to wMaxPacketSize for OUT eps.

-- 
balbi

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