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

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

[Puma Hsu]

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

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 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..c546d9533410 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) || (temp & STS_HCE) || hibernated) {
 
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				!(xhci_all_ports_seen_u0(xhci))) {
-- 
2.34.1.448.ga2b2bfdf31-goog

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

[Greg KH]

On Tue, Dec 28, 2021 at 02:02:46PM +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.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> ---
>  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..c546d9533410 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) || (temp & STS_HCE) || hibernated) {
>  
>  		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
>  				!(xhci_all_ports_seen_u0(xhci))) {
> -- 
> 2.34.1.448.ga2b2bfdf31-goog
> 

What commit does this fix?  Does it need to be backported to older
kernels as well?

thanks,

greg k-h

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

[Sergey Shtylyov]

Hello!

On 12/28/21 9:02 AM, 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.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> ---
>  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..c546d9533410 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) || (temp & STS_HCE) || hibernated) {

	if ((temp & (STS_SRE | STS_HCE)) || hibernated) {

[...]

MBR, Sergey

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

[Puma Hsu]

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.

Thanks in advance.


Thanks in advance.




  •  Puma Hsu 許誌宏
  •  Software Engineer, Pixel Phone
  •  Tel: +886 2 8729 0870
  •  pumahsu@google.com





On Tue, Dec 28, 2021 at 4:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 28, 2021 at 02:02:46PM +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.
> >
> > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > ---
> >  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..c546d9533410 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) || (temp & STS_HCE) || hibernated) {
> >
> >               if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
> >                               !(xhci_all_ports_seen_u0(xhci))) {
> > --
> > 2.34.1.448.ga2b2bfdf31-goog
> >
>
> What commit does this fix?  Does it need to be backported to older
> kernels as well?
>
> thanks,
>
> greg k-h

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

[Puma Hsu]

On Tue, Dec 28, 2021 at 10:34 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>
> Hello!
>
> On 12/28/21 9:02 AM, 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.
> >
> > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > ---
> >  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..c546d9533410 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) || (temp & STS_HCE) || hibernated) {
>
>         if ((temp & (STS_SRE | STS_HCE)) || hibernated) {
>
> [...]
>
> MBR, Sergey

Thanks for advising, I will submit patch v2.

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

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Wed, Dec 29, 2021 at 01:53:04PM +0800, Puma Hsu wrote:
> 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 does not actually fix an issue that you have seen in any device
or testing?  So it is not relevant for older kernels but just "nice to
have"?

How did you test this if you can not duplicate the problem?

thanks,

greg k-h

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

[Puma Hsu]

On Wed, Dec 29, 2021 at 4:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Wed, Dec 29, 2021 at 01:53:04PM +0800, Puma Hsu wrote:
> > 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 does not actually fix an issue that you have seen in any device
> or testing?  So it is not relevant for older kernels but just "nice to
> have"?
>
> How did you test this if you can not duplicate the problem?
>

Yes, we actually see that the HCE may be detected while running xhci_resume
on our product platform, so I'm able to verify this commit can fix
such a condition.
For older kernels, I'm not sure whether someone had ever met such issue, but I
believe the Run/Stop bit of USBCMD cannot be set once HCE is raised.

Thanks.
Puma Hsu

> thanks,
>
> greg k-h

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

[Greg KH]

On Wed, Dec 29, 2021 at 05:11:47PM +0800, Puma Hsu wrote:
> On Wed, Dec 29, 2021 at 4:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > A: http://en.wikipedia.org/wiki/Top_post
> > Q: Were do I find info about this thing called top-posting?
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> >
> > A: No.
> > Q: Should I include quotations after my reply?
> >
> > http://daringfireball.net/2007/07/on_top
> >
> > On Wed, Dec 29, 2021 at 01:53:04PM +0800, Puma Hsu wrote:
> > > 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 does not actually fix an issue that you have seen in any device
> > or testing?  So it is not relevant for older kernels but just "nice to
> > have"?
> >
> > How did you test this if you can not duplicate the problem?
> >
> 
> Yes, we actually see that the HCE may be detected while running xhci_resume
> on our product platform, so I'm able to verify this commit can fix
> such a condition.

Given that your product platform is an older kernel version than 5.17, I
think that you also want this in the older kernel releases, so please
mark it for stable backporting.

thanks,

greg k-h

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

[Puma Hsu]

On Wed, Dec 29, 2021 at 5:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Dec 29, 2021 at 05:11:47PM +0800, Puma Hsu wrote:
> > On Wed, Dec 29, 2021 at 4:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > A: http://en.wikipedia.org/wiki/Top_post
> > > Q: Were do I find info about this thing called top-posting?
> > > A: Because it messes up the order in which people normally read text.
> > > Q: Why is top-posting such a bad thing?
> > > A: Top-posting.
> > > Q: What is the most annoying thing in e-mail?
> > >
> > > A: No.
> > > Q: Should I include quotations after my reply?
> > >
> > > http://daringfireball.net/2007/07/on_top
> > >
> > > On Wed, Dec 29, 2021 at 01:53:04PM +0800, Puma Hsu wrote:
> > > > 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 does not actually fix an issue that you have seen in any device
> > > or testing?  So it is not relevant for older kernels but just "nice to
> > > have"?
> > >
> > > How did you test this if you can not duplicate the problem?
> > >
> >
> > Yes, we actually see that the HCE may be detected while running xhci_resume
> > on our product platform, so I'm able to verify this commit can fix
> > such a condition.
>
> Given that your product platform is an older kernel version than 5.17, I
> think that you also want this in the older kernel releases, so please
> mark it for stable backporting.
>
Thank you for advising.
Could I know how to do this? Just add "Cc: <stable@vger.kernel.org>"
to the commit message?

Thanks.
Puma Hsu

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

[Greg KH]

On Wed, Dec 29, 2021 at 06:21:05PM +0800, Puma Hsu wrote:
> On Wed, Dec 29, 2021 at 5:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Dec 29, 2021 at 05:11:47PM +0800, Puma Hsu wrote:
> > > On Wed, Dec 29, 2021 at 4:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > A: http://en.wikipedia.org/wiki/Top_post
> > > > Q: Were do I find info about this thing called top-posting?
> > > > A: Because it messes up the order in which people normally read text.
> > > > Q: Why is top-posting such a bad thing?
> > > > A: Top-posting.
> > > > Q: What is the most annoying thing in e-mail?
> > > >
> > > > A: No.
> > > > Q: Should I include quotations after my reply?
> > > >
> > > > http://daringfireball.net/2007/07/on_top
> > > >
> > > > On Wed, Dec 29, 2021 at 01:53:04PM +0800, Puma Hsu wrote:
> > > > > 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 does not actually fix an issue that you have seen in any device
> > > > or testing?  So it is not relevant for older kernels but just "nice to
> > > > have"?
> > > >
> > > > How did you test this if you can not duplicate the problem?
> > > >
> > >
> > > Yes, we actually see that the HCE may be detected while running xhci_resume
> > > on our product platform, so I'm able to verify this commit can fix
> > > such a condition.
> >
> > Given that your product platform is an older kernel version than 5.17, I
> > think that you also want this in the older kernel releases, so please
> > mark it for stable backporting.
> >
> Thank you for advising.
> Could I know how to do this? Just add "Cc: <stable@vger.kernel.org>"
> to the commit message?

Yes, please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

It should describe this well, if not, please let us know.

thanks,

greg k-h