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

[Baolin Wang]

On 13 October 2016 at 20:17, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

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


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi Felipe,

On 13 October 2016 at 19:22, Felipe Balbi  wrote:
>
> Hi,
>
> Janusz Dziedzic  writes:
 Baolin Wang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baolin Wang]

Hi,

On 13 October 2016 at 19:22, Felipe Balbi  wrote:
>
> Hi,
>
> Janusz Dziedzic  writes:
 Baolin Wang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Janusz Dziedzic  writes:
>>> Baolin Wang  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


signature.asc
Description: PGP signature

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

[Janusz Dziedzic]

On 13 October 2016 at 12:41, Baolin Wang  wrote:
> On 13 October 2016 at 17:49, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
>
> Hi,
>
> Baolin Wang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

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


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 13 October 2016 at 16:28, Janusz Dziedzic  wrote:
> On 13 October 2016 at 10:21, Baolin Wang  wrote:
>> Hi,
>>
>> On 13 October 2016 at 16:16, Janusz Dziedzic  
>> wrote:
>>> On 13 October 2016 at 09:37, Baolin Wang  wrote:
 Hi,

 On 13 October 2016 at 15:06, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>> ---
>> 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 majord...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Dziedzic]

On 13 October 2016 at 10:21, Baolin Wang  wrote:
> Hi,
>
> On 13 October 2016 at 16:16, Janusz Dziedzic  
> wrote:
>> On 13 October 2016 at 09:37, Baolin Wang  wrote:
>>> Hi,
>>>
>>> On 13 October 2016 at 15:06, Felipe Balbi  wrote:

 Hi,

 Baolin Wang  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 
> ---
> 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 majord...@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 majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Baolin.wang
> Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baolin Wang]

Hi,

On 13 October 2016 at 16:16, Janusz Dziedzic  wrote:
> On 13 October 2016 at 09:37, Baolin Wang  wrote:
>> Hi,
>>
>> On 13 October 2016 at 15:06, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  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 
 ---
 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 majord...@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 majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Janusz Dziedzic]

On 13 October 2016 at 09:37, Baolin Wang  wrote:
> Hi,
>
> On 13 October 2016 at 15:06, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  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 
>>> ---
>>> 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 majord...@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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Baolin Wang]

Hi,

On 13 October 2016 at 15:06, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>> ---
>> 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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Baolin Wang  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 
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

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 
---
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html