| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87inwgi7nh.fsf@linux.intel.com>
Date: Fri, 08 Jul 2016 16:25:22 +0300
From: Felipe Balbi <balbi@...nel.org>
To: Michal Nazarewicz <mina86@...a86.com>,
Baolin Wang <baolin.wang@...aro.org>,
Felipe Ferreri Tonello <eu@...ipetonello.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, eu@...ipetonello.com,
dan.carpenter@...cle.com, USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize
Hi,
Michal Nazarewicz <mina86@...a86.com> writes:
> On Fri, Jul 08 2016, Baolin Wang wrote:
>> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@...a86.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@...aro.org>
>>>
>>> Acked-by: Michal Nazarewicz <mina86@...a86.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@...ipetonello.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@...a86.com>
Signed-off-by: Felipe F. Tonello <eu@...ipetonello.com>
Signed-off-by: Felipe Balbi <felipe.balbi@...ux.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
Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)
Powered by blists - more mailing lists