| Message ID | 20211228060246.2958070-1-pumahsu@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xhci: re-initialize the HC during resume if HCE was set | expand |
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
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
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
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.
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
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
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
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
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
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))) {
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(-)