MIME-Version: 1.0
References: <20211229112551.3483931-1-pumahsu@google.com> <Yd1tUKhyZf26OVNQ@kroah.com>
In-Reply-To: <Yd1tUKhyZf26OVNQ@kroah.com>
From:   Puma Hsu <pumahsu@google.com>
Date:   Thu, 13 Jan 2022 15:54:27 +0800
Message-ID: <CAGCq0LZb8nQDvcz=LswWi4qKd-65ys6iPjTKh=46dVtYLDEUVw@mail.gmail.com>
Subject: Re: [PATCH v3] xhci: re-initialize the HC during resume if HCE was set
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     mathias.nyman@intel.com, Sergey Shtylyov <s.shtylyov@omp.ru>,
        Albert Wang <albertccwang@google.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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

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

> 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.
I think this could be a recovery if xhci finds HCE during the resume process.
If someone finds HCE in other functions, it may also need to do the
recovery too.


> thanks,
>
> greg k-h