[PATCH v5 0/3] HID: Intel-thc-hid: Refine error recovery flow

[PATCH v5 0/3] HID: Intel-thc-hid: Refine error recovery flow

[Even Xu]

This series refines the fatal error recovery flow for the Intel THC
(Touch Host Controller) subsystem, covering both the QuickI2C and
QuickSPI drivers.

Currently, when a fatal DMA error is detected in the IRQ thread handler,
the recovery is performed inline: the interrupt handler calls
try_recover() directly, which unconfigures and reconfigures the DMA
engine.

This approach has several problems:
1. Recovery runs in the IRQ thread context, which is not ideal for
   potentially slow reset operations.
2. The interrupt is re-enabled before recovery completes, risking an
   interrupt storm if DMA errors persist.
3. The DMA reset logic is open-coded in each protocol driver, leading
   to duplication and divergence over time.

This patch series addresses all of the above:

By adding a new thc_rxdma_reset() API to the THC core layer, QuickI2C
and QuickSPI drivers can call it respectively to refine the recovery
callback.

The synchronous try_recover() call in the IRQ thread is replaced with
schedule_work(), deferring recovery to a workqueue.  Within the work
function:
- The interrupt line is disabled before any DMA manipulation.
- thc_rxdma_reset() is used instead of the open-coded sequence.
- On failure the device is marked DISABLED and the interrupt remains
  off, preventing an interrupt storm.

Change log:
v5:
 - Add log message in quicki2c/quickspi driver to indicate driver
   function was disabled when recover failed.

v4:
 - Refine cancel_work_sync() flow to reduce race condition of DMA
   operation confliction and reschedule.

v3:
 - Quiesce external interrupt and disable THC internal interrupt in
   remove() and shutdown() callback before cancel_work_sync().

v2:
 - Use dev_err() instead of dev_err_once() so repeated failures during
   recurring recovery are not silently suppressed.
 - Pause both RxDMA channels via thc_wait_for_dma_pause() before calling
   thc_dma_unconfigure() to ensure the DMA engines are inactive before
   clearing PRD base addresses, preventing potential IOMMU faults or
   memory corruption.
 - Hold a runtime PM reference inside try_recover() to prevent the
   device from suspending while the work accesses hardware registers.
 - Add cancel_work_sync() in quicki2c_remove() and quicki2c_shutdown()
   to prevent use-after-free if recovery work is still queued at teardown.
 - Only re-enable the interrupt in the IRQ thread handler when no recovery
   is needed; the work function handles re-enabling after successful reset,
   avoiding an interrupt storm from the uncleared hardware error state.


Even Xu (3):
  HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
  HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
  HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

 .../intel-quicki2c/pci-quicki2c.c             | 66 +++++++++++------
 .../intel-quicki2c/quicki2c-dev.h             |  5 ++
 .../intel-quickspi/pci-quickspi.c             | 71 ++++++++++++-------
 .../intel-quickspi/quickspi-dev.h             |  6 ++
 .../intel-thc-hid/intel-thc/intel-thc-dma.c   | 51 +++++++++++++
 .../intel-thc-hid/intel-thc/intel-thc-dma.h   |  1 +
 6 files changed, 151 insertions(+), 49 deletions(-)

-- 
2.43.0

[PATCH v5 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

[Even Xu]

Add a helper function thc_rxdma_reset() to do read DMA reset, it can be
called when fatal DMA error happens.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-thc-hid/intel-thc/intel-thc-dma.c   | 51 +++++++++++++++++++
 .../intel-thc-hid/intel-thc/intel-thc-dma.h   |  1 +
 2 files changed, 52 insertions(+)

diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
index 6ee675e0a738..7ceb8aeeccd3 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
@@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
 	return 0;
 }
 
+/**
+ * thc_rxdma_reset - Reset all read DMA engines
+ *
+ * @dev: The pointer of THC private device context
+ *
+ * This is a helper function to reset RxDMA configure. It's typically used
+ * for RxDMA recovery when fatal error happens.
+ *
+ * Return: 0 if successful or error code on failure.
+ */
+int thc_rxdma_reset(struct thc_device *dev)
+{
+	int ret;
+
+	if (mutex_lock_interruptible(&dev->thc_bus_lock))
+		return -EINTR;
+
+	ret = thc_interrupt_quiesce(dev, true);
+	if (ret) {
+		dev_err(dev->dev, "Quiesce interrupt failed during RxDMA reset\n");
+		goto end;
+	}
+
+	ret = thc_wait_for_dma_pause(dev, THC_RXDMA1);
+	if (ret) {
+		dev_err(dev->dev, "Wait for RxDMA1 pause failed during RxDMA reset\n");
+		goto end;
+	}
+
+	ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
+	if (ret) {
+		dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
+		goto end;
+	}
+
+	thc_dma_unconfigure(dev);
+
+	ret = thc_dma_configure(dev);
+	if (ret) {
+		dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
+		goto end;
+	}
+
+	thc_interrupt_quiesce(dev, false);
+
+end:
+	mutex_unlock(&dev->thc_bus_lock);
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");
+
 static int read_dma_buffer(struct thc_device *dev,
 			   struct thc_dma_configuration *read_config,
 			   u8 prd_table_index, void *read_buff)
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
index 541d33995baf..715423453a9d 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.h
@@ -145,6 +145,7 @@ int  thc_dma_allocate(struct thc_device *dev);
 int  thc_dma_configure(struct thc_device *dev);
 void thc_dma_unconfigure(struct thc_device *dev);
 void thc_dma_release(struct thc_device *dev);
+int  thc_rxdma_reset(struct thc_device *dev);
 int  thc_rxdma_read(struct thc_device *dev, enum thc_dma_channel dma_channel,
 		    void *read_buff, size_t *read_len, int *read_finished);
 int  thc_swdma_read(struct thc_device *dev, void *write_buff, size_t write_len,
-- 
2.43.0

[PATCH v5 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback

[Even Xu]

Refine recover flow:
1. Use workqueue to handle recover flow instead of processing in irq
   handler.
2. Call thc_rxdma_reset() API to simplify the recover operation.
3. Disable interrupt during whole recover flow.
4. If recover fails, disable interrupt to avoid interrupt storm.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-quicki2c/pci-quicki2c.c             | 66 ++++++++++++-------
 .../intel-quicki2c/quicki2c-dev.h             |  5 ++
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
index 2ec52cb35a13..62bd872b80c9 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
@@ -255,28 +255,33 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
 }
 
 /**
- * try_recover - Try to recovery THC and Device
- * @qcdev: Pointer to quicki2c_device structure
+ * try_recover - Recover callback to recover THC
+ * @work: pointer to work_struct
  *
  * This function is an error handler, called when fatal error happens.
- * It try to reset touch device and re-configure THC to recovery
- * communication between touch device and THC.
- *
- * Return: 0 if successful or error code on failure
+ * It try to reset Touch Device and re-configure THC to recover
+ * transferring between Device and THC.
  */
-static int try_recover(struct quicki2c_device *qcdev)
+static void try_recover(struct work_struct *work)
 {
-	int ret;
+	struct quicki2c_device *qcdev = container_of(work, struct quicki2c_device, recover_work);
 
-	thc_dma_unconfigure(qcdev->thc_hw);
+	if (READ_ONCE(qcdev->recovery_disabled))
+		return;
 
-	ret = thc_dma_configure(qcdev->thc_hw);
-	if (ret) {
-		dev_err(qcdev->dev, "Reconfig DMA failed\n");
-		return ret;
+	if (pm_runtime_resume_and_get(qcdev->dev))
+		return;
+
+	thc_interrupt_enable(qcdev->thc_hw, false);
+
+	if (thc_rxdma_reset(qcdev->thc_hw)) {
+		qcdev->state = QUICKI2C_DISABLED;
+		dev_err(qcdev->dev, "RxDMA reset failed during recover, disable QuickI2C\n");
+	} else {
+		thc_interrupt_enable(qcdev->thc_hw, true);
 	}
 
-	return 0;
+	pm_runtime_put_autosuspend(qcdev->dev);
 }
 
 static int handle_input_report(struct quicki2c_device *qcdev)
@@ -353,11 +358,10 @@ static irqreturn_t quicki2c_irq_thread_handler(int irq, void *dev_id)
 	}
 
 exit:
-	thc_interrupt_enable(qcdev->thc_hw, true);
-
 	if (err_recover)
-		if (try_recover(qcdev))
-			qcdev->state = QUICKI2C_DISABLED;
+		schedule_work(&qcdev->recover_work);
+	else
+		thc_interrupt_enable(qcdev->thc_hw, true);
 
 	pm_runtime_put_autosuspend(qcdev->dev);
 
@@ -396,6 +400,8 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
 	qcdev->ddata = ddata;
 
 	init_waitqueue_head(&qcdev->reset_ack_wq);
+	WRITE_ONCE(qcdev->recovery_disabled, false);
+	INIT_WORK(&qcdev->recover_work, try_recover);
 
 	/* THC hardware init */
 	qcdev->thc_hw = thc_dev_init(qcdev->dev, qcdev->mem_addr);
@@ -446,6 +452,9 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
  */
 static void quicki2c_dev_deinit(struct quicki2c_device *qcdev)
 {
+	WRITE_ONCE(qcdev->recovery_disabled, true);
+	cancel_work_sync(&qcdev->recover_work);
+
 	thc_interrupt_quiesce(qcdev->thc_hw, true);
 	thc_interrupt_enable(qcdev->thc_hw, false);
 	thc_ltr_unconfig(qcdev->thc_hw);
@@ -779,12 +788,13 @@ static void quicki2c_remove(struct pci_dev *pdev)
 		return;
 
 	quicki2c_hid_remove(qcdev);
+
+	quicki2c_dev_deinit(qcdev);
+
 	quicki2c_dma_deinit(qcdev);
 
 	pm_runtime_get_noresume(qcdev->dev);
 
-	quicki2c_dev_deinit(qcdev);
-
 	pci_clear_master(pdev);
 }
 
@@ -803,10 +813,10 @@ static void quicki2c_shutdown(struct pci_dev *pdev)
 	if (!qcdev)
 		return;
 
+	quicki2c_dev_deinit(qcdev);
+
 	/* Must stop DMA before reboot to avoid DMA entering into unknown state */
 	quicki2c_dma_deinit(qcdev);
-
-	quicki2c_dev_deinit(qcdev);
 }
 
 static int quicki2c_suspend(struct device *device)
@@ -833,6 +843,9 @@ static int quicki2c_suspend(struct device *device)
 	if (ret)
 		return ret;
 
+	WRITE_ONCE(qcdev->recovery_disabled, true);
+	cancel_work_sync(&qcdev->recover_work);
+
 	ret = thc_interrupt_quiesce(qcdev->thc_hw, true);
 	if (ret)
 		return ret;
@@ -874,6 +887,8 @@ static int quicki2c_resume(struct device *device)
 	if (ret)
 		return ret;
 
+	WRITE_ONCE(qcdev->recovery_disabled, false);
+
 	if (!device_may_wakeup(qcdev->dev))
 		return quicki2c_set_power(qcdev, HIDI2C_ON);
 
@@ -890,6 +905,9 @@ static int quicki2c_freeze(struct device *device)
 	if (!qcdev)
 		return -ENODEV;
 
+	WRITE_ONCE(qcdev->recovery_disabled, true);
+	cancel_work_sync(&qcdev->recover_work);
+
 	ret = thc_interrupt_quiesce(qcdev->thc_hw, true);
 	if (ret)
 		return ret;
@@ -921,6 +939,8 @@ static int quicki2c_thaw(struct device *device)
 	if (ret)
 		return ret;
 
+	WRITE_ONCE(qcdev->recovery_disabled, false);
+
 	return 0;
 }
 
@@ -945,6 +965,8 @@ static int quicki2c_poweroff(struct device *device)
 
 	thc_ltr_unconfig(qcdev->thc_hw);
 
+	quicki2c_dev_deinit(qcdev);
+
 	quicki2c_dma_deinit(qcdev);
 
 	return 0;
diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
index 34ebda286028..6d25a846153e 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
@@ -189,6 +189,8 @@ struct acpi_device;
  * @i2c_max_frame_size: Max RX frame size (unit in Bytes)
  * @i2c_int_delay_enable: Indicate interrupt delay feature enabled or not
  * @i2c_int_delay: Interrupt detection delay value (unit in 10 us)
+ * @recover_work: Work structure for recovery
+ * @recovery_disabled: Whether recovery work is blocked during teardown
  */
 struct quicki2c_device {
 	struct device *dev;
@@ -220,6 +222,9 @@ struct quicki2c_device {
 	u32 i2c_max_frame_size;
 	u32 i2c_int_delay_enable;
 	u32 i2c_int_delay;
+
+	struct work_struct recover_work;
+	bool recovery_disabled;
 };
 
 #endif /* _QUICKI2C_DEV_H_ */
-- 
2.43.0

[PATCH v5 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

[Even Xu]

Refine recover flow:
1. Use workqueue to handle recover flow instead of processing in irq
   handler.
2. Call thc_rxdma_reset() API to simplify the recover operation.
3. Disable interrupt during whole recover flow.
4. If recover fails, disable interrupt to avoid interrupt storm.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-quickspi/pci-quickspi.c             | 71 ++++++++++++-------
 .../intel-quickspi/quickspi-dev.h             |  6 ++
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index 4ae2e1718b30..404f0d2f1b9d 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -252,34 +252,33 @@ static irqreturn_t quickspi_irq_quick_handler(int irq, void *dev_id)
 }
 
 /**
- * try_recover - Try to recovery THC and Device
- * @qsdev: pointer to quickspi device
+ * try_recover - Recover callback to recover THC
+ * @work: pointer to work_struct
  *
- * This function is a error handler, called when fatal error happens.
- * It try to reset Touch Device and re-configure THC to recovery
+ * This function is an error handler, called when fatal error happens.
+ * It try to reset Touch Device and re-configure THC to recover
  * transferring between Device and THC.
- *
- * Return: 0 if successful or error code on failed.
  */
-static int try_recover(struct quickspi_device *qsdev)
+static void try_recover(struct work_struct *work)
 {
-	int ret;
+	struct quickspi_device *qsdev = container_of(work, struct quickspi_device, recover_work);
 
-	ret = reset_tic(qsdev);
-	if (ret) {
-		dev_err(qsdev->dev, "Reset touch device failed, ret = %d\n", ret);
-		return ret;
-	}
+	if (READ_ONCE(qsdev->recovery_disabled))
+		return;
 
-	thc_dma_unconfigure(qsdev->thc_hw);
+	if (pm_runtime_resume_and_get(qsdev->dev))
+		return;
 
-	ret = thc_dma_configure(qsdev->thc_hw);
-	if (ret) {
-		dev_err(qsdev->dev, "Re-configure THC DMA failed, ret = %d\n", ret);
-		return ret;
+	thc_interrupt_enable(qsdev->thc_hw, false);
+
+	if (thc_rxdma_reset(qsdev->thc_hw)) {
+		qsdev->state = QUICKSPI_DISABLED;
+		dev_err(qsdev->dev, "RxDMA reset failed during recover, disable QuickSPI\n");
+	} else {
+		thc_interrupt_enable(qsdev->thc_hw, true);
 	}
 
-	return 0;
+	pm_runtime_put_autosuspend(qsdev->dev);
 }
 
 /**
@@ -337,11 +336,10 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
 	}
 
 end:
-	thc_interrupt_enable(qsdev->thc_hw, true);
-
 	if (err_recover)
-		if (try_recover(qsdev))
-			qsdev->state = QUICKSPI_DISABLED;
+		schedule_work(&qsdev->recover_work);
+	else
+		thc_interrupt_enable(qsdev->thc_hw, true);
 
 	pm_runtime_put_autosuspend(qsdev->dev);
 
@@ -385,6 +383,8 @@ static struct quickspi_device *quickspi_dev_init(struct pci_dev *pdev, void __io
 	init_waitqueue_head(&qsdev->report_desc_got_wq);
 	init_waitqueue_head(&qsdev->get_report_cmpl_wq);
 	init_waitqueue_head(&qsdev->set_report_cmpl_wq);
+	WRITE_ONCE(qsdev->recovery_disabled, false);
+	INIT_WORK(&qsdev->recover_work, try_recover);
 
 	/* thc hw init */
 	qsdev->thc_hw = thc_dev_init(qsdev->dev, qsdev->mem_addr);
@@ -461,6 +461,10 @@ static struct quickspi_device *quickspi_dev_init(struct pci_dev *pdev, void __io
  */
 static void quickspi_dev_deinit(struct quickspi_device *qsdev)
 {
+	WRITE_ONCE(qsdev->recovery_disabled, true);
+	cancel_work_sync(&qsdev->recover_work);
+
+	thc_interrupt_quiesce(qsdev->thc_hw, true);
 	thc_interrupt_enable(qsdev->thc_hw, false);
 	thc_ltr_unconfig(qsdev->thc_hw);
 	thc_wot_unconfig(qsdev->thc_hw);
@@ -711,12 +715,13 @@ static void quickspi_remove(struct pci_dev *pdev)
 		return;
 
 	quickspi_hid_remove(qsdev);
+
+	quickspi_dev_deinit(qsdev);
+
 	quickspi_dma_deinit(qsdev);
 
 	pm_runtime_get_noresume(qsdev->dev);
 
-	quickspi_dev_deinit(qsdev);
-
 	pci_clear_master(pdev);
 }
 
@@ -737,10 +742,10 @@ static void quickspi_shutdown(struct pci_dev *pdev)
 	if (!qsdev)
 		return;
 
+	quickspi_dev_deinit(qsdev);
+
 	/* Must stop DMA before reboot to avoid DMA entering into unknown state */
 	quickspi_dma_deinit(qsdev);
-
-	quickspi_dev_deinit(qsdev);
 }
 
 static int quickspi_suspend(struct device *device)
@@ -759,6 +764,9 @@ static int quickspi_suspend(struct device *device)
 			return ret;
 	}
 
+	WRITE_ONCE(qsdev->recovery_disabled, true);
+	cancel_work_sync(&qsdev->recover_work);
+
 	ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
 	if (ret)
 		return ret;
@@ -784,6 +792,8 @@ static int quickspi_resume(struct device *device)
 	if (ret)
 		return ret;
 
+	WRITE_ONCE(qsdev->recovery_disabled, false);
+
 	/*
 	 * A wake-enabled device keeps its power and state across suspend, so
 	 * only restore the THC context. Resetting it here would discard a
@@ -864,6 +874,9 @@ static int quickspi_freeze(struct device *device)
 	if (!qsdev)
 		return -ENODEV;
 
+	WRITE_ONCE(qsdev->recovery_disabled, true);
+	cancel_work_sync(&qsdev->recover_work);
+
 	ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
 	if (ret)
 		return ret;
@@ -895,6 +908,8 @@ static int quickspi_thaw(struct device *device)
 	if (ret)
 		return ret;
 
+	WRITE_ONCE(qsdev->recovery_disabled, false);
+
 	return 0;
 }
 
@@ -919,6 +934,8 @@ static int quickspi_poweroff(struct device *device)
 
 	thc_ltr_unconfig(qsdev->thc_hw);
 
+	quickspi_dev_deinit(qsdev);
+
 	quickspi_dma_deinit(qsdev);
 
 	return 0;
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index bf5e18f5a5f4..2936c8b1532c 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -8,6 +8,7 @@
 #include <linux/hid-over-spi.h>
 #include <linux/sizes.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 
 #include "quickspi-protocol.h"
 
@@ -126,6 +127,8 @@ struct acpi_device;
  * @get_feature_cmpl: indicate get feature received or not
  * @set_feature_cmpl_wq: workqueue for waiting set feature to device
  * @set_feature_cmpl: indicate set feature send complete or not
+ * @recover_work: Work structure for recovery
+ * @recovery_disabled: Whether recovery work is blocked during teardown
  */
 struct quickspi_device {
 	struct device *dev;
@@ -173,6 +176,9 @@ struct quickspi_device {
 
 	wait_queue_head_t set_report_cmpl_wq;
 	bool set_report_cmpl;
+
+	struct work_struct recover_work;
+	bool recovery_disabled;
 };
 
 #endif /* _QUICKSPI_DEV_H_ */
-- 
2.43.0

Re: [PATCH v5 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] thc_rxdma_reset() destructively unconfigures all DMA channels (including TX and SW) without pausing them first, risking severe memory corruption if they were left active by a previous timeout.
- [High] Error paths in thc_rxdma_reset() bypass the interrupt unquiesce operation, leaving device interrupts permanently disabled.
- [High] The PRD (Physical Region Descriptor) tables are not re-initialized before restarting the RxDMA engines, leading to immediate buffer overruns upon hardware restart.
- [Medium] The return value of thc_interrupt_quiesce(dev, false) is ignored, potentially masking a failure that leaves the device permanently incapable of generating interrupts.

Pre-existing issues:
- [High] thc_dma_write() overwrites its timeout error code, falsely returning success to callers.
- [High] thc_swdma_read() leaks interrupt quiesced state and leaves hardware wedged on error paths.
--

--- Patch [1]: [PATCH v5 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA ---
commit c2eb7694465cfedd737d39cb05604d90ef90d739
Author: Even Xu <even.xu@intel.com>
Subject: HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

This commit adds a helper function thc_rxdma_reset() to perform a read DMA
reset, which can be called when a fatal DMA error happens.

> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> index 6ee675e0a7384..7ceb8aeeccd37 100644
> --- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> @@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
[ ... ]
> +int thc_rxdma_reset(struct thc_device *dev)
> +{
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&dev->thc_bus_lock))
> +		return -EINTR;
> +
> +	ret = thc_interrupt_quiesce(dev, true);
> +	if (ret) {
> +		dev_err(dev->dev, "Quiesce interrupt failed during RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	ret = thc_wait_for_dma_pause(dev, THC_RXDMA1);
> +	if (ret) {
> +		dev_err(dev->dev, "Wait for RxDMA1 pause failed during RxDMA reset\n");
> +		goto end;

[Severity: High]
Does this error path bypass the interrupt unquiesce operation?

If wait_for_dma_pause fails, execution jumps directly to the 'end' label.
The 'end' label simply unlocks the mutex and returns, entirely skipping the
thc_interrupt_quiesce(dev, false) cleanup step.

Could this leave the hardware in an inconsistent state where device
interrupts are permanently disabled and the device cannot be recovered?

> +	}
> +
> +	ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
> +	if (ret) {
> +		dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	thc_dma_unconfigure(dev);

[Severity: Critical]
Is it safe to call thc_dma_unconfigure() here while TX and SW DMA engines
might still be active?

If a synchronous operation like thc_dma_write() or thc_swdma_read() times out,
it releases thc_bus_lock while leaving its respective hardware DMA engine
active (START bit = 1). If thc_rxdma_reset() is subsequently called to
recover the device, it invokes thc_dma_unconfigure().

thc_dma_unconfigure() loops over all four DMA channels and sets their
Physical Region Descriptor (PRD) base addresses to 0, but it does not clear
the START bits for TXDMA and SWDMA:

void thc_dma_unconfigure(struct thc_device *dev)
{
	int chan;

	for (chan = 0; chan < MAX_THC_DMA_CHANNEL; chan++) {
		dma_set_prd_base_addr(dev, 0, &dev->dma_ctx->dma_config[chan]);
		dma_clear_prd_control(dev, &dev->dma_ctx->dma_config[chan]);
	}

	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_1_OFFSET,
			  THC_M_PRT_READ_DMA_CNTRL_START, 0);

	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_2_OFFSET,
			  THC_M_PRT_READ_DMA_CNTRL_START, 0);
}

By zeroing out the PRD base address for a still-running DMA engine, could
this direct the hardware to fetch descriptors or write data to physical
address 0, leading to severe memory corruption or IOMMU faults?

> +
> +	ret = thc_dma_configure(dev);

[Severity: High]
Are the PRD tables correctly re-initialized in memory before restarting the
RxDMA engines?

The hardware modifies PRD table entries (e.g., updating length fields) upon
receiving data. Under normal operation, the driver restores these original
sizes in thc_dma_read() using thc_copy_one_sgl_to_prd().

When a fatal error occurs and thc_rxdma_reset() is called, the hardware
read and write pointers are abruptly reset to 0 via thc_dma_configure().
However, if the in-memory PRD tables are not re-initialized, could the
hardware fetch PRD index 0 containing a reduced length and stale flags from
a past partial transaction?

Would this cause the hardware to assume a smaller buffer capacity, triggering
a Buffer Overrun or PRD Entry Error upon the next DMA write?

> +	if (ret) {
> +		dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	thc_interrupt_quiesce(dev, false);

[Severity: Medium]
Should the return value of thc_interrupt_quiesce() be checked and propagated?

This function polls the hardware and can return -ETIMEDOUT if the hardware
state is lost or unresponsive. By discarding this return value and instead
returning the 0 (success) from the prior thc_dma_configure() step, could the
caller falsely assume recovery was successful while the device remains
incapable of generating interrupts?

> +
> +end:
> +	mutex_unlock(&dev->thc_bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");

[Severity: High]
This isn't a bug introduced by this patch, but does thc_dma_write()
overwrite its timeout error code on failure paths?

Looking at thc_dma_write() in the same file:

	ret = wait_event_interruptible_timeout(dev->write_complete_wait,
					       dev->write_done, 1 * HZ);
	if (ret <= 0 || !dev->write_done) {
		dev_err_once(dev->dev, "timeout for waiting TxDMA completion\n");
		ret = -ETIMEDOUT;
		goto end;
	}
	...
end:
	mutex_unlock(&dev->thc_bus_lock);

	if (restore_interrupts)
		ret = thc_interrupt_quiesce(dev, false);

	return ret;

If wait_event_interruptible_timeout() times out, ret is set to -ETIMEDOUT.
However, since unquiescing typically succeeds and returns 0, the original
-ETIMEDOUT error is overwritten by 0.

Could this falsely return success to callers when the DMA write actually
timed out, leading to data loss and state desynchronization?

[Severity: High]
This isn't a bug introduced by this patch, but do the error paths in
thc_swdma_read() leak the interrupt quiesced state?

Looking at thc_swdma_read() in the same file:

	ret = thc_swdma_read_start(dev, write_buff, write_len, prd_tbl_len);
	if (ret)
		goto end;

	ret = wait_event_interruptible_timeout(dev->swdma_complete_wait,
					       dev->swdma_done, 1 * HZ);
	if (ret <= 0 || !dev->swdma_done) {
		dev_err_once(dev->dev, "timeout for waiting SWDMA completion\n");
		ret = -ETIMEDOUT;
		goto end;
	}

	ret = thc_dma_read(dev, &dev->dma_ctx->dma_config[THC_SWDMA],
			   read_buff, read_len, NULL);
	if (ret)
		goto end;

	ret = thc_swdma_read_completion(dev);

end:
	mutex_unlock(&dev->thc_bus_lock);
	return ret;

If the software DMA read times out or if thc_dma_read() fails, execution
jumps to the 'end' label, bypassing thc_swdma_read_completion().

Since thc_swdma_read_start() previously quiesced interrupts and stopped
RXDMA2, could bypassing the completion routine cause the device to
permanently leak its interrupt quiesced state? Would this leave the SWDMA
engine active and prevent THC_RXDMA2 from being restarted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729050145.2272597-1-even.xu@intel.com?part=1

Re: [PATCH v5 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Race condition during device removal allows the recovery workqueue item to be scheduled after it was cancelled, leading to a Use-After-Free on `qsdev`.

Pre-existing issues:
- [High] The HID device is destroyed before hardware interrupts are disabled, creating a race condition where the IRQ handler can dereference a freed HID device pointer.
--

--- Patch [3]: [PATCH v5 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback ---
commit 08604fa17c3b0d7c83e325ba80b11d716e4a7d1a
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

This commit refines the recover flow by using a workqueue to handle
recovery instead of processing it in the irq handler. It also calls
thc_rxdma_reset() to simplify the operation and disables interrupts
during the recovery flow.

> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -337,11 +336,10 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
>  	}
>  
>  end:
> -	thc_interrupt_enable(qsdev->thc_hw, true);
> -
>  	if (err_recover)
> -		if (try_recover(qsdev))
> -			qsdev->state = QUICKSPI_DISABLED;
> +		schedule_work(&qsdev->recover_work);
> +	else
> +		thc_interrupt_enable(qsdev->thc_hw, true);
>  
>  	pm_runtime_put_autosuspend(qsdev->dev);
>  
[ ... ]
> @@ -461,6 +461,10 @@ static struct quickspi_device *quickspi_dev_init(struct pci_dev *pdev, void __io
>   */
>  static void quickspi_dev_deinit(struct quickspi_device *qsdev)
>  {
> +	WRITE_ONCE(qsdev->recovery_disabled, true);
> +	cancel_work_sync(&qsdev->recover_work);
> +
> +	thc_interrupt_quiesce(qsdev->thc_hw, true);
>  	thc_interrupt_enable(qsdev->thc_hw, false);
>  	thc_ltr_unconfig(qsdev->thc_hw);
>  	thc_wot_unconfig(qsdev->thc_hw);

[Severity: High]
Can this sequence cause a use-after-free on qsdev?

Looking at quickspi_dev_deinit(), the recovery work is cancelled before
hardware interrupts are quiesced and disabled. If a hardware error
interrupt occurs immediately after cancel_work_sync() completes, the
IRQ thread will still run.

In quickspi_irq_thread_handler(), the work is unconditionally scheduled
without checking the recovery_disabled flag:

    if (err_recover)
        schedule_work(&qsdev->recover_work);

If this happens during device removal in quickspi_remove(), devres will
subsequently free qsdev, but the workqueue will execute try_recover()
afterwards and dereference the freed memory.

Could we either check the recovery_disabled flag before scheduling,
or disable interrupts before cancelling the work?

> @@ -711,12 +715,13 @@ static void quickspi_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quickspi_hid_remove(qsdev);
> +
> +	quickspi_dev_deinit(qsdev);
> +
>  	quickspi_dma_deinit(qsdev);
>  
>  	pm_runtime_get_noresume(qsdev->dev);
>  
> -	quickspi_dev_deinit(qsdev);
> -
>  	pci_clear_master(pdev);
>  }

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a
use-after-free of the HID device?

In quickspi_remove(), quickspi_hid_remove() is called to destroy the
HID device before quickspi_dev_deinit() disables hardware interrupts.

If a data interrupt fires in the window between these two calls,
quickspi_irq_thread_handler() will process it:

    if (int_mask & BIT(THC_RXDMA2_INT)) {
        ...
        quickspi_handle_input_data(qsdev, input_len);
    }

Since the HID device has already been freed by quickspi_hid_remove(),
attempting to report input data could dereference a freed pointer.

Should we ensure interrupts are disabled before destroying the HID device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729050145.2272597-1-even.xu@intel.com?part=3

Re: [PATCH v5 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Use-After-Free and race condition during device teardown due to improper workqueue cancellation.
- [Medium] Failed suspend operation leaves the device recovery mechanism permanently disabled.

Pre-existing issues:
- [High] Missing cleanup of IRQ vectors on error paths and device removal, leading to an IRQ resource leak.
- [High] Unsafe MMIO register access in hard IRQ handler without PM runtime verification.
- [High] Use-After-Free on the HID device structure during removal.
--

--- Patch [2]: [PATCH v5 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback ---
commit f51d43428a895d224d1667aba97de68a9108ddc3
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback

This commit introduces a workqueue for the recovery flow in the quicki2c
driver instead of processing it in the IRQ handler. It also simplifies
recovery using the thc_rxdma_reset() API and disables interrupts during
the whole recover flow to prevent storms on failure.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..539da002aa4fd 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -245,28 +245,33 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
>  }

[Severity: High]
This is a pre-existing issue, but could this cause an unsafe MMIO access?

Looking at quicki2c_irq_quick_handler():

static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
{
    ...
    /* Disable THC interrupt before current interrupt be handled */
    thc_interrupt_enable(qcdev->thc_hw, false);
    ...
}

The probe function allows fallback to shared legacy INTx interrupts via
pci_alloc_irq_vectors(). If the device is in D3hot (runtime suspended) and
a shared interrupt triggers on the same line, could this handler access the
thc_interrupt_enable MMIO register on powered-down hardware?

PM subsystem guidelines suggest checking pm_runtime_get_if_active() before
register access in shared IRQ handlers.

[ ... ]

> @@ -439,6 +445,9 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
>   */
>  static void quicki2c_dev_deinit(struct quicki2c_device *qcdev)
>  {
> +	WRITE_ONCE(qcdev->recovery_disabled, true);
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	thc_interrupt_quiesce(qcdev->thc_hw, true);
>  	thc_interrupt_enable(qcdev->thc_hw, false);

[Severity: High]
Is there a race condition here that could lead to a use-after-free?

Because cancel_work_sync() is called before the hardware interrupt source
is disabled with thc_interrupt_enable(), could a concurrent or preempted
threaded IRQ handler call schedule_work() immediately after
cancel_work_sync() returns?

If the work is placed back on the queue and the qcdev structure is
subsequently freed during removal, would the workqueue eventually execute
try_recover() on freed memory?

[ ... ]

> @@ -772,12 +781,13 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quicki2c_hid_remove(qcdev);
> +
> +	quicki2c_dev_deinit(qcdev);

[Severity: High]
This is a pre-existing issue, but does destroying the HID device before
disabling hardware interrupts create a race window?

If an RX interrupt arrives after quicki2c_hid_remove() completes but before
quicki2c_dev_deinit() disables interrupts, could the threaded IRQ handler
execute handle_input_report() and invoke quicki2c_hid_send_report(),
leading to a dereference of the freed qcdev->hid_dev pointer?

[Severity: High]
This is also a pre-existing issue, but are the IRQ vectors leaked
during removal?

The probe function allocates vectors via pci_alloc_irq_vectors() with
PCI_IRQ_ALL_TYPES, but there does not appear to be a corresponding
pci_free_irq_vectors() in quicki2c_remove() or the probe error paths.

Does this result in a permanent leak of system IRQ resources?

>  	quicki2c_dma_deinit(qcdev);
>  
>  	pm_runtime_get_noresume(qcdev->dev);
>  
> -	quicki2c_dev_deinit(qcdev);
> -
>  	pci_clear_master(pdev);
>  }

[ ... ]

> @@ -826,6 +836,9 @@ static int quicki2c_suspend(struct device *device)
>  	if (ret)
>  		return ret;
>  
> +	WRITE_ONCE(qcdev->recovery_disabled, true);
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	ret = thc_interrupt_quiesce(qcdev->thc_hw, true);
>  	if (ret)
>  		return ret;

[Severity: Medium]
If thc_interrupt_quiesce() fails and returns an error here, is
qcdev->recovery_disabled left permanently set to true?

Since a failed suspend aborts the transition and won't trigger a subsequent
resume callback to reset the flag, would this leave the driver in a state
where try_recover() silently aborts on all future errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729050145.2272597-1-even.xu@intel.com?part=2