[PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe F. Tonello]

Fixed all comments suggested by the linux-usb list.

changes in v6:
 - Removed patches already applied in Balbi's tree
 - Cleanups on pre-allocation usb requrests patch
 - Fixed indentention on patch 1
 - Added patch which fails set_alt if a failure happened while
   allocating usb requests

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 (3):
  usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
  usb: gadget: f_midi: fail if set_alt fails to allocate requests
  usb: gadget: f_midi: pre-allocate IN requests

 drivers/usb/gadget/function/f_midi.c | 175 +++++++++++++++++++++++++++--------
 drivers/usb/gadget/legacy/gmidi.c    |   2 +-
 2 files changed, 135 insertions(+), 42 deletions(-)

-- 
2.5.0

[PATCH v6 1/3] 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 29bfca1..e804231 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];
@@ -321,8 +322,8 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	unsigned i;
 	int err;
 
-	/* For Control Device interface we do nothing */
-	if (intf == 0)
+	/* we only set alt for MIDIStreaming interface */
+	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

[PATCH v6 2/3] usb: gadget: f_midi: fail if set_alt fails to allocate requests

[Felipe F. Tonello]

This ensures that the midi function will only work if the proper number of
IN and OUT requrests are allocated. Otherwise the function will work with less
requests then what the user wants.

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

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index e804231..79dc611 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -344,9 +344,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		req->complete = f_midi_complete;
 		err = usb_ep_queue(midi->out_ep, req, GFP_ATOMIC);
 		if (err) {
-			ERROR(midi, "%s queue req: %d\n",
+			ERROR(midi, "%s: couldn't enqueue request: %d\n",
 				    midi->out_ep->name, err);
 			free_ep_req(midi->out_ep, req);
+			return err;
 		}
 	}
 
-- 
2.5.0

[PATCH v6 3/3] 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 | 166 +++++++++++++++++++++++++++--------
 drivers/usb/gadget/legacy/gmidi.c    |   2 +-
 2 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 79dc611..fb1fe96d 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;
+	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
+	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
+	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,20 @@ 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);
+	}
+
 	/* 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 =
@@ -358,6 +379,7 @@ static void f_midi_disable(struct usb_function *f)
 {
 	struct f_midi *midi = func_to_midi(f);
 	struct usb_composite_dev *cdev = f->config->cdev;
+	struct usb_request *req = NULL;
 
 	DBG(cdev, "disable\n");
 
@@ -367,6 +389,10 @@ 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_get(&midi->in_req_fifo, &req))
+		free_ep_req(midi->in_ep, req);
 }
 
 static int f_midi_snd_free(struct snd_device *device)
@@ -488,57 +514,113 @@ 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 buffer overrun, then we ignore this transmission.
+		 * IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
+		 * requests have been completed or b) snd_rawmidi_write() times out. */
+		if (req->length > 0)
+			return;
+
+		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);
+
+	return;
+
+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)
@@ -664,6 +746,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 |
@@ -1053,6 +1136,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);
@@ -1126,6 +1210,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_last_port = 0;
+
+	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
+	if (status)
+		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 v6 3/3] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

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

On 01/12/15 18:31, 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 | 166 +++++++++++++++++++++++++++--------
>  drivers/usb/gadget/legacy/gmidi.c    |   2 +-
>  2 files changed, 129 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 79dc611..fb1fe96d 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;
> +	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
> +	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> +	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,20 @@ 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);
> +	}
> +
>  	/* 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 =
> @@ -358,6 +379,7 @@ static void f_midi_disable(struct usb_function *f)
>  {
>  	struct f_midi *midi = func_to_midi(f);
>  	struct usb_composite_dev *cdev = f->config->cdev;
> +	struct usb_request *req = NULL;
>  
>  	DBG(cdev, "disable\n");
>  
> @@ -367,6 +389,10 @@ 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_get(&midi->in_req_fifo, &req))
> +		free_ep_req(midi->in_ep, req);
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -488,57 +514,113 @@ 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 buffer overrun, then we ignore this transmission.
> +		 * IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
> +		 * requests have been completed or b) snd_rawmidi_write() times out. */
> +		if (req->length > 0)
> +			return;
> +
> +		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);
> +
> +	return;
> +
> +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)
> @@ -664,6 +746,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 |
> @@ -1053,6 +1136,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);
> @@ -1126,6 +1210,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_last_port = 0;
> +
> +	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
> +	if (status)
> +		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);
> 

Just as a PS, I do have another patch refactoring the state machine on
this driver, but it requires this change to be applied. So once it gets
approved I will send the patch.

Felipe

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

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

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

Hi all,

On 01/12/15 18:30, Felipe F. Tonello wrote:
> Fixed all comments suggested by the linux-usb list.
> 
> changes in v6:
>  - Removed patches already applied in Balbi's tree
>  - Cleanups on pre-allocation usb requrests patch
>  - Fixed indentention on patch 1
>  - Added patch which fails set_alt if a failure happened while
>    allocating usb requests
> 
> 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 (3):
>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>   usb: gadget: f_midi: pre-allocate IN requests
> 
>  drivers/usb/gadget/function/f_midi.c | 175 +++++++++++++++++++++++++++--------
>  drivers/usb/gadget/legacy/gmidi.c    |   2 +-
>  2 files changed, 135 insertions(+), 42 deletions(-)
> 

Ping?

-- 
Felipe

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

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Balbi]

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


Hi

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
> Hi all,
>
> On 01/12/15 18:30, Felipe F. Tonello wrote:
>> Fixed all comments suggested by the linux-usb list.
>> 
>> changes in v6:
>>  - Removed patches already applied in Balbi's tree
>>  - Cleanups on pre-allocation usb requrests patch
>>  - Fixed indentention on patch 1
>>  - Added patch which fails set_alt if a failure happened while
>>    allocating usb requests
>> 
>> 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 (3):
>>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>>   usb: gadget: f_midi: pre-allocate IN requests
>> 
>>  drivers/usb/gadget/function/f_midi.c | 175 +++++++++++++++++++++++++++--------
>>  drivers/usb/gadget/legacy/gmidi.c    |   2 +-
>>  2 files changed, 135 insertions(+), 42 deletions(-)
>> 
>
> Ping?

It should be in my testing/next now, I had to wait until Greg merged
fixes to linus before applying.

-- 
balbi

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

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

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

Hi Balbi,

On 16/12/15 16:03, Felipe Balbi wrote:
> 
> Hi
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> Hi all,
>>
>> On 01/12/15 18:30, Felipe F. Tonello wrote:
>>> Fixed all comments suggested by the linux-usb list.
>>>
>>> changes in v6:
>>>  - Removed patches already applied in Balbi's tree
>>>  - Cleanups on pre-allocation usb requrests patch
>>>  - Fixed indentention on patch 1
>>>  - Added patch which fails set_alt if a failure happened while
>>>    allocating usb requests
>>>
>>> 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 (3):
>>>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>>>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>>>   usb: gadget: f_midi: pre-allocate IN requests
>>>
>>>  drivers/usb/gadget/function/f_midi.c | 175 +++++++++++++++++++++++++++--------
>>>  drivers/usb/gadget/legacy/gmidi.c    |   2 +-
>>>  2 files changed, 135 insertions(+), 42 deletions(-)
>>>
>>
>> Ping?
> 
> It should be in my testing/next now, I had to wait until Greg merged
> fixes to linus before applying.
> 

Thanks, I still have two more patches that I will be sending as soon as
these get to your next branch.

Felipe

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