* [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically @ 2016-10-04 8:42 Baolin Wang 2016-10-04 8:42 ` [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget Baolin Wang 2016-10-13 7:06 ` [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi 0 siblings, 2 replies; 20+ messages in thread From: Baolin Wang @ 2016-10-04 8:42 UTC (permalink / raw) To: balbi; +Cc: gregkh, broonie, linux-usb, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget 2016-10-04 8:42 [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang @ 2016-10-04 8:42 ` Baolin Wang 2016-10-13 7:08 ` Felipe Balbi 2016-10-13 7:06 ` [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi 1 sibling, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-04 8:42 UTC (permalink / raw) To: balbi; +Cc: gregkh, broonie, linux-usb, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget 2016-10-04 8:42 ` [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget Baolin Wang @ 2016-10-13 7:08 ` Felipe Balbi 2016-10-13 7:51 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 7:08 UTC (permalink / raw) To: Baolin Wang; +Cc: gregkh, broonie, linux-usb, linux-kernel, baolin.wang [-- 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 --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget 2016-10-13 7:08 ` Felipe Balbi @ 2016-10-13 7:51 ` Baolin Wang 2016-10-13 7:54 ` Felipe Balbi 0 siblings, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 7:51 UTC (permalink / raw) To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget 2016-10-13 7:51 ` Baolin Wang @ 2016-10-13 7:54 ` Felipe Balbi 2016-10-13 8:01 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 7:54 UTC (permalink / raw) To: Baolin Wang; +Cc: Greg KH, Mark Brown, USB, LKML [-- 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 --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget 2016-10-13 7:54 ` Felipe Balbi @ 2016-10-13 8:01 ` Baolin Wang 0 siblings, 0 replies; 20+ messages in thread From: Baolin Wang @ 2016-10-13 8:01 UTC (permalink / raw) To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-04 8:42 [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang 2016-10-04 8:42 ` [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget Baolin Wang @ 2016-10-13 7:06 ` Felipe Balbi 2016-10-13 7:37 ` Baolin Wang 1 sibling, 1 reply; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 7:06 UTC (permalink / raw) To: Baolin Wang; +Cc: gregkh, broonie, linux-usb, linux-kernel, baolin.wang [-- 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 --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 7:06 ` [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi @ 2016-10-13 7:37 ` Baolin Wang 2016-10-13 8:16 ` Janusz Dziedzic 0 siblings, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 7:37 UTC (permalink / raw) To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 7:37 ` Baolin Wang @ 2016-10-13 8:16 ` Janusz Dziedzic 2016-10-13 8:21 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Janusz Dziedzic @ 2016-10-13 8:16 UTC (permalink / raw) To: Baolin Wang; +Cc: Felipe Balbi, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 8:16 ` Janusz Dziedzic @ 2016-10-13 8:21 ` Baolin Wang 2016-10-13 8:28 ` Janusz Dziedzic 0 siblings, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 8:21 UTC (permalink / raw) To: Janusz Dziedzic; +Cc: Felipe Balbi, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 8:21 ` Baolin Wang @ 2016-10-13 8:28 ` Janusz Dziedzic 2016-10-13 8:39 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Janusz Dziedzic @ 2016-10-13 8:28 UTC (permalink / raw) To: Baolin Wang; +Cc: Felipe Balbi, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 8:28 ` Janusz Dziedzic @ 2016-10-13 8:39 ` Baolin Wang 2016-10-13 9:49 ` Felipe Balbi 0 siblings, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 8:39 UTC (permalink / raw) To: Janusz Dziedzic; +Cc: Felipe Balbi, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 8:39 ` Baolin Wang @ 2016-10-13 9:49 ` Felipe Balbi 2016-10-13 10:41 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 9:49 UTC (permalink / raw) To: Baolin Wang, Janusz Dziedzic; +Cc: Greg KH, Mark Brown, USB, LKML [-- 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 --] ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 9:49 ` Felipe Balbi @ 2016-10-13 10:41 ` Baolin Wang 2016-10-13 11:16 ` Janusz Dziedzic 0 siblings, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 10:41 UTC (permalink / raw) To: Felipe Balbi; +Cc: Janusz Dziedzic, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 10:41 ` Baolin Wang @ 2016-10-13 11:16 ` Janusz Dziedzic 2016-10-13 11:22 ` Felipe Balbi 0 siblings, 1 reply; 20+ messages in thread From: Janusz Dziedzic @ 2016-10-13 11:16 UTC (permalink / raw) To: Baolin Wang; +Cc: Felipe Balbi, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 11:16 ` Janusz Dziedzic @ 2016-10-13 11:22 ` Felipe Balbi 2016-10-13 11:30 ` Baolin Wang 2016-10-13 11:46 ` Baolin Wang 0 siblings, 2 replies; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 11:22 UTC (permalink / raw) To: Janusz Dziedzic, Baolin Wang; +Cc: Greg KH, Mark Brown, USB, LKML [-- 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 --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 11:22 ` Felipe Balbi @ 2016-10-13 11:30 ` Baolin Wang 2016-10-13 11:46 ` Baolin Wang 1 sibling, 0 replies; 20+ messages in thread From: Baolin Wang @ 2016-10-13 11:30 UTC (permalink / raw) To: Felipe Balbi; +Cc: Janusz Dziedzic, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 11:22 ` Felipe Balbi 2016-10-13 11:30 ` Baolin Wang @ 2016-10-13 11:46 ` Baolin Wang 2016-10-13 12:17 ` Felipe Balbi 1 sibling, 1 reply; 20+ messages in thread From: Baolin Wang @ 2016-10-13 11:46 UTC (permalink / raw) To: Felipe Balbi; +Cc: Janusz Dziedzic, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 11:46 ` Baolin Wang @ 2016-10-13 12:17 ` Felipe Balbi 2016-10-13 12:23 ` Baolin Wang 0 siblings, 1 reply; 20+ messages in thread From: Felipe Balbi @ 2016-10-13 12:17 UTC (permalink / raw) To: Baolin Wang; +Cc: Janusz Dziedzic, Greg KH, Mark Brown, USB, LKML [-- 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 --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically 2016-10-13 12:17 ` Felipe Balbi @ 2016-10-13 12:23 ` Baolin Wang 0 siblings, 0 replies; 20+ messages in thread From: Baolin Wang @ 2016-10-13 12:23 UTC (permalink / raw) To: Felipe Balbi; +Cc: Janusz Dziedzic, Greg KH, Mark Brown, USB, LKML 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-10-13 12:29 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-04 8:42 [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang 2016-10-04 8:42 ` [RESEND PATCH v3 2/2] usb: dwc3: Wait for control tranfer completed when stopping gadget Baolin Wang 2016-10-13 7:08 ` Felipe Balbi 2016-10-13 7:51 ` Baolin Wang 2016-10-13 7:54 ` Felipe Balbi 2016-10-13 8:01 ` Baolin Wang 2016-10-13 7:06 ` [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi 2016-10-13 7:37 ` Baolin Wang 2016-10-13 8:16 ` Janusz Dziedzic 2016-10-13 8:21 ` Baolin Wang 2016-10-13 8:28 ` Janusz Dziedzic 2016-10-13 8:39 ` Baolin Wang 2016-10-13 9:49 ` Felipe Balbi 2016-10-13 10:41 ` Baolin Wang 2016-10-13 11:16 ` Janusz Dziedzic 2016-10-13 11:22 ` Felipe Balbi 2016-10-13 11:30 ` Baolin Wang 2016-10-13 11:46 ` Baolin Wang 2016-10-13 12:17 ` Felipe Balbi 2016-10-13 12:23 ` Baolin Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.