Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450814 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/9] Gadget endpoint request allocation and MIDI "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200
    [PATCH 6/9] usb: gadget: f_midi: drop substreams when disabling endpoint "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200
    [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200
      Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint Michal Nazarewicz <mina86@mina86.com> - 2016-07-27 22:00 +0200
        Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for  OUT endpoint Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-08-02 19:50 +0200
    [PATCH 4/9] usb: gadget: f_midi: defaults buflen sizes to 512 "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200
      Re: [PATCH 4/9] usb: gadget: f_midi: defaults buflen sizes to 512 Michal Nazarewicz <mina86@mina86.com> - 2016-07-27 21:40 +0200
    [PATCH 9/9] usb: gadget: f_hid: use alloc_ep_req() "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200
    [PATCH 3/9] usb: gadget: f_midi: remove alignment code for OUT endpoint "Felipe F. Tonello" <eu@felipetonello.com> - 2016-07-26 21:20 +0200

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

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

[toc] | [next] | [standalone]

#1450816 — [PATCH 6/9] usb: gadget: f_midi: drop substreams when disabling endpoint

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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»

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1450820 — [PATCH 4/9] usb: gadget: f_midi: defaults buflen sizes to 512

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

[toc] | [prev] | [next] | [standalone]

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

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»

[toc] | [prev] | [next] | [standalone]

#1450821 — [PATCH 9/9] usb: gadget: f_hid: use alloc_ep_req()

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

[toc] | [prev] | [next] | [standalone]

#1450823 — [PATCH 3/9] usb: gadget: f_midi: remove alignment code for OUT endpoint

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web