[EXT] Re: [PATCH] usb: ehci-hcd: Add IAA handshake for removing async QH

[Ye Li]

Hi Marek,

On Mon, 2021-03-08 at 09:50 +0100, Marek Vasut wrote:
> Caution: EXT Email
> 
> On 3/8/21 4:35 AM, Ye Li wrote:
> [...]
> > 
> > +static int ehci_iaa_cycle(struct ehci_ctrl *ctrl)
> > +{
> > +     u32 cmd, status;
> > +     int ret;
> > +
> > +     /* Enable Interrupt on Async Advance Doorbell. */
> > +     cmd = ehci_readl(&ctrl->hcor->or_usbcmd);
> > +     cmd |= CMD_IAAD;
> > +     ehci_writel(&ctrl->hcor->or_usbcmd, cmd);
> > +
> > +     ret = handshake((uint32_t *)&ctrl->hcor->or_usbsts, STS_IAA,
> > STS_IAA,
> Is the (uint32_t *) cast really needed ?
> 
I will remove it in v2.

> > 
> > +                     10 * 1000); /* 10ms timeout */
> > +     if (ret < 0)
> > +             printf("EHCI fail timeout STS_IAA set\n");
> Shouldn't there be some abort ^ if ret < 0 ?
> Also, add the return value into the printf, it is useful for
> debugging.
I don't think need to abort anything when ret < 0. The ret value is
very simple in handshake, when ret < 0, it must be -1. 
Actually the codes refer the ehci_enable_async for the usage of
handshake.


> 
> > 
> > +     status = ehci_readl(&ctrl->hcor->or_usbsts);
> > +     if ((status & STS_IAA))
> Drop the double parenthesis here, one set of () is enough.
> 
I will remove it v2.

> > 
> > +             ehci_writel(&ctrl->hcor->or_usbsts, STS_IAA);
> > +
> > +     return ret;
> > +}
> Is this a bugfix for this release or is this for next release ?
> We're in rc3 already, so I would suggest to be careful.
It is ok to add the fix for next release. From our tests, the issue is
not very easy to reproduce by just using usb read/write.  In 
SCT, generally we need to run about 1 hour to see the issue. 

Best regards,
Ye Li