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