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

[Felipe Balbi]

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.

-- 
balbi

signature.asc
Description: PGP signature