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: <CAGrhNMyMbyojdsFoUcx6NVad5Amc5VXf41YPDmDckOwS5cug-g@mail.gmail.com>
Date:	Mon, 12 Oct 2015 12:58:30 +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 Mon, Oct 12, 2015 at 11:16 AM, Clemens Ladisch <clemens@...isch.de> wrote:
> Felipe Tonello wrote:
>> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@...isch.de> wrote:
>>> Felipe Tonello wrote:
>>>>               } 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.
>
> The triggers are for some MIDI port of the _same_ USB endpoint, so
> they're not unrelated.  f_midi_transmit() collects data from all ports
> anyway; separating the data from different ports into different USB
> packets would just needlessly introduce additional latency.

Ok.

>
>> 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.
>
> It is _not_ incorrect if you realize that f_midi_transmit() applies to
> the endpoint, not to any particular port.
>
>>> (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.
>
> I believe a better way to implement this is to allocate a fixed number
> of requests, and to always reuse them.

How many?

>
>> The problem of having requests when host isn't listening will happen
>> anyway because there is no way to know that until completion.
>
> But if you have no upper limit on the number of queues requests, you
> will eventually run out of (DMA) memory.

And that's what happening at the moment. One of my patches are to fix
a memory leak when that happens.

But it would be ideal to have a FIFO of requests and perhaps ignore
new requests if the FIFO is full.

So, allocate (pre-allocate?) new requests until the FIFO is full. Upon
completion, remove the request from FIFO, but still reuse it on
f_midi_transmit() and queue it on the FIFO again if there is still
data from ALSA, otherwise just free the request.

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/