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

[Felipe Tonello]

Hi Clemens

On Mon, Oct 12, 2015 at 11:16 AM, Clemens Ladisch <clemens@xxxxxxxxxx> wrote:
> Felipe Tonello wrote:
>> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/