Re: [PATCH 1/2] usb: dwc3: gadget: set xfer resource per endpoint

[John Youn]

On 5/31/2016 11:52 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <john.y...@synopsys.com> writes:
>> On 5/30/2016 7:19 AM, Felipe Balbi wrote:
>>> Instead of looping through all endpoints when
>>> enabling ep0, let's allow for each endpoint to set
>>> its own xfer resource. This solves an issue which
>>> happens when we issue END_TRANSFER due to a reset
>>> interrupt. Endpoints will be left without a xfer
>>> resource to use.
>>>
>>> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 25 ++++++++-----------------
>>>  1 file changed, 8 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 3d0745dece0c..6f5a4feef8af 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -410,30 +410,21 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc, 
>>> struct dwc3_ep *dep)
>>>  {
>>>     struct dwc3_gadget_ep_cmd_params params;
>>>     u32                     cmd;
>>> -   int                     i;
>>>     int                     ret;
>>>  
>>> -   if (dep->number)
>>> -           return 0;
>>> -
>>> -   memset(&params, 0x00, sizeof(params));
>>> -   cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>> -
>>> -   ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>>> -   if (ret)
>>> -           return ret;
>>> +   if (dep->number == 0) {
>>> +           memset(&params, 0x00, sizeof(params));
>>> +           cmd = DWC3_DEPCMD_DEPSTARTCFG;
>>>  
>>> -   for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
>>> -           struct dwc3_ep *dep = dwc->eps[i];
>>> -
>>> -           if (!dep)
>>> -                   continue;
>>> -
>>> -           ret = dwc3_gadget_set_xfer_resource(dwc, dep);
>>> +           ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>>>             if (ret)
>>>                     return ret;
>>>     }
>>>  
>>> +   ret = dwc3_gadget_set_xfer_resource(dwc, dep);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>>     return 0;
>>>  }
>>>  
>>>
>>
>> Hi Felipe,
>>
>> This reverts back to the original buggy behavior. This will fail when
>> a SET_INTERFACE occurs multiple times.
> 
> aaaah I forgot about that. Good point.
> 
>> You can run testusb to see the failure:
>> testusb -t 9 -c 5000 -s 2048 -a
> 
> I'll test this one today
> 
>> We've actually found a problem with the current code as well. There is
>> both a documentation problem and a controller problem which we will
>> fix in an upcoming release.
> 
> Oh, I can't wait to see :-) If you want help testing anything, let me
> know.
> 
>> However, I'm not aware of any issue with END_TRANSFER, could you let
>> me know how to reproduce it?
> 
> it's a rare and difficult bug to reproduce. If you take my testing/next
> (I didn't check if it happens with v4.7-rc1) - $subject and keep large
> mass storage transfers going on, eventually you'll see mass storage
> hang. After some 30s, host side will timeout and cancel all URBs and
> issue a bus reset. This will, in turn, cause the gadget driver to issue
> END_TRANSFER to a possible in-flight transfer.
> 
> After Reset completion, the gadget will reenumerate and, when gadget
> driver queues to a bulk EP, StartTransfer will return "No Resource". The
> reason for that is that END_TRANSFER deallocates the resource, according
> to section 6.3.5.2 of 2.60a databook.

By the way I don't see this section in the 2.60a databook. Can you
check again?

Thanks,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html