From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755382AbcGHN2K (ORCPT ); Fri, 8 Jul 2016 09:28:10 -0400 Received: from mga03.intel.com ([134.134.136.65]:6875 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754926AbcGHN2H (ORCPT ); Fri, 8 Jul 2016 09:28:07 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,330,1464678000"; d="asc'?scan'208";a="135917560" 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: <87inwgi7nh.fsf@linux.intel.com> References: <8ff4f814afc70cf9c00cc6718d7ae93fae94884a.1467881565.git.baolin.wang@linaro.org> <87inwgi7nh.fsf@linux.intel.com> 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:27:09 +0300 Message-ID: <87furki7ki.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 again, Felipe Balbi writes: > 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_aligne= d_size >>>>> attribute, which means it need to align the request buffer's size to = an ep's >>>>> maxpacketsize. >>>>> >>>>> Thus we add usb_ep_align_maybe() function to check if it is need to a= lign >>>>> 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/gadge= t/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, uns= igned 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_e= p, >>>>> + midi->buflen); >>>>> + req =3D midi_alloc_ep_req(midi->out_ep, >>>>> + max_t(unsigned, length, bulk_out_desc.wMaxPacke= tSize)); >>>> >>>> Perhaps: >>>> >>>> + length =3D midi->buflen < bulk_out_desc.wMaxPacketSize >>>> + ? bulk_out_desc.wMaxPacketSize >>>> + : usb_ep_align_maybe(midi->gadget, midi->out_e= p, >>>> + 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 doi= ng. 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 wMaxPac= ketSize >=20=20=20=20=20 > buflen by default (256) is smaller than wMaxPacketSize (512) in high-= speed > devices. >=20=20=20=20=20 > That caused the OUT endpoint to freeze if the host send any data pack= et of > length greater than 256 bytes. >=20=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=20 > This patch fixes this problem by setting the minimum usb_request's bu= ffer size > for the OUT endpoint as its wMaxPacketSize. >=20=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/fu= nction/f_midi.c > index 56e2dde99b03..9ad51dcab982 100644 > --- 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, uns= igned 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, 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? another way to look at this is that buflen < wMaxPacketSize for bulk endpoints is kinda weird, so we might just go ahead and allocate buflen aligned to wMaxPacketSize for OUT eps. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXf6otAAoJEIaOsuA1yqREYzsP/iUbWDlXD8M9L4mgBP1Ii/bm r31nF05MqkJ4ZD0uj67JKjlQi8HY2yo12RYeZIcv1zAU03ty92vuGE02tY+i1hea ytQpedweK14bgkA/N0q4eORmTdktSTLCAXyiKDwwYeagjgm0KUAZcJmmoFhSrvha zwXzclySVCNRQfStnICI+djRtVOUIG7KQ2ydUrMgyyRoyAdSHz+ek7Cbo6dsrigc 5BWxLO+omjj4Pp4OBBUibQEt8xTMjeRebloytk2COuziKn4/DCRJ0F1l7Qutxk9d 67Kgp1fDNTNbKQsNxMDrlqjCoXtN5xaC2qvSUtLheuV8Nl7gXjz7tRXZJuK6Zv86 RtHKWe5H8aPv1cFEIjgH8Oi5ilg3Pd5iIVaI0G5Oh8ExfUBINVIY/911hPKXf0x8 WBiR1sKPhQIRafhjWNIdWS8omnd1djU/s3iZL+gH6OdC7TOuBmwGUvDbfBGV9OTd y56seOCTSdqt6ElyLCy6qRV712umy2rwTiH0dG22OOFJhiCiWTglu/lf8DGqOYzz 9o+YZ1SksaIP/ggGlJJnmeFAi3SEnpUSMn3zJqzW2OeX+TJYBLNVLAZG9Au1kyee DnEv8QXScQUBA2EsCaNoRi9tm0bEdcgVgM9Zep/Xds8VM8Rwr0vj2cqLsV/ghS10 rCPAr8wJY4OyRCUkrng5 =qu/8 -----END PGP SIGNATURE----- --=-=-=--