From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755328AbcGHN0Z (ORCPT ); Fri, 8 Jul 2016 09:26:25 -0400 Received: from mga02.intel.com ([134.134.136.20]:52911 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755087AbcGHN0O (ORCPT ); Fri, 8 Jul 2016 09:26:14 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,330,1464678000"; d="asc'?scan'208";a="842768357" From: Felipe Balbi To: Michal Nazarewicz , Baolin Wang , Felipe Ferreri Tonello Cc: Greg KH , eu@felipetonello.com, dan.carpenter@oracle.com, USB , LKML , Mark Brown Subject: Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize In-Reply-To: References: <8ff4f814afc70cf9c00cc6718d7ae93fae94884a.1467881565.git.baolin.wang@linaro.org> User-Agent: Notmuch/0.22+58~g3a45d29 (https://notmuchmail.org) Emacs/25.0.95.1 (x86_64-pc-linux-gnu) Date: Fri, 08 Jul 2016 16:25:22 +0300 Message-ID: <87inwgi7nh.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, Michal Nazarewicz writes: > On Fri, Jul 08 2016, Baolin Wang wrote: >> On 7 July 2016 at 20:51, Michal Nazarewicz wrote: >>> On Thu, Jul 07 2016, Baolin Wang wrote: >>>> Some gadget device (such as dwc3 gadget) requires quirk_ep_out_aligned= _size >>>> attribute, which means it need to align the request buffer's size to a= n ep's >>>> maxpacketsize. >>>> >>>> Thus we add usb_ep_align_maybe() function to check if it is need to al= ign >>>> the request buffer's size to an ep's maxpacketsize. >>>> >>>> Signed-off-by: Baolin Wang >>> >>> Acked-by: Michal Nazarewicz >>> >>>> --- >>>> drivers/usb/gadget/function/f_midi.c | 18 +++++++++++------- >>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget= /function/f_midi.c >>>> index 58fc199..2e3f11e 100644 >>>> --- a/drivers/usb/gadget/function/f_midi.c >>>> +++ b/drivers/usb/gadget/function/f_midi.c >>>> @@ -328,7 +328,7 @@ static int f_midi_start_ep(struct f_midi *midi, >>>> static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsi= gned alt) >>>> { >>>> struct f_midi *midi =3D func_to_midi(f); >>>> - unsigned i; >>>> + unsigned i, length; >>>> int err; >>>> >>>> /* we only set alt for MIDIStreaming interface */ >>>> @@ -345,9 +345,11 @@ 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 =3D >>>> - midi_alloc_ep_req(midi->in_ep, midi->buflen); >>>> + struct usb_request *req; >>>> >>>> + length =3D usb_ep_align_maybe(midi->gadget, midi->in_ep, >>>> + midi->buflen); >>>> + req =3D midi_alloc_ep_req(midi->in_ep, length); >>>> if (req =3D=3D NULL) >>>> return -ENOMEM; >>>> >>>> @@ -359,10 +361,12 @@ 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 =3D 0; i < midi->qlen && err =3D=3D 0; i++) { >>>> - struct usb_request *req =3D >>>> - midi_alloc_ep_req(midi->out_ep, >>>> - max_t(unsigned, midi->buflen, >>>> - bulk_out_desc.wMaxPacketSize)); >>>> + struct usb_request *req; >>>> + >>>> + length =3D usb_ep_align_maybe(midi->gadget, midi->out_ep, >>>> + midi->buflen); >>>> + req =3D midi_alloc_ep_req(midi->out_ep, >>>> + max_t(unsigned, length, bulk_out_desc.wMaxPacket= Size)); >>> >>> Perhaps: >>> >>> + length =3D midi->buflen < bulk_out_desc.wMaxPacketSize >>> + ? bulk_out_desc.wMaxPacketSize >>> + : usb_ep_align_maybe(midi->gadget, midi->out_ep, >>> + midi->buflen); >>> + req =3D midi_alloc_ep_req(midi->out_ep, length); >>> >>> I find it somewhat cleaner. Up to you. >> >> But if the gadget does not requires 'quirk_ep_out_aligned_size', then >> we also can keep midi->buflen length although midi->buflen < >> bulk_out_desc.wMaxPacketSize, right? Thanks for your comment. > > I don=E2=80=99t know. That=E2=80=99s not what the original code was doin= g. The > original code was using: > > max_t(unsigned, midi->buflen, bulk_out_desc.wMaxPacketSize)); > > for some reason.> My take on this is that it's calling max_t() to try and align to wMaxPacketSize. We can see from original commit what was the intent: commit 03d27ade4941076b34c823d63d91dc895731a595 Author: Felipe F. Tonello Date: Wed Mar 9 19:39:30 2016 +0000 usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacke= tSize =20=20=20=20 buflen by default (256) is smaller than wMaxPacketSize (512) in high-sp= eed devices. =20=20=20=20 That caused the OUT endpoint to freeze if the host send any data packet= of length greater than 256 bytes. =20=20=20=20 This is an example dump of what happended on that enpoint: HOST: [DATA][Length=3D260][...] DEVICE: [NAK] HOST: [PING] DEVICE: [NAK] HOST: [PING] DEVICE: [NAK] ... HOST: [PING] DEVICE: [NAK] =20=20=20=20 This patch fixes this problem by setting the minimum usb_request's buff= er size for the OUT endpoint as its wMaxPacketSize. =20=20=20=20 Acked-by: Michal Nazarewicz Signed-off-by: Felipe F. Tonello Signed-off-by: Felipe Balbi diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/func= tion/f_midi.c index 56e2dde99b03..9ad51dcab982 100644 =2D-- a/drivers/usb/gadget/function/f_midi.c +++ b/drivers/usb/gadget/function/f_midi.c @@ -360,7 +360,9 @@ static int f_midi_set_alt(struct usb_function *f, unsig= ned intf, unsigned alt) /* allocate a bunch of read buffers and queue them all at once. */ for (i =3D 0; i < midi->qlen && err =3D=3D 0; i++) { struct usb_request *req =3D =2D midi_alloc_ep_req(midi->out_ep, midi->buflen); + midi_alloc_ep_req(midi->out_ep, + max_t(unsigned, midi->buflen, + bulk_out_desc.wMaxPacketSize)); if (req =3D=3D NULL) return -ENOMEM; Seems to me usb_ep_align_maybe() would cover this case just as well. But then, Felipe's UDC driver seems to need quirk_ep_out_aligned_size. Felipe? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXf6nCAAoJEIaOsuA1yqREdzQQALFOHlS02uoqjG81ZP/rqlmb wy+Yv89dq8EZG2sThVXlJIcXsCHyBW6u/TpCwY30V0iOQ/QVRm3NzQmV3wPWlhXg gzCGLIBwmRs6WilGfhbZ33roJzBE1J7F8K2hPclpNrQBz+KRCB/yjQUIbXDDwXUD 8WAqc161odFUg5SvRqF6D6//ElAOQebn/tIIDHaC6z+cLNEK3QoS3DffguT4IKy/ /P7qqEfltOj4sD4qQbrr3ryYJpUf1453IOMIIP8wWAeaebz01VZxIxWUYc3RealS EckVL4tH5lxUnqa7ju+5GsXKs3XW8NJ+iPe3oe+yBy+U4AfLj9EBjTH2Y8+FaCH0 atUpQ+jKLIiwm+HpSZonYfb+Ux0Y0I79SO64FO2Y0qKLdwZMsCC/5YqyBAkp/rKI mhqg6lULfhnjhB9JHTbGq+Rj1tprMvPjx+9AAPXC8LmQGILzTEmqQJucORkrlXZ0 BV/Y40qtTAWWnf/bkJOM4Yb8QDpE0URDPG9p+grkHDoiAuZ7NTZgBsuok30LN5Ud 2nS7bZBduqTqbNmkV8VUww85uHer6FkBwWlh8nQuSX5ShRxxzOL3xpdJ/0BaHpqX wUyhFa5K3yFBDjTraSC5zxYgFv54IlqM0ugmTnouzfiUWuxpBF2rA47wZBdEwOVr lg9PROHrrJaCL8TANlh3 =FV5j -----END PGP SIGNATURE----- --=-=-=--