[PATCH v3 0/4] USB MIDI Gadget bug fixes and improvements

[PATCH v3 0/4] USB MIDI Gadget bug fixes and improvements

[Felipe F. Tonello]

Here is the third version of this patch set. It includes memory leakage bug
fix, improvements and code cleanups.

Felipe F. Tonello (4):
  usb: gadget: f_midi: free usb request when done
  usb: gadget: f_midi: free request when usb_ep_queue fails
  usb: gadget: f_midi: Transmit data only when IN ep is enabled
  usb: gadget: f_midi: remove duplicated code

 drivers/usb/gadget/function/f_midi.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

-- 
2.1.4

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

[Felipe F. Tonello]

req->actual == req->length means that there is no data left to enqueue,
so free the request.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.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;
-- 
2.1.4

[PATCH v3 2/4] usb: gadget: f_midi: free request when usb_ep_queue fails

[Felipe F. Tonello]

This fix a memory leak that will occur in this case.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 93212ca..163c0ce 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -546,9 +546,11 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 		int err;
 
 		err = usb_ep_queue(ep, req, GFP_ATOMIC);
-		if (err < 0)
+		if (err < 0) {
 			ERROR(midi, "%s queue req: %d\n",
 			      midi->in_ep->name, err);
+			free_ep_req(ep, req);
+		}
 	} else {
 		free_ep_req(ep, req);
 	}
-- 
2.1.4

[PATCH v3 3/4] usb: gadget: f_midi: Transmit data only when IN ep is enabled

[Felipe F. Tonello]

This makes sure f_midi doesn't try to enqueue data when the IN endpoint is
disabled, ie, USB cable is disconnected.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 163c0ce..bbfe43b 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -87,6 +87,7 @@ struct f_midi {
 	int index;
 	char *id;
 	unsigned int buflen, qlen;
+	bool in_ep_enabled;
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -331,6 +332,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	err = f_midi_start_ep(midi, f, midi->in_ep);
 	if (err)
 		return err;
+	midi->in_ep_enabled = true;
 
 	err = f_midi_start_ep(midi, f, midi->out_ep);
 	if (err)
@@ -386,6 +388,8 @@ static void f_midi_disable(struct usb_function *f)
 	 */
 	usb_ep_disable(midi->in_ep);
 	usb_ep_disable(midi->out_ep);
+
+	midi->in_ep_enabled = false;
 }
 
 static int f_midi_snd_free(struct snd_device *device)
@@ -542,7 +546,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 		}
 	}
 
-	if (req->length > 0) {
+	if (req->length > 0 && midi->in_ep_enabled) {
 		int err;
 
 		err = usb_ep_queue(ep, req, GFP_ATOMIC);
@@ -1159,6 +1163,7 @@ 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_ep_enabled = false;
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
-- 
2.1.4

[PATCH v3 4/4] usb: gadget: f_midi: remove duplicated code

[Felipe F. Tonello]

This code is duplicated from f_midi_start_ep(midi, f, midi->out_ep).

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index bbfe43b..0656a48 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -325,7 +325,6 @@ 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);
-	struct usb_composite_dev *cdev = f->config->cdev;
 	unsigned i;
 	int err;
 
@@ -338,25 +337,6 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	if (err)
 		return err;
 
-	if (midi->out_ep->driver_data)
-		usb_ep_disable(midi->out_ep);
-
-	err = config_ep_by_speed(midi->gadget, f, midi->out_ep);
-	if (err) {
-		ERROR(cdev, "can't configure %s: %d\n",
-		      midi->out_ep->name, err);
-		return err;
-	}
-
-	err = usb_ep_enable(midi->out_ep);
-	if (err) {
-		ERROR(cdev, "can't start %s: %d\n",
-		      midi->out_ep->name, err);
-		return err;
-	}
-
-	midi->out_ep->driver_data = midi;
-
 	/* 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 =
-- 
2.1.4

Re: [PATCH v3 0/4] USB MIDI Gadget bug fixes and improvements

[Felipe Tonello]

On Tue, Sep 29, 2015 at 1:01 PM, Felipe F. Tonello <eu@felipetonello.com> wrote:
> Here is the third version of this patch set. It includes memory leakage bug
> fix, improvements and code cleanups.
>
> Felipe F. Tonello (4):
>   usb: gadget: f_midi: free usb request when done
>   usb: gadget: f_midi: free request when usb_ep_queue fails
>   usb: gadget: f_midi: Transmit data only when IN ep is enabled
>   usb: gadget: f_midi: remove duplicated code
>
>  drivers/usb/gadget/function/f_midi.c | 36 +++++++++++-------------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
>
> --
> 2.1.4
>

ping?

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

[Clemens Ladisch]

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@felipetonello.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.

(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.)


Regards,
Clemens

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]


Hi,

Clemens Ladisch <clemens@ladisch.de> writes:
> 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@felipetonello.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.
>
> (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.)

so, should I drop this series from my TODO queue ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Clemens Ladisch]

Felipe Balbi wrote:
> Clemens Ladisch <clemens@ladisch.de> writes:
>> 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@felipetonello.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.
>
> so, should I drop this series from my TODO queue ?

Yes, this patch needs to be dropped.


Regards,
Clemens

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

[Felipe Tonello]

Hi Balbi,

On Sun, Oct 11, 2015 at 8:08 PM, Clemens Ladisch <clemens@ladisch.de> wrote:
> Felipe Balbi wrote:
>> Clemens Ladisch <clemens@ladisch.de> writes:
>>> 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@felipetonello.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.
>>
>> so, should I drop this series from my TODO queue ?
>
> Yes, this patch needs to be dropped.

You don't need to drop the whole series. Just this patch needs more
work, the rest are fine (including bug fixes).

Felipe Tonello

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

[Felipe Tonello]

Hi Clemens

On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@ladisch.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@felipetonello.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

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

[Clemens Ladisch]

Felipe Tonello wrote:
> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@ladisch.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.

> 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.

> 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.


Regards,
Clemens

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@ladisch.de> wrote:
> Felipe Tonello wrote:
>> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@ladisch.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

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

[Clemens Ladisch]

Felipe Tonello wrote:
> On Mon, Oct 12, 2015 at 11:16 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
>> Felipe Tonello wrote:
>>> 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?

Enough to get proper pipelining.  At least two, maybe not more.
(Depends on how fast those tiny CPUs can queue the next request.)

>>> 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.

Yes, that's exactly what I'm proposing.


Regards,
Clemens