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

[Baolin Wang]

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.

>
> 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