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

[PATCH v4 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 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: f_midi: set altsettings only for MIDIStreaming interface
  usb: gadget: gmidi: Cleanup legacy code
  usb: gadget: f_midi: pre-allocate IN requests

 drivers/usb/gadget/function/f_midi.c       | 201 ++++++++++++++++++++---------
 drivers/usb/gadget/function/f_sourcesink.c |   6 -
 drivers/usb/gadget/function/g_zero.h       |   1 -
 drivers/usb/gadget/legacy/gmidi.c          |  12 +-
 drivers/usb/gadget/u_f.c                   |   8 ++
 drivers/usb/gadget/u_f.h                   |   3 +-
 6 files changed, 151 insertions(+), 80 deletions(-)

-- 
2.1.4

[PATCH v4 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index edb84ca..e08f365 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -87,6 +87,7 @@ struct f_midi {
 	int index;
 	char *id;
 	unsigned int buflen, qlen;
+	bool in_ep_enabled;
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	err = f_midi_start_ep(midi, f, midi->in_ep);
 	if (err)
 		return err;
+	midi->in_ep_enabled = true;
 
 	err = f_midi_start_ep(midi, f, midi->out_ep);
 	if (err)
@@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f)
 	 */
 	usb_ep_disable(midi->in_ep);
 	usb_ep_disable(midi->out_ep);
+
+	midi->in_ep_enabled = false;
 }
 
 static int f_midi_snd_free(struct snd_device *device)
@@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 		}
 	}
 
-	if (req->length > 0) {
+	if (req->length > 0 && midi->in_ep_enabled) {
 		int err;
 
 		err = usb_ep_queue(ep, req, GFP_ATOMIC);
@@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->index = opts->index;
 	midi->buflen = opts->buflen;
 	midi->qlen = opts->qlen;
+	midi->in_ep_enabled = false;
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
-- 
2.1.4

[PATCH v4 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 | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index e08f365..c19f154 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -326,7 +326,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;
 
@@ -339,25 +338,6 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	if (err)
 		return err;
 
-	if (midi->out_ep->driver_data)
-		usb_ep_disable(midi->out_ep);
-
-	err = config_ep_by_speed(midi->gadget, f, midi->out_ep);
-	if (err) {
-		ERROR(cdev, "can't configure %s: %d\n",
-		      midi->out_ep->name, err);
-		return err;
-	}
-
-	err = usb_ep_enable(midi->out_ep);
-	if (err) {
-		ERROR(cdev, "can't start %s: %d\n",
-		      midi->out_ep->name, err);
-		return err;
-	}
-
-	midi->out_ep->driver_data = midi;
-
 	/* allocate a bunch of read buffers and queue them all at once. */
 	for (i = 0; i < midi->qlen && err == 0; i++) {
 		struct usb_request *req =
-- 
2.1.4

[PATCH v4 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                   | 8 ++++++++
 drivers/usb/gadget/u_f.h                   | 3 +--
 5 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index c19f154..4c01c8a 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -202,12 +202,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 3a5ae99..eedea7f 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -307,12 +307,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
 	return alloc_ep_req(ep, len, 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..f78bd1f 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -14,6 +14,7 @@
 #include <linux/usb/gadget.h>
 #include "u_f.h"
 
+/* 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)
 {
 	struct usb_request      *req;
@@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
 	return req;
 }
 EXPORT_SYMBOL_GPL(alloc_ep_req);
+
+void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	kfree(req->buf);
+	usb_ep_free_request(ep, req);
+}
+EXPORT_SYMBOL_GPL(free_ep_req);
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 1d5f0eb..2a1a6fb 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -46,7 +46,6 @@ struct usb_ep;
 struct usb_request;
 
 struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
+void free_ep_req(struct usb_ep *ep, struct usb_request *req);
 
 #endif /* __U_F_H__ */
-
-
-- 
2.1.4

[PATCH v4 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 4c01c8a..0e9cdeb 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -344,6 +344,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.1.4

[PATCH v4 5/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, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 0e9cdeb..a617df3 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	unsigned i;
 	int err;
 
+	/* We don't care if it is not MIDIStreaming interface */
+	if (intf != ms_interface_desc.bInterfaceNumber)
+		return 0;
+
 	err = f_midi_start_ep(midi, f, midi->in_ep);
 	if (err)
 		return err;
-- 
2.1.4

[PATCH v4 6/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 | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
index da19c48..055390b 100644
--- a/drivers/usb/gadget/legacy/gmidi.c
+++ b/drivers/usb/gadget/legacy/gmidi.c
@@ -21,21 +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 "gadget_chips.h"
 
 #include "u_midi.h"
 
@@ -44,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.1.4

[PATCH v4 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 | 173 +++++++++++++++++++++++++++--------
 drivers/usb/gadget/legacy/gmidi.c    |   2 +-
 2 files changed, 136 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index a617df3..f10cf7d 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 {
 	char *id;
 	unsigned int buflen, qlen;
 	bool in_ep_enabled;
+	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
@@ -336,6 +343,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 =
@@ -369,6 +391,19 @@ 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;
 	midi->in_ep_enabled = false;
 }
 
@@ -491,57 +526,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 || !midi->in_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 && midi->in_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)
@@ -667,6 +756,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 |
@@ -1068,6 +1158,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);
@@ -1142,6 +1233,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->buflen = opts->buflen;
 	midi->qlen = opts->qlen;
 	midi->in_ep_enabled = false;
+	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 055390b..3522687 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.1.4

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

[Robert Baldyga]

Hi Felipe,

On 10/26/2015 05:55 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>
> ---
>  drivers/usb/gadget/function/f_midi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index edb84ca..e08f365 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -87,6 +87,7 @@ struct f_midi {
>  	int index;
>  	char *id;
>  	unsigned int buflen, qlen;
> +	bool in_ep_enabled;

It's not necessary, you can use ep->enabled flag instead.

>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	err = f_midi_start_ep(midi, f, midi->in_ep);
>  	if (err)
>  		return err;
> +	midi->in_ep_enabled = true;
>  
>  	err = f_midi_start_ep(midi, f, midi->out_ep);
>  	if (err)
> @@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f)
>  	 */
>  	usb_ep_disable(midi->in_ep);
>  	usb_ep_disable(midi->out_ep);
> +
> +	midi->in_ep_enabled = false;
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>  		}
>  	}
>  
> -	if (req->length > 0) {
> +	if (req->length > 0 && midi->in_ep_enabled) {

You should rather test it at the beginning of this function. Or, even
better, when tasklet is scheduled, because tasklet is the only way this
function can be called when endpoints are disabled.

>  		int err;
>  
>  		err = usb_ep_queue(ep, req, GFP_ATOMIC);
> @@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	midi->index = opts->index;
>  	midi->buflen = opts->buflen;
>  	midi->qlen = opts->qlen;
> +	midi->in_ep_enabled = false;
>  	++opts->refcnt;
>  	mutex_unlock(&opts->lock);
>  
> 

Best regards,
Robert

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

[Robert Baldyga]

On 10/26/2015 05:55 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 4c01c8a..0e9cdeb 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -344,6 +344,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);
>  		}
>  	}
>  
> 

Don't we have similar problem in f_midi_transmit() and f_midi_complete()
functions?

Best regards,
Robert

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

[Robert Baldyga]

On 10/26/2015 05:55 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, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 0e9cdeb..a617df3 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	unsigned i;
>  	int err;
>  
> +	/* We don't care if it is not MIDIStreaming interface */
> +	if (intf != ms_interface_desc.bInterfaceNumber)
> +		return 0;
> +

These global descriptors are overwritten in bind() of each instance of
f_midi, so you have no guarantee that your bInterfaceNumber is correct
for your current instance. Instead you should store value obtained from
usb_interface_id() during bind().

>  	err = f_midi_start_ep(midi, f, midi->in_ep);
>  	if (err)
>  		return err;
> 

Best regards,
Robert

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

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga
<r.baldyga@hackerion.com> wrote:
> Hi Felipe,
>
> On 10/26/2015 05:55 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>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..e08f365 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -87,6 +87,7 @@ struct f_midi {
>>       int index;
>>       char *id;
>>       unsigned int buflen, qlen;
>> +     bool in_ep_enabled;
>
> It's not necessary, you can use ep->enabled flag instead.

There is no such flag in usb_ep struct[1].

[1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170

>
>>  };
>>
>>  static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>       err = f_midi_start_ep(midi, f, midi->in_ep);
>>       if (err)
>>               return err;
>> +     midi->in_ep_enabled = true;
>>
>>       err = f_midi_start_ep(midi, f, midi->out_ep);
>>       if (err)
>> @@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f)
>>        */
>>       usb_ep_disable(midi->in_ep);
>>       usb_ep_disable(midi->out_ep);
>> +
>> +     midi->in_ep_enabled = false;
>>  }
>>
>>  static int f_midi_snd_free(struct snd_device *device)
>> @@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>>               }
>>       }
>>
>> -     if (req->length > 0) {
>> +     if (req->length > 0 && midi->in_ep_enabled) {
>
> You should rather test it at the beginning of this function. Or, even
> better, when tasklet is scheduled, because tasklet is the only way this
> function can be called when endpoints are disabled.

Not in this case, because this function needs to consume the triggered
data from ALSA, otherwise a timeout will happen (which is worse).
Of course this is not the best solution, but it is an incremental improvement.

Patch 7 has the proper solution, which checks this flag at the
beginning of the function as expected. Also, it is more optimal
because it drops all substreams buffers, instead of copying it.

>
>>               int err;
>>
>>               err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> @@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>>       midi->index = opts->index;
>>       midi->buflen = opts->buflen;
>>       midi->qlen = opts->qlen;
>> +     midi->in_ep_enabled = false;
>>       ++opts->refcnt;
>>       mutex_unlock(&opts->lock);
>>
>>
>

Felipe

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

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:30 PM, Robert Baldyga
<r.baldyga@hackerion.com> wrote:
> On 10/26/2015 05:55 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, 4 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index 0e9cdeb..a617df3 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>       unsigned i;
>>       int err;
>>
>> +     /* We don't care if it is not MIDIStreaming interface */
>> +     if (intf != ms_interface_desc.bInterfaceNumber)
>> +             return 0;
>> +
>
> These global descriptors are overwritten in bind() of each instance of
> f_midi, so you have no guarantee that your bInterfaceNumber is correct
> for your current instance. Instead you should store value obtained from
> usb_interface_id() during bind().

Ok.

But then this interface descriptors shouldn't be global static,
because they will always reflect the latest bind() only. Right?

>
>>       err = f_midi_start_ep(midi, f, midi->in_ep);
>>       if (err)
>>               return err;
>>

Felipe

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

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:23 PM, Robert Baldyga
<r.baldyga@hackerion.com> wrote:
> On 10/26/2015 05:55 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 4c01c8a..0e9cdeb 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -344,6 +344,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);
>>               }
>>       }
>>
>>
>
> Don't we have similar problem in f_midi_transmit() and f_midi_complete()
> functions?

Yes and it is been addressed on Patch 7.

Felipe

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

[Robert Baldyga]

On 10/26/2015 11:49 PM, Felipe Tonello wrote:
> Hi Robert,
> 
> On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga
> <r.baldyga@hackerion.com> wrote:
>> Hi Felipe,
>>
>> On 10/26/2015 05:55 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>
>>> ---
>>>  drivers/usb/gadget/function/f_midi.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>> index edb84ca..e08f365 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -87,6 +87,7 @@ struct f_midi {
>>>       int index;
>>>       char *id;
>>>       unsigned int buflen, qlen;
>>> +     bool in_ep_enabled;
>>
>> It's not necessary, you can use ep->enabled flag instead.
> 
> There is no such flag in usb_ep struct[1].
> 
> [1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170

It's already in next branch of Felipe Balbi's tree.

Look here:
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/

Best regards,
Robert

> 
>>
>>>  };
>>>
>>>  static inline struct f_midi *func_to_midi(struct usb_function *f)
>>> @@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>       err = f_midi_start_ep(midi, f, midi->in_ep);
>>>       if (err)
>>>               return err;
>>> +     midi->in_ep_enabled = true;
>>>
>>>       err = f_midi_start_ep(midi, f, midi->out_ep);
>>>       if (err)
>>> @@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f)
>>>        */
>>>       usb_ep_disable(midi->in_ep);
>>>       usb_ep_disable(midi->out_ep);
>>> +
>>> +     midi->in_ep_enabled = false;
>>>  }
>>>
>>>  static int f_midi_snd_free(struct snd_device *device)
>>> @@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>>>               }
>>>       }
>>>
>>> -     if (req->length > 0) {
>>> +     if (req->length > 0 && midi->in_ep_enabled) {
>>
>> You should rather test it at the beginning of this function. Or, even
>> better, when tasklet is scheduled, because tasklet is the only way this
>> function can be called when endpoints are disabled.
> 
> Not in this case, because this function needs to consume the triggered
> data from ALSA, otherwise a timeout will happen (which is worse).
> Of course this is not the best solution, but it is an incremental improvement.
> 
> Patch 7 has the proper solution, which checks this flag at the
> beginning of the function as expected. Also, it is more optimal
> because it drops all substreams buffers, instead of copying it.
> 
>>
>>>               int err;
>>>
>>>               err = usb_ep_queue(ep, req, GFP_ATOMIC);
>>> @@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>>>       midi->index = opts->index;
>>>       midi->buflen = opts->buflen;
>>>       midi->qlen = opts->qlen;
>>> +     midi->in_ep_enabled = false;
>>>       ++opts->refcnt;
>>>       mutex_unlock(&opts->lock);
>>>
>>>
>>
> 
> Felipe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Robert Baldyga]

On 10/26/2015 11:53 PM, Felipe Tonello wrote:
> Hi Robert,
> 
> On Mon, Oct 26, 2015 at 10:30 PM, Robert Baldyga
> <r.baldyga@hackerion.com> wrote:
>> On 10/26/2015 05:55 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, 4 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>> index 0e9cdeb..a617df3 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>       unsigned i;
>>>       int err;
>>>
>>> +     /* We don't care if it is not MIDIStreaming interface */
>>> +     if (intf != ms_interface_desc.bInterfaceNumber)
>>> +             return 0;
>>> +
>>
>> These global descriptors are overwritten in bind() of each instance of
>> f_midi, so you have no guarantee that your bInterfaceNumber is correct
>> for your current instance. Instead you should store value obtained from
>> usb_interface_id() during bind().
> 
> Ok.
> 
> But then this interface descriptors shouldn't be global static,
> because they will always reflect the latest bind() only. Right?

They are copied for each instance of USB function, so they are actually
template to fill and copy in bind(). I'm currently working on some
patches changing a bit behavior of set_alt() to make it clearer, but for
now the only way to distinguish between altsettings properly is to store
bInterfaceNumber value for each function instance in its bind().

Best regards,
Robert

> 
>>
>>>       err = f_midi_start_ep(midi, f, midi->in_ep);
>>>       if (err)
>>>               return err;
>>>
> 
> Felipe

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

[Robert Baldyga]

On 10/26/2015 05:55 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>
> ---
>  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                   | 8 ++++++++
>  drivers/usb/gadget/u_f.h                   | 3 +--
>  5 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index c19f154..4c01c8a 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -202,12 +202,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 3a5ae99..eedea7f 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -307,12 +307,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>  	return alloc_ep_req(ep, len, 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..f78bd1f 100644
> --- a/drivers/usb/gadget/u_f.c
> +++ b/drivers/usb/gadget/u_f.c
> @@ -14,6 +14,7 @@
>  #include <linux/usb/gadget.h>
>  #include "u_f.h"
>  
> +/* 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)
>  {
>  	struct usb_request      *req;
> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>  	return req;
>  }
>  EXPORT_SYMBOL_GPL(alloc_ep_req);
> +
> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> +{
> +	kfree(req->buf);
> +	usb_ep_free_request(ep, req);
> +}
> +EXPORT_SYMBOL_GPL(free_ep_req);
> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
> index 1d5f0eb..2a1a6fb 100644
> --- a/drivers/usb/gadget/u_f.h
> +++ b/drivers/usb/gadget/u_f.h
> @@ -46,7 +46,6 @@ struct usb_ep;
>  struct usb_request;
>  
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>  
>  #endif /* __U_F_H__ */
> -
> -
> 

Isn't it simple enough to be static inline?

Best regards,
Robert

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

[Felipe Ferreri Tonello]

Hi Robert,

On 27/10/15 06:53, Robert Baldyga wrote:
> On 10/26/2015 05:55 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>
>> ---
>>  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                   | 8 ++++++++
>>  drivers/usb/gadget/u_f.h                   | 3 +--
>>  5 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index c19f154..4c01c8a 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -202,12 +202,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 3a5ae99..eedea7f 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -307,12 +307,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>>  	return alloc_ep_req(ep, len, 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..f78bd1f 100644
>> --- a/drivers/usb/gadget/u_f.c
>> +++ b/drivers/usb/gadget/u_f.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/usb/gadget.h>
>>  #include "u_f.h"
>>  
>> +/* 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)
>>  {
>>  	struct usb_request      *req;
>> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>>  	return req;
>>  }
>>  EXPORT_SYMBOL_GPL(alloc_ep_req);
>> +
>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	kfree(req->buf);
>> +	usb_ep_free_request(ep, req);
>> +}
>> +EXPORT_SYMBOL_GPL(free_ep_req);
>> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
>> index 1d5f0eb..2a1a6fb 100644
>> --- a/drivers/usb/gadget/u_f.h
>> +++ b/drivers/usb/gadget/u_f.h
>> @@ -46,7 +46,6 @@ struct usb_ep;
>>  struct usb_request;
>>  
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>  
>>  #endif /* __U_F_H__ */
>> -
>> -
>>
> 
> Isn't it simple enough to be static inline?

inline yes. And the compiler will do it automatically. But I can add it
for clarity.

Make it static it doesn't make sense. This function is exported in the
kernel.

-- 
Felipe

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

[Felipe Ferreri Tonello]

Hi Robert,

On 27/10/15 06:47, Robert Baldyga wrote:
> On 10/26/2015 11:53 PM, Felipe Tonello wrote:
>> Hi Robert,
>>
>> On Mon, Oct 26, 2015 at 10:30 PM, Robert Baldyga
>> <r.baldyga@hackerion.com> wrote:
>>> On 10/26/2015 05:55 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, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index 0e9cdeb..a617df3 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>>       unsigned i;
>>>>       int err;
>>>>
>>>> +     /* We don't care if it is not MIDIStreaming interface */
>>>> +     if (intf != ms_interface_desc.bInterfaceNumber)
>>>> +             return 0;
>>>> +
>>>
>>> These global descriptors are overwritten in bind() of each instance of
>>> f_midi, so you have no guarantee that your bInterfaceNumber is correct
>>> for your current instance. Instead you should store value obtained from
>>> usb_interface_id() during bind().
>>
>> Ok.
>>
>> But then this interface descriptors shouldn't be global static,
>> because they will always reflect the latest bind() only. Right?
> 
> They are copied for each instance of USB function, so they are actually
> template to fill and copy in bind(). I'm currently working on some
> patches changing a bit behavior of set_alt() to make it clearer, but for
> now the only way to distinguish between altsettings properly is to store
> bInterfaceNumber value for each function instance in its bind().

Ok. Thanks.

-- 
Felipe

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

[Felipe Ferreri Tonello]

Hi Robert,

On 27/10/15 06:41, Robert Baldyga wrote:
> On 10/26/2015 11:49 PM, Felipe Tonello wrote:
>> Hi Robert,
>>
>> On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga
>> <r.baldyga@hackerion.com> wrote:
>>> Hi Felipe,
>>>
>>> On 10/26/2015 05:55 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>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index edb84ca..e08f365 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -87,6 +87,7 @@ struct f_midi {
>>>>       int index;
>>>>       char *id;
>>>>       unsigned int buflen, qlen;
>>>> +     bool in_ep_enabled;
>>>
>>> It's not necessary, you can use ep->enabled flag instead.
>>
>> There is no such flag in usb_ep struct[1].
>>
>> [1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170
> 
> It's already in next branch of Felipe Balbi's tree.
> 
> Look here:
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/

Cool. Thanks.

I though that this flag would be very useful, but didn't want to add to
the main struct as it seems no other driver cared about this flag. But
it is good to see that it is been merged now.

-- 
Felipe

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

[Robert Baldyga]

On 10/27/2015 10:18 AM, Felipe Ferreri Tonello wrote:
> Hi Robert,
> 
> On 27/10/15 06:53, Robert Baldyga wrote:
>> On 10/26/2015 05:55 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>
>>> ---
>>>  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                   | 8 ++++++++
>>>  drivers/usb/gadget/u_f.h                   | 3 +--
>>>  5 files changed, 9 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>> index c19f154..4c01c8a 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -202,12 +202,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 3a5ae99..eedea7f 100644
>>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>>> @@ -307,12 +307,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>>>  	return alloc_ep_req(ep, len, 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..f78bd1f 100644
>>> --- a/drivers/usb/gadget/u_f.c
>>> +++ b/drivers/usb/gadget/u_f.c
>>> @@ -14,6 +14,7 @@
>>>  #include <linux/usb/gadget.h>
>>>  #include "u_f.h"
>>>  
>>> +/* 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)
>>>  {
>>>  	struct usb_request      *req;
>>> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>>>  	return req;
>>>  }
>>>  EXPORT_SYMBOL_GPL(alloc_ep_req);
>>> +
>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> +	kfree(req->buf);
>>> +	usb_ep_free_request(ep, req);
>>> +}
>>> +EXPORT_SYMBOL_GPL(free_ep_req);
>>> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
>>> index 1d5f0eb..2a1a6fb 100644
>>> --- a/drivers/usb/gadget/u_f.h
>>> +++ b/drivers/usb/gadget/u_f.h
>>> @@ -46,7 +46,6 @@ struct usb_ep;
>>>  struct usb_request;
>>>  
>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>>  
>>>  #endif /* __U_F_H__ */
>>> -
>>> -
>>>
>>
>> Isn't it simple enough to be static inline?
> 
> inline yes. And the compiler will do it automatically. But I can add it
> for clarity.

No, compiler will never make function inline when you export its symbol.
To make it inline you should place it in header.

> 
> Make it static it doesn't make sense. This function is exported in the
> kernel.
> 

Best regards,
Robert

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

[Felipe Ferreri Tonello]

Hi Robert,

On 27/10/15 09:47, Robert Baldyga wrote:
> On 10/27/2015 10:18 AM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 27/10/15 06:53, Robert Baldyga wrote:
>>> On 10/26/2015 05:55 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>
>>>> ---
>>>>  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                   | 8 ++++++++
>>>>  drivers/usb/gadget/u_f.h                   | 3 +--
>>>>  5 files changed, 9 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index c19f154..4c01c8a 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -202,12 +202,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 3a5ae99..eedea7f 100644
>>>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>>>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>>>> @@ -307,12 +307,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>>>>  	return alloc_ep_req(ep, len, 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..f78bd1f 100644
>>>> --- a/drivers/usb/gadget/u_f.c
>>>> +++ b/drivers/usb/gadget/u_f.c
>>>> @@ -14,6 +14,7 @@
>>>>  #include <linux/usb/gadget.h>
>>>>  #include "u_f.h"
>>>>  
>>>> +/* 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)
>>>>  {
>>>>  	struct usb_request      *req;
>>>> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>>>>  	return req;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(alloc_ep_req);
>>>> +
>>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>>> +{
>>>> +	kfree(req->buf);
>>>> +	usb_ep_free_request(ep, req);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(free_ep_req);
>>>> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
>>>> index 1d5f0eb..2a1a6fb 100644
>>>> --- a/drivers/usb/gadget/u_f.h
>>>> +++ b/drivers/usb/gadget/u_f.h
>>>> @@ -46,7 +46,6 @@ struct usb_ep;
>>>>  struct usb_request;
>>>>  
>>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len);
>>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>>>  
>>>>  #endif /* __U_F_H__ */
>>>> -
>>>> -
>>>>
>>>
>>> Isn't it simple enough to be static inline?
>>
>> inline yes. And the compiler will do it automatically. But I can add it
>> for clarity.
> 
> No, compiler will never make function inline when you export its symbol.
> To make it inline you should place it in header.

Correct.

I will improve it on next revision.

-- 
Felipe

Re: [PATCH v4 0/7] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

Hi Balbi,

On 26/10/15 16:55, Felipe F. Tonello wrote:
> 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 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: f_midi: set altsettings only for MIDIStreaming interface
>   usb: gadget: gmidi: Cleanup legacy code
>   usb: gadget: f_midi: pre-allocate IN requests
> 
>  drivers/usb/gadget/function/f_midi.c       | 201 ++++++++++++++++++++---------
>  drivers/usb/gadget/function/f_sourcesink.c |   6 -
>  drivers/usb/gadget/function/g_zero.h       |   1 -
>  drivers/usb/gadget/legacy/gmidi.c          |  12 +-
>  drivers/usb/gadget/u_f.c                   |   8 ++
>  drivers/usb/gadget/u_f.h                   |   3 +-
>  6 files changed, 151 insertions(+), 80 deletions(-)
> 

I have rebased this patchset on top of your next branch. It removes the
need for patch 1 and patch 5.

I am waiting for more comments on other patches to fix things if needed
before sending v5.

-- 
Felipe