linux-kernel - Re: [PATCH v3 1/4] usb: gadget: f_midi: free usb request when done

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGrhNMygOfcip5nY92CxDL875hyeo2FJZZtiQTmOOBTGCH0g5w@mail.gmail.com>
Date:	Mon, 12 Oct 2015 10:46:07 +0100
From:	Felipe Tonello <eu@...ipetonello.com>
To:	Clemens Ladisch <clemens@...isch.de>
Cc:	USB list <linux-usb@...r.kernel.org>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	Peter Chen <Peter.Chen@...escale.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Felipe Balbi <balbi@...com>,
	Andrzej Pietrasiewicz <andrzej.p@...sung.com>
Subject: Re: [PATCH v3 1/4] usb: gadget: f_midi: free usb request when done

Hi Clemens

On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@...isch.de> wrote:
> Felipe Tonello wrote:
>> req->actual == req->length means that there is no data left to enqueue,
>
> This condition is not checked in the patch.
>
>> so free the request.
>>
>> Signed-off-by: Felipe F. Tonello <eu@...ipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..93212ca 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -256,9 +256,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>>                       /* We received stuff. req is queued again, below */
>>                       f_midi_handle_out_data(ep, req);
>>               } else if (ep == midi->in_ep) {
>> -                     /* Our transmit completed. See if there's more to go.
>> -                      * f_midi_transmit eats req, don't queue it again. */
>> -                     f_midi_transmit(midi, req);
>> +                     /* Our transmit completed. Don't queue it again. */
>> +                     free_ep_req(ep, req);
>>                       return;
>>               }
>>               break;
>
> The ALSA framework will give you a notification _once_.  If the
> resulting data is larger than midi->buflen, then you have to continue
> sending packets.  This is exactly what the call to f_midi_transmit()
> does.

That's true. But what it will also happen is that f_midi_transmit()
will potentially eat up data from an unrelated ALSA trigger.
In the end it is all fine, because the function will realise the
request->len == 0 so it will free the request. But logically speaking
it is incorrect and error prone.

>
> (To decrease latency, it might be a good idea to queue multiple requests,
> but you wouldn't want to continue piling up requests if the host isn't
> listening.  sound/usb/midi.c just allocates a fixed number of requests,
> and always reuses them.)

I believe that is the best way to implement. Create multiple requests
until the ALSA substreams buffer are empty and free the request on
completion.

The problem of having requests when host isn't listening will happen
anyway because there is no way to know that until completion.

What do you think?

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/