Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1354385 > unrolled thread
| Started by | "Felipe F. Tonello" <eu@felipetonello.com> |
|---|---|
| First post | 2016-03-09 20:40 +0100 |
| Last post | 2016-03-15 10:50 +0100 |
| Articles | 7 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-09 20:40 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Steve Calfee <stevecalfee@gmail.com> - 2016-03-09 23:50 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-10 10:30 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Clemens Ladisch <clemens@ladisch.de> - 2016-03-11 09:50 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Michal Nazarewicz <mina86@mina86.com> - 2016-03-12 00:10 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-14 17:00 +0100
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize Clemens Ladisch <clemens@ladisch.de> - 2016-03-15 10:50 +0100
| From | "Felipe F. Tonello" <eu@felipetonello.com> |
|---|---|
| Date | 2016-03-09 20:40 +0100 |
| Subject | [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <raS7E-44y-5@gated-at.bofh.it> |
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>
---
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));
if (req == NULL)
return -ENOMEM;
--
2.7.2
[toc] | [next] | [standalone]
| From | Steve Calfee <stevecalfee@gmail.com> |
|---|---|
| Date | 2016-03-09 23:50 +0100 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <raV5w-6jx-11@gated-at.bofh.it> |
| In reply to | #1354385 |
On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> 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>
> ---
> 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));
> if (req == NULL)
> return -ENOMEM;
>
Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?
Regards, Steve
[toc] | [prev] | [next] | [standalone]
| From | Felipe Ferreri Tonello <eu@felipetonello.com> |
|---|---|
| Date | 2016-03-10 10:30 +0100 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <rb54S-4Pn-5@gated-at.bofh.it> |
| In reply to | #1354537 |
[Multipart message — attachments visible in raw view] — view raw
Hi Steve,
On 09/03/16 22:43, Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> 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>
>> ---
>> 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));
>> if (req == NULL)
>> return -ENOMEM;
>>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?
>
No, because of the *max_t(unsigned, midi->buflen,
bulk_out_desc.wMaxPacketSize)*.
Maybe that's not the most clear indentation but I had to break it in
order to avoid passing 80 columns.
--
Felipe
[toc] | [prev] | [next] | [standalone]
| From | Clemens Ladisch <clemens@ladisch.de> |
|---|---|
| Date | 2016-03-11 09:50 +0100 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <rbqVK-3vt-41@gated-at.bofh.it> |
| In reply to | #1354537 |
Steve Calfee wrote: > On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello <eu@felipetonello.com> wrote: >> [...] >> This patch fixes this problem by setting the minimum usb_request's buffer size >> for the OUT endpoint as its wMaxPacketSize. >> >> --- 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) >> 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)); > > Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize? No. f_midi_handle_out_data() uses only the request buffer. Regards, Clemens
[toc] | [prev] | [next] | [standalone]
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2016-03-12 00:10 +0100 |
| Message-ID | <rbElZ-54U-51@gated-at.bofh.it> |
| In reply to | #1354385 |
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).
> if (req == NULL)
> return -ENOMEM;
>
I’m also wondering whether it would be beneficial to get rid of buflen
all together and use wMaxPacketSie for in endpoints as well? Is that
feasible?
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
[toc] | [prev] | [next] | [standalone]
| From | Felipe Ferreri Tonello <eu@felipetonello.com> |
|---|---|
| Date | 2016-03-14 17:00 +0100 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <rcD4v-5Mh-23@gated-at.bofh.it> |
| In reply to | #1356277 |
[Multipart message — attachments visible in raw view] — view raw
Hi Michal,
On 11/03/16 23:07, Michal Nazarewicz wrote:
> 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).
I didn't know that was a constraint. If so, I agree with you.
>
>> if (req == NULL)
>> return -ENOMEM;
>>
>
> I’m also wondering whether it would be beneficial to get rid of buflen
> all together and use wMaxPacketSie for in endpoints as well? Is that
> feasible?
Yes, we could just remove the buflen parameter.
The only scenario where I can see buflen been "useful" is if the user
wants to have a smaller buffer size for the OUT endpoint. Should we
support this case or not?
I can rework this patch for any case we agree on.
--
Felipe
[toc] | [prev] | [next] | [standalone]
| From | Clemens Ladisch <clemens@ladisch.de> |
|---|---|
| Date | 2016-03-15 10:50 +0100 |
| Subject | Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize |
| Message-ID | <rcTLY-fp-5@gated-at.bofh.it> |
| In reply to | #1357387 |
Felipe Ferreri Tonello wrote: > On 11/03/16 23:07, Michal Nazarewicz wrote: >> I’m also wondering whether it would be beneficial to get rid of buflen >> all together and use wMaxPacketSie for in endpoints as well? Is that >> feasible? > > Yes, we could just remove the buflen parameter. > > The only scenario where I can see buflen been "useful" is if the user > wants to have a smaller buffer size for the OUT endpoint. Should we > support this case or not? Splitting data into multiple packets would not make sense from a performance perspective. The only possible reason would be to work around a (theoretical) bug in some host software that does not handle larger buffers, but there aren't that many host implementations, and I am not aware of any with such a bug. Regards, Clemens
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web