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

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

[Baolin Wang]

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>
---
 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));
 		if (req == NULL)
 			return -ENOMEM;
 
-- 
1.7.9.5

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

[Michal Nazarewicz]

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.

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

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

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

[Baolin Wang]

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.

>
>>               if (req == NULL)
>>                       return -ENOMEM;
>>
>
> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»



-- 
Baolin.wang
Best Regards

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

[Michal Nazarewicz]

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.>

>>>               if (req == NULL)
>>>                       return -ENOMEM;
>>>
>>
>> --
>> Best regards
>> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
>> «If at first you don’t succeed, give up skydiving»
>
>
>
> -- 
> Baolin.wang
> Best Regards

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> 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>
> ---
>  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);

this is not needed for in_ep, only out_ep, right?

> +		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);

after calling usb_ep_align_maybe()...

> +		req = midi_alloc_ep_req(midi->out_ep,
> +			max_t(unsigned, length, bulk_out_desc.wMaxPacketSize));

... max_t() is pointless. length will *always* >= wMaxPacketSize.

-- 
balbi

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

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

[Felipe Balbi]

[-- 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 --]

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

[Felipe Balbi]

[-- 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 --]

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

[Michal Nazarewicz]

> Baolin Wang <baolin.wang@linaro.org> writes:
>> @@ -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);

On Fri, Jul 08 2016, Felipe Balbi wrote:
> after calling usb_ep_align_maybe()...
>
>> +		req = midi_alloc_ep_req(midi->out_ep,
>> +			max_t(unsigned, length, bulk_out_desc.wMaxPacketSize));
>
> ... max_t() is pointless. length will *always* >= wMaxPacketSize.

That is only true for gadgets with the quirk.  usb_ep_align_maybe is
a noöp for gadgets without the quirk.

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

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

[Michal Nazarewicz]

On Fri, Jul 08 2016, Felipe Balbi wrote:
> 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?

In that case, I agree that max is likely unnecessary.

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

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

[Baolin Wang]

On 8 July 2016 at 21:21, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> 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>
>> ---
>>  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);
>
> this is not needed for in_ep, only out_ep, right?

Yes. You are right.

>
>> +             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);
>
> after calling usb_ep_align_maybe()...
>
>> +             req = midi_alloc_ep_req(midi->out_ep,
>> +                     max_t(unsigned, length, bulk_out_desc.wMaxPacketSize));
>
> ... max_t() is pointless. length will *always* >= wMaxPacketSize.

It is not. If the gadget device dose not require
quirk_ep_out_aligned_size, then the lengh may be <= wMaxPacketSize.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 8 July 2016 at 21:27, Felipe Balbi <balbi@kernel.org> wrote:
>
> 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.

OK. So I will accept Michal's suggestion as below:

 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);

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 8 July 2016 at 21:04, Michal Nazarewicz <mina86@mina86.com> wrote:
> 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));

OK.

>
> for some reason.>
>
>>>>               if (req == NULL)
>>>>                       return -ENOMEM;
>>>>
>>>
>>> --
>>> Best regards
>>> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
>>> «If at first you don’t succeed, give up skydiving»
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards
>
> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»



-- 
Baolin.wang
Best Regards