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

[John Youn <John.Youn@synopsys.com>]

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.

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.

John