Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753566AbbGTShr (ORCPT ); Mon, 20 Jul 2015 14:37:47 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:35051 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751721AbbGTShp (ORCPT ); Mon, 20 Jul 2015 14:37:45 -0400 Date: Mon, 20 Jul 2015 13:37:41 -0500 From: Felipe Balbi To: John Youn CC: "balbi@ti.com" , Subbaraya Sundeep Bhatta , "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails Message-ID: <20150720183741.GC5394@saruman.tx.rr.com> Reply-To: References: <20150702030009.GB29462@saruman.tx.rr.com> <2B3535C5ECE8B5419E3ECBE3007729090175267F20@US01WEMBX2.internal.synopsys.com> <20150707032448.GB13135@saruman.tx.rr.com> <20150711192914.GA24195@saruman.tx.rr.com> <2B3535C5ECE8B5419E3ECBE3007729090175270D38@US01WEMBX2.internal.synopsys.com> <20150713185857.GD32056@saruman.tx.rr.com> <20150720175148.GB5394@saruman.tx.rr.com> <2B3535C5ECE8B5419E3ECBE300772909017527B5A8@US01WEMBX2.internal.synopsys.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Y5rl02BVI9TCfPar" Content-Disposition: inline In-Reply-To: <2B3535C5ECE8B5419E3ECBE300772909017527B5A8@US01WEMBX2.internal.synopsys.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11517 Lines: 305 --Y5rl02BVI9TCfPar Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 20, 2015 at 06:16:46PM +0000, John Youn wrote: > On 7/20/2015 10:51 AM, Felipe Balbi wrote: > > Hi, > >=20 > > On Wed, Jul 15, 2015 at 09:49:05AM +0000, Subbaraya Sundeep Bhatta wrot= e: > >> Hi John, > >> > >>> -----Original Message----- > >>> From: Felipe Balbi [mailto:balbi@ti.com] > >>> Sent: Tuesday, July 14, 2015 12:29 AM > >>> To: John Youn > >>> Cc: balbi@ti.com; Subbaraya Sundeep Bhatta; gregkh@linuxfoundation.or= g; > >>> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; > >>> stable@vger.kernel.org > >>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command > >>> sent to DEPCMD register fails > >>> > >>> Hi, > >>> > >>> On Mon, Jul 13, 2015 at 05:50:49PM +0000, John Youn wrote: > >>>> On 7/11/2015 12:29 PM, Felipe Balbi wrote: > >>>>> Hi, > >>>>> > >>>>> On Sat, Jul 11, 2015 at 05:17:32PM +0000, Subbaraya Sundeep Bhatta > >>> wrote: > >>>>>>>>>> Hi Felipe, > >>>>>>>>>> > >>>>>>>>>> Just an update on this. > >>>>>>>>>> > >>>>>>>>>> I'm trying to get this working with our latest IP with dwc3 > >>>>>>>>>> from your testing/next branch. It fails the usbtest with a > >>>>>>>>>> problem unrelated to this patch. > >>>>>>>>>> . > >>>>>>>>>> It passes on 4.1.1. > >>>>>>>>>> > >>>>>>>>>> I'll have to look into the failure but I won't get to it until > >>>>>>>>>> next week as I'm off the rest of this week. > >>>>>>>>> > >>>>>>>>> interesting... If you could post failure signature, I can help > >>>>>>>>> looking at it, but I guess it's too late to ask :-) > >>>>>>>>> > >>>>>>>>> thanks for helping though > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> Hi Felipe, > >>>>>>>> > >>>>>>>> Nevermind about my issue, it ended up being a setup-related > >>> problem. > >>>>>>>> > >>>>>>>> I actually do see the same error as you due to this series of pa= tches. > >>>>>>>> Except I see it happening before even the first iteration. I get > >>>>>>>> a completion status of 1 for the Set Endpoint Transfer Resources > >>>>>>>> command. I'm not sure why this is. > >>>>>>>> > >>>>>>>> I don't see any conflict with any previous Transfer Complete. > >>>>>> > >>>>>> Same behavior at my end too. Fails before first iteration and I get > >>>>>> completion status of 1 for Set Endpoint Resource command. Attached > >>>>>> the logs of testing done with this patch and without this patch. > >>>>>> Without this patch I often see completion status of 1 for Set > >>>>>> Endpoint Transfer Resources command for Bulk and Isoc endpoints but > >>>>>> test proceeds because driver just logs command completion status > >>>>>> and moves on. We can revert this patch for time being. IP version = is > >>> 2.90a. > >>>>> > >>>>> yeah, that's what I mean, it really seems like it's the IP misbehav= ing. > >>>>> > >>>>> John, let's try to figure out what's the root cause of this, we > >>>>> really want to use command completion status at some point, but for > >>>>> now we need to revert the patch :-( > >>>>> > >>>>> Let me know if you want me to log STARS ticket on your solvnet syst= em. > >>>>> > >>>>> cheers > >>>>> > >>>> > >>>> Hi Felipe, > >>>> > >>>> We found the issue last week. > >>>> > >>>> The start config command isn't getting called during SET_INTERFACE. > >>>> Thus the transfer resource index isn't getting reset, and with > >>>> multiple SET_INTERFACE commands it will eventually exhaust the > >>>> resources. > >>>> > >>>> I tried out a fix and it works for me. I'll send it out separately f= or > >>>> review. > >> > >> Thanks John for debugging :). Yes we are not handling SET_INTERFACE si= milar to > >> SET_CONFIGURATION in driver. I guess we follow=20 > >> "Alternate Initialization on SetInterface Request" sequence as per dat= a book. > >> Felipe can confirm this. > >> > >>> > >>> Thanks a lot John. Not sure how come we missed that for such a long t= ime > >>> :-) Let's Cc stable and get it plugged ASAP :-) > >>> > >>>> Also, I noticed that the trace message that shows control transfers > >>>> doesn't show the SET_INTERFACE properly. Any idea why this is? > >>>> > >>>> For example, here is the line in the trace that corresponds to the > >>>> SET_INTERFACE: > >>>> irq/33-dwc3-10808 [003] d... 2443.494368: dwc3_ctrl_req: > >>> bRequestType > >>>> 01 bRequest 0b wValue 0001 wIndex 0000 wLength 0 > >> > >> Can you please elaborate? What is expected here? Did you mean it shows= wrong info > >> (other than the request actually sent by Host) ? > >=20 > > I have been looking more at this and the reason why we're failing is a > > patch from Paul, which I quote below: > >=20 > > commit b23c843992b659d537514e6493d673284f5d6724 > > Author: Paul Zimmerman > > Date: Fri Sep 30 10:58:42 2011 +0300 > >=20 > > usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs > > =20 > > DEPSTARTCFG for non-EP0 EPs must only be sent once per config > > =20 > > [ balbi@ti.com : changed config_start to start_config_issued ] > > =20 > > Signed-off-by: Paul Zimmerman > > Signed-off-by: Felipe Balbi > > Signed-off-by: Greg Kroah-Hartman > >=20 > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 0231eba1f53d..502582ce1fc6 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -529,6 +529,7 @@ static inline void dwc3_trb_to_nat(struct dwc3_trb_= hw *hw, struct dwc3_trb *nat) > > * @ep0_status_pending: ep0 status response without a req is pending > > * @ep0_bounced: true when we used bounce buffer > > * @ep0_expect_in: true when we expect a DATA IN transfer > > + * @start_config_issued: true when StartConfig command has been issued > > * @ep0_next_event: hold the next expected event > > * @ep0state: state of endpoint zero > > * @link_state: link state > > @@ -576,6 +577,7 @@ struct dwc3 { > > unsigned ep0_status_pending:1; > > unsigned ep0_bounced:1; > > unsigned ep0_expect_in:1; > > + unsigned start_config_issued:1; > > =20 > > enum dwc3_ep0_next ep0_next_event; > > enum dwc3_ep0_state ep0state; > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > > index cc27c4ec498d..2def48ed30ea 100644 > > --- a/drivers/usb/dwc3/ep0.c > > +++ b/drivers/usb/dwc3/ep0.c > > @@ -455,6 +455,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, st= ruct usb_ctrlrequest *ctrl) > > u32 cfg; > > int ret; > > =20 > > + dwc->start_config_issued =3D false; > > cfg =3D le16_to_cpu(ctrl->wValue); > > =20 > > switch (dwc->dev_state) { > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index b2820aa9fa46..9c0174a8f46c 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -237,8 +237,12 @@ static int dwc3_gadget_start_config(struct dwc3 *d= wc, struct dwc3_ep *dep) > > if (dep->number !=3D 1) { > > cmd =3D DWC3_DEPCMD_DEPSTARTCFG; > > /* XferRscIdx =3D=3D 0 for ep0 and 2 for the remaining */ > > - if (dep->number > 1) > > + if (dep->number > 1) { > > + if (dwc->start_config_issued) > > + return 0; > > + dwc->start_config_issued =3D true; > > cmd |=3D DWC3_DEPCMD_PARAM(2); > > + } > > =20 > > return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, ¶ms); > > } > > @@ -1161,6 +1165,8 @@ static int dwc3_gadget_start(struct usb_gadget *g, > > reg |=3D DWC3_DCFG_SUPERSPEED; > > dwc3_writel(dwc->regs, DWC3_DCFG, reg); > > =20 > > + dwc->start_config_issued =3D false; > > + > > /* Start with SuperSpeed Default */ > > dwc3_gadget_ep0_desc.wMaxPacketSize =3D cpu_to_le16(512); > > =20 > > @@ -1592,6 +1598,7 @@ static void dwc3_gadget_disconnect_interrupt(stru= ct dwc3 *dwc) > > =20 > > dwc3_stop_active_transfers(dwc); > > dwc3_disconnect_gadget(dwc); > > + dwc->start_config_issued =3D false; > > =20 > > dwc->gadget.speed =3D USB_SPEED_UNKNOWN; > > } > > @@ -1643,6 +1650,7 @@ static void dwc3_gadget_reset_interrupt(struct dw= c3 *dwc) > > =20 > > dwc3_stop_active_transfers(dwc); > > dwc3_clear_stall_all_ep(dwc); > > + dwc->start_config_issued =3D false; > > =20 > > /* Reset device address to zero */ > > reg =3D dwc3_readl(dwc->regs, DWC3_DCFG); > >=20 > >=20 > > So the problem is that even though endpoint *is* indeed disabled, > > start_config_issued is still true and we return before sending that > > command. According to Paul's patch it's important that we send > > start_config only once per config. > >=20 > > However, I do see that section 9.1.5, table 9-5 of Databook 2.90a has no > > conditionals around DEPSTARTCFG. Reverting that patch from Paul, I can > > see that it all works fine. > >=20 > > I'll send the revert still today, but I need you guys to confirm that it > > really works on your platforms. According to Paul he "can't see how the > > driver would work without" start_config_issued trick. > >=20 > > cheers > >=20 >=20 > Hi Felipe, >=20 > I think Paul's patch is still valid. However the > DEPSTARTCFG(XferRscIdx=3D2) should only be issued once per > SET_CONFIG *OR* SET_INTERFACE. hmm.. This isn't very clear on databook, perhaps Synopsys might want to update the documentation with a note further clarifying this detail ? > This command will reset the Transfer Resource Index to 2 (there > are already 2 used for EP0). Then subsequent DEPXFERCFG commands > will take the next XferRscIdx and increment. Without this, you > will assign the same transfer resource for different endpoints > which could potentially cause problems. >=20 > The only other time DEPSTARTCFG should be issued is a power on or > reset before you configure EP0. In that case you should do > DEPSTARTCFG(XferRscIdx=3D0). >=20 > This caused problems because, although it was reset on > SET_CONFIG, the usbtest did many SET_INTERFACE requests where it > wasn't being reset. Eventually it ran out of transfer resources > and errored out. >=20 > I'll send you a patch to review shortly. However, I have not been > able to test it yet beyond a few simple cases and usbtest. I can run a ton of other tests here as long as I have a patch :-) testusb/usbtest is a very good test case though, as long as it runs for a few days without any failures, we can rest assured it works. cheers --=20 balbi --Y5rl02BVI9TCfPar Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVrT/1AAoJEIaOsuA1yqREX+IQAKkAmpVhrXVyMg7hKvDhyBrK IntuR6f4tQ62FSr5b6H/BqbAt8FaR53c+gH7G5yYzNpPb+gg4EphFkpEI6ccU6Tc ccDEHaFhI6zIN2PMZ02VCQqF/2m175TZV3bRgH4OznaYqwUIb4N8CsZ+NG0oYHni kT68MzzNztpaqFqySE7TIo+IuXYEzgoogpnK9Nvo6J/onFfEBq2Hfxq8OJvlbK61 jcG+tTVvySL+8pMSXrmhOK9GlxYvR+B2rMEvOLo7Jz6or4RlA0gEKjabPW9iWQ1B i1+0hJUJfUP9S6ICONI5T9O+UC/eGDNPpeCd2O3E6K5yw0+7KfuVuL528b36DKP6 LkqQiN/4PFeIydDeCNySJgjXfKjUsSDojy+ZxpMKyzF2ypAP1GcwsOMnyfY1pxDG 34naJYrEFimxBmKpeHA4KfURKeH4MepbXUei/X+BP5DAuvNe3FmQJlp6rrqiSfrH fR1aUAUOYAIFCASS2Tc+qOnLVxSxc0jGGIk1c6PbjP+62LHj31cCCZFyP464TG07 atK9/5TbEQ/ZUZZzTW4c+pnrS//jTkUNlEjaVQAxm0WFkl4dRCx78JugKOoc5Ugr HuSXgDiDBr3nASd33UAsg6l5OpRrt8Df5s/dTkcw0gPbGAneUvSkxMMbEOH/KE0J FHqOlXFYskR+36pAhmqb =re/m -----END PGP SIGNATURE----- --Y5rl02BVI9TCfPar-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/