| Message ID | 521625dd7f5e335e2a681ec65ebffc5832207e5f.1475570367.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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 --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
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