Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Janusz Dziedzic]

On 13 October 2016 at 12:41, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> On 13 October 2016 at 17:49, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>>>> index 1783406..ca2ae5b 100644
>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>>> @@ -241,6 +241,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
>>>>>>>>>       int                     susphy = false;
>>>>>>>>>       int                     ret = -EINVAL;
>>>>>>>>>
>>>>>>>>> +     if (!dwc->pullups_connected)
>>>>>>>>> +             return -ESHUTDOWN;
>>>>>>>>> +
>>>>>>
>>>>>> you skip trace_dwc3_gadget_ep_cmd()
>>>>>
>>>>> Yes, we did not need trace here since we did not send out the command.
>>>>>
>>>> What in such case: enumeration will not work and this will be because
>>>> this ESHUTDOWN or wrong pullups_connected usage. Without a trace you
>>>> will not know where the problem is.
>>>> In my opinion this trace could be useful.
>>>
>>> We have returned the '-ESHUTDOWN' error number for user to know what
>>> happened.
>>
>> No, this is actually not good and Janusz has a very valid point
>> here. When we're debugging something in dwc3, we want to rely on
>> tracepoints to tell us what's going on. I don't want to have to add more
>> debugging messages to print out that ESHUTDOWN error just so I can
>> figure out what's going on. You should be patching this in a way that
>> the tracepoint is still printed out properly.
>
> Fine. Will fix this in next version.
>

BTW, did you test this patch with device mode?
Seems in my setup this fail - DWC3_DEPCMD_SETEPCONFIG for ep0out and
gadget_start() failed (enumeration fail).
Don't we need to queue ep0 cmds before pullup?

BR
Janusz
>>
>> Keep in mind that you should be setting ret to -ESHUTDOWN and make sure
>> the trace is printed. Same patch should, then, patch trace.h to handle
>> -ESHUTDOWN as an error case, i.e. following hunk should be added:
>>
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index d93780e84f07..70b783f0507d 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -319,6 +319,8 @@ static inline const char *dwc3_ep_cmd_status_string(int status)
>>         switch (status) {
>>         case -ETIMEDOUT:
>>                 return "Timed Out";
>> +       case -ESHUTDOWN:
>> +               return "Shut Down";
>>         case 0:
>>                 return "Successful";
>>         case DEPEVT_TRANSFER_NO_RESOURCE:
>>
>> --
>> balbi
>
>
>
> --
> Baolin.wang
> Best Regards