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

When HCE(Host Controller Error) is set, it means an internal
error condition has been detected. It needs to re-initialize
the HC too.

Cc: stable@vger.kernel.org
Signed-off-by: Puma Hsu <pumahsu@google.com>
---
v2: Follow Sergey Shtylyov <s.shtylyov@omp.ru>'s comment.
v3: Add stable@vger.kernel.org for stable release.

 drivers/usb/host/xhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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@linuxfoundation.org> 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 :)

> > >
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> >
> > 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?

> > > ---
> > > v2: Follow Sergey Shtylyov <s.shtylyov@omp.ru>'s comment.
> > > v3: Add stable@vger.kernel.org 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?

> > 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?

thanks,

greg k-h

On Thu, Jan 13, 2022 at 4:21 PM Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.org> 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@vger.kernel.org
> > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > >
> > > 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@vger.kernel.org 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@omp.ru>'s comment.
> > > > v3: Add stable@vger.kernel.org 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

>>> 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.
> 

As all the code to reset the host during resume already exists, and is well tried
due to issues in resume being so common, I think it makes sense to add the HCE case 
here as well. It's a simple fix that makes the life of users better.

That said we shouldn't hide the reason for reset like this.
Print a debug message telling about the HCE so that everybody working with xHCI
can see it and start fixing the rootcause.

Another HCE check could be added to command timeout code, but just to show a
warning for now.
Reset might not always clear HCE, and we don't want to be stuck in a reset loop.
This check  could be a separate patch

-Mathias