Re: [PATCH] usb: dwc2: gadget: stall handshakes returned by control pipes in status stage

[Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>]

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
>