LinuxLists.cc - [PATCH v5 0/7] USB MIDI Gadget improvements and bug fixes

2015-11-16 11:08:35

by Felipe Ferreri Tonello

[permalink] [raw]

Subject: Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

Hi Robert,

On 13/11/15 08:31, Robert Baldyga wrote:
> Hi Felipe,
>
> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>> the request. If that happens the complete function will never be called, thus
>> never freeing the request.
>>
>> Signed-off-by: Felipe F. Tonello <[email protected]>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index f36db2d..76ea53c 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>> if (err) {
>> ERROR(midi, "%s queue req: %d\n",
>> midi->out_ep->name, err);
>> + free_ep_req(midi->out_ep, req);
>> }
>> }
>>
>>
>
> There is one more thing I haven't noticed before. We can have situation
> when all requests were allocated successfully, but their allocation
> failed. What we get then is set_alt() returning 0, while no request is
> allocated, hence the function is, in fact, inactive.

Right. So in this case should we return some error? We can restrict the
function to work iff allocates the 'qlen' number of allocations,
otherwise returns an error and frees all other requests (IN and OUT).

--
Felipe

Attachments:

0x92698E6A.asc (4.72 kB)

2015-11-16 11:41:23

Hi Robert,

On 16/11/15 11:43, Robert Baldyga wrote:
> On 11/16/2015 12:08 PM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 13/11/15 08:31, Robert Baldyga wrote:
>>> Hi Felipe,
>>>
>>> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>>>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>>>> the request. If that happens the complete function will never be called, thus
>>>> never freeing the request.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>>> ---
>>>> drivers/usb/gadget/function/f_midi.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index f36db2d..76ea53c 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>> if (err) {
>>>> ERROR(midi, "%s queue req: %d\n",
>>>> midi->out_ep->name, err);
>>>> + free_ep_req(midi->out_ep, req);
>>>> }
>>>> }
>>>>
>>>>
>>>
>>> There is one more thing I haven't noticed before. We can have situation
>>> when all requests were allocated successfully, but their allocation
>>> failed. What we get then is set_alt() returning 0, while no request is
>>> allocated, hence the function is, in fact, inactive.
>>
>> Right. So in this case should we return some error? We can restrict the
>> function to work iff allocates the 'qlen' number of allocations,
>> otherwise returns an error and frees all other requests (IN and OUT).
>>
>
> Yes, IMO it's a proper solution. When user sets qlen to given value he
> expects that exact number of requests to be allocated and enqueued, and
> if we cannot do that we should consider this as an error.
>

Ok. I will do that in a new patch on v6 since this patch is correct
anyway and Blabi already applied to his test branch.

Felipe

Attachments:

0x92698E6A.asc (4.72 kB)

2015-11-25 16:54:16

by Felipe Ferreri Tonello

[permalink] [raw]

Subject: Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

Hi Robert,

On 13/11/15 12:38, Robert Baldyga wrote:
> Hi Felipe,
>
> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>> This patch introduces pre-allocation of IN endpoint USB requests. This
>> improves on latency (requires no usb request allocation on transmit) and avoid
>> several potential probles on allocating too many usb requests (which involves
>> DMA pool allocation problems).
>>
>> This implementation also handles better multiple MIDI Gadget ports, always
>> processing the last processed MIDI substream if the last USB request wasn't
>> enought to handle the whole stream.
>>
>> Signed-off-by: Felipe F. Tonello <[email protected]>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 174 +++++++++++++++++++++++++++--------
>> drivers/usb/gadget/legacy/gmidi.c | 2 +-
>> 2 files changed, 137 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 615d632..6ac39f6 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -23,6 +23,7 @@
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> +#include <linux/kfifo.h>
>>
>> #include <sound/core.h>
>> #include <sound/initval.h>
>> @@ -88,6 +89,9 @@ struct f_midi {
>> int index;
>> char *id;
>> unsigned int buflen, qlen;
>> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> + unsigned int in_req_num;
>> + unsigned int in_last_port;
>> };
>>
>> static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct usb_function *f)
>> return container_of(f, struct f_midi, func);
>> }
>>
>> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
>> +static void f_midi_transmit(struct f_midi *midi);
>>
>> DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>> DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
>> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *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);
>> + req->length = 0;
>> + f_midi_transmit(midi);
>> return;
>> }
>> break;
>> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>> case -ESHUTDOWN: /* disconnect from host */
>> VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>> req->actual, req->length);
>> - if (ep == midi->out_ep)
>> + if (ep == midi->out_ep) {
>> f_midi_handle_out_data(ep, req);
>> -
>> - free_ep_req(ep, req);
>> + /* We don't need to free IN requests because it's handled
>> + * by the midi->in_req_fifo. */
>> + free_ep_req(ep, req);
>> + }
>> return;
>>
>> case -EOVERFLOW: /* buffer overrun on read means that
>> @@ -334,6 +341,21 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>> if (err)
>> return err;
>>
>> + /* 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);
>> +
>> + if (req == NULL)
>> + return -ENOMEM;
>
> We need to free previously allocated requests in case of failure.
>
>> +
>> + req->length = 0;
>> + req->complete = f_midi_complete;
>> +
>> + kfifo_put(&midi->in_req_fifo, req);
>> + midi->in_req_num++;
>> + }
>> +
>> /* 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 =
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> */
>> usb_ep_disable(midi->in_ep);
>> usb_ep_disable(midi->out_ep);
>> +
>> + /* release IN requests */
>> + while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> + struct usb_request *req = NULL;
>> + unsigned int len;
>> +
>> + len = kfifo_get(&midi->in_req_fifo, &req);
>> + if (len == 1)
>> + free_ep_req(midi->in_ep, req);
>> + else
>> + ERROR(midi, "%s couldn't free usb request: something went wrong with kfifo\n",
>> + midi->in_ep->name);
>> + }
>> + midi->in_req_num = 0;
>> }
>>
>> static int f_midi_snd_free(struct snd_device *device)
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
>> }
>> }
>>
>> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>> {
>> - struct usb_ep *ep = midi->in_ep;
>> - int i;
>> -
>> - if (!ep)
>> - return;
>> -
>> - if (!req)
>> - req = midi_alloc_ep_req(ep, midi->buflen);
>> -
>> - if (!req) {
>> - ERROR(midi, "%s: alloc_ep_request failed\n", __func__);
>> - return;
>> - }
>> - req->length = 0;
>> - req->complete = f_midi_complete;
>> + unsigned int i;
>>
>> for (i = 0; i < MAX_PORTS; i++) {
>> struct gmidi_in_port *port = midi->in_port[i];
>> struct snd_rawmidi_substream *substream = midi->in_substream[i];
>>
>> - if (!port || !port->active || !substream)
>> + if (!port)
>> + break;
>> +
>> + if (!port->active || !substream)
>> continue;
>>
>> - while (req->length + 3 < midi->buflen) {
>> - uint8_t b;
>> - if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
>> - port->active = 0;
>> + snd_rawmidi_drop_output(substream);
>> + }
>> +}
>> +
>> +static void f_midi_transmit(struct f_midi *midi)
>> +{
>> + struct usb_ep *ep = midi->in_ep;
>> + bool active;
>> +
>> + /* We only care about USB requests if IN endpoint is enabled */
>> + if (!ep || !ep->enabled)
>> + goto drop_out;
>> +
>> + do {
>> + struct usb_request *req = NULL;
>> + unsigned int len, i;
>> +
>> + active = false;
>> +
>> + /* We peek the request in order to reuse it if it fails
>> + * to enqueue on its endpoint */
>> + len = kfifo_peek(&midi->in_req_fifo, &req);
>> + if (len != 1) {
>> + ERROR(midi, "%s: Couldn't get usb request\n", __func__);
>> + goto drop_out;
>> + }
>> +
>> + if (req->length > 0) {
>> + WARNING(midi, "%s: All USB requests have been used. Current queue size "
>> + "is %u, consider increasing it.\n", __func__, midi->in_req_num);
>> + goto drop_out;
>> + }
>> +
>> + for (i = midi->in_last_port; i < MAX_PORTS; i++) {
>> + struct gmidi_in_port *port = midi->in_port[i];
>> + struct snd_rawmidi_substream *substream = midi->in_substream[i];
>> +
>> + if (!port) {
>> + /* Reset counter when we reach the last available port */
>> + midi->in_last_port = 0;
>> + break;
>> + }
>> +
>> + if (!port->active || !substream)
>> + continue;
>> +
>> + while (req->length + 3 < midi->buflen) {
>> + uint8_t b;
>> +
>> + if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
>> + port->active = 0;
>> + break;
>> + }
>> + f_midi_transmit_byte(req, port, b);
>> + }
>> +
>> + active = !!port->active;
>> + /* Check if last port is still active, which means that
>> + * there is still data on that substream but this current
>> + * request run out of space. */
>> + if (active) {
>> + midi->in_last_port = i;
>> + /* There is no need to re-iterate though midi ports. */
>> break;
>> }
>> - f_midi_transmit_byte(req, port, b);
>> }
>> - }
>>
>> - if (req->length > 0 && ep->enabled) {
>> - int err;
>> + if (req->length > 0) {
>> + int err;
>>
>> - err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> - if (err < 0)
>> - ERROR(midi, "%s queue req: %d\n",
>> - midi->in_ep->name, err);
>> - } else {
>> - free_ep_req(ep, req);
>> - }
>> + err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> + if (err < 0) {
>> + ERROR(midi, "%s failed to queue req: %d\n",
>> + midi->in_ep->name, err);
>> + req->length = 0; /* Re-use request next time. */
>> + } else {
>> + /* Upon success, put request at the back of the queue. */
>> + kfifo_skip(&midi->in_req_fifo);
>> + kfifo_put(&midi->in_req_fifo, req);
>
> There are simpler and clearer ways to implement cyclic buffer than using
> kfifo. To be honest, it took ma a while to realize what's going on. As
> long as you mark unused requests by setting req->length to zero you only
> need to store index of last used req to be able to achieve the same effect.

That is true but that's exactly what I am trying to avoid here. I didn't
want to add a counter and deal with counter++ and counter-- all the
time. kfifo is fast and has a clean and nice interface to deal with that.

I can write a comment right next to it just to make things clearer.

>
>> + }
>> + }
>> + } while (active);
>> +
>> +drop_out:
>> + f_midi_drop_out_substreams(midi);
>> }
>>
>> static void f_midi_in_tasklet(unsigned long data)
>> {
>> struct f_midi *midi = (struct f_midi *) data;
>> - f_midi_transmit(midi, NULL);
>> + f_midi_transmit(midi);
>> }
>>
>> static int f_midi_in_open(struct snd_rawmidi_substream *substream)
>> @@ -663,6 +753,7 @@ static int f_midi_register_card(struct f_midi *midi)
>> goto fail;
>> }
>> midi->rmidi = rmidi;
>> + midi->in_last_port = 0;
>> strcpy(rmidi->name, card->shortname);
>> rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
>> SNDRV_RAWMIDI_INFO_INPUT |
>> @@ -1057,6 +1148,7 @@ static void f_midi_free(struct usb_function *f)
>> mutex_lock(&opts->lock);
>> for (i = opts->in_ports - 1; i >= 0; --i)
>> kfree(midi->in_port[i]);
>> + kfifo_free(&midi->in_req_fifo);
>> kfree(midi);
>> --opts->refcnt;
>> mutex_unlock(&opts->lock);
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>> midi->index = opts->index;
>> midi->buflen = opts->buflen;
>> midi->qlen = opts->qlen;
>> + midi->in_req_num = 0;
>> + midi->in_last_port = 0;
>> +
>> + if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> + goto setup_fail;
>> +
>> ++opts->refcnt;
>> mutex_unlock(&opts->lock);
>>
>> diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
>> index be8e91d..f68c188 100644
>> --- a/drivers/usb/gadget/legacy/gmidi.c
>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>> @@ -53,7 +53,7 @@ MODULE_PARM_DESC(buflen, "MIDI buffer length");
>>
>> static unsigned int qlen = 32;
>> module_param(qlen, uint, S_IRUGO);
>> -MODULE_PARM_DESC(qlen, "USB read request queue length");
>> +MODULE_PARM_DESC(qlen, "USB read and write request queue length");
>>
>> static unsigned int in_ports = 1;
>> module_param(in_ports, uint, S_IRUGO);
>>
>
> Best regards,
> Robert
>

Felipe

Attachments:

0x92698E6A.asc (4.72 kB)

2015-11-25 17:23:15

Hi Clemens

On 27/11/15 09:05, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 13/11/15 08:55, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> +static void f_midi_transmit(struct f_midi *midi)
>>>> +{
>>>> +...
>>>> + len = kfifo_peek(&midi->in_req_fifo, &req);
>>>> + ...
>>>> + if (req->length > 0) {
>>>> + WARNING(midi, "%s: All USB requests have been used. Current queue size "
>>>> + "is %u, consider increasing it.\n", __func__, midi->in_req_num);
>>>> + goto drop_out;
>>>> + }
>>>
>>> There are two cases where the in_req FIFO might overflow:
>>> 1) the gadget is trying to send too much data at once; or
>>> 2) the host does not bother to read any of the data.
>>>
>>> In case 1), the appropriate action would be to do nothing, so that the
>>> remaining data is sent after some currently queued packets have been
>>> transmitted. In case 2), the appropriate action would be to drop the
>>> data (even better, the _oldest_ data), and spamming the log with error
>>> messages would not help.
>>
>> True. In this case the log will be spammed.
>>
>> How would you suggest to drop the oldest data? That doesn't really seem
>> to be feasible.
>
> There is usb_ep_dequeue(). Its documentation warns about some hardware,
> but it would be possible to at least try it.
>
>>> I'm not quite sure if trying to detect which of these cases we have is
>>> possible, or worthwhile. Anyway, with a packet size of 64, the queue
>>> size would be 32*64 = 2KB, which should be enough for everyone. So I
>>> propose to ignore case 1), and to drop the error message.
>
> After some thought, I'm not so sure anymore -- the ability to buffer
> more than 2 KB of data is part of the snd_rawmidi_write() API, so this
> could introduce a regression. And I can imagine cases where one would
> actually want to transmit large amounts data.

One thing to consider is that the ALSA rawmidi device buffer is
sequential and our USB request buffer is not. This means that our 32
(qlen) * 256 (buflen) = 8KB of data is non-linear. Some requests might
have 3 or 4 bytes (average size of a normal MIDI message) of data and
some others might contain the full 256 bytes (for SysEx messages).

I am considering this especially for MPE (Multidimensional Polyphonic
Expression) MIDI protocol. On few benchmarks I did, a device that
implements this protocol generates around 500-2000 b/s of *raw* MIDI
data. And in practice only 4 (average MIDI message) * 32 (USB requests
defined by qlen) bytes will be used. Which means that the 8KB USB
request buffer will be under used.

So I think we have to treat the ALSA buffers and the USB request buffers
differently.

That's why I think this approach is fine by allowing the user to
increase that number of requests and its size if it needs to deal with a
higher throughput devices.

>
> I think the safest approach would be to behave similar to the old driver,
> i.e., when the queue overflows, do nothing (not even dropping data), and
> rely on the transmit completion handler to continue. (This implies that
> ALSA's buffer can fill up, and that snd_rawmidi_write() can block.)
>

The previous implementation would not block, even though
snd_rawmidi_write() can block, because it was been created a new USB
request for each write call and data was been consumed even if this
request would not be enqueued to the endpoint.

But, anyway, I agree with your suggestion.

>
> It you want to dequeue outdated data, I think this should be done with
> a timeout, i.e., when the host did not read anything for some tens of
> milliseconds or so. This would be independent of the fill level of the
> queue, and could be done either for individual packets, or just on the
> entire endpoint queue.

That can be done. But I believe in another patch since it is not
required to work for this patch.

== Conclusion ==

Based on our conversation and your suggestions, I think that to just
ignore if an overrun occurs to the USB requests is fine. Upon completion
the request will be reused.
Important to note that if the overrun occurs, it will cause user-space
to block until a) the completion function is called successfully or b)
snd_rawmidi_write() times out. Which I think this is expected by ALSA users.

Does that make sense?

If yes then I will send the v6 of this patch.

Felipe

Attachments:

0x92698E6A.asc (7.03 kB)

2015-11-27 19:52:23

by Clemens Ladisch

[permalink] [raw]

Subject: Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

Felipe Ferreri Tonello wrote:
> One thing to consider is that the ALSA rawmidi device buffer is
> sequential and our USB request buffer is not. This means that our 32
> (qlen) * 256 (buflen) = 8KB of data is non-linear. Some requests might
> have 3 or 4 bytes (average size of a normal MIDI message) of data and
> some others might contain the full 256 bytes (for SysEx messages).

f_midi_transmit() always fills up the USB packet as much as possible, so
the number of MIDI messages per request will increase automatically when
the ALSA buffer fills up faster that it is emptied by f_midi.

> == Conclusion ==
>
> Based on our conversation and your suggestions, I think that to just
> ignore if an overrun occurs to the USB requests is fine. Upon completion
> the request will be reused.
> Important to note that if the overrun occurs, it will cause user-space
> to block until a) the completion function is called successfully or b)
> snd_rawmidi_write() times out. Which I think this is expected by ALSA users.
>
> Does that make sense?

Yes.

Regards,
Clemens