[PATCH v5 0/7] USB MIDI Gadget improvements and bug fixes

[PATCH v5 0/7] USB MIDI Gadget improvements and bug fixes

[Felipe F. Tonello]

Patch 7 has changes on how to transmit IN USB requests. It implements a FIFO
of pre-allocated usb requests and uses then as needed, instead of allocating
then on demand. This is my initial implementation and is open for
suggestions and comments.

Patches 1-6 is pretty much straight forward.

changes in v5:
 - Use ep->enabled insetad of creating driver specific flag
 - Save MIDIStreaming interface id in driver data
 - define free_ep_req as static inline in header

changes in v4:
 - pre-alocation of in requests.
 - more code clean up
 - fix memory leak on out requests
 - configure endpoints only when setting up MIDIStreaming interface

Felipe F. Tonello (7):
  usb: gadget: f_midi: Transmit data only when IN ep is enabled
  usb: gadget: f_midi: remove duplicated code
  usb: gadget: define free_ep_req as universal function
  usb: gadget: f_midi: fix leak on failed to enqueue out requests
  usb: gadget: gmidi: Cleanup legacy code
  usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
  usb: gadget: f_midi: pre-allocate IN requests

 drivers/usb/gadget/function/f_midi.c       | 196 ++++++++++++++++++++---------
 drivers/usb/gadget/function/f_sourcesink.c |   6 -
 drivers/usb/gadget/function/g_zero.h       |   1 -
 drivers/usb/gadget/legacy/gmidi.c          |  10 +-
 drivers/usb/gadget/u_f.c                   |   1 -
 drivers/usb/gadget/u_f.h                   |  10 +-
 6 files changed, 145 insertions(+), 79 deletions(-)

-- 
2.5.0

[PATCH v5 1/7] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index ce3c8a6..eeb989d 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -545,7 +545,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 		}
 	}
 
-	if (req->length > 0) {
+	if (req->length > 0 && ep->enabled) {
 		int err;
 
 		err = usb_ep_queue(ep, req, GFP_ATOMIC);
-- 
2.5.0

Re: [PATCH v5 1/7] usb: gadget: f_midi: Transmit data only when IN ep is enabled

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> 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>

Reviewed-by: Robert Baldyga <r.baldyga@samsung.com>

> ---
>  drivers/usb/gadget/function/f_midi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index ce3c8a6..eeb989d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -545,7 +545,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>  		}
>  	}
>  
> -	if (req->length > 0) {
> +	if (req->length > 0 && ep->enabled) {
>  		int err;
>  
>  		err = usb_ep_queue(ep, req, GFP_ATOMIC);
> 

[PATCH v5 2/7] 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 | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index eeb989d..488111d 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -324,7 +324,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;
 
@@ -340,24 +339,6 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	if (err)
 		return err;
 
-	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.5.0

Re: [PATCH v5 2/7] usb: gadget: f_midi: remove duplicated code

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This code is duplicated from f_midi_start_ep(midi, f, midi->out_ep).
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Reviewed-by: Robert Baldyga <r.baldyga@samsung.com>

> ---
>  drivers/usb/gadget/function/f_midi.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index eeb989d..488111d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -324,7 +324,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;
>  
> @@ -340,24 +339,6 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	if (err)
>  		return err;
>  
> -	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 =
> 

[PATCH v5 3/7] usb: gadget: define free_ep_req as universal function

[Felipe F. Tonello]

This function is shared between gadget functions, so this avoid unnecessary
duplicated code and potentially avoid memory leaks.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c       |  6 ------
 drivers/usb/gadget/function/f_sourcesink.c |  6 ------
 drivers/usb/gadget/function/g_zero.h       |  1 -
 drivers/usb/gadget/u_f.c                   |  1 -
 drivers/usb/gadget/u_f.h                   | 10 ++++++++--
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 488111d..f36db2d 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -201,12 +201,6 @@ static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
 	return alloc_ep_req(ep, length, length);
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{
-	kfree(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static const uint8_t f_midi_cin_length[] = {
 	0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
 };
diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index d7646d3..74f95b1 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -303,12 +303,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
 	return alloc_ep_req(ep, len, ss->buflen);
 }
 
-void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{
-	kfree(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
 {
 	int			value;
diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h
index 15f1809..5ed90b4 100644
--- a/drivers/usb/gadget/function/g_zero.h
+++ b/drivers/usb/gadget/function/g_zero.h
@@ -59,7 +59,6 @@ void lb_modexit(void);
 int lb_modinit(void);
 
 /* common utilities */
-void free_ep_req(struct usb_ep *ep, struct usb_request *req);
 void disable_endpoints(struct usb_composite_dev *cdev,
 		struct usb_ep *in, struct usb_ep *out,
 		struct usb_ep *iso_in, struct usb_ep *iso_out);
diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index c6276f0..4bc7eea 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -11,7 +11,6 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/usb/gadget.h>
 #include "u_f.h"
 
 struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 1d5f0eb..4247cc0 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -16,6 +16,8 @@
 #ifndef __U_F_H__
 #define __U_F_H__
 
+#include <linux/usb/gadget.h>
+
 /* Variable Length Array Macros **********************************************/
 #define vla_group(groupname) size_t groupname##__next = 0
 #define vla_group_size(groupname) groupname##__next
@@ -45,8 +47,12 @@
 struct usb_ep;
 struct usb_request;
 
+/* Requests allocated via alloc_ep_req() must be freed by free_ep_req(). */
 struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
+static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	kfree(req->buf);
+	usb_ep_free_request(ep, req);
+}
 
 #endif /* __U_F_H__ */
-
-
-- 
2.5.0

Re: [PATCH v5 3/7] usb: gadget: define free_ep_req as universal function

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This function is shared between gadget functions, so this avoid unnecessary
> duplicated code and potentially avoid memory leaks.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Reviewed-by: Robert Baldyga <r.baldyga@samsung.com>

> ---
>  drivers/usb/gadget/function/f_midi.c       |  6 ------
>  drivers/usb/gadget/function/f_sourcesink.c |  6 ------
>  drivers/usb/gadget/function/g_zero.h       |  1 -
>  drivers/usb/gadget/u_f.c                   |  1 -
>  drivers/usb/gadget/u_f.h                   | 10 ++++++++--
>  5 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 488111d..f36db2d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -201,12 +201,6 @@ static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
>  	return alloc_ep_req(ep, length, length);
>  }
>  
> -static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> -{
> -	kfree(req->buf);
> -	usb_ep_free_request(ep, req);
> -}
> -
>  static const uint8_t f_midi_cin_length[] = {
>  	0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>  };
> diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
> index d7646d3..74f95b1 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -303,12 +303,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>  	return alloc_ep_req(ep, len, ss->buflen);
>  }
>  
> -void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> -{
> -	kfree(req->buf);
> -	usb_ep_free_request(ep, req);
> -}
> -
>  static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
>  {
>  	int			value;
> diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h
> index 15f1809..5ed90b4 100644
> --- a/drivers/usb/gadget/function/g_zero.h
> +++ b/drivers/usb/gadget/function/g_zero.h
> @@ -59,7 +59,6 @@ void lb_modexit(void);
>  int lb_modinit(void);
>  
>  /* common utilities */
> -void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>  void disable_endpoints(struct usb_composite_dev *cdev,
>  		struct usb_ep *in, struct usb_ep *out,
>  		struct usb_ep *iso_in, struct usb_ep *iso_out);
> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
> index c6276f0..4bc7eea 100644
> --- a/drivers/usb/gadget/u_f.c
> +++ b/drivers/usb/gadget/u_f.c
> @@ -11,7 +11,6 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include <linux/usb/gadget.h>
>  #include "u_f.h"
>  
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
> index 1d5f0eb..4247cc0 100644
> --- a/drivers/usb/gadget/u_f.h
> +++ b/drivers/usb/gadget/u_f.h
> @@ -16,6 +16,8 @@
>  #ifndef __U_F_H__
>  #define __U_F_H__
>  
> +#include <linux/usb/gadget.h>
> +
>  /* Variable Length Array Macros **********************************************/
>  #define vla_group(groupname) size_t groupname##__next = 0
>  #define vla_group_size(groupname) groupname##__next
> @@ -45,8 +47,12 @@
>  struct usb_ep;
>  struct usb_request;
>  
> +/* Requests allocated via alloc_ep_req() must be freed by free_ep_req(). */
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
> +static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> +{
> +	kfree(req->buf);
> +	usb_ep_free_request(ep, req);
> +}
>  
>  #endif /* __U_F_H__ */
> -
> -
> 

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

[Felipe F. Tonello]

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 <eu@felipetonello.com>
---
 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);
 		}
 	}
 
-- 
2.5.0

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

[Robert Baldyga]

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

Best regards,
Robert

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

[Felipe Ferreri Tonello]

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

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

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 4830 bytes --]

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

[Robert Baldyga]

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

--
Best regards,
Robert

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

[Felipe Ferreri Tonello]

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

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

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 4830 bytes --]

[PATCH v5 5/7] usb: gadget: gmidi: Cleanup legacy code

[Felipe F. Tonello]

Remove unnecessary headers and variables.

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

diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
index 8a18348..be8e91d 100644
--- a/drivers/usb/gadget/legacy/gmidi.c
+++ b/drivers/usb/gadget/legacy/gmidi.c
@@ -21,19 +21,12 @@
 /* #define VERBOSE_DEBUG */
 
 #include <linux/kernel.h>
-#include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/device.h>
 
-#include <sound/core.h>
 #include <sound/initval.h>
-#include <sound/rawmidi.h>
 
-#include <linux/usb/ch9.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/gadget.h>
-#include <linux/usb/audio.h>
-#include <linux/usb/midi.h>
 
 #include "u_midi.h"
 
@@ -42,7 +35,6 @@
 MODULE_AUTHOR("Ben Williamson");
 MODULE_LICENSE("GPL v2");
 
-static const char shortname[] = "g_midi";
 static const char longname[] = "MIDI Gadget";
 
 USB_GADGET_COMPOSITE_OPTIONS();
-- 
2.5.0

Re: [PATCH v5 5/7] usb: gadget: gmidi: Cleanup legacy code

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> Remove unnecessary headers and variables.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Reviewed-by: Robert Baldyga <r.baldyga@samsung.com>

> ---
>  drivers/usb/gadget/legacy/gmidi.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
> index 8a18348..be8e91d 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -21,19 +21,12 @@
>  /* #define VERBOSE_DEBUG */
>  
>  #include <linux/kernel.h>
> -#include <linux/slab.h>
>  #include <linux/module.h>
> -#include <linux/device.h>
>  
> -#include <sound/core.h>
>  #include <sound/initval.h>
> -#include <sound/rawmidi.h>
>  
> -#include <linux/usb/ch9.h>
>  #include <linux/usb/composite.h>
>  #include <linux/usb/gadget.h>
> -#include <linux/usb/audio.h>
> -#include <linux/usb/midi.h>
>  
>  #include "u_midi.h"
>  
> @@ -42,7 +35,6 @@
>  MODULE_AUTHOR("Ben Williamson");
>  MODULE_LICENSE("GPL v2");
>  
> -static const char shortname[] = "g_midi";
>  static const char longname[] = "MIDI Gadget";
>  
>  USB_GADGET_COMPOSITE_OPTIONS();
> 

[PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Felipe F. Tonello]

This avoids duplication of USB requests for OUT endpoint and
re-enabling endpoints.

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 76ea53c..615d632 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -75,6 +75,7 @@ struct f_midi {
 	struct usb_ep		*in_ep, *out_ep;
 	struct snd_card		*card;
 	struct snd_rawmidi	*rmidi;
+	u8					ms_id;
 
 	struct snd_rawmidi_substream *in_substream[MAX_PORTS];
 	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
@@ -322,7 +323,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	int err;
 
 	/* For Control Device interface we do nothing */
-	if (intf == 0)
+	if (intf != midi->ms_id)
 		return 0;
 
 	err = f_midi_start_ep(midi, f, midi->in_ep);
@@ -730,6 +731,7 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
 		goto fail;
 	ms_interface_desc.bInterfaceNumber = status;
 	ac_header_desc.baInterfaceNr[0] = status;
+	midi->ms_id = status;
 
 	status = -ENODEV;
 
-- 
2.5.0

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Sergei Shtylyov]

Hello.

On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:

> This avoids duplication of USB requests for OUT endpoint and
> re-enabling endpoints.
>
> 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 76ea53c..615d632 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -75,6 +75,7 @@ struct f_midi {
>   	struct usb_ep		*in_ep, *out_ep;
>   	struct snd_card		*card;
>   	struct snd_rawmidi	*rmidi;
> +	u8					ms_id;

   Why 'ms_id' is not aligned with the above field names?

[...]

MBR, Sergei

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Felipe Ferreri Tonello]

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

Hi Sergei,

On 10/11/15 18:43, Sergei Shtylyov wrote:
> Hello.
> 
> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
> 
>> This avoids duplication of USB requests for OUT endpoint and
>> re-enabling endpoints.
>>
>> 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 76ea53c..615d632 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -75,6 +75,7 @@ struct f_midi {
>>       struct usb_ep        *in_ep, *out_ep;
>>       struct snd_card        *card;
>>       struct snd_rawmidi    *rmidi;
>> +    u8                    ms_id;
> 
>   Why 'ms_id' is not aligned with the above field names?

It is actually aligned. Perhaps because of the +?

Here is from my local git diff:

--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -75,6 +75,7 @@ struct f_midi {
    struct usb_ep       *in_ep, *out_ep;
    struct snd_card     *card;
    struct snd_rawmidi  *rmidi;
+   u8                  ms_id;

    struct snd_rawmidi_substream *in_substream[MAX_PORTS];
    struct snd_rawmidi_substream *out_substream[MAX_PORTS];


-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 4830 bytes --]

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Sergei Shtylyov]

Hello.

On 11/11/2015 12:38 PM, Felipe Ferreri Tonello wrote:

>>> This avoids duplication of USB requests for OUT endpoint and
>>> re-enabling endpoints.
>>>
>>> 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 76ea53c..615d632 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -75,6 +75,7 @@ struct f_midi {
>>>        struct usb_ep        *in_ep, *out_ep;
>>>        struct snd_card        *card;
>>>        struct snd_rawmidi    *rmidi;
>>> +    u8                    ms_id;
>>
>>    Why 'ms_id' is not aligned with the above field names?
>
> It is actually aligned. Perhaps because of the +?

   No, + should not affect this, it only affects things when a line doesn't 
start with a tab.

> Here is from my local git diff:
>
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -75,6 +75,7 @@ struct f_midi {
>      struct usb_ep       *in_ep, *out_ep;
>      struct snd_card     *card;
>      struct snd_rawmidi  *rmidi;
> +   u8                  ms_id;

    Unfortunately, tabs got replaces with spaces in this fragment.

[...]

MBR, Sergei

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 10/11/15 18:43, Sergei Shtylyov wrote:
>> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
>>> @@ -75,6 +75,7 @@ struct f_midi {
>>>       struct usb_ep        *in_ep, *out_ep;
>>>       struct snd_card        *card;
>>>       struct snd_rawmidi    *rmidi;
>>> +    u8                    ms_id;
>>
>>   Why 'ms_id' is not aligned with the above field names?
>
> It is actually aligned.

It's not in the original mail, which contains tab characters.

> Here is from my local git diff:
>
> @@ -75,6 +75,7 @@ struct f_midi {
>     struct usb_ep       *in_ep, *out_ep;
>     struct snd_card     *card;
>     struct snd_rawmidi  *rmidi;
> +   u8                  ms_id;

Apparently, you're using four spaces per tab.  Linux uses eight.


Regards,
Clemens

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Felipe Ferreri Tonello]

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

Hi Clemens,

On 13/11/15 08:11, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 10/11/15 18:43, Sergei Shtylyov wrote:
>>> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
>>>> @@ -75,6 +75,7 @@ struct f_midi {
>>>>       struct usb_ep        *in_ep, *out_ep;
>>>>       struct snd_card        *card;
>>>>       struct snd_rawmidi    *rmidi;
>>>> +    u8                    ms_id;
>>>
>>>   Why 'ms_id' is not aligned with the above field names?
>>
>> It is actually aligned.
> 
> It's not in the original mail, which contains tab characters.
> 
>> Here is from my local git diff:
>>
>> @@ -75,6 +75,7 @@ struct f_midi {
>>     struct usb_ep       *in_ep, *out_ep;
>>     struct snd_card     *card;
>>     struct snd_rawmidi  *rmidi;
>> +   u8                  ms_id;
> 
> Apparently, you're using four spaces per tab.  Linux uses eight.

Yes. I'll fix that.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 4830 bytes --]

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This avoids duplication of USB requests for OUT endpoint and
> re-enabling endpoints.

>  ...
>  	/* For Control Device interface we do nothing */
> -	if (intf == 0)
> +	if (intf != midi->ms_id)
>  		return 0;

The comment now is misleading.


Regards,
Clemens

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

[Felipe F. Tonello]

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 <eu@felipetonello.com>
---
 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;
+
+		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);
+			}
+		}
+	} 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);
-- 
2.5.0

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

[Clemens Ladisch]

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.

> ...
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -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;

As far as I can see, in_req_num must always have the same value as qlen.

> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
> +	while (!kfifo_is_empty(&midi->in_req_fifo)) {
> +		...
> +		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);
> +	}

kfifo_get() already checks for the FIFO being empty, so you can just
drop kfifi_is_empty().

> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
> ...
> +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.

This code shows the error message for case 1), but does the action for
case 2).

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.

> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
> +	if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
> +		goto setup_fail;

The setup_fail code expects an error code in the status variable.


Regards,
Clemens

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

[Felipe Ferreri Tonello]

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

Hi Clemens

On 13/11/15 08:55, Clemens Ladisch wrote:
> 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.
> 
>> ...
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -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;
> 
> As far as I can see, in_req_num must always have the same value as qlen.

Yes, I've removed in_req_num.

> 
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> +	while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> +		...
>> +		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);
>> +	}
> 
> kfifo_get() already checks for the FIFO being empty, so you can just
> drop kfifi_is_empty().

Ok. I've simplified this code. I wasn't really happy with it either.

> 
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
>> ...
>> +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.

> 
> This code shows the error message for case 1), but does the action for
> case 2).
> 
> 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.

Agree. It would be useful for users to know about case 1), but like you
said it is probably not worthwhile to do to so.

> 
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>> +	if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> +		goto setup_fail;
> 
> The setup_fail code expects an error code in the status variable.

Done.

Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

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

[Clemens Ladisch]

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.

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


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.


Regards,
Clemens

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

[Felipe Ferreri Tonello]

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

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

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

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

[Clemens Ladisch]

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

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

[Robert Baldyga]

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

> +			}
> +		}
> +	} 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

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

[Felipe Ferreri Tonello]

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

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

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 4908 bytes --]