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

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

[RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget

[Baolin Wang]

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 <baolin.wang@linaro.org>
---
 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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

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

Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget

[Baolin Wang]

Hi,

On 13 October 2016 at 15:08, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> 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

Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi,
>
> On 13 October 2016 at 15:08, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget

[Baolin Wang]

On 13 October 2016 at 15:54, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi,
>>
>> On 13 October 2016 at 15:08, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> 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

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

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

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

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

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

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

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

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

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