From: Zhang Lixu lixu.zhang@intel.com
[ Upstream commit dfbd535db74df0343ca39670e06326d7aee8c8f4 ]
During s2idle suspend/resume testing on some systems, occasional several tens of seconds delays were observed in HID sensor resume handling. Trace analysis revealed repeated "link not ready" timeout errors during set/get_report operations, which were traced to the hid_ishtp_cl_resume_handler() timing out while waiting for the ISHTP resume acknowledgment. The previous timeout was set to 50ms, which proved insufficient on affected machines.
Empirical measurements on failing systems showed that the time from ISH resume initiation to receiving the ISHTP resume ack could be as long as 180ms. As a result, the 50ms timeout caused failures.
To address this, increase the wait timeout for ISHTP resume ack from 50ms to 300ms, providing a safer margin for slower hardware. Additionally, add error logging when a timeout occurs to aid future debugging and issue triage. No functional changes are made beyond the timeout adjustment and improved error reporting.
Signed-off-by: Zhang Lixu lixu.zhang@intel.com Reviewed-by: Andy Shevchenko andriy.shevchenko@intel.com Acked-by: Srinivas Pandruvada srinivas.pandruvada@linux.intel.com Signed-off-by: Jiri Kosina jkosina@suse.com Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
**Backport Status: YES**
This commit should be backported to stable kernel trees. Here's my extensive analysis:
## 1. Bug Fix Nature
The commit addresses a **real user-facing bug** that causes "several tens of seconds delays" during HID sensor resume handling on s2idle suspend/resume cycles. The code changes show this fixes timeout failures in `hid_ishtp_cl_resume_handler()` that were causing "link not ready" errors and preventing proper resume of HID sensors.
## 2. Small and Contained Changes
The commit makes minimal, focused changes: - **Primary change**: Increases `WAIT_FOR_RESUME_ACK_MS` from 50ms to 300ms - **Location consolidation**: Moves the constant definition from multiple files to a single header (`ishtp-dev.h:51`) - **Error logging addition**: Adds two lines in `ishtp-hid- client.c:762-763` for debugging - **Total impact**: Only 12 lines changed across 4 files
## 3. No Architectural Changes
The commit makes no structural changes - it only: - Adjusts a timeout value based on empirical measurements (180ms observed in failing systems) - Adds diagnostic logging - Consolidates a constant definition
## 4. Clear Regression Risk Assessment
The change has **minimal regression risk**: - Increasing timeout from 50ms to 300ms cannot break existing working systems - Systems that resumed within 50ms will continue to work - Systems that needed 50-180ms (previously failing) will now work - The 300ms provides safety margin without being excessive
## 5. Subsystem Impact
The changes are confined to the Intel ISH HID driver subsystem: - `drivers/hid/intel-ish-hid/` - a specific hardware driver - Does not affect core kernel functionality - Only impacts systems with Intel ISH (Integrated Sensor Hub) hardware
## 6. Historical Context
Looking at the git history: - The 50ms timeout was introduced in commit `e48bf29cf9d6d6` (2021) for async resume - Multiple recent fixes in this subsystem (`07583a00106`, `823987841424`) show it's actively maintained - The commit is already marked with "Upstream commit dfbd535db74df0343ca39670e06326d7aee8c8f4" indicating it's been accepted upstream
## 7. Stable Tree Rules Compliance
The commit perfectly aligns with stable tree criteria: - **Fixes a real bug**: Resume failures causing multi-second delays - **Tested solution**: Based on empirical measurements (180ms observed) - **Minimal change**: Simple timeout adjustment - **Hardware-specific**: Only affects Intel ISH hardware users - **No new features**: Pure bugfix with diagnostic improvement
## 8. Additional Evidence
The commit message explicitly states: - "The previous timeout was set to 50ms, which proved insufficient on affected machines" - "Empirical measurements on failing systems showed that the time... could be as long as 180ms" - This is based on actual testing and measurements, not speculation
The error logging addition (`hid_ishtp_trace()` and `dev_err()`) will help diagnose any future issues without changing functionality.
This is an ideal candidate for stable backporting - it fixes a concrete bug with minimal risk and clear benefit to affected users.
drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 --- drivers/hid/intel-ish-hid/ishtp-hid-client.c | 3 +++ drivers/hid/intel-ish-hid/ishtp/bus.c | 3 --- drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 3 +++ 4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c index c57483224db6f..9d150ce234f25 100644 --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c @@ -264,9 +264,6 @@ static void ish_shutdown(struct pci_dev *pdev)
static struct device __maybe_unused *ish_resume_device;
-/* 50ms to get resume response */ -#define WAIT_FOR_RESUME_ACK_MS 50 - /** * ish_resume_handler() - Work function to complete resume * @work: work struct diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c index 6550ad5bfbb53..d8c3c54a8c0f2 100644 --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c @@ -759,6 +759,9 @@ static void hid_ishtp_cl_resume_handler(struct work_struct *work) if (ishtp_wait_resume(ishtp_get_ishtp_device(hid_ishtp_cl))) { client_data->suspended = false; wake_up_interruptible(&client_data->ishtp_resume_wait); + } else { + hid_ishtp_trace(client_data, "hid client: wait for resume timed out"); + dev_err(cl_data_to_dev(client_data), "wait for resume timed out"); } }
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c index 5ac7d70a7c843..93a0432e70581 100644 --- a/drivers/hid/intel-ish-hid/ishtp/bus.c +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c @@ -852,9 +852,6 @@ EXPORT_SYMBOL(ishtp_device); */ bool ishtp_wait_resume(struct ishtp_device *dev) { - /* 50ms to get resume response */ - #define WAIT_FOR_RESUME_ACK_MS 50 - /* Waiting to get resume response */ if (dev->resume_flag) wait_event_interruptible_timeout(dev->resume_wait, diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h index ec9f6e87aaf23..23db97ecf21cd 100644 --- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h +++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h @@ -47,6 +47,9 @@
#define MAX_DMA_DELAY 20
+/* 300ms to get resume response */ +#define WAIT_FOR_RESUME_ACK_MS 300 + /* ISHTP device states */ enum ishtp_dev_state { ISHTP_DEV_INITIALIZING = 0,