xhci: Introduce max wait timeout in xhci_handshake()
Subsystems (2)
PATCH 1/1 xhci: Introduce max wait timeout in xhci_handshake()
The time to finish a xhci_handshake() is platform specific and sometimes during suspend resume test the followng errors were encountered:
[53455.418330] ACPI: Waking up from system sleep state S4
[66838.490856] xhci_hcd 0000:00:14.0: xHCI dying, ignoring interrupt.
Shouldn't IRQs be disabled?After changing the poll time granularity from 1 usec to 20 usec in xhci_handshake() this issue was not reproduced. While tuning on the poll time granularity might be painful on different platforms, it is applicable to introduce a module parameter to allow the xhci driver to wait for at max 16 ms.
--- drivers/usb/host/xhci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/host/xhci.c +5 -1
@@ -38,6 +38,10 @@ static unsigned long long quirks;module_param (quirks, ullong,S_IRUGO );MODULE_PARM_DESC (quirks,"Bit flags for quirks to be enabled as default" ); +static int wait_handshake; +module_param (wait_handshake, int,0644 ); +MODULE_PARM_DESC (wait_handshake,"Force wait for completion of handshake" ); +static bool td_on_ring (struct xhci_td *td,struct xhci_ring *ring) {struct xhci_segment *seg = ring->first_seg;
@@ -74,7 +78,7 @@ int xhci_handshake(void __iomem *ptr, u32 mask, u32 done, int usec) ret =readl_poll_timeout_atomic (ptr, result, (result & mask) == done || result ==U32_MAX , -1 , usec); +1 , wait_handshake ?XHCI_MAX_HALT_USEC : usec);if (result ==U32_MAX )/* card removed */ return -ENODEV ;
Signature · 1 line
2.17.1
Greg KH
maintainerQuoted 6 lines · Chen Yu
The time to finish a
xhci_handshake()is platform specific and sometimes during suspend resume test the followng errors were encountered:[53455.418330] ACPI: Waking up from system sleep state S4 [66838.490856] xhci_hcd 0000:00:14.0: xHCI dying, ignoring interrupt. Shouldn't IRQs be disabled?
Quoted 7 lines
After changing the poll time granularity from 1 usec to 20 usec in
xhci_handshake()this issue was not reproduced. While tuning on the poll time granularity might be painful on different platforms, it is applicable to introduce a module parameter to allow the xhci driver to wait for at max 16 ms.Reported-by "Muchowski, MaciejX" <[email protected]>
I doubt the "X" is part of this person's name, please just spell it out without the "," please.
Quoted 10 lines
Signed-off-by Chen Yu <[email protected]>--- drivers/usb/host/xhci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/usb/host/xhci.c +0
-0
drivers/usb/host/xhci.c static unsigned long long quirks; · line 43
Quoted 6 lines
drivers/usb/host/xhci.c +3
-0@@ -38,6 +38,10 @@ static unsigned long long quirks;module_param (quirks, ullong,S_IRUGO );MODULE_PARM_DESC (quirks,"Bit flags for quirks to be enabled as default" ); +static int wait_handshake; +module_param (wait_handshake, int,0644 ); +MODULE_PARM_DESC (wait_handshake,"Force wait for completion of handshake" );
This is not the 1990's, we are not adding new module parameters that no one will know how to change.
Make this dynamic, and per-device, and work properly instead. This can not handle multiple controllers in the system at all :(
thanks,
greg k-h
Hi Greg,
thanks for taking a look at this.
Quoted 13 lines · Greg KH
Quoted 12 lines · Chen Yu
The time to finish a
xhci_handshake()is platform specific and sometimes during suspend resume test the followng errors were encountered:[53455.418330] ACPI: Waking up from system sleep state S4 [66838.490856] xhci_hcd 0000:00:14.0: xHCI dying, ignoring interrupt. Shouldn't IRQs be disabled?After changing the poll time granularity from 1 usec to 20 usec in
xhci_handshake()this issue was not reproduced. While tuning on the poll time granularity might be painful on different platforms, it is applicable to introduce a module parameter to allow the xhci driver to wait for at max 16 ms.
Quoted 5 lines
Quoted 1 line
Reported-by "Muchowski, MaciejX" <[email protected]>I doubt the "X" is part of this person's name, please just spell it out without the "," please.
Okay, will do.
Quoted 13 lines
Quoted 13 lines
Signed-off-by Chen Yu <[email protected]>--- drivers/usb/host/xhci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/usb/host/xhci.c +0
-0@@ -38,6 +38,10 @@ static unsigned long long quirks;module_param (quirks, ullong,S_IRUGO );MODULE_PARM_DESC (quirks,"Bit flags for quirks to be enabled as default" );
Quoted 7 lines
drivers/usb/host/xhci.c static unsigned long long quirks; · line 43
Quoted 3 lines
drivers/usb/host/xhci.c +3
-0+static int wait_handshake; +module_param (wait_handshake, int,0644 ); +MODULE_PARM_DESC (wait_handshake,"Force wait for completion of handshake" );This is not the 1990's, we are not adding new module parameters that no one will know how to change.
Okay.
Quoted 3 lines
Make this dynamic, and per-device, and work properly instead. This can not handle multiple controllers in the system at all :(
Okay. After checking the error log, enlarging the timeout might not be enough and I'll dig into it a little deeper and send feedback.
thanks,
Chenyu
Quoted 3 lines
thanks,
greg k-h