Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1438431 > unrolled thread
| Started by | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| First post | 2016-07-07 11:20 +0200 |
| Last post | 2016-07-08 16:10 +0200 |
| Articles | 9 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Baolin Wang <baolin.wang@linaro.org> - 2016-07-07 11:20 +0200
Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Michal Nazarewicz <mina86@mina86.com> - 2016-07-07 15:00 +0200
Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Baolin Wang <baolin.wang@linaro.org> - 2016-07-08 04:30 +0200
Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Michal Nazarewicz <mina86@mina86.com> - 2016-07-08 15:10 +0200
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> - 2016-07-08 15:30 +0200
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> - 2016-07-08 15:30 +0200
Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Michal Nazarewicz <mina86@mina86.com> - 2016-07-08 16:10 +0200
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> - 2016-07-08 15:30 +0200
Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Michal Nazarewicz <mina86@mina86.com> - 2016-07-08 16:10 +0200
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-07-07 11:20 +0200 |
| Subject | [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize |
| Message-ID | <rSdDr-4Xv-7@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2016-07-07 15:00 +0200 |
| Message-ID | <rSh4m-71b-39@gated-at.bofh.it> |
| In reply to | #1438431 |
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»
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-07-08 04:30 +0200 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize |
| Message-ID | <rStIe-6ZF-7@gated-at.bofh.it> |
| In reply to | #1438596 |
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
[toc] | [prev] | [next] | [standalone]
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2016-07-08 15:10 +0200 |
| Message-ID | <rSDHz-5cy-9@gated-at.bofh.it> |
| In reply to | #1439070 |
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»
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-07-08 15:30 +0200 |
| Message-ID | <rSE0V-5l8-7@gated-at.bofh.it> |
| In reply to | #1439433 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-07-08 15:30 +0200 |
| Message-ID | <rSE0V-5l8-9@gated-at.bofh.it> |
| In reply to | #1439433 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2016-07-08 16:10 +0200 |
| Message-ID | <rSEDF-5P7-51@gated-at.bofh.it> |
| In reply to | #1439448 |
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»
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-07-08 15:30 +0200 |
| Message-ID | <rSE0V-5l8-11@gated-at.bofh.it> |
| In reply to | #1438431 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2016-07-08 16:10 +0200 |
| Message-ID | <rSEDE-5P7-33@gated-at.bofh.it> |
| In reply to | #1439444 |
> 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»
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web