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: 5778 bytes --]


Hi,

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?

-- 
balbi

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