Message-ID: <xa1t4mbqbr5v.fsf@mina86.com>
Date:	Mon, 28 Mar 2016 10:42:04 -0400
From:	Michal Nazarewicz <mina86@...a86.com>
To:	"Felipe F. Tonello" <eu@...ipetonello.com>,
	linux-usb@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
	Clemens Ladisch <clemens@...isch.de>
Subject: Re: [PATCH v2] usb: gadget: f_midi: fixed a bug when buflen was smaller than wMaxPacketSize

On Wed, Mar 23 2016, Felipe F. Tonello wrote:
> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
> devices.
>
> That caused the OUT endpoint to freeze if the host send any data packet of
> length greater than 256 bytes.
>
> This is an example dump of what happended on that enpoint:
> HOST:   [DATA][Length=260][...]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> HOST:   [PING]
> DEVICE: [NAK]
> ...
> HOST:   [PING]
> DEVICE: [NAK]
>
> Users should not change the buffer size to anything other than wMaxPacketSize
> because that can cause bugs (this bug) or performance issues. Thus, this patch
> fixes this problem by eliminating buflen entirely and replacing it with
> wMaxPacketSize of the appropriate endpoint where needed.
>
> Signed-off-by: Felipe F. Tonello <eu@...ipetonello.com>

Acked-by: Michal Nazarewicz <mina86@...a86.com>

My only concern is that now old customers who set buflen module
parameter will break.

It might be better to leave it, document as deprecated and ignore its
value except for printing a warning if user sets it.  With that
approach, a comment indicating the parameter should be removed all
together in say a year, should be added as well.

I’m fine either way though.  Not sure how many people use this gadget
, the parameter and up to date kernel.  Perhaps it’s fine to just break
those few?

> ---
>
> v2:
>  - Removed buflen
>  - use le16_to_cpu() in order to avoid endianess issues
>
>  drivers/usb/gadget/function/f_midi.c | 14 ++++++--------
>  drivers/usb/gadget/function/u_midi.h |  1 -
>  drivers/usb/gadget/legacy/gmidi.c    |  5 -----
>  3 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 8475e3dc82d4..42545538f565 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -93,7 +93,7 @@ struct f_midi {
>  	unsigned int out_ports;
>  	int index;
>  	char *id;
> -	unsigned int buflen, qlen;
> +	unsigned int qlen;
>  	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
>  	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>  	spinlock_t transmit_lock;
> @@ -352,7 +352,8 @@ 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);
> +			midi_alloc_ep_req(midi->in_ep,
> +				le16_to_cpu(bulk_in_desc.wMaxPacketSize));
>  
>  		if (req == NULL)
>  			return -ENOMEM;
> @@ -366,7 +367,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, midi->buflen);
> +			midi_alloc_ep_req(midi->out_ep,
> +				le16_to_cpu(bulk_out_desc.wMaxPacketSize));
>  		if (req == NULL)
>  			return -ENOMEM;
>  
> @@ -615,7 +617,7 @@ static int f_midi_do_transmit(struct f_midi *midi, struct usb_ep *ep)
>  		if (!port->active || !substream)
>  			continue;
>  
> -		while (req->length + 3 < midi->buflen) {
> +		while (req->length + 3 < le16_to_cpu(bulk_in_desc.wMaxPacketSize)) {
>  			uint8_t b;
>  
>  			if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
> @@ -1080,7 +1082,6 @@ end:									\
>  CONFIGFS_ATTR(f_midi_opts_, name);
>  
>  F_MIDI_OPT(index, true, SNDRV_CARDS);
> -F_MIDI_OPT(buflen, false, 0);
>  F_MIDI_OPT(qlen, false, 0);
>  F_MIDI_OPT(in_ports, true, MAX_PORTS);
>  F_MIDI_OPT(out_ports, true, MAX_PORTS);
> @@ -1135,7 +1136,6 @@ CONFIGFS_ATTR(f_midi_opts_, id);
>  
>  static struct configfs_attribute *midi_attrs[] = {
>  	&f_midi_opts_attr_index,
> -	&f_midi_opts_attr_buflen,
>  	&f_midi_opts_attr_qlen,
>  	&f_midi_opts_attr_in_ports,
>  	&f_midi_opts_attr_out_ports,
> @@ -1173,7 +1173,6 @@ 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->qlen = 32;
>  	opts->in_ports = 1;
>  	opts->out_ports = 1;
> @@ -1254,7 +1253,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	midi->in_ports = opts->in_ports;
>  	midi->out_ports = opts->out_ports;
>  	midi->index = opts->index;
> -	midi->buflen = opts->buflen;
>  	midi->qlen = opts->qlen;
>  	midi->in_last_port = 0;
>  
> diff --git a/drivers/usb/gadget/function/u_midi.h b/drivers/usb/gadget/function/u_midi.h
> index 22510189758e..f48b18fcea25 100644
> --- a/drivers/usb/gadget/function/u_midi.h
> +++ b/drivers/usb/gadget/function/u_midi.h
> @@ -25,7 +25,6 @@ struct f_midi_opts {
>  	bool				id_allocated;
>  	unsigned int			in_ports;
>  	unsigned int			out_ports;
> -	unsigned int			buflen;
>  	unsigned int			qlen;
>  
>  	/*
> diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
> index fc2ac150f5ff..436b09155edb 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -47,10 +47,6 @@ 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;
> -module_param(buflen, uint, S_IRUGO);
> -MODULE_PARM_DESC(buflen, "MIDI buffer length");
> -
>  static unsigned int qlen = 32;
>  module_param(qlen, uint, S_IRUGO);
>  MODULE_PARM_DESC(qlen, "USB read and write request queue length");
> @@ -154,7 +150,6 @@ static int midi_bind(struct usb_composite_dev *cdev)
>  	midi_opts->id = id;
>  	midi_opts->in_ports = in_ports;
>  	midi_opts->out_ports = out_ports;
> -	midi_opts->buflen = buflen;
>  	midi_opts->qlen = qlen;
>  
>  	status = usb_string_ids_tab(cdev, strings_dev);
> -- 
> 2.7.4
>

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

Powered by blists - more mailing lists