Re: [PATCH v3] xhci: re-initialize the HC during resume if HCE was set

[Puma Hsu]

On Thu, Jan 13, 2022 at 4:21 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jan 13, 2022 at 03:54:27PM +0800, Puma Hsu wrote:
> > On Tue, Jan 11, 2022 at 7:43 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 29, 2021 at 07:25:51PM +0800, Puma Hsu wrote:
> > > > When HCE(Host Controller Error) is set, it means an internal
> > > > error condition has been detected. It needs to re-initialize
> > > > the HC too.
> > >
> > > What is "It" in the last sentence?
> >
> > Maybe I can change "It" to "Software", xHCI specification uses
> > "Software" when describing this.
>
> Please change it to something better :)

I will fix it in next patch version.

> > > >
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Puma Hsu <pumahsu@xxxxxxxxxx>
> > >
> > > What commit id does this fix?
> >
> > This commit is not used to fix a specific commit. We find a condition
> > that when XHCI runs the resume process but the HCE flag is set, then
> > the Run/Stop bit of USBCMD cannot be set so that HC would not be
> > enabled. In fact, HC may already meet a problem at this moment.
> > Besides, in xHCI requirements specification revision 1.2, Table 5-21
> > BIT(12) claims that Software should re-initialize the xHC when HCE is
> > set. Therefore, I think this commit could be the error handling for
> > HCE.
>
> So this problem has been there since the driver was first added to the
> kernel?  Should it go to stable kernels as well?  If so, how far back in
> time?

I think XHCI hasn’t handled HCE, so yes this may be a long problem.
I have cced stable@xxxxxxxxxxxxxxx for stable backporting, but I’m not sure
how far it should backport since it seems this might be a rare case if no one
reported this issue?

> > > > ---
> > > > v2: Follow Sergey Shtylyov <s.shtylyov@xxxxxx>'s comment.
> > > > v3: Add stable@xxxxxxxxxxxxxxx for stable release.
> > > >
> > > >  drivers/usb/host/xhci.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > > index dc357cabb265..ab440ce8420f 100644
> > > > --- a/drivers/usb/host/xhci.c
> > > > +++ b/drivers/usb/host/xhci.c
> > > > @@ -1146,8 +1146,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> > > >               temp = readl(&xhci->op_regs->status);
> > > >       }
> > > >
> > > > -     /* If restore operation fails, re-initialize the HC during resume */
> > > > -     if ((temp & STS_SRE) || hibernated) {
> > > > +     /* If restore operation fails or HC error is detected, re-initialize the HC during resume */
> > > > +     if ((temp & (STS_SRE | STS_HCE)) || hibernated) {
> > >
> > > But if STS_HCE is set on suspend, that means the suspend was broken so
> > > you wouldn't get here, right?
> >
> > In xhci_suspend(), it seems doesn't really check whether STS_HCE is
> > set and then break the suspend(The only case for checking HCE is when
> > STS_SAVE setting failed). So suspend function may be still able to
> > finish even if HCE is set? Then xhci_resume will still be called.
>
> Is this a problem?

It could be, but I'm not sure and I think it may be not so serious if
HCE was raised
while suspend, because host controller doesn’t have job while suspend.
And we are
trying to recover it while resume.

> > > Or can the error happen between suspend and resume?
> > >
> > > This seems like a big hammer for when the host controller throws an
> > > error.  Why is this the only place that it should be checked for?  What
> > > caused the error that can now allow it to be fixed?
> >
> > I believe this is not the only place that the host controller may set
> > HCE, the host controller may set HCE anytime it sees an error in my
> > opinion, not only in suspend or resume.
>
> Then where else should it be checked?  Where else will your silicon set
> this bit as part of the normal operating process?

We observed this flag while resume in our silicon so far. According to the XHCI
specification 4.24.1, “Software should implement an algorithm for checking the
HCE flag if the xHC is not responding.”, so maybe it would be better
to implement
a new API to recover host controller whenever the driver side finds no response
from host controller in the future.

> thanks,
>
> greg k-h