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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]