diff mbox

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

Message ID 521625dd7f5e335e2a681ec65ebffc5832207e5f.1475570367.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang Oct. 4, 2016, 8:42 a.m. UTC
When system has stpped the gadget, we should avoid queuing any requests
which will cause tranfer failed. Thus adding some disconnect checking to
avoid this situation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
Changes since v2:
 - Move disconnect checking into dwc3_send_gadget_ep_cmd().
 - Rename completion name and issue complete() at one place.
 - Move completion initialization into dwc3_gadget_init().

Changes since v1:
 - Split into 2 separate ptaches.
 - Choose complete mechanism instead of polling.
---
 drivers/usb/dwc3/gadget.c |    3 +++
 1 file changed, 3 insertions(+)

-- 
1.7.9.5

Comments

(Exiting) Baolin Wang Oct. 13, 2016, 10:41 a.m. UTC | #1
On 13 October 2016 at 17:49, Felipe Balbi <balbi@kernel.org> wrote:
>

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


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
(Exiting) Baolin Wang Oct. 13, 2016, 11:30 a.m. UTC | #2
Hi,

On 13 October 2016 at 19:22, Felipe Balbi <balbi@kernel.org> wrote:
>

> Hi,

>

> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

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

>>>

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

>

> Baolin, it's clear to me that you're not testing any of the patches

> you're sending me. I just reviewed this part of the code and we _do_

> indeed enable the control pipe before connecting pullups and that *must*

> be done this way, otherwise we won't be able to receive first Setup

> packet from host.


I am very sorry for this mistake. Since the original patch is adding
some 'pullups_connected' check in other places to avoid queuing
requests, but you suggested me this only affected the endpoint command
transfer, so I just follow your advice without one clear testing. I
really sorry for that. Please ignore this patch and I promise I will
test it enough before sending out any patches. Sorry again for
troubles.

>

> How have you tested this? Against which tree?

>

> --

> balbi




-- 
Baolin.wang
Best Regards
(Exiting) Baolin Wang Oct. 13, 2016, 12:23 p.m. UTC | #3
On 13 October 2016 at 20:17, Felipe Balbi <balbi@kernel.org> wrote:
>

> Hi,

>

> Baolin Wang <baolin.wang@linaro.org> writes:

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

>>>

>>> Baolin, it's clear to me that you're not testing any of the patches

>>

>> I am sure I tested every patch I send to you. But this one is really

>> my mistake and I thought this is just one small change with just eye

>> review. I am really sorry for troubles.

>

> fair enough, luckily Janusz caught it before any harm could be

> done. Thanks for taking the time to explain.


Thanks for understanding and thanks for Janusz's testing.


-- 
Baolin.wang
Best Regards
diff mbox

Patch

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;
+
 	/*
 	 * Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
 	 * we're issuing an endpoint command, we must check if