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 <[email protected]>
---
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
When we change the USB function with configfs dynamically, we possibly met this
situation: one core is doing the control transfer, another core is trying to
unregister the USB gadget from userspace, we must wait for completing this
control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/ep0.c | 2 ++
drivers/usb/dwc3/gadget.c | 23 +++++++++++++++++++++++
3 files changed, 27 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2317e7..9128725 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -745,6 +745,7 @@ struct dwc3_scratchpad_array {
* @ep0_usb_req: dummy req used while handling STD USB requests
* @ep0_bounce_addr: dma address of ep0_bounce
* @scratch_addr: dma address of scratchbuf
+ * @ep0_in_setup: One control transfer is completed and enter setup phase
* @lock: for synchronizing
* @dev: pointer to our struct device
* @xhci: pointer to our xHCI child
@@ -843,6 +844,7 @@ struct dwc3 {
dma_addr_t ep0_bounce_addr;
dma_addr_t scratch_addr;
struct dwc3_request ep0_usb_req;
+ struct completion ep0_in_setup;
/* device lock */
spinlock_t lock;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index fe79d77..06c167a 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -311,6 +311,8 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
ret = dwc3_ep0_start_trans(dwc, 0, dwc->ctrl_req_addr, 8,
DWC3_TRBCTL_CONTROL_SETUP, false);
WARN_ON(ret < 0);
+
+ complete(&dwc->ep0_in_setup);
}
static struct dwc3_ep *dwc3_wIndex_to_dep(struct dwc3 *dwc, __le16 wIndex_le)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ca2ae5b..fef023a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1437,6 +1437,15 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
if (pm_runtime_suspended(dwc->dev))
return 0;
+ /*
+ * Per databook, when we want to stop the gadget, if a control transfer
+ * is still in process, complete it and get the core into setup phase.
+ */
+ if (!is_on && dwc->ep0state != EP0_SETUP_PHASE) {
+ reinit_completion(&dwc->ep0_in_setup);
+ return -EBUSY;
+ }
+
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
if (is_on) {
if (dwc->revision <= DWC3_REVISION_187A) {
@@ -1487,10 +1496,22 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
is_on = !!is_on;
+try_again:
spin_lock_irqsave(&dwc->lock, flags);
ret = dwc3_gadget_run_stop(dwc, is_on, false);
spin_unlock_irqrestore(&dwc->lock, flags);
+ if (ret == -EBUSY) {
+ ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
+ msecs_to_jiffies(500));
+ if (ret == 0) {
+ dev_err(dwc->dev, "timeout to stop gadget.\n");
+ ret = -ETIMEDOUT;
+ } else {
+ goto try_again;
+ }
+ }
+
return ret;
}
@@ -2914,6 +2935,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
goto err4;
}
+ init_completion(&dwc->ep0_in_setup);
+
dwc->gadget.ops = &dwc3_gadget_ops;
dwc->gadget.speed = USB_SPEED_UNKNOWN;
dwc->gadget.sg_supported = true;
--
1.7.9.5
Hi,
Baolin Wang <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
Hi,
Baolin Wang <[email protected]> writes:
> @@ -1487,10 +1496,22 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>
> is_on = !!is_on;
>
> +try_again:
> spin_lock_irqsave(&dwc->lock, flags);
> ret = dwc3_gadget_run_stop(dwc, is_on, false);
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> + if (ret == -EBUSY) {
> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> + msecs_to_jiffies(500));
> + if (ret == 0) {
> + dev_err(dwc->dev, "timeout to stop gadget.\n");
> + ret = -ETIMEDOUT;
> + } else {
> + goto try_again;
you are not really reading my comments. It's the third time I tell you
there's no need for try_again. If you can't complete a control transfer
in 500ms, you already have other issues. Take this thing out of here.
--
balbi
Hi,
On 13 October 2016 at 15:06, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Baolin Wang <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> balbi
--
Baolin.wang
Best Regards
Hi,
On 13 October 2016 at 15:08, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Baolin Wang <[email protected]> writes:
>> @@ -1487,10 +1496,22 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>
>> is_on = !!is_on;
>>
>> +try_again:
>> spin_lock_irqsave(&dwc->lock, flags);
>> ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> spin_unlock_irqrestore(&dwc->lock, flags);
>>
>> + if (ret == -EBUSY) {
>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
>> + msecs_to_jiffies(500));
>> + if (ret == 0) {
>> + dev_err(dwc->dev, "timeout to stop gadget.\n");
>> + ret = -ETIMEDOUT;
>> + } else {
>> + goto try_again;
>
> you are not really reading my comments. It's the third time I tell you
> there's no need for try_again. If you can't complete a control transfer
> in 500ms, you already have other issues. Take this thing out of here.
I think you misunderstood the code. If there is 500ms timeout, we will
return '-ETIMEDOUT' error. If the control transfer is completed before
timeout, we can not just return and we need try again to stop the
gadget, right? Any other good suggestion? Thanks.
--
Baolin.wang
Best Regards
Hi,
Baolin Wang <[email protected]> writes:
> Hi,
>
> On 13 October 2016 at 15:08, Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> Baolin Wang <[email protected]> writes:
>>> @@ -1487,10 +1496,22 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>
>>> is_on = !!is_on;
>>>
>>> +try_again:
>>> spin_lock_irqsave(&dwc->lock, flags);
>>> ret = dwc3_gadget_run_stop(dwc, is_on, false);
>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>>
>>> + if (ret == -EBUSY) {
>>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
>>> + msecs_to_jiffies(500));
>>> + if (ret == 0) {
>>> + dev_err(dwc->dev, "timeout to stop gadget.\n");
>>> + ret = -ETIMEDOUT;
>>> + } else {
>>> + goto try_again;
>>
>> you are not really reading my comments. It's the third time I tell you
>> there's no need for try_again. If you can't complete a control transfer
>> in 500ms, you already have other issues. Take this thing out of here.
>
> I think you misunderstood the code. If there is 500ms timeout, we will
> return '-ETIMEDOUT' error. If the control transfer is completed before
> timeout, we can not just return and we need try again to stop the
> gadget, right? Any other good suggestion? Thanks.
Yeah, change the patch a bit so you wait for completion before calling
dwc3_gadget_runt_stop()? I mean, move the !is_on && ep0_state check
before calling dwc3_gadget_run_stop() and wait_for_completion_timeout()
there.
--
balbi
On 13 October 2016 at 15:54, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Baolin Wang <[email protected]> writes:
>> Hi,
>>
>> On 13 October 2016 at 15:08, Felipe Balbi <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <[email protected]> writes:
>>>> @@ -1487,10 +1496,22 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>
>>>> is_on = !!is_on;
>>>>
>>>> +try_again:
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> ret = dwc3_gadget_run_stop(dwc, is_on, false);
>>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>>>
>>>> + if (ret == -EBUSY) {
>>>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
>>>> + msecs_to_jiffies(500));
>>>> + if (ret == 0) {
>>>> + dev_err(dwc->dev, "timeout to stop gadget.\n");
>>>> + ret = -ETIMEDOUT;
>>>> + } else {
>>>> + goto try_again;
>>>
>>> you are not really reading my comments. It's the third time I tell you
>>> there's no need for try_again. If you can't complete a control transfer
>>> in 500ms, you already have other issues. Take this thing out of here.
>>
>> I think you misunderstood the code. If there is 500ms timeout, we will
>> return '-ETIMEDOUT' error. If the control transfer is completed before
>> timeout, we can not just return and we need try again to stop the
>> gadget, right? Any other good suggestion? Thanks.
>
> Yeah, change the patch a bit so you wait for completion before calling
> dwc3_gadget_runt_stop()? I mean, move the !is_on && ep0_state check
> before calling dwc3_gadget_run_stop() and wait_for_completion_timeout()
> there.
OK. I will refactor the patch. Thanks.
>
> --
> balbi
--
Baolin.wang
Best Regards
On 13 October 2016 at 09:37, Baolin Wang <[email protected]> wrote:
> Hi,
>
> On 13 October 2016 at 15:06, Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> Baolin Wang <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 [email protected]
>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
On 13 October 2016 at 16:16, Janusz Dziedzic <[email protected]> wrote:
> On 13 October 2016 at 09:37, Baolin Wang <[email protected]> wrote:
>> Hi,
>>
>> On 13 October 2016 at 15:06, Felipe Balbi <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 [email protected]
>>>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Baolin.wang
Best Regards
On 13 October 2016 at 16:28, Janusz Dziedzic <[email protected]> wrote:
> On 13 October 2016 at 10:21, Baolin Wang <[email protected]> wrote:
>> Hi,
>>
>> On 13 October 2016 at 16:16, Janusz Dziedzic <[email protected]> wrote:
>>> On 13 October 2016 at 09:37, Baolin Wang <[email protected]> wrote:
>>>> Hi,
>>>>
>>>> On 13 October 2016 at 15:06, Felipe Balbi <[email protected]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Baolin Wang <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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 [email protected]
>>>>>> 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 [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards
--
Baolin.wang
Best Regards
On 13 October 2016 at 10:21, Baolin Wang <[email protected]> wrote:
> Hi,
>
> On 13 October 2016 at 16:16, Janusz Dziedzic <[email protected]> wrote:
>> On 13 October 2016 at 09:37, Baolin Wang <[email protected]> wrote:
>>> Hi,
>>>
>>> On 13 October 2016 at 15:06, Felipe Balbi <[email protected]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Baolin Wang <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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 [email protected]
>>>>> 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 [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Baolin.wang
> Best Regards
Hi,
Baolin Wang <[email protected]> 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
On 13 October 2016 at 17:49, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Baolin Wang <[email protected]> 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
On 13 October 2016 at 12:41, Baolin Wang <[email protected]> wrote:
> On 13 October 2016 at 17:49, Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> Baolin Wang <[email protected]> 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
Hi,
Janusz Dziedzic <[email protected]> writes:
>>> Baolin Wang <[email protected]> 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
Hi,
On 13 October 2016 at 19:22, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Janusz Dziedzic <[email protected]> writes:
>>>> Baolin Wang <[email protected]> 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
Hi Felipe,
On 13 October 2016 at 19:22, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Janusz Dziedzic <[email protected]> writes:
>>>> Baolin Wang <[email protected]> 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
Hi,
Baolin Wang <[email protected]> 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
On 13 October 2016 at 20:17, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Baolin Wang <[email protected]> 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