* [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage
@ 2019-10-31 10:04 Jun Chen
2019-11-28 12:46 ` Minas Harutyunyan
0 siblings, 1 reply; 3+ messages in thread
From: Jun Chen @ 2019-10-31 10:04 UTC (permalink / raw)
To: linux-usb; +Cc: hminas, Jun Chen
From: Jun Chen <jun.chen@vatics.com>
According to USB2.0 spec 8.5.3, "If the control sequence
has no Data stage, then it consists of a Setup stage
followed by a Status stage consisting of an IN transaction."
But when doing control read in some HOST (like MS Windows),
after a SETUP transaction with no Data stage, the sequence
stay in the Status stage of an OUT transaction until timeout.
This patch Stall both IN and OUT on ep0 in status stage,
fix the unhandling state when we got an error command
with zero length control read request.
It's also based on the USB2.0 spec 8.5.3.4,
"The protocol STALL condition lasts until the receipt of
the next SETUP transaction, and the function will return
STALL in response to any IN or OUT transaction on the pipe
until the SETUP transaction is received."
Signed-off-by: Jun Chen <jun.chen@vatics.com>
---
drivers/usb/dwc2/gadget.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6be10e496..73b5944 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1853,23 +1853,30 @@ static void dwc2_hsotg_stall_ep0(struct dwc2_hsotg *hsotg)
struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
u32 reg;
u32 ctrl;
+ u32 direction;
- dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n", ep0->dir_in);
- reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
+ direction = ep0->dir_in;
+ do {
+ dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n", ep0->dir_in);
+ reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
- /*
- * DxEPCTL_Stall will be cleared by EP once it has
- * taken effect, so no need to clear later.
- */
+ /*
+ * DxEPCTL_Stall will be cleared by EP once it has
+ * taken effect, so no need to clear later.
+ */
- ctrl = dwc2_readl(hsotg, reg);
- ctrl |= DXEPCTL_STALL;
- ctrl |= DXEPCTL_CNAK;
- dwc2_writel(hsotg, ctrl, reg);
+ ctrl = dwc2_readl(hsotg, reg);
+ ctrl |= DXEPCTL_STALL;
+ ctrl |= DXEPCTL_CNAK;
+ dwc2_writel(hsotg, ctrl, reg);
- dev_dbg(hsotg->dev,
- "written DXEPCTL=0x%08x to %08x (DXEPCTL=0x%08x)\n",
- ctrl, reg, dwc2_readl(hsotg, reg));
+ dev_dbg(hsotg->dev,
+ "written DXEPCTL=0x%08x to %08x (DXEPCTL=0x%08x)\n",
+ ctrl, reg, dwc2_readl(hsotg, reg));
+
+ if (hsotg->ep0_state == DWC2_EP0_STATUS_IN)
+ ep0->dir_in = (ep0->dir_in == 1) ? 0 : 1;
+ } while (ep0->dir_in != direction);
/*
* complete won't be called, so we enqueue
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage
2019-10-31 10:04 [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage Jun Chen
@ 2019-11-28 12:46 ` Minas Harutyunyan
[not found] ` <CADY+QgtWY2Hsw-D6jMC5bGCj6A1BL4DFu6dZgFwNgxwJrFXn4A@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Minas Harutyunyan @ 2019-11-28 12:46 UTC (permalink / raw)
To: Jun Chen, linux-usb; +Cc: Jun Chen
Hi Jun,
On 10/31/2019 2:04 PM, Jun Chen wrote:
> From: Jun Chen <jun.chen@vatics.com>
>
> According to USB2.0 spec 8.5.3, "If the control sequence
> has no Data stage, then it consists of a Setup stage
> followed by a Status stage consisting of an IN transaction."
>
> But when doing control read in some HOST (like MS Windows),
> after a SETUP transaction with no Data stage, the sequence
> stay in the Status stage of an OUT transaction until timeout.
>
Could you please provide debug log of above scenario?
> This patch Stall both IN and OUT on ep0 in status stage,
> fix the unhandling state when we got an error command
> with zero length control read request.
>
> It's also based on the USB2.0 spec 8.5.3.4,
> "The protocol STALL condition lasts until the receipt of
> the next SETUP transaction, and the function will return
> STALL in response to any IN or OUT transaction on the pipe
> until the SETUP transaction is received."
>
> Signed-off-by: Jun Chen <jun.chen@vatics.com>
> ---
> drivers/usb/dwc2/gadget.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6be10e496..73b5944 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1853,23 +1853,30 @@ static void dwc2_hsotg_stall_ep0(struct dwc2_hsotg *hsotg)
> struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
> u32 reg;
> u32 ctrl;
> + u32 direction;
>
> - dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n", ep0->dir_in);
> - reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
> + direction = ep0->dir_in;
> + do {
> + dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n", ep0->dir_in);
> + reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
>
> - /*
> - * DxEPCTL_Stall will be cleared by EP once it has
> - * taken effect, so no need to clear later.
> - */
> + /*
> + * DxEPCTL_Stall will be cleared by EP once it has
> + * taken effect, so no need to clear later.
> + */
>
> - ctrl = dwc2_readl(hsotg, reg);
> - ctrl |= DXEPCTL_STALL;
> - ctrl |= DXEPCTL_CNAK;
> - dwc2_writel(hsotg, ctrl, reg);
> + ctrl = dwc2_readl(hsotg, reg);
> + ctrl |= DXEPCTL_STALL;
> + ctrl |= DXEPCTL_CNAK;
> + dwc2_writel(hsotg, ctrl, reg);
>
> - dev_dbg(hsotg->dev,
> - "written DXEPCTL=0x%08x to %08x (DXEPCTL=0x%08x)\n",
> - ctrl, reg, dwc2_readl(hsotg, reg));
> + dev_dbg(hsotg->dev,
> + "written DXEPCTL=0x%08x to %08x (DXEPCTL=0x%08x)\n",
> + ctrl, reg, dwc2_readl(hsotg, reg));
> +
> + if (hsotg->ep0_state == DWC2_EP0_STATUS_IN)
> + ep0->dir_in = (ep0->dir_in == 1) ? 0 : 1;
> + } while (ep0->dir_in != direction);
>
> /*
> * complete won't be called, so we enqueue
>
Thanks,
Minas
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage
[not found] ` <CADY+QgtWY2Hsw-D6jMC5bGCj6A1BL4DFu6dZgFwNgxwJrFXn4A@mail.gmail.com>
@ 2019-12-02 11:53 ` Minas Harutyunyan
0 siblings, 0 replies; 3+ messages in thread
From: Minas Harutyunyan @ 2019-12-02 11:53 UTC (permalink / raw)
To: pt, Minas Harutyunyan; +Cc: linux-usb, Jun Chen
Hi Jun,
On 11/29/2019 2:06 PM, pt wrote:
> Hi Minas,
> In this situation, there is no debug log in Linux side (because
> gadget set stall IN, but host waiting for OUT)
> So I wrote a debug procedure with USB analyzer log images,
> please read here https://rm2brothers.cc/uvc_stall_0_len.html
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__rm2brothers.cc_uvc-5Fstall-5F0-5Flen.html&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=92lbZa_kRi14EpsJoD09lwzQ1pln2WfPpX5yshbCOwQ&s=Dv2DmWPFqHsQLcuYDq0OVhnZ8UUjGiUubmksrjioN3I&e=> to
> check log.
>
> Thanks,
> Jun
>
In mentioned by you case "in this special case, when we got error
command with zero length", host behave prematurely - sending OUT ZLP
(Status OUT stage), instead of sending IN token for Status IN stage.
This behavior described in USB 20 spec in "5.5.5 Control Transfer Data
Sequences". It's known as Control transfers exceptional cases, which
actually supported by HSOTG core, but in dwc2 exceptional cases not handled.
We have plans (not scheduled yet) to add to dwc2 handling of exceptional
cases of control transfers as described in HSOTG programming guide,
which will resolve your case without any additional stalling.
Your patch uses stall mechanism to solve one of exceptional cases. I
treat it as workaround and can't Ack this patch.
Thanks,
Minas
> Minas Harutyunyan <Minas.Harutyunyan@synopsys.com
> <mailto:Minas.Harutyunyan@synopsys.com>> 於 2019年11月28日 週四 下午8:46
> 寫道：
>
> Hi Jun,
>
> On 10/31/2019 2:04 PM, Jun Chen wrote:
> > From: Jun Chen <jun.chen@vatics.com <mailto:jun.chen@vatics.com>>
> >
> > According to USB2.0 spec 8.5.3, "If the control sequence
> > has no Data stage, then it consists of a Setup stage
> > followed by a Status stage consisting of an IN transaction."
> >
> > But when doing control read in some HOST (like MS Windows),
> > after a SETUP transaction with no Data stage, the sequence
> > stay in the Status stage of an OUT transaction until timeout.
> >
> Could you please provide debug log of above scenario?
>
> > This patch Stall both IN and OUT on ep0 in status stage,
> > fix the unhandling state when we got an error command
> > with zero length control read request.
> >
> > It's also based on the USB2.0 spec 8.5.3.4,
> > "The protocol STALL condition lasts until the receipt of
> > the next SETUP transaction, and the function will return
> > STALL in response to any IN or OUT transaction on the pipe
> > until the SETUP transaction is received."
> >
> > Signed-off-by: Jun Chen <jun.chen@vatics.com
> <mailto:jun.chen@vatics.com>>
> > ---
> > drivers/usb/dwc2/gadget.c | 33 ++++++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 6be10e496..73b5944 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -1853,23 +1853,30 @@ static void dwc2_hsotg_stall_ep0(struct
> dwc2_hsotg *hsotg)
> > struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
> > u32 reg;
> > u32 ctrl;
> > + u32 direction;
> >
> > - dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n", ep0->dir_in);
> > - reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
> > + direction = ep0->dir_in;
> > + do {
> > + dev_dbg(hsotg->dev, "ep0 stall (dir=%d)\n",
> ep0->dir_in);
> > + reg = (ep0->dir_in) ? DIEPCTL0 : DOEPCTL0;
> >
> > - /*
> > - * DxEPCTL_Stall will be cleared by EP once it has
> > - * taken effect, so no need to clear later.
> > - */
> > + /*
> > + * DxEPCTL_Stall will be cleared by EP once it has
> > + * taken effect, so no need to clear later.
> > + */
> >
> > - ctrl = dwc2_readl(hsotg, reg);
> > - ctrl |= DXEPCTL_STALL;
> > - ctrl |= DXEPCTL_CNAK;
> > - dwc2_writel(hsotg, ctrl, reg);
> > + ctrl = dwc2_readl(hsotg, reg);
> > + ctrl |= DXEPCTL_STALL;
> > + ctrl |= DXEPCTL_CNAK;
> > + dwc2_writel(hsotg, ctrl, reg);
> >
> > - dev_dbg(hsotg->dev,
> > - "written DXEPCTL=0x%08x to %08x (DXEPCTL=0x%08x)\n",
> > - ctrl, reg, dwc2_readl(hsotg, reg));
> > + dev_dbg(hsotg->dev,
> > + "written DXEPCTL=0x%08x to %08x
> (DXEPCTL=0x%08x)\n",
> > + ctrl, reg, dwc2_readl(hsotg, reg));
> > +
> > + if (hsotg->ep0_state == DWC2_EP0_STATUS_IN)
> > + ep0->dir_in = (ep0->dir_in == 1) ? 0 : 1;
> > + } while (ep0->dir_in != direction);
> >
> > /*
> > * complete won't be called, so we enqueue
> >
>
> Thanks,
> Minas
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-02 11:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 10:04 [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage Jun Chen
2019-11-28 12:46 ` Minas Harutyunyan
[not found] ` <CADY+QgtWY2Hsw-D6jMC5bGCj6A1BL4DFu6dZgFwNgxwJrFXn4A@mail.gmail.com>
2019-12-02 11:53 ` Minas Harutyunyan
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.