* [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation
@ 2025-10-17  2:22 Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface Zhang Lixu
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
- Separating hibernate callbacks in dev_pm_ops for clearer power state transitions
- Using IPC RESET in ish_wakeup() to ensure reliable device wakeup
- Scheduling firmware reset work on RESET_NOTIFY/ACK for robust recovery
- Resetting client state on resume from D3 to maintain consistency
- Enhancing resume logic in ishtp-hid-client for better stability
These patches enhance reliability, improve power management flow. All changes
have been validated on TwinLake (ISH 5.4), ArrowLake (ISH 5.6), and PantherLake
(ISH 5.8) platforms.
v2:
  - Rebased on top of [PATCH] HID: intel-ish-hid: Use dedicated unbound workqueues to prevent resume blocking
  - Changes in [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup()
    * Set the HW ready timeout to 10 seconds, matching the original timeout
      value used in ish_wakeup(), to prevent timeout issues on devices like
      the Lenovo ThinkPad X1 Titanium Gen 1 that require approximately 4
      seconds to become ready after wakeup.
    * Added RECVD_HW_READY_TIMEOUT macro for better code maintainability.
Zhang Lixu (6):
  HID: intel-ish-hid: Add ishtp_get_connection_state() interface
  HID: intel-ishtp-hid: Clear suspended flag only after connected on
    resume
  HID: intel-ish-ipc: Reset clients state on resume from D3
  HID: intel-ish-hid: ipc: Always schedule FW reset work on
    RESET_NOTIFY/ACK
  HID: intel-ish-hid: Use IPC RESET instead of void message in
    ish_wakeup()
  HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops
 drivers/hid/intel-ish-hid/ipc/ipc.c          | 78 +++++++++-----------
 drivers/hid/intel-ish-hid/ipc/pci-ish.c      | 29 +++++++-
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 11 ++-
 drivers/hid/intel-ish-hid/ishtp/client.c     |  6 ++
 include/linux/intel-ish-client-if.h          |  1 +
 5 files changed, 76 insertions(+), 49 deletions(-)
-- 
2.43.0
^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 2/6] HID: intel-ishtp-hid: Clear suspended flag only after connected on resume Zhang Lixu
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
Add the ishtp_get_connection_state() function for struct ishtp_cl, allowing
ishtp client drivers to retrieve the current connection state.
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/client.c | 6 ++++++
 include/linux/intel-ish-client-if.h      | 1 +
 2 files changed, 7 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 21a2c0773cc2..40f510b1c072 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -1261,6 +1261,12 @@ void ishtp_set_connection_state(struct ishtp_cl *cl, int state)
 }
 EXPORT_SYMBOL(ishtp_set_connection_state);
 
+int ishtp_get_connection_state(struct ishtp_cl *cl)
+{
+	return cl->state;
+}
+EXPORT_SYMBOL(ishtp_get_connection_state);
+
 void ishtp_cl_set_fw_client_id(struct ishtp_cl *cl, int fw_client_id)
 {
 	cl->fw_client_id = fw_client_id;
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index b235fd84f478..2cd4f65aaa37 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -109,6 +109,7 @@ struct ishtp_device *ishtp_get_ishtp_device(struct ishtp_cl *cl);
 void ishtp_set_tx_ring_size(struct ishtp_cl *cl, int size);
 void ishtp_set_rx_ring_size(struct ishtp_cl *cl, int size);
 void ishtp_set_connection_state(struct ishtp_cl *cl, int state);
+int ishtp_get_connection_state(struct ishtp_cl *cl);
 void ishtp_cl_set_fw_client_id(struct ishtp_cl *cl, int fw_client_id);
 
 void ishtp_put_device(struct ishtp_cl_device *cl_dev);
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 2/6] HID: intel-ishtp-hid: Clear suspended flag only after connected on resume
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 3/6] HID: intel-ish-ipc: Reset clients state on resume from D3 Zhang Lixu
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
When resuming from suspend-to-RAM or hibernate, the ISH firmware is powered
on from D3, causing all previous client connections between the firmware
and driver to be lost. Although the underlying ishtp bus driver initiates a
client reconnection flow, this process is asynchronous. As a result, when
hid_ishtp_cl_resume_handler() is executed, the connection may not have been
re-established yet. Clearing the suspended flag prematurely in this
scenario can lead to a timeout when the upper-layer HID sensor driver set
feature during resume.
To prevent such timeouts, only clear the suspended flag after confirming
that the connection state is ISHTP_CL_CONNECTED.
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index f61add862b6b..f37b3bc2bb7d 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -757,8 +757,15 @@ static void hid_ishtp_cl_resume_handler(struct work_struct *work)
 	struct ishtp_cl *hid_ishtp_cl = client_data->hid_ishtp_cl;
 
 	if (ishtp_wait_resume(ishtp_get_ishtp_device(hid_ishtp_cl))) {
-		client_data->suspended = false;
-		wake_up_interruptible(&client_data->ishtp_resume_wait);
+		/*
+		 * Clear the suspended flag only when the connection is established.
+		 * If the connection is not established, the suspended flag will be cleared after
+		 * the connection is made.
+		 */
+		if (ishtp_get_connection_state(hid_ishtp_cl) == ISHTP_CL_CONNECTED) {
+			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");
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 3/6] HID: intel-ish-ipc: Reset clients state on resume from D3
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 2/6] HID: intel-ishtp-hid: Clear suspended flag only after connected on resume Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 4/6] HID: intel-ish-hid: ipc: Always schedule FW reset work on RESET_NOTIFY/ACK Zhang Lixu
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
When ISH resumes from D3, the connection between ishtp clients and firmware
is lost. The ish_resume() function schedules resume_work asynchronously to
re-initiate the connection and then returns immediately. This can cause a
race where the upper-layer ishtp client driver's .resume() may execute
before the connection is fully restored, leaving the client in a stale
connected state. If the client sends messages during this window, the
firmware cannot respond.
To avoid this, reset the ishtp clients' state before returning from
ish_resume() if ISH is resuming from D3.
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index b748ac6fbfdc..e4499c83c62e 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -147,6 +147,12 @@ static inline bool ish_should_enter_d0i3(struct pci_dev *pdev)
 
 static inline bool ish_should_leave_d0i3(struct pci_dev *pdev)
 {
+	struct ishtp_device *dev = pci_get_drvdata(pdev);
+	u32 fwsts = dev->ops->get_fw_status(dev);
+
+	if (dev->suspend_flag || !IPC_IS_ISH_ILUP(fwsts))
+		return false;
+
 	return !pm_resume_via_firmware() || pdev->device == PCI_DEVICE_ID_INTEL_ISH_CHV;
 }
 
@@ -277,10 +283,8 @@ static void __maybe_unused ish_resume_handler(struct work_struct *work)
 {
 	struct pci_dev *pdev = to_pci_dev(ish_resume_device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
-	uint32_t fwsts = dev->ops->get_fw_status(dev);
 
-	if (ish_should_leave_d0i3(pdev) && !dev->suspend_flag
-			&& IPC_IS_ISH_ILUP(fwsts)) {
+	if (ish_should_leave_d0i3(pdev)) {
 		if (device_may_wakeup(&pdev->dev))
 			disable_irq_wake(pdev->irq);
 
@@ -384,6 +388,10 @@ static int __maybe_unused ish_resume(struct device *device)
 	ish_resume_device = device;
 	dev->resume_flag = 1;
 
+	/* If ISH resume from D3, reset ishtp clients before return */
+	if (!ish_should_leave_d0i3(pdev))
+		ishtp_reset_handler(dev);
+
 	queue_work(dev->unbound_wq, &resume_work);
 
 	return 0;
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 4/6] HID: intel-ish-hid: ipc: Always schedule FW reset work on RESET_NOTIFY/ACK
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
                   ` (2 preceding siblings ...)
  2025-10-17  2:22 ` [PATCH v2 3/6] HID: intel-ish-ipc: Reset clients state on resume from D3 Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup() Zhang Lixu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
Both ISH firmware and driver can actively send MNG_RESET_NOTIFY to initiate
an FW reset handshake. Upon receiving this, the peer should reply with
MNG_RESET_NOTIFY_ACK. Therefore, the driver should schedule the FW reset
handshake work function when receiving either MNG_RESET_NOTIFY or
MNG_RESET_NOTIFY_ACK.
Previously, driver only scheduled the work function on MNG_RESET_NOTIFY.
This patch ensures the work function is scheduled on both messages, but
only replies with MNG_RESET_NOTIFY_ACK when receiving MNG_RESET_NOTIFY.
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 39 ++++++++++++++---------------
 1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 9958f2968c4f..01a139e17cb5 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -481,6 +481,20 @@ static int timed_wait_for_timeout(struct ishtp_device *dev, int condition,
 	return ret;
 }
 
+static void ish_send_reset_notify_ack(struct ishtp_device *dev)
+{
+	/* Read reset ID */
+	u32 reset_id = ish_reg_read(dev, IPC_REG_ISH2HOST_MSG) & 0xFFFF;
+
+	/*
+	 * Set HOST2ISH.ILUP. Apparently we need this BEFORE sending
+	 * RESET_NOTIFY_ACK - FW will be checking for it
+	 */
+	ish_set_host_rdy(dev);
+	/* Send RESET_NOTIFY_ACK (with reset_id) */
+	ipc_send_mng_msg(dev, MNG_RESET_NOTIFY_ACK, &reset_id, sizeof(u32));
+}
+
 #define TIME_SLICE_FOR_FW_RDY_MS		100
 #define TIME_SLICE_FOR_INPUT_RDY_MS		100
 #define TIMEOUT_FOR_FW_RDY_MS			2000
@@ -496,13 +510,9 @@ static int timed_wait_for_timeout(struct ishtp_device *dev, int condition,
  */
 static int ish_fw_reset_handler(struct ishtp_device *dev)
 {
-	uint32_t	reset_id;
 	unsigned long	flags;
 	int ret;
 
-	/* Read reset ID */
-	reset_id = ish_reg_read(dev, IPC_REG_ISH2HOST_MSG) & 0xFFFF;
-
 	/* Clear IPC output queue */
 	spin_lock_irqsave(&dev->wr_processing_spinlock, flags);
 	list_splice_init(&dev->wr_processing_list, &dev->wr_free_list);
@@ -521,15 +531,6 @@ static int ish_fw_reset_handler(struct ishtp_device *dev)
 	/* Send clock sync at once after reset */
 	ishtp_dev->prev_sync = 0;
 
-	/*
-	 * Set HOST2ISH.ILUP. Apparently we need this BEFORE sending
-	 * RESET_NOTIFY_ACK - FW will be checking for it
-	 */
-	ish_set_host_rdy(dev);
-	/* Send RESET_NOTIFY_ACK (with reset_id) */
-	ipc_send_mng_msg(dev, MNG_RESET_NOTIFY_ACK, &reset_id,
-			 sizeof(uint32_t));
-
 	/* Wait for ISH FW'es ILUP and ISHTP_READY */
 	ret = timed_wait_for_timeout(dev, WAIT_FOR_FW_RDY,
 				     TIME_SLICE_FOR_FW_RDY_MS,
@@ -563,8 +564,6 @@ static void fw_reset_work_fn(struct work_struct *work)
 	if (!rv) {
 		/* ISH is ILUP & ISHTP-ready. Restart ISHTP */
 		msleep_interruptible(TIMEOUT_FOR_HW_RDY_MS);
-		ishtp_dev->recvd_hw_ready = 1;
-		wake_up_interruptible(&ishtp_dev->wait_hw_ready);
 
 		/* ISHTP notification in IPC_RESET sequence completion */
 		if (!work_pending(work))
@@ -625,15 +624,14 @@ static void	recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
 		break;
 
 	case MNG_RESET_NOTIFY:
-		if (!ishtp_dev) {
-			ishtp_dev = dev;
-		}
-		queue_work(dev->unbound_wq, &fw_reset_work);
-		break;
+		ish_send_reset_notify_ack(ishtp_dev);
+		fallthrough;
 
 	case MNG_RESET_NOTIFY_ACK:
 		dev->recvd_hw_ready = 1;
 		wake_up_interruptible(&dev->wait_hw_ready);
+		if (!work_pending(&fw_reset_work))
+			queue_work(dev->unbound_wq, &fw_reset_work);
 		break;
 	}
 }
@@ -1001,6 +999,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
 		list_add_tail(&tx_buf->link, &dev->wr_free_list);
 	}
 
+	ishtp_dev = dev;
 	ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
 	if (ret) {
 		dev_err(dev->devc, "Failed to initialise FW reset work\n");
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup()
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
                   ` (3 preceding siblings ...)
  2025-10-17  2:22 ` [PATCH v2 4/6] HID: intel-ish-hid: ipc: Always schedule FW reset work on RESET_NOTIFY/ACK Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17  2:22 ` [PATCH v2 6/6] HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops Zhang Lixu
  2025-10-17 15:48 ` [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Jiri Kosina
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
On ISH power-up, the bootloader enters sleep after preparing to load the
main firmware, waiting for the driver to be ready. When the driver is
ready, it sends a void message to wake up the bootloader and load the main
firmware. The main firmware then sends MNG_RESET_NOTIFY to the driver for
handshake.
This void message-based IPC handshake only works if the main firmware has
not been loaded. During hibernation resume, if the restore kernel has the
ISH driver, the driver wakes up the bootloader to load the main firmware
and perform IPC handshake. However, when switching to the image kernel,
since the main firmware is already loaded, sending a void message in the
.restore() callback does not trigger IPC handshake.
By sending MNG_RESET_NOTIFY (IPC RESET message) in ish_wakeup() instead of
a void message, we can explicitly wake up the bootloader and perform IPC
handshake, regardless of the firmware state. Additionally, since
ish_ipc_reset() already waits for recvd_hw_ready, the redundant wait for
recvd_hw_ready in ish_hw_start() is removed.
The timeout for waiting for HW ready is set to 10 seconds, matching the
original timeout value used in ish_wakeup(), to ensure reliable wakeup on
hardware that requires more time, such as the Lenovo ThinkPad X1 Titanium
Gen 1.
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 39 ++++++++++++-----------------
 1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 01a139e17cb5..59355e4a61f8 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -728,22 +728,28 @@ int ish_disable_dma(struct ishtp_device *dev)
  * ish_wakeup() - wakeup ishfw from waiting-for-host state
  * @dev: ishtp device pointer
  *
- * Set the dma enable bit and send a void message to FW,
+ * Set the dma enable bit and send a IPC RESET message to FW,
  * it wil wakeup FW from waiting-for-host state.
+ *
+ * Return: 0 for success else error code.
  */
-static void ish_wakeup(struct ishtp_device *dev)
+static int ish_wakeup(struct ishtp_device *dev)
 {
+	int ret;
+
 	/* Set dma enable bit */
 	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
 
 	/*
-	 * Send 0 IPC message so that ISH FW wakes up if it was already
+	 * Send IPC RESET message so that ISH FW wakes up if it was already
 	 * asleep.
 	 */
-	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
+	ret = ish_ipc_reset(dev);
 
 	/* Flush writes to doorbell and REMAP2 */
 	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+
+	return ret;
 }
 
 /**
@@ -792,11 +798,11 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
 
 	/* Now we can enable ISH DMA operation and wakeup ISHFW */
-	ish_wakeup(dev);
-
-	return	0;
+	return ish_wakeup(dev);
 }
 
+#define RECVD_HW_READY_TIMEOUT (10 * HZ)
+
 /**
  * _ish_ipc_reset() - IPC reset
  * @dev: ishtp device pointer
@@ -831,7 +837,8 @@ static int _ish_ipc_reset(struct ishtp_device *dev)
 	}
 
 	wait_event_interruptible_timeout(dev->wait_hw_ready,
-					 dev->recvd_hw_ready, 2 * HZ);
+					 dev->recvd_hw_ready,
+					 RECVD_HW_READY_TIMEOUT);
 	if (!dev->recvd_hw_ready) {
 		dev_err(dev->devc, "Timed out waiting for HW ready\n");
 		rv = -ENODEV;
@@ -855,21 +862,7 @@ int ish_hw_start(struct ishtp_device *dev)
 	set_host_ready(dev);
 
 	/* After that we can enable ISH DMA operation and wakeup ISHFW */
-	ish_wakeup(dev);
-
-	/* wait for FW-initiated reset flow */
-	if (!dev->recvd_hw_ready)
-		wait_event_interruptible_timeout(dev->wait_hw_ready,
-						 dev->recvd_hw_ready,
-						 10 * HZ);
-
-	if (!dev->recvd_hw_ready) {
-		dev_err(dev->devc,
-			"[ishtp-ish]: Timed out waiting for FW-initiated reset\n");
-		return	-ENODEV;
-	}
-
-	return 0;
+	return ish_wakeup(dev);
 }
 
 /**
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2 6/6] HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
                   ` (4 preceding siblings ...)
  2025-10-17  2:22 ` [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup() Zhang Lixu
@ 2025-10-17  2:22 ` Zhang Lixu
  2025-10-17 15:48 ` [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Jiri Kosina
  6 siblings, 0 replies; 8+ messages in thread
From: Zhang Lixu @ 2025-10-17  2:22 UTC (permalink / raw)
  To: linux-input, srinivas.pandruvada, jikos, benjamin.tissoires; +Cc: lixu.zhang
The same suspend and resume callbacks are used for both suspend-to-RAM/idle
and hibernation. These callbacks invoke pm_suspend_via_firmware() and
pm_resume_via_firmware(), respectively. In the .freeze() of hibernation,
pm_suspend_via_firmware() returns false, causing the driver to put ISH into
D0i3. However, during the .thaw(), pm_resume_via_firmware() returns true,
leading the driver to treat ISH as resuming from D3 instead of D0i3. The
asymmetric behavior between .freeze() and .thaw() during hibernation can
cause the client connection states on the firmware side and the driver side
to become inconsistent.
To address the inconsistent client connection states issue, separate
hibernate-related callbacks (freeze, thaw) in dev_pm_ops. Since ISH does
not need to save any firmware-related state when entering hibernation, it
is sufficient to call pci_save_state() in .freeze() to prevent the PCI bus
from changing the ISH power state. No actions are required in .thaw().
Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index e4499c83c62e..1612e8cb23f0 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -397,7 +397,20 @@ static int __maybe_unused ish_resume(struct device *device)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(ish_pm_ops, ish_suspend, ish_resume);
+static int __maybe_unused ish_freeze(struct device *device)
+{
+	struct pci_dev *pdev = to_pci_dev(device);
+
+	return pci_save_state(pdev);
+}
+
+static const struct dev_pm_ops __maybe_unused ish_pm_ops = {
+	.suspend = pm_sleep_ptr(ish_suspend),
+	.resume = pm_sleep_ptr(ish_resume),
+	.freeze = pm_sleep_ptr(ish_freeze),
+	.restore = pm_sleep_ptr(ish_resume),
+	.poweroff = pm_sleep_ptr(ish_suspend),
+};
 
 static ssize_t base_version_show(struct device *cdev,
 				 struct device_attribute *attr, char *buf)
-- 
2.43.0
^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation
  2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
                   ` (5 preceding siblings ...)
  2025-10-17  2:22 ` [PATCH v2 6/6] HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops Zhang Lixu
@ 2025-10-17 15:48 ` Jiri Kosina
  6 siblings, 0 replies; 8+ messages in thread
From: Jiri Kosina @ 2025-10-17 15:48 UTC (permalink / raw)
  To: Zhang Lixu; +Cc: linux-input, srinivas.pandruvada, benjamin.tissoires
On Fri, 17 Oct 2025, Zhang Lixu wrote:
> - Separating hibernate callbacks in dev_pm_ops for clearer power state transitions
> - Using IPC RESET in ish_wakeup() to ensure reliable device wakeup
> - Scheduling firmware reset work on RESET_NOTIFY/ACK for robust recovery
> - Resetting client state on resume from D3 to maintain consistency
> - Enhancing resume logic in ishtp-hid-client for better stability
> 
> These patches enhance reliability, improve power management flow. All changes
> have been validated on TwinLake (ISH 5.4), ArrowLake (ISH 5.6), and PantherLake
> (ISH 5.8) platforms.
> 
> v2:
>   - Rebased on top of [PATCH] HID: intel-ish-hid: Use dedicated unbound workqueues to prevent resume blocking
>   - Changes in [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup()
>     * Set the HW ready timeout to 10 seconds, matching the original timeout
>       value used in ish_wakeup(), to prevent timeout issues on devices like
>       the Lenovo ThinkPad X1 Titanium Gen 1 that require approximately 4
>       seconds to become ready after wakeup.
>     * Added RECVD_HW_READY_TIMEOUT macro for better code maintainability.
Applied to hid.git#for-6.19/intel-ish-v2, thanks.
-- 
Jiri Kosina
SUSE Labs
^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-10-17 15:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-17  2:22 [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 2/6] HID: intel-ishtp-hid: Clear suspended flag only after connected on resume Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 3/6] HID: intel-ish-ipc: Reset clients state on resume from D3 Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 4/6] HID: intel-ish-hid: ipc: Always schedule FW reset work on RESET_NOTIFY/ACK Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup() Zhang Lixu
2025-10-17  2:22 ` [PATCH v2 6/6] HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops Zhang Lixu
2025-10-17 15:48 ` [PATCH v2 0/6] HID: intel-ish-hid: Various power management improvements for hibernation Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).