Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753984AbcC1OmM (ORCPT ); Mon, 28 Mar 2016 10:42:12 -0400 Received: from mail-wm0-f48.google.com ([74.125.82.48]:33339 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753334AbcC1OmK convert rfc822-to-8bit (ORCPT ); Mon, 28 Mar 2016 10:42:10 -0400 From: Michal Nazarewicz To: "Felipe F. Tonello" , linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org, Felipe Balbi , Clemens Ladisch Subject: Re: [PATCH v2] usb: gadget: f_midi: fixed a bug when buflen was smaller than wMaxPacketSize In-Reply-To: <1458753021-27400-1-git-send-email-eu@felipetonello.com> Organization: http://mina86.com/ References: <1458753021-27400-1-git-send-email-eu@felipetonello.com> User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.1 (x86_64-unknown-linux-gnu) Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII= X-Face: -TR8(rDTHy/(xl?SfWd1|3:TTgDIatE^t'vop%*gVg[kn$t{EpK(P"VQ=~T2#ysNmJKN$"yTRLB4YQs$4{[.]Fc1)*O]3+XO^oXM>Q#b^ix,O)Zbn)q[y06$`e3?C)`CwR9y5riE=fv^X@x$y?D:XO6L&x4f-}}I4=VRNwiA^t1-ZrVK^07.Pi/57c_du'& X-PGP: 50751FF4 X-PGP-FP: AC1F 5F5C D418 88F8 CC84 5858 2060 4012 5075 1FF4 X-Hashcash: 1:20:160328:balbi@kernel.org::B87dHYgO4eDRHI8O:00mSb X-Hashcash: 1:20:160328:clemens@ladisch.de::72T9ffHP2URxQhDQ:00000000000000000000000000000000000000000003gaH X-Hashcash: 1:20:160328:eu@felipetonello.com::y56VlaEM57iTNSWT:000000000000000000000000000000000000000005vuL X-Hashcash: 1:20:160328:linux-kernel@vger.kernel.org::8mlD6bs32DO17Tu5:0000000000000000000000000000000007nBj X-Hashcash: 1:20:160328:linux-usb@vger.kernel.org::6Gzwc3OS50PDTIBk:000000000000000000000000000000000000KKjT Date: Mon, 28 Mar 2016 10:42:04 -0400 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6081 Lines: 167 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 Acked-by: Michal Nazarewicz 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»