Re: [PATCH v2 3/3] usb: dwc3: gadget: return error if command sent to DEPCMD register fails

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 10301 bytes --]

On Mon, Jul 20, 2015 at 06:16:46PM +0000, John Youn wrote:
> On 7/20/2015 10:51 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Jul 15, 2015 at 09:49:05AM +0000, Subbaraya Sundeep Bhatta wrote:
> >> 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.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
> >>>
> >>> 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 patches.
> >>>>>>>> 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 misbehaving.
> >>>>>
> >>>>> 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 system.
> >>>>>
> >>>>> 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 for
> >>>> review.
> >>
> >> Thanks John for debugging :). Yes we are not handling SET_INTERFACE similar to
> >> SET_CONFIGURATION in driver. I guess we follow 
> >> "Alternate Initialization on SetInterface Request" sequence as per data book.
> >> Felipe can confirm this.
> >>
> >>>
> >>> Thanks a lot John. Not sure how come we missed that for such a long time
> >>> :-) 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) ?
> > 
> > I have been looking more at this and the reason why we're failing is a
> > patch from Paul, which I quote below:
> > 
> > commit b23c843992b659d537514e6493d673284f5d6724
> > Author: Paul Zimmerman <Paul.Zimmerman@synopsys.com>
> > Date:   Fri Sep 30 10:58:42 2011 +0300
> > 
> >     usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs
> >     
> >     DEPSTARTCFG for non-EP0 EPs must only be sent once per config
> >     
> >     [ balbi@ti.com : changed config_start to start_config_issued ]
> >     
> >     Signed-off-by: Paul Zimmerman <paulz@synopsys.com>
> >     Signed-off-by: Felipe Balbi <balbi@ti.com>
> >     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > 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;
> >  
> >  	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, struct usb_ctrlrequest *ctrl)
> >  	u32 cfg;
> >  	int ret;
> >  
> > +	dwc->start_config_issued = false;
> >  	cfg = le16_to_cpu(ctrl->wValue);
> >  
> >  	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 *dwc, struct dwc3_ep *dep)
> >  	if (dep->number != 1) {
> >  		cmd = DWC3_DEPCMD_DEPSTARTCFG;
> >  		/* XferRscIdx == 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 = true;
> >  			cmd |= DWC3_DEPCMD_PARAM(2);
> > +		}
> >  
> >  		return dwc3_send_gadget_ep_cmd(dwc, 0, cmd, &params);
> >  	}
> > @@ -1161,6 +1165,8 @@ static int dwc3_gadget_start(struct usb_gadget *g,
> >  	reg |= DWC3_DCFG_SUPERSPEED;
> >  	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> >  
> > +	dwc->start_config_issued = false;
> > +
> >  	/* Start with SuperSpeed Default */
> >  	dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
> >  
> > @@ -1592,6 +1598,7 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
> >  
> >  	dwc3_stop_active_transfers(dwc);
> >  	dwc3_disconnect_gadget(dwc);
> > +	dwc->start_config_issued = false;
> >  
> >  	dwc->gadget.speed = USB_SPEED_UNKNOWN;
> >  }
> > @@ -1643,6 +1650,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> >  
> >  	dwc3_stop_active_transfers(dwc);
> >  	dwc3_clear_stall_all_ep(dwc);
> > +	dwc->start_config_issued = false;
> >  
> >  	/* Reset device address to zero */
> >  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> > 
> > 
> > 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.
> > 
> > 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.
> > 
> > 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.
> > 
> > cheers
> > 
> 
> Hi Felipe,
> 
> I think Paul's patch is still valid. However the
> DEPSTARTCFG(XferRscIdx=2) 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.
> 
> 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=0).
> 
> 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.
> 
> 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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]