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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux