[PATCH v2 0/9] Gadget endpoint request allocation and MIDI

[PATCH v2 0/9] 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 #2.

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

Patch #9 is more of a POC. It 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.

Everything else is self-explanatory.

Changes from v1:
 * Added patches 1, 2, 7, 8 ,9.
 * Patch 3 removes max_t() for buffer alignment with wMaxPacketSize

Felipe F. Tonello (9):
  usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align
  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                   |   2 +-
 include/linux/usb/gadget.h                 |  17 +-
 8 files changed, 174 insertions(+), 139 deletions(-)

-- 
2.9.0

[PATCH 1/9] 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..b8fa6901b881 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 ? len : usb_ep_align(ep, len);
 }
 
 /**
-- 
2.9.0

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

diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 4bc7eea8bfc8..d1933b0b76c3 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, int len, int default_len)
 {
@@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 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);
-- 
2.9.0

[PATCH 3/9] 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.0

[PATCH 4/9] 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.0

[PATCH 5/9] 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.0

[PATCH 6/9] 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.0

[PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe F. Tonello]

This parameter 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                   |  2 +-
 6 files changed, 11 insertions(+), 38 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 d1933b0b76c3..dfc40f7155ac 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, int 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, req->length) : 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 4247cc098a89..2852c9d7a85d 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);
 static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
 {
 	kfree(req->buf);
-- 
2.9.0

[PATCH 8/9] 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.0

[PATCH 9/9] 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.0

[PATCH v2 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe F. Tonello]

This parameter 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                   |  2 +-
 6 files changed, 11 insertions(+), 38 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 d1933b0b76c3..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, int 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 4247cc098a89..2852c9d7a85d 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);
 static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
 {
 	kfree(req->buf);
-- 
2.9.0

Re: [PATCH v2 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

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

Forgot to mention, but changes from v1 is a typo alloc_ep_req().

On 26/07/16 20:18, Felipe F. Tonello wrote:
> This parameter 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                   |  2 +-
>  6 files changed, 11 insertions(+), 38 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 d1933b0b76c3..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, int 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 4247cc098a89..2852c9d7a85d 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);
>  static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>  {
>  	kfree(req->buf);
> 

-- 
Felipe

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

Re: [PATCH 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> 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>

Acked-by: Michal Nazarewicz <mina86@mina86.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..b8fa6901b881 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 ? len : usb_ep_align(ep, len);

I’d drop the negation:

+	return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;

>  }
>  
>  /**
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 4/9] usb: gadget: f_midi: defaults buflen sizes to 512

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> 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>

Acked-by: Michal Nazarewicz <mina86@mina86.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.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> 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 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
> index 4bc7eea8bfc8..d1933b0b76c3 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, int len, int default_len)
>  {
> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 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);

I’m a bit scared of this change.

Drivers which call alloc_ep_req and then ignore req->length using the
same length they passed to the function will silently drop data.

Drivers which do not ignore req->length may end up overwriting some
other buffer, e.g.:

	some_buffer = kmalloc(length, GFP_KERNEL);
        req = alloc_ep_req(ep, length, 0);
        … later …
        memcpy(some_buffer, req->buf, req->length);

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> This parameter was not really necessary and gadget drivers would almost always

I personally like when commit messages can be read without subject, so
perhaps:

	The default_length parameter of alloc_ep_req was not …

But that’s just me.

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

Acked-by: Michal Nazarewicz <mina86@mina86.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                   |  2 +-
>  6 files changed, 11 insertions(+), 38 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 d1933b0b76c3..dfc40f7155ac 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, int 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, req->length) : 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 4247cc098a89..2852c9d7a85d 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);
>  static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>  {
>  	kfree(req->buf);
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 8/9] usb: gadget: f_hid: use free_ep_req()

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> We should always use free_ep_req() when allocating requests with
> alloc_ep_req().
>
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Acked-by: Michal Nazarewicz <mina86@mina86.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.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Michal Nazarewicz]

On Tue, Jul 26 2016, 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>

Acked-by: Michal Nazarewicz <mina86@mina86.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.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Felipe Ferreri Tonello]

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

Hi Michal,

On 27/07/16 20:37, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> 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>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.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..b8fa6901b881 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 ? len : usb_ep_align(ep, len);
> 
> I’d drop the negation:
> 
> +	return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;

Agreed.

> 
>>  }
>>  
>>  /**
>> -- 
>> 2.9.0
>>
> 

-- 
Felipe

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

Re: [PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

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

Hi Michal,

On 27/07/16 21:02, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> This parameter was not really necessary and gadget drivers would almost always
> 
> I personally like when commit messages can be read without subject, so
> perhaps:
> 
> 	The default_length parameter of alloc_ep_req was not …
> 
> But that’s just me.

Good judgment.

> 
>> 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>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.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                   |  2 +-
>>  6 files changed, 11 insertions(+), 38 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 d1933b0b76c3..dfc40f7155ac 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, int 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, req->length) : 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 4247cc098a89..2852c9d7a85d 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);
>>  static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>  {
>>  	kfree(req->buf);
>> -- 
>> 2.9.0
>>
> 

-- 
Felipe

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

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

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

Hi Michal,

On 27/07/16 20:59, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> 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 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>> index 4bc7eea8bfc8..d1933b0b76c3 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, int len, int default_len)
>>  {
>> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 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);
> 
> I’m a bit scared of this change.

I agree, it's scary. :D

> 
> Drivers which call alloc_ep_req and then ignore req->length using the
> same length they passed to the function will silently drop data.
> 
> Drivers which do not ignore req->length may end up overwriting some
> other buffer, e.g.:
> 
> 	some_buffer = kmalloc(length, GFP_KERNEL);
>         req = alloc_ep_req(ep, length, 0);
>         … later …
>         memcpy(some_buffer, req->buf, req->length);

True. The same happens if the data associated with an OUT endpoint is
smaller than wMaxPacketSize.

This patch doesn't fix all problems associated with that, but it allows
better practice to take place. It returns to the driver the actual
allocated size, like several POSIX functions.

I haven't seen any problems on all gadgets that rely on alloc_ep_req().
Maybe as we port other gadgets to this use this function instead of
usb_ep_alloc_request() we might find some issues.

Perhaps we should add better documentation to alloc_ep_req()?

-- 
Felipe

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