[PATCH 0/6] HID: intel-ish-hid: Various power management improvements for hibernation

[PATCH 0/6] HID: intel-ish-hid: Various power management improvements for hibernation

[Zhang Lixu]

This patch series introduces several improvements to the power management and hibernation
handling in the intel-ish-hid driver. Key changes include:

- 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.

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          | 73 +++++++++-----------
 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, 72 insertions(+), 48 deletions(-)


base-commit: 8f7f8b1b3f4c613dd886f53f768f82816b41eaa3
-- 
2.43.0

[PATCH 1/6] HID: intel-ish-hid: Add ishtp_get_connection_state() interface

[Zhang Lixu]

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 dfbf7d9d7bb5..cccdf60b0247 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -107,6 +107,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

[PATCH 2/6] HID: intel-ishtp-hid: Clear suspended flag only after connected on resume

[Zhang Lixu]

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 d8c3c54a8c0f..7fb554482100 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

[PATCH 3/6] HID: intel-ish-ipc: Reset clients state on resume from D3

[Zhang Lixu]

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 9d150ce234f2..ccd195adbbd6 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);
+
 	schedule_work(&resume_work);
 
 	return 0;
-- 
2.43.0

[PATCH 4/6] HID: intel-ish-hid: ipc: Always schedule FW reset work on RESET_NOTIFY/ACK

[Zhang Lixu]

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 3ddaa2cd39d5..740566146f29 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;
-		}
-		schedule_work(&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))
+			schedule_work(&fw_reset_work);
 		break;
 	}
 }
@@ -982,6 +980,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

[PATCH 5/6] HID: intel-ish-hid: Use IPC RESET instead of void message in ish_wakeup()

[Zhang Lixu]

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.

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 | 34 ++++++++++-------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 740566146f29..b5cd17994df4 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,9 +798,7 @@ 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);
 }
 
 /**
@@ -855,21 +859,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

[PATCH 6/6] HID: intel-ish-hid: ipc: Separate hibernate callbacks in dev_pm_ops

[Zhang Lixu]

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 ccd195adbbd6..36445d2bb927 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

Re: [PATCH 0/6] HID: intel-ish-hid: Various power management improvements for hibernation

[Jiri Kosina]

On Fri, 19 Sep 2025, Zhang Lixu wrote:

> This patch series introduces several improvements to the power management and hibernation
> handling in the intel-ish-hid driver. Key changes include:
> 
> - 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.

Applied to hid.git#for-6.19/intel-ish, thanks!

-- 
Jiri Kosina
SUSE Labs