Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1495298 > unrolled thread
| Started by | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| First post | 2016-10-04 10:50 +0200 |
| Last post | 2016-10-13 11:30 +0200 |
| Articles | 15 — 3 participants |
Back to article view | Back to linux.kernel
[RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-04 10:50 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi <balbi@kernel.org> - 2016-10-13 09:20 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 09:50 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Janusz Dziedzic <janusz.dziedzic@tieto.com> - 2016-10-13 10:20 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 10:30 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 10:50 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi <balbi@kernel.org> - 2016-10-13 12:00 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 12:50 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Janusz Dziedzic <janusz.dziedzic@tieto.com> - 2016-10-13 13:20 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi <balbi@kernel.org> - 2016-10-13 13:30 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 13:40 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 13:50 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi <balbi@kernel.org> - 2016-10-13 14:20 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-10-13 14:30 +0200
Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Janusz Dziedzic <janusz.dziedzic@tieto.com> - 2016-10-13 11:30 +0200
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-04 10:50 +0200 |
| Subject | [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <sotAd-he-7@gated-at.bofh.it> |
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(+) 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 -- 1.7.9.5
[toc] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-10-13 09:20 +0200 |
| Message-ID | <srIt4-oQ-1@gated-at.bofh.it> |
| In reply to | #1495298 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Baolin Wang <baolin.wang@linaro.org> writes:
> When system has stpped the gadget, we should avoid queuing any requests
queueing is *not* a problem. Starting is. In fact, that's what your
patch is doing.
> which will cause tranfer failed. Thus adding some disconnect checking to
^^^^^^^
transfer
> 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(+)
>
> 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
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 09:50 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srIW6-zk-7@gated-at.bofh.it> |
| In reply to | #1500057 |
Hi, On 13 October 2016 at 15:06, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Baolin Wang <baolin.wang@linaro.org> writes: >> When system has stpped the gadget, we should avoid queuing any requests > > queueing is *not* a problem. Starting is. In fact, that's what your > patch is doing. OK. > >> which will cause tranfer failed. Thus adding some disconnect checking to > ^^^^^^^ > transfer Sorry for spelling mistake, will fix it. > >> 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(+) >> >> 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 >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > balbi -- Baolin.wang Best Regards
[toc] | [prev] | [next] | [standalone]
| From | Janusz Dziedzic <janusz.dziedzic@tieto.com> |
|---|---|
| Date | 2016-10-13 10:20 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srJp7-YE-1@gated-at.bofh.it> |
| In reply to | #1500069 |
On 13 October 2016 at 09:37, Baolin Wang <baolin.wang@linaro.org> wrote: > Hi, > > On 13 October 2016 at 15:06, Felipe Balbi <balbi@kernel.org> wrote: >> >> Hi, >> >> Baolin Wang <baolin.wang@linaro.org> writes: >>> When system has stpped the gadget, we should avoid queuing any requests >> >> queueing is *not* a problem. Starting is. In fact, that's what your >> patch is doing. > > OK. > >> >>> which will cause tranfer failed. Thus adding some disconnect checking to >> ^^^^^^^ >> transfer > > Sorry for spelling mistake, will fix it. > >> >>> 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(+) >>> >>> 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() BR Janusz >>> /* >>> * 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 >>> -- >>> 1.7.9.5 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> balbi > > > > -- > Baolin.wang > Best Regards > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 10:30 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srJyO-120-3@gated-at.bofh.it> |
| In reply to | #1500086 |
Hi, On 13 October 2016 at 16:16, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: > On 13 October 2016 at 09:37, Baolin Wang <baolin.wang@linaro.org> wrote: >> Hi, >> >> On 13 October 2016 at 15:06, Felipe Balbi <balbi@kernel.org> wrote: >>> >>> Hi, >>> >>> Baolin Wang <baolin.wang@linaro.org> writes: >>>> When system has stpped the gadget, we should avoid queuing any requests >>> >>> queueing is *not* a problem. Starting is. In fact, that's what your >>> patch is doing. >> >> OK. >> >>> >>>> which will cause tranfer failed. Thus adding some disconnect checking to >>> ^^^^^^^ >>> transfer >> >> Sorry for spelling mistake, will fix it. >> >>> >>>> 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(+) >>>> >>>> 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. > > BR > Janusz > >>>> /* >>>> * 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 >>>> -- >>>> 1.7.9.5 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> -- >>> balbi >> >> >> >> -- >> Baolin.wang >> Best Regards >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Baolin.wang Best Regards
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 10:50 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srJS9-18s-9@gated-at.bofh.it> |
| In reply to | #1500090 |
On 13 October 2016 at 16:28, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: > On 13 October 2016 at 10:21, Baolin Wang <baolin.wang@linaro.org> wrote: >> Hi, >> >> On 13 October 2016 at 16:16, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: >>> On 13 October 2016 at 09:37, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> Hi, >>>> >>>> On 13 October 2016 at 15:06, Felipe Balbi <balbi@kernel.org> wrote: >>>>> >>>>> Hi, >>>>> >>>>> Baolin Wang <baolin.wang@linaro.org> writes: >>>>>> When system has stpped the gadget, we should avoid queuing any requests >>>>> >>>>> queueing is *not* a problem. Starting is. In fact, that's what your >>>>> patch is doing. >>>> >>>> OK. >>>> >>>>> >>>>>> which will cause tranfer failed. Thus adding some disconnect checking to >>>>> ^^^^^^^ >>>>> transfer >>>> >>>> Sorry for spelling mistake, will fix it. >>>> >>>>> >>>>>> 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(+) >>>>>> >>>>>> 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. > > BR > Janusz > >>> >>> BR >>> Janusz >>> >>>>>> /* >>>>>> * 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 >>>>>> -- >>>>>> 1.7.9.5 >>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>>> -- >>>>> balbi >>>> >>>> >>>> >>>> -- >>>> Baolin.wang >>>> Best Regards >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Baolin.wang >> Best Regards -- Baolin.wang Best Regards
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-10-13 12:00 +0200 |
| Message-ID | <srKXT-1LD-3@gated-at.bofh.it> |
| In reply to | #1500098 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 12:50 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srLKi-2lV-19@gated-at.bofh.it> |
| In reply to | #1500131 |
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
[toc] | [prev] | [next] | [standalone]
| From | Janusz Dziedzic <janusz.dziedzic@tieto.com> |
|---|---|
| Date | 2016-10-13 13:20 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srMdk-2NV-13@gated-at.bofh.it> |
| In reply to | #1500155 |
On 13 October 2016 at 12:41, Baolin Wang <baolin.wang@linaro.org> wrote:
> 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.
>
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-10-13 13:30 +0200 |
| Message-ID | <srMn0-2Ty-15@gated-at.bofh.it> |
| In reply to | #1500183 |
[Multipart message — attachments visible in raw view] — view raw
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. How have you tested this? Against which tree? -- balbi
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 13:40 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srMwG-2X1-13@gated-at.bofh.it> |
| In reply to | #1500190 |
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
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 13:50 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srMGn-30B-43@gated-at.bofh.it> |
| In reply to | #1500190 |
Hi Felipe, 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 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. > 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. > > How have you tested this? Against which tree? > > -- > balbi -- Baolin.wang Best Regards
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-10-13 14:20 +0200 |
| Message-ID | <srN9o-3q6-33@gated-at.bofh.it> |
| In reply to | #1500214 |
[Multipart message — attachments visible in raw view] — view raw
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. -- balbi
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-10-13 14:30 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srNj3-3tk-1@gated-at.bofh.it> |
| In reply to | #1500233 |
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
[toc] | [prev] | [next] | [standalone]
| From | Janusz Dziedzic <janusz.dziedzic@tieto.com> |
|---|---|
| Date | 2016-10-13 11:30 +0200 |
| Subject | Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <srJS9-18s-11@gated-at.bofh.it> |
| In reply to | #1500090 |
On 13 October 2016 at 10:21, Baolin Wang <baolin.wang@linaro.org> wrote: > Hi, > > On 13 October 2016 at 16:16, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: >> On 13 October 2016 at 09:37, Baolin Wang <baolin.wang@linaro.org> wrote: >>> Hi, >>> >>> On 13 October 2016 at 15:06, Felipe Balbi <balbi@kernel.org> wrote: >>>> >>>> Hi, >>>> >>>> Baolin Wang <baolin.wang@linaro.org> writes: >>>>> When system has stpped the gadget, we should avoid queuing any requests >>>> >>>> queueing is *not* a problem. Starting is. In fact, that's what your >>>> patch is doing. >>> >>> OK. >>> >>>> >>>>> which will cause tranfer failed. Thus adding some disconnect checking to >>>> ^^^^^^^ >>>> transfer >>> >>> Sorry for spelling mistake, will fix it. >>> >>>> >>>>> 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(+) >>>>> >>>>> 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. BR Janusz >> >> BR >> Janusz >> >>>>> /* >>>>> * 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 >>>>> -- >>>>> 1.7.9.5 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> -- >>>> balbi >>> >>> >>> >>> -- >>> Baolin.wang >>> Best Regards >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Baolin.wang > Best Regards
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web