[PATCH v4 00/10] Gadget endpoint request allocation and MIDI

[PATCH v4 00/10] Gadget endpoint request allocation and MIDI

[Felipe F. Tonello]

As discussed with Baolin Wang, Michal Nazarewicz and Felipe Balbi. I propose
the forced buffer alignment of OUT endpoints USB requests. This is implemented
by patches #1 and #3.

That not just simplifies the driver code, but it also prevents nasty bugs when
buflen is not aligned or even less than wMaxPacketSize.

Patch #10 removes direct calls to usb_ep_alloc_request() and use alloc_ep_req()
instead. If accepted, then we should apply to all other gadgets that uses
usb_ep_alloc_request() when possible and encorage drivers to use it instead.

Changes from v3:
 * Added patch #2 which uses size_t on alloc_ep_req() instead of int.

Changes from v2:
 * Simplified logic in patch #1;
 * Added documentation to alloc_ep_req and free_ep_req;
 * Improved commit message on patch #8.

Changes from v1:
 * Added patches #1, #3, #8, #9 ,#10;
 * Patch #4 removes max_t() for buffer alignment with wMaxPacketSize.

Felipe F. Tonello (10):
  usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align
  usb: gadget: change len to size_t on alloc_ep_req()
  usb: gadget: align buffer size when allocating for OUT endpoint
  usb: gadget: f_midi: remove alignment code for OUT endpoint
  usb: gadget: f_midi: defaults buflen sizes to 512
  usb: gadget: f_midi: refactor state machine
  usb: gadget: f_midi: drop substreams when disabling endpoint
  usb: gadget: remove useless parameter in alloc_ep_req()
  usb: gadget: f_hid: use free_ep_req()
  usb: gadget: f_hid: use alloc_ep_req()

 drivers/usb/gadget/function/f_hid.c        |  26 +---
 drivers/usb/gadget/function/f_loopback.c   |   9 +-
 drivers/usb/gadget/function/f_midi.c       | 240 +++++++++++++++++------------
 drivers/usb/gadget/function/f_sourcesink.c |  11 +-
 drivers/usb/gadget/legacy/gmidi.c          |   2 +-
 drivers/usb/gadget/u_f.c                   |   6 +-
 drivers/usb/gadget/u_f.h                   |  17 +-
 include/linux/usb/gadget.h                 |  17 +-
 8 files changed, 188 insertions(+), 140 deletions(-)

-- 
2.9.2

[PATCH v4 01/10] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Felipe F. Tonello]

USB spec specifies wMaxPacketSize to be little endian (as other properties),
so when using this variable in the driver we should convert to the current
CPU endianness if necessary.

This patch also introduces usb_ep_align() which does always returns the
aligned buffer size for an endpoint. This is useful to be used by USB requests
allocator functions.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 include/linux/usb/gadget.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 612dbdfa388e..3cc93237ff98 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -418,8 +418,20 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
 	list_for_each_entry(tmp, &(gadget)->ep_list, ep_list)
 
 /**
+ * usb_ep_align - returns @len aligned to ep's maxpacketsize.
+ * @ep: the endpoint whose maxpacketsize is used to align @len
+ * @len: buffer size's length to align to @ep's maxpacketsize
+ *
+ * This helper is used to align buffer's size to an ep's maxpacketsize.
+ */
+static inline size_t usb_ep_align(struct usb_ep *ep, size_t len)
+{
+	return round_up(len, (size_t)le16_to_cpu(ep->desc->wMaxPacketSize));
+}
+
+/**
  * usb_ep_align_maybe - returns @len aligned to ep's maxpacketsize if gadget
- *	requires quirk_ep_out_aligned_size, otherwise reguens len.
+ *	requires quirk_ep_out_aligned_size, otherwise returns len.
  * @g: controller to check for quirk
  * @ep: the endpoint whose maxpacketsize is used to align @len
  * @len: buffer size's length to align to @ep's maxpacketsize
@@ -430,8 +442,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
 static inline size_t
 usb_ep_align_maybe(struct usb_gadget *g, struct usb_ep *ep, size_t len)
 {
-	return !g->quirk_ep_out_aligned_size ? len :
-			round_up(len, (size_t)ep->desc->wMaxPacketSize);
+	return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;
 }
 
 /**
-- 
2.9.2

[PATCH v4 02/10] usb: gadget: change len to size_t on alloc_ep_req()

[Felipe F. Tonello]

Length of buffers should be of type size_t whenever possible. Altough
recommended, this change has no real practical change, unless a driver has a
uses a huge or negative buffer size - it might help find these bugs.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/u_f.c | 2 +-
 drivers/usb/gadget/u_f.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 4bc7eea8bfc8..6811761fec64 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -13,7 +13,7 @@
 
 #include "u_f.h"
 
-struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
+struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len)
 {
 	struct usb_request      *req;
 
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 4247cc098a89..21b75710c4a4 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -48,7 +48,7 @@ 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);
+struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
 static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
 {
 	kfree(req->buf);
-- 
2.9.2

[PATCH v4 03/10] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe F. Tonello]

Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
always aligned with wMaxPacketSize (512 usually). This makes sure
that no buffer has the wrong size, which can cause nasty bugs.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/u_f.c |  3 +++
 drivers/usb/gadget/u_f.h | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 6811761fec64..907f8144813c 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -12,6 +12,7 @@
  */
 
 #include "u_f.h"
+#include <linux/usb/ch9.h>
 
 struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len)
 {
@@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len)
 	req = usb_ep_alloc_request(ep, GFP_ATOMIC);
 	if (req) {
 		req->length = len ?: default_len;
+		if (usb_endpoint_dir_out(ep->desc))
+			req->length = usb_ep_align(ep, req->length);
 		req->buf = kmalloc(req->length, GFP_ATOMIC);
 		if (!req->buf) {
 			usb_ep_free_request(ep, req);
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 21b75710c4a4..69a1d10df04f 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -47,8 +47,22 @@
 struct usb_ep;
 struct usb_request;
 
-/* Requests allocated via alloc_ep_req() must be freed by free_ep_req(). */
+/**
+ * alloc_ep_req - returns a usb_request allocated by the gadget driver and
+ * allocates the request's buffer.
+ *
+ * @ep: the endpoint to allocate a usb_request
+ * @len: usb_requests's buffer suggested size
+ * @default_len: used if @len is not provided, ie, is 0
+ *
+ * In case @ep direction is OUT, the @len will be aligned to ep's
+ * wMaxPacketSize. In order to avoid memory leaks or drops, *always* use
+ * usb_requests's length (req->length) to refer to the allocated buffer size.
+ * Requests allocated via alloc_ep_req() *must* be freed by free_ep_req().
+ */
 struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
+
+/* Frees a usb_request previously allocated by alloc_ep_req() */
 static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
 {
 	kfree(req->buf);
-- 
2.9.2

[PATCH v4 04/10] usb: gadget: f_midi: remove alignment code for OUT endpoint

[Felipe F. Tonello]

The new version of alloc_ep_req() already aligns the buffer size to
wMaxPacketSize on OUT endpoints.

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

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 58fc199a18ec..39018dea7035 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -360,9 +360,8 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	/* 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 =
-			midi_alloc_ep_req(midi->out_ep,
-				max_t(unsigned, midi->buflen,
-					bulk_out_desc.wMaxPacketSize));
+			midi_alloc_ep_req(midi->out_ep, midi->buflen);
+
 		if (req == NULL)
 			return -ENOMEM;
 
-- 
2.9.2

[PATCH v4 05/10] usb: gadget: f_midi: defaults buflen sizes to 512

[Felipe F. Tonello]

512 is the value used by wMaxPacketSize, as specified by the USB Spec. This
makes sure this driver uses, by default, the most optimal value for IN and OUT
endpoint requests.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 2 +-
 drivers/usb/gadget/legacy/gmidi.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 39018dea7035..a7b50ac947f8 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1122,7 +1122,7 @@ static struct usb_function_instance *f_midi_alloc_inst(void)
 	opts->func_inst.free_func_inst = f_midi_free_inst;
 	opts->index = SNDRV_DEFAULT_IDX1;
 	opts->id = SNDRV_DEFAULT_STR1;
-	opts->buflen = 256;
+	opts->buflen = 512;
 	opts->qlen = 32;
 	opts->in_ports = 1;
 	opts->out_ports = 1;
diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
index fc2ac150f5ff..0bf39c3ccdb1 100644
--- a/drivers/usb/gadget/legacy/gmidi.c
+++ b/drivers/usb/gadget/legacy/gmidi.c
@@ -47,7 +47,7 @@ static char *id = SNDRV_DEFAULT_STR1;
 module_param(id, charp, S_IRUGO);
 MODULE_PARM_DESC(id, "ID string for the USB MIDI Gadget adapter.");
 
-static unsigned int buflen = 256;
+static unsigned int buflen = 512;
 module_param(buflen, uint, S_IRUGO);
 MODULE_PARM_DESC(buflen, "MIDI buffer length");
 
-- 
2.9.2

[PATCH v4 06/10] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor results in a cleaner state machine code and promotes
consistency, readability, and maintanability of this driver.

This refactor state machine was well tested and it is currently running in
production code and devices.

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

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index a7b50ac947f8..09d769e18b50 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -51,6 +51,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+	STATE_INITIAL = 0,	/* pseudo state */
+	STATE_1PARAM,
+	STATE_2PARAM_1,
+	STATE_2PARAM_2,
+	STATE_SYSEX_0,
+	STATE_SYSEX_1,
+	STATE_SYSEX_2,
+	STATE_REAL_TIME,
+	STATE_FINISHED,		/* pseudo state */
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -61,13 +74,6 @@ struct gmidi_in_port {
 	int active;
 	uint8_t cable;
 	uint8_t state;
-#define STATE_UNKNOWN	0
-#define STATE_1PARAM	1
-#define STATE_2PARAM_1	2
-#define STATE_2PARAM_2	3
-#define STATE_SYSEX_0	4
-#define STATE_SYSEX_1	5
-#define STATE_SYSEX_2	6
 	uint8_t data[2];
 };
 
@@ -403,118 +409,166 @@ static int f_midi_snd_free(struct snd_device *device)
 	return 0;
 }
 
-static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
-					uint8_t p1, uint8_t p2, uint8_t p3)
-{
-	unsigned length = req->length;
-	u8 *buf = (u8 *)req->buf + length;
-
-	buf[0] = p0;
-	buf[1] = p1;
-	buf[2] = p2;
-	buf[3] = p3;
-	req->length = length + 4;
-}
-
 /*
  * Converts MIDI commands to USB MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 				 struct gmidi_in_port *port, uint8_t b)
 {
-	uint8_t p0 = port->cable << 4;
+	uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+	uint8_t next_state = STATE_INITIAL;
+
+	switch (b) {
+	case 0xf8 ... 0xff:
+		/* System Real-Time Messages */
+		p[0] |= 0x0f;
+		p[1] = b;
+		next_state = port->state;
+		port->state = STATE_REAL_TIME;
+		break;
+
+	case 0xf7:
+		/* End of SysEx */
+		switch (port->state) {
+		case STATE_SYSEX_0:
+			p[0] |= 0x05;
+			p[1] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		case STATE_SYSEX_1:
+			p[0] |= 0x06;
+			p[1] = port->data[0];
+			p[2] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		case STATE_SYSEX_2:
+			p[0] |= 0x07;
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		default:
+			/* Ignore byte */
+			next_state = port->state;
+			port->state = STATE_INITIAL;
+		}
+		break;
 
-	if (b >= 0xf8) {
-		f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-	} else if (b >= 0xf0) {
+	case 0xf0 ... 0xf6:
+		/* System Common Messages */
+		port->data[0] = port->data[1] = 0;
+		port->state = STATE_INITIAL;
 		switch (b) {
 		case 0xf0:
 			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
+			port->data[1] = 0;
+			next_state = STATE_SYSEX_1;
 			break;
 		case 0xf1:
 		case 0xf3:
 			port->data[0] = b;
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 			break;
 		case 0xf2:
 			port->data[0] = b;
-			port->state = STATE_2PARAM_1;
+			next_state = STATE_2PARAM_1;
 			break;
 		case 0xf4:
 		case 0xf5:
-			port->state = STATE_UNKNOWN;
+			next_state = STATE_INITIAL;
 			break;
 		case 0xf6:
-			f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-			port->state = STATE_UNKNOWN;
-			break;
-		case 0xf7:
-			switch (port->state) {
-			case STATE_SYSEX_0:
-				f_midi_transmit_packet(req,
-					p0 | 0x05, 0xf7, 0, 0);
-				break;
-			case STATE_SYSEX_1:
-				f_midi_transmit_packet(req,
-					p0 | 0x06, port->data[0], 0xf7, 0);
-				break;
-			case STATE_SYSEX_2:
-				f_midi_transmit_packet(req,
-					p0 | 0x07, port->data[0],
-					port->data[1], 0xf7);
-				break;
-			}
-			port->state = STATE_UNKNOWN;
+			p[0] |= 0x05;
+			p[1] = 0xf6;
+			next_state = STATE_FINISHED;
 			break;
 		}
-	} else if (b >= 0x80) {
+		break;
+
+	case 0x80 ... 0xef:
+		/*
+		 * Channel Voice Messages, Channel Mode Messages
+		 * and Control Change Messages.
+		 */
 		port->data[0] = b;
+		port->data[1] = 0;
+		port->state = STATE_INITIAL;
 		if (b >= 0xc0 && b <= 0xdf)
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 		else
-			port->state = STATE_2PARAM_1;
-	} else { /* b < 0x80 */
+			next_state = STATE_2PARAM_1;
+		break;
+
+	case 0x00 ... 0x7f:
+		/* Message parameters */
 		switch (port->state) {
 		case STATE_1PARAM:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-			} else {
-				p0 |= 0x02;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req, p0, port->data[0], b, 0);
+			if (port->data[0] < 0xf0)
+				p[0] |= port->data[0] >> 4;
+			else
+				p[0] |= 0x02;
+
+			p[1] = port->data[0];
+			p[2] = b;
+			/* This is to allow Running State Messages */
+			next_state = STATE_1PARAM;
 			break;
 		case STATE_2PARAM_1:
 			port->data[1] = b;
-			port->state = STATE_2PARAM_2;
+			next_state = STATE_2PARAM_2;
 			break;
 		case STATE_2PARAM_2:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-				port->state = STATE_2PARAM_1;
-			} else {
-				p0 |= 0x03;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req,
-				p0, port->data[0], port->data[1], b);
+			if (port->data[0] < 0xf0)
+				p[0] |= port->data[0] >> 4;
+			else
+				p[0] |= 0x03;
+
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = b;
+			/* This is to allow Running State Messages */
+			next_state = STATE_2PARAM_1;
 			break;
 		case STATE_SYSEX_0:
 			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
+			next_state = STATE_SYSEX_1;
 			break;
 		case STATE_SYSEX_1:
 			port->data[1] = b;
-			port->state = STATE_SYSEX_2;
+			next_state = STATE_SYSEX_2;
 			break;
 		case STATE_SYSEX_2:
-			f_midi_transmit_packet(req,
-				p0 | 0x04, port->data[0], port->data[1], b);
-			port->state = STATE_SYSEX_0;
+			p[0] |= 0x04;
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = b;
+			next_state = STATE_SYSEX_0;
 			break;
 		}
+		break;
+	}
+
+	/* States where we have to write into the USB request */
+	if (next_state == STATE_FINISHED ||
+	    port->state == STATE_SYSEX_2 ||
+	    port->state == STATE_1PARAM ||
+	    port->state == STATE_2PARAM_2 ||
+	    port->state == STATE_REAL_TIME) {
+
+		unsigned int length = req->length;
+		u8 *buf = (u8 *)req->buf + length;
+
+		memcpy(buf, p, sizeof(p));
+		req->length = length + sizeof(p);
+
+		if (next_state == STATE_FINISHED) {
+			next_state = STATE_INITIAL;
+			port->data[0] = port->data[1] = 0;
+		}
 	}
+
+	port->state = next_state;
 }
 
 static void f_midi_drop_out_substreams(struct f_midi *midi)
@@ -641,7 +695,7 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 	VDBG(midi, "%s()\n", __func__);
 	port = midi->in_ports_array + substream->number;
 	port->substream = substream;
-	port->state = STATE_UNKNOWN;
+	port->state = STATE_INITIAL;
 	return 0;
 }
 
-- 
2.9.2

[PATCH v4 07/10] usb: gadget: f_midi: drop substreams when disabling endpoint

[Felipe F. Tonello]

This change makes sure that the ALSA buffers are cleaned if an endpoint
becomes disabled.

Before this change, if the internal ALSA buffer did overflow, the MIDI
function would stop sending MIDI to the host.

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

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 09d769e18b50..3a47596afcab 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -305,6 +305,19 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
 	}
 }
 
+static void f_midi_drop_out_substreams(struct f_midi *midi)
+{
+	unsigned int i;
+
+	for (i = 0; i < midi->in_ports; i++) {
+		struct gmidi_in_port *port = midi->in_ports_array + i;
+		struct snd_rawmidi_substream *substream = port->substream;
+
+		if (port->active && substream)
+			snd_rawmidi_drop_output(substream);
+	}
+}
+
 static int f_midi_start_ep(struct f_midi *midi,
 			   struct usb_function *f,
 			   struct usb_ep *ep)
@@ -402,6 +415,8 @@ static void f_midi_disable(struct usb_function *f)
 	/* release IN requests */
 	while (kfifo_get(&midi->in_req_fifo, &req))
 		free_ep_req(midi->in_ep, req);
+
+	f_midi_drop_out_substreams(midi);
 }
 
 static int f_midi_snd_free(struct snd_device *device)
@@ -571,18 +586,6 @@ static void f_midi_transmit_byte(struct usb_request *req,
 	port->state = next_state;
 }
 
-static void f_midi_drop_out_substreams(struct f_midi *midi)
-{
-	unsigned int i;
-
-	for (i = 0; i < midi->in_ports; i++) {
-		struct gmidi_in_port *port = midi->in_ports_array + i;
-		struct snd_rawmidi_substream *substream = port->substream;
-		if (port->active && substream)
-			snd_rawmidi_drop_output(substream);
-	}
-}
-
 static int f_midi_do_transmit(struct f_midi *midi, struct usb_ep *ep)
 {
 	struct usb_request *req = NULL;
-- 
2.9.2

[PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe F. Tonello]

The default_length parameter of alloc_ep_req was not really necessary
and gadget drivers would almost always create an inline function to pass
the same value to len and default_len.

So this patch also removes duplicate code from few drivers.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_hid.c        | 10 ++--------
 drivers/usb/gadget/function/f_loopback.c   |  9 +--------
 drivers/usb/gadget/function/f_midi.c       | 10 ++--------
 drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
 drivers/usb/gadget/u_f.c                   |  7 +++----
 drivers/usb/gadget/u_f.h                   |  3 +--
 6 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 51980c50546d..e82a7468252e 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
 /*-------------------------------------------------------------------------*/
 /*                                usb_function                             */
 
-static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
-						    unsigned length)
-{
-	return alloc_ep_req(ep, length, length);
-}
-
 static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_hidg *hidg = (struct f_hidg *) req->context;
@@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		 */
 		for (i = 0; i < hidg->qlen && status == 0; i++) {
 			struct usb_request *req =
-					hidg_alloc_ep_req(hidg->out_ep,
-							  hidg->report_length);
+				alloc_ep_req(hidg->out_ep,
+					hidg->report_length);
 			if (req) {
 				req->complete = hidg_set_report_complete;
 				req->context  = hidg;
diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c
index 3a9f8f9c77bd..701ee0f11c33 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
 	VDBG(cdev, "%s disabled\n", loop->function.name);
 }
 
-static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
-{
-	struct f_loopback	*loop = ep->driver_data;
-
-	return alloc_ep_req(ep, len, loop->buflen);
-}
-
 static int alloc_requests(struct usb_composite_dev *cdev,
 			  struct f_loopback *loop)
 {
@@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
 		if (!in_req)
 			goto fail;
 
-		out_req = lb_alloc_ep_req(loop->out_ep, 0);
+		out_req = alloc_ep_req(loop->out_ep, loop->buflen);
 		if (!out_req)
 			goto fail_in;
 
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 3a47596afcab..abf26364b46f 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
 	NULL,
 };
 
-static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
-						    unsigned length)
-{
-	return alloc_ep_req(ep, length, length);
-}
-
 static const uint8_t f_midi_cin_length[] = {
 	0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
 };
@@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	/* 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);
+			alloc_ep_req(midi->in_ep, midi->buflen);
 
 		if (req == NULL)
 			return -ENOMEM;
@@ -379,7 +373,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	/* 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 =
-			midi_alloc_ep_req(midi->out_ep, midi->buflen);
+			alloc_ep_req(midi->out_ep, midi->buflen);
 
 		if (req == NULL)
 			return -ENOMEM;
diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index df0189ddfdd5..517d78d6da78 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -291,13 +291,6 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
 
 /*-------------------------------------------------------------------------*/
 
-static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
-{
-	struct f_sourcesink		*ss = ep->driver_data;
-
-	return alloc_ep_req(ep, len, ss->buflen);
-}
-
 static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
 {
 	int			value;
@@ -606,11 +599,11 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
 	} else {
 		ep = is_in ? ss->in_ep : ss->out_ep;
 		qlen = ss->bulk_qlen;
-		size = 0;
+		size = ss->buflen;
 	}
 
 	for (i = 0; i < qlen; i++) {
-		req = ss_alloc_ep_req(ep, size);
+		req = alloc_ep_req(ep, size);
 		if (!req)
 			return -ENOMEM;
 
diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 907f8144813c..18839732c840 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -14,15 +14,14 @@
 #include "u_f.h"
 #include <linux/usb/ch9.h>
 
-struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len)
+struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len)
 {
 	struct usb_request      *req;
 
 	req = usb_ep_alloc_request(ep, GFP_ATOMIC);
 	if (req) {
-		req->length = len ?: default_len;
-		if (usb_endpoint_dir_out(ep->desc))
-			req->length = usb_ep_align(ep, req->length);
+		req->length = usb_endpoint_dir_out(ep->desc) ?
+			usb_ep_align(ep, len) : len;
 		req->buf = kmalloc(req->length, GFP_ATOMIC);
 		if (!req->buf) {
 			usb_ep_free_request(ep, req);
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 69a1d10df04f..7d53a4773d1a 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -53,14 +53,13 @@ struct usb_request;
  *
  * @ep: the endpoint to allocate a usb_request
  * @len: usb_requests's buffer suggested size
- * @default_len: used if @len is not provided, ie, is 0
  *
  * In case @ep direction is OUT, the @len will be aligned to ep's
  * wMaxPacketSize. In order to avoid memory leaks or drops, *always* use
  * usb_requests's length (req->length) to refer to the allocated buffer size.
  * Requests allocated via alloc_ep_req() *must* be freed by free_ep_req().
  */
-struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
+struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len);
 
 /* Frees a usb_request previously allocated by alloc_ep_req() */
 static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-- 
2.9.2

[PATCH v4 09/10] usb: gadget: f_hid: use free_ep_req()

[Felipe F. Tonello]

We should always use free_ep_req() when allocating requests with
alloc_ep_req().

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

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index e82a7468252e..a010496e4e05 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -671,11 +671,8 @@ fail_free_descs:
 	usb_free_all_descriptors(f);
 fail:
 	ERROR(f->config->cdev, "hidg_bind FAILED\n");
-	if (hidg->req != NULL) {
-		kfree(hidg->req->buf);
-		if (hidg->in_ep != NULL)
-			usb_ep_free_request(hidg->in_ep, hidg->req);
-	}
+	if (hidg->req != NULL)
+		free_ep_req(hidg->in_ep, hidg->req);
 
 	return status;
 }
@@ -904,8 +901,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 
 	/* disable/free request and end point */
 	usb_ep_disable(hidg->in_ep);
-	kfree(hidg->req->buf);
-	usb_ep_free_request(hidg->in_ep, hidg->req);
+	free_ep_req(hidg->in_ep, hidg->req);
 
 	usb_free_all_descriptors(f);
 }
-- 
2.9.2

[PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe F. Tonello]

Use gadget's framework allocation function instead of directly calling
usb_ep_alloc_request().

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

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index a010496e4e05..89d2e9a5a04f 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 
 	/* preallocate request and buffer */
 	status = -ENOMEM;
-	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
+	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
 	if (!hidg->req)
 		goto fail;
 
-	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
-	if (!hidg->req->buf)
-		goto fail;
-
 	/* set descriptor dynamic values */
 	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
 	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
-- 
2.9.2

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Balbi]

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


Hi,

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> The default_length parameter of alloc_ep_req was not really necessary
> and gadget drivers would almost always create an inline function to pass
> the same value to len and default_len.
>
> So this patch also removes duplicate code from few drivers.
>
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>  drivers/usb/gadget/u_f.c                   |  7 +++----
>  drivers/usb/gadget/u_f.h                   |  3 +--
>  6 files changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 51980c50546d..e82a7468252e 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>  /*-------------------------------------------------------------------------*/
>  /*                                usb_function                             */
>  
> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
> -						    unsigned length)
> -{
> -	return alloc_ep_req(ep, length, length);
> -}

actually, I prefer to keep these little helpers. I was recently playing
with adding SG list support to g_zero (I should have patches soon) and
it was actually very nice to have the sourcesink helper as I could just
ditch alloc_ep_req(). The change to the driver was local to
ss_alloc_ep_req() and nothing else changed :-)

-- 
balbi

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

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[John Youn]

On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
> Use gadget's framework allocation function instead of directly calling
> usb_ep_alloc_request().
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/gadget/function/f_hid.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index a010496e4e05..89d2e9a5a04f 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	/* preallocate request and buffer */
>  	status = -ENOMEM;
> -	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
> +	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>  	if (!hidg->req)
>  		goto fail;
>  
> -	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
> -	if (!hidg->req->buf)
> -		goto fail;
> -
>  	/* set descriptor dynamic values */
>  	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>  	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> 

Hi Felipe,

This commit on your testing/next breaks compilation.

../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to function ‘alloc_ep_req’
  hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
              ^
In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
../drivers/usb/gadget/u_f.h:63:21: note: declared here
 struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
                     ^

Regards,
John

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>> Use gadget's framework allocation function instead of directly calling
>> usb_ep_alloc_request().
>> 
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_hid.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>> 
>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>> index a010496e4e05..89d2e9a5a04f 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>>  
>>  	/* preallocate request and buffer */
>>  	status = -ENOMEM;
>> -	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>> +	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>  	if (!hidg->req)
>>  		goto fail;
>>  
>> -	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>> -	if (!hidg->req->buf)
>> -		goto fail;
>> -
>>  	/* set descriptor dynamic values */
>>  	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>  	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>> 
>
> Hi Felipe,
>
> This commit on your testing/next breaks compilation.
>
> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to function ‘alloc_ep_req’
>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>               ^
> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);

true that :-) Dropping from my queue.

-- 
balbi

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

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

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

Hi Blabi,

On 18/08/16 08:12, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>> The default_length parameter of alloc_ep_req was not really necessary
>> and gadget drivers would almost always create an inline function to pass
>> the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>>  drivers/usb/gadget/u_f.c                   |  7 +++----
>>  drivers/usb/gadget/u_f.h                   |  3 +--
>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>>  /*-------------------------------------------------------------------------*/
>>  /*                                usb_function                             */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -						    unsigned length)
>> -{
>> -	return alloc_ep_req(ep, length, length);
>> -}
> 
> actually, I prefer to keep these little helpers. I was recently playing
> with adding SG list support to g_zero (I should have patches soon) and
> it was actually very nice to have the sourcesink helper as I could just
> ditch alloc_ep_req(). The change to the driver was local to
> ss_alloc_ep_req() and nothing else changed :-)
> 

Right, but then it is worth to have the helper function. In this
particular case, I am removing a useless helper functions, especially
that the previous patch removes the need for the optional parameter in
alloc_ep_req.

-- 
Felipe

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

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

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

Hi,

On 22/08/16 08:45, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>> Use gadget's framework allocation function instead of directly calling
>>> usb_ep_alloc_request().
>>>
>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c | 6 +-----
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>> index a010496e4e05..89d2e9a5a04f 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>>>  
>>>  	/* preallocate request and buffer */
>>>  	status = -ENOMEM;
>>> -	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>> +	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>  	if (!hidg->req)
>>>  		goto fail;
>>>  
>>> -	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>> -	if (!hidg->req->buf)
>>> -		goto fail;
>>> -
>>>  	/* set descriptor dynamic values */
>>>  	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>>  	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>
>>
>> Hi Felipe,
>>
>> This commit on your testing/next breaks compilation.
>>
>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to function ‘alloc_ep_req’
>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>               ^
>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
> 
> true that :-) Dropping from my queue.
> 

Are you applying the previous patches? Specially that this is the last
patch in the series, how can it break with you if it doesn't break here?
What should I do then?

Thanks

-- 
Felipe

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

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Balbi]

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


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>> The default_length parameter of alloc_ep_req was not really necessary
>>> and gadget drivers would almost always create an inline function to pass
>>> the same value to len and default_len.
>>>
>>> So this patch also removes duplicate code from few drivers.
>>>
>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>>>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>>>  drivers/usb/gadget/u_f.c                   |  7 +++----
>>>  drivers/usb/gadget/u_f.h                   |  3 +--
>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>> index 51980c50546d..e82a7468252e 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>>>  /*-------------------------------------------------------------------------*/
>>>  /*                                usb_function                             */
>>>  
>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>> -						    unsigned length)
>>> -{
>>> -	return alloc_ep_req(ep, length, length);
>>> -}
>> 
>> actually, I prefer to keep these little helpers. I was recently playing
>> with adding SG list support to g_zero (I should have patches soon) and
>> it was actually very nice to have the sourcesink helper as I could just
>> ditch alloc_ep_req(). The change to the driver was local to
>> ss_alloc_ep_req() and nothing else changed :-)
>> 
>
> Right, but then it is worth to have the helper function. In this
> particular case, I am removing a useless helper functions, especially
> that the previous patch removes the need for the optional parameter in
> alloc_ep_req.

it's a static inline :-) It won't do any bad to keep it. And, as I said,
if we want to ditch aloc_ep_req() eventually, then we have just one
place to patch ;-)

-- 
balbi

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

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Balbi]

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


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> John Youn <John.Youn@synopsys.com> writes:
>>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>>> Use gadget's framework allocation function instead of directly calling
>>>> usb_ep_alloc_request().
>>>>
>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_hid.c | 6 +-----
>>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>>> index a010496e4e05..89d2e9a5a04f 100644
>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>>>>  
>>>>  	/* preallocate request and buffer */
>>>>  	status = -ENOMEM;
>>>> -	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>>> +	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>  	if (!hidg->req)
>>>>  		goto fail;
>>>>  
>>>> -	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>>> -	if (!hidg->req->buf)
>>>> -		goto fail;
>>>> -
>>>>  	/* set descriptor dynamic values */
>>>>  	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>>>  	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>>
>>>
>>> Hi Felipe,
>>>
>>> This commit on your testing/next breaks compilation.
>>>
>>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to function ‘alloc_ep_req’
>>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>               ^
>>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
>> 
>> true that :-) Dropping from my queue.
>> 
>
> Are you applying the previous patches? Specially that this is the last
> patch in the series, how can it break with you if it doesn't break here?
> What should I do then?

Can you rebase your series on top of my testing/next? My HEAD is
at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

-- 
balbi

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

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

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

Hi Balbi,

On 23/08/16 12:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>> and gadget drivers would almost always create an inline function to pass
>>>> the same value to len and default_len.
>>>>
>>>> So this patch also removes duplicate code from few drivers.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>>>>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>>>>  drivers/usb/gadget/u_f.c                   |  7 +++----
>>>>  drivers/usb/gadget/u_f.h                   |  3 +--
>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>>> index 51980c50546d..e82a7468252e 100644
>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>>>>  /*-------------------------------------------------------------------------*/
>>>>  /*                                usb_function                             */
>>>>  
>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>> -						    unsigned length)
>>>> -{
>>>> -	return alloc_ep_req(ep, length, length);
>>>> -}
>>>
>>> actually, I prefer to keep these little helpers. I was recently playing
>>> with adding SG list support to g_zero (I should have patches soon) and
>>> it was actually very nice to have the sourcesink helper as I could just
>>> ditch alloc_ep_req(). The change to the driver was local to
>>> ss_alloc_ep_req() and nothing else changed :-)
>>>
>>
>> Right, but then it is worth to have the helper function. In this
>> particular case, I am removing a useless helper functions, especially
>> that the previous patch removes the need for the optional parameter in
>> alloc_ep_req.
> 
> it's a static inline :-) It won't do any bad to keep it. And, as I said,
> if we want to ditch aloc_ep_req() eventually, then we have just one
> place to patch ;-)

Yes, sure. But why drop alloc_ep_req()?

So should I keep all these helper functions? If so, I actually still
need to fix them to use the newer alloc_ep_req() API.

-- 
Felipe

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

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

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

Hi Balbi,

On 23/08/16 12:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>> John Youn <John.Youn@synopsys.com> writes:
>>>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>>>> Use gadget's framework allocation function instead of directly calling
>>>>> usb_ep_alloc_request().
>>>>>
>>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>>> ---
>>>>>  drivers/usb/gadget/function/f_hid.c | 6 +-----
>>>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>>>> index a010496e4e05..89d2e9a5a04f 100644
>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>>>>>  
>>>>>  	/* preallocate request and buffer */
>>>>>  	status = -ENOMEM;
>>>>> -	hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>>>> +	hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>>  	if (!hidg->req)
>>>>>  		goto fail;
>>>>>  
>>>>> -	hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>>>> -	if (!hidg->req->buf)
>>>>> -		goto fail;
>>>>> -
>>>>>  	/* set descriptor dynamic values */
>>>>>  	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>>>>  	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> This commit on your testing/next breaks compilation.
>>>>
>>>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>>>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to function ‘alloc_ep_req’
>>>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>               ^
>>>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>>>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int default_len);
>>>
>>> true that :-) Dropping from my queue.
>>>
>>
>> Are you applying the previous patches? Specially that this is the last
>> patch in the series, how can it break with you if it doesn't break here?
>> What should I do then?
> 
> Can you rebase your series on top of my testing/next? My HEAD is
> at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

It was at that time. I will rebease it then on v5. Just waiting for your
comments on the other thread.

Thanks

-- 
Felipe

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

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Balbi]

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


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>>> and gadget drivers would almost always create an inline function to pass
>>>>> the same value to len and default_len.
>>>>>
>>>>> So this patch also removes duplicate code from few drivers.
>>>>>
>>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>>> ---
>>>>>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>>>>>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>>>>>  drivers/usb/gadget/u_f.c                   |  7 +++----
>>>>>  drivers/usb/gadget/u_f.h                   |  3 +--
>>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>>>> index 51980c50546d..e82a7468252e 100644
>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>>>>>  /*-------------------------------------------------------------------------*/
>>>>>  /*                                usb_function                             */
>>>>>  
>>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>>> -						    unsigned length)
>>>>> -{
>>>>> -	return alloc_ep_req(ep, length, length);
>>>>> -}
>>>>
>>>> actually, I prefer to keep these little helpers. I was recently playing
>>>> with adding SG list support to g_zero (I should have patches soon) and
>>>> it was actually very nice to have the sourcesink helper as I could just
>>>> ditch alloc_ep_req(). The change to the driver was local to
>>>> ss_alloc_ep_req() and nothing else changed :-)
>>>>
>>>
>>> Right, but then it is worth to have the helper function. In this
>>> particular case, I am removing a useless helper functions, especially
>>> that the previous patch removes the need for the optional parameter in
>>> alloc_ep_req.
>> 
>> it's a static inline :-) It won't do any bad to keep it. And, as I said,
>> if we want to ditch aloc_ep_req() eventually, then we have just one
>> place to patch ;-)
>
> Yes, sure. But why drop alloc_ep_req()?

because we can't find a generic way of allocating sglist with enough
entries :-) Some drivers (like f_fs.c) could actually have zero-copy
sglist by just pinning user pages with get_user_pages_fast() and
following with sg_alloc_from_pages().

g_zero, however, would just "emulate" sglist by just allocating a
statically sized sg table and initializing chunks of the linear req->buf
to each sg entry.

> So should I keep all these helper functions? If so, I actually still
> need to fix them to use the newer alloc_ep_req() API.

yeah, keeping the helper functions would be nice. IMHO, alloc_ep_req()
doesn't have a long life, but it's pretty good for the time being.

-- 
balbi

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

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

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



On 29/08/16 08:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>>>>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>>>> and gadget drivers would almost always create an inline function to pass
>>>>>> the same value to len and default_len.
>>>>>>
>>>>>> So this patch also removes duplicate code from few drivers.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>>>> ---
>>>>>>  drivers/usb/gadget/function/f_hid.c        | 10 ++--------
>>>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +--------
>>>>>>  drivers/usb/gadget/function/f_midi.c       | 10 ++--------
>>>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++---------
>>>>>>  drivers/usb/gadget/u_f.c                   |  7 +++----
>>>>>>  drivers/usb/gadget/u_f.h                   |  3 +--
>>>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
>>>>>> index 51980c50546d..e82a7468252e 100644
>>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
>>>>>>  /*-------------------------------------------------------------------------*/
>>>>>>  /*                                usb_function                             */
>>>>>>  
>>>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>>>> -						    unsigned length)
>>>>>> -{
>>>>>> -	return alloc_ep_req(ep, length, length);
>>>>>> -}
>>>>>
>>>>> actually, I prefer to keep these little helpers. I was recently playing
>>>>> with adding SG list support to g_zero (I should have patches soon) and
>>>>> it was actually very nice to have the sourcesink helper as I could just
>>>>> ditch alloc_ep_req(). The change to the driver was local to
>>>>> ss_alloc_ep_req() and nothing else changed :-)
>>>>>
>>>>
>>>> Right, but then it is worth to have the helper function. In this
>>>> particular case, I am removing a useless helper functions, especially
>>>> that the previous patch removes the need for the optional parameter in
>>>> alloc_ep_req.
>>>
>>> it's a static inline :-) It won't do any bad to keep it. And, as I said,
>>> if we want to ditch aloc_ep_req() eventually, then we have just one
>>> place to patch ;-)
>>
>> Yes, sure. But why drop alloc_ep_req()?
> 
> because we can't find a generic way of allocating sglist with enough
> entries :-) Some drivers (like f_fs.c) could actually have zero-copy
> sglist by just pinning user pages with get_user_pages_fast() and
> following with sg_alloc_from_pages().
> 
> g_zero, however, would just "emulate" sglist by just allocating a
> statically sized sg table and initializing chunks of the linear req->buf
> to each sg entry.

I see. :)

> 
>> So should I keep all these helper functions? If so, I actually still
>> need to fix them to use the newer alloc_ep_req() API.
> 
> yeah, keeping the helper functions would be nice. IMHO, alloc_ep_req()
> doesn't have a long life, but it's pretty good for the time being.

Ok, I did it on my last patchset I sent, I think you already applied to
your tree.

-- 
Felipe

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