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

[PATCH v2 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:
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             | 42 ++++++++-------
 .../intel-quicki2c/quicki2c-dev.h             |  2 +
 .../intel-quickspi/pci-quickspi.c             | 46 ++++++++---------
 .../intel-quickspi/quickspi-dev.h             |  3 ++
 .../intel-thc-hid/intel-thc/intel-thc-dma.c   | 51 +++++++++++++++++++
 .../intel-thc-hid/intel-thc/intel-thc-dma.h   |  1 +
 6 files changed, 102 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH v2 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

Re: [PATCH v2 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 4 potential issue(s) to consider:

New issues:
- [High] thc_rxdma_reset() fails to re-initialize the RxDMA PRD tables, leading to hardware reading shrunken buffer sizes on restart.
- [High] thc_rxdma_reset() unsafely clears PRD base addresses for all DMA channels while TX or SW DMA engines may still be running.
- [Medium] thc_rxdma_reset() ignores the return value of thc_interrupt_quiesce(dev, false) on the success path.

Pre-existing issues:
- [High] Incorrect polling condition and execution order in thc_interrupt_quiesce during unquiesce (int_quiesce = false).
--

commit b0beafd1bc7a02f7a2277c5a6d69c17c58724f01
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

Add a helper function thc_rxdma_reset() to do read DMA reset, it can be
called when 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)
> +{
[ ... ]
> +	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: High]
Could this cause memory corruption or IOMMU faults if TX or SW DMA engines
are still running?

thc_rxdma_reset() explicitly pauses RxDMA1 and RxDMA2 but does not pause
TXDMA or SWDMA. It then calls thc_dma_unconfigure(), which loops over all
DMA channels (MAX_THC_DMA_CHANNEL) and blindly overwrites their PRD base
addresses to 0x0.

If TXDMA was left running (for instance, if a prior thc_dma_write() timed
out without clearing the TXDMA start bit), setting its PRD base address to 0
while it is actively fetching could cause the hardware to fetch descriptors
from physical address 0.

> +
> +	ret = thc_dma_configure(dev);

[Severity: High]
Does this leave dirty 'len' descriptors in the PRD memory layout?

thc_dma_configure() restarts the DMA engine pointers to 0, but it does not
re-initialize the PRD memory layout. The hardware updates the 'len' field
of PRD entries to indicate the actual length of received packets.

Restarting the DMA from index 0 without restoring 'len' to the maximum
buffer capacity could cause the hardware to use previously received smaller
packet sizes as the maximum capacity for new transfers, resulting in
truncated packets or DMA errors.

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

[Severity: Medium]
Is it safe to ignore the return value of thc_interrupt_quiesce() here?

Discarding this return value on the success path could silently hide a
hardware timeout or regmap read failure, potentially leaving interrupts
disabled while the function reports success.

[Severity: High]
This is a pre-existing issue, but is the polling condition and execution
order in thc_interrupt_quiesce() correct when unquiescing?

When thc_interrupt_quiesce() is called with int_quiesce = false, it polls
the HW_STS bit expecting it to be 1 BEFORE it clears the QUIESCE_EN bit:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_interrupt_quiesce() {
    ...
    ret = regmap_read_poll_timeout(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET, ctrl,
                                   ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_HW_STS,
                                   THC_REGMAP_POLLING_INTERVAL_US, THC_QUIESCE_EN_TIMEOUT_US);
    ...
    /* Unquiesce device interrupt - Clear the quiesce bit */
    if (!int_quiesce)
        regmap_write_bits(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
                          THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN, 0);
}

Since the device is already quiesced, HW_STS is already 1, causing the poll
to return immediately. It then clears the QUIESCE_EN bit and returns without
ever waiting for HW_STS to become 0 to acknowledge the unquiesce. This could
cause the driver to proceed before interrupts are actually re-enabled.

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

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

[PATCH v2 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             | 42 ++++++++++---------
 .../intel-quicki2c/quicki2c-dev.h             |  2 +
 2 files changed, 25 insertions(+), 19 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 46d3e9a01999..11e0e129b44c 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,28 @@ 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 (pm_runtime_resume_and_get(qcdev->dev))
+		return;
 
-	ret = thc_dma_configure(qcdev->thc_hw);
-	if (ret) {
-		dev_err(qcdev->dev, "Reconfig DMA failed\n");
-		return ret;
-	}
+	thc_interrupt_enable(qcdev->thc_hw, false);
 
-	return 0;
+	if (thc_rxdma_reset(qcdev->thc_hw))
+		qcdev->state = QUICKI2C_DISABLED;
+	else
+		thc_interrupt_enable(qcdev->thc_hw, true);
+
+	pm_runtime_put_autosuspend(qcdev->dev);
 }
 
 static int handle_input_report(struct quicki2c_device *qcdev)
@@ -343,11 +343,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);
 
@@ -386,6 +385,7 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
 	qcdev->ddata = ddata;
 
 	init_waitqueue_head(&qcdev->reset_ack_wq);
+	INIT_WORK(&qcdev->recover_work, try_recover);
 
 	/* THC hardware init */
 	qcdev->thc_hw = thc_dev_init(qcdev->dev, qcdev->mem_addr);
@@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev)
 	if (!qcdev)
 		return;
 
+	cancel_work_sync(&qcdev->recover_work);
+
 	quicki2c_hid_remove(qcdev);
 	quicki2c_dma_deinit(qcdev);
 
@@ -796,6 +798,8 @@ static void quicki2c_shutdown(struct pci_dev *pdev)
 	if (!qcdev)
 		return;
 
+	cancel_work_sync(&qcdev->recover_work);
+
 	/* Must stop DMA before reboot to avoid DMA entering into unknown state */
 	quicki2c_dma_deinit(qcdev);
 
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 61dbdece59a1..aedf85291e60 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
@@ -222,6 +222,8 @@ struct quicki2c_device {
 	wait_queue_head_t reset_ack_wq;
 	bool reset_ack;
 
+	struct work_struct recover_work;
+
 	u32 i2c_max_frame_size_enable;
 	u32 i2c_max_frame_size;
 	u32 i2c_int_delay_enable;
-- 
2.43.0

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

[sashiko-bot]

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

New issues:
- [High] Use-After-Free due to premature `cancel_work_sync` during device removal and shutdown.

Pre-existing issues:
- [High] Missing `pci_free_irq_vectors()` in probe error paths and device removal leaks IRQ resources.
- [High] The hard IRQ handler accesses hardware registers unconditionally, violating PM subsystem guidelines.
--

commit e88da9d40562f5d0be755568ae60010a7ba925df
Author: Even Xu <even.xu@intel.com>

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

This commit refines the recover flow by offloading it to a workqueue instead
of processing it within the IRQ handler. It simplifies the operation with a
reset API and ensures interrupts are disabled during the recovery process.

> 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..11e0e129b44c3 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,28 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
>  }

[Severity: High]
This is a pre-existing issue, but does quicki2c_irq_quick_handler() access
hardware registers unconditionally?

If this device shares an INTx interrupt line, a spurious interrupt from
another device could cause quicki2c_irq_quick_handler() to execute. It then
calls thc_interrupt_enable() which writes to memory-mapped registers. If the
device is in a low-power state, this hardware access can trigger a bus fault.

Should pm_runtime_get_if_active() be used to check if the device is active
before touching the hardware registers?

[ ... ]

> @@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  	if (!qcdev)
>  		return;
>  
> +	cancel_work_sync(&qcdev->recover_work);

[Severity: High]
Can this sequence lead to a use-after-free?

cancel_work_sync() is called here, but hardware interrupts are not yet
quiesced, and the state is still active. If an interrupt fires immediately
after cancel_work_sync(), quicki2c_irq_thread_handler() can execute and call:

    schedule_work(&qcdev->recover_work);

This re-enqueues the work item. After quicki2c_remove() returns, devres
frees the qcdev structure. When the scheduled work later executes, it will
dereference the freed qcdev pointer.

Should the work be cancelled after interrupts are disabled and the device
state is set to QUICKI2C_DISABLED?

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

[Severity: High]
This is a pre-existing issue, but does quicki2c_remove() leak IRQ resources?

The remove path unbinds the device but doesn't appear to call 
pci_free_irq_vectors(). Similarly, if quicki2c_probe() fails after 
pci_alloc_irq_vectors(), the error path (disable_pci_device) does not 
free the vectors.

Repeated unbind/bind operations or probe deferrals will continually leak
these vectors, which can eventually exhaust system IRQ capacity.

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

[PATCH v2 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             | 46 +++++++++----------
 .../intel-quickspi/quickspi-dev.h             |  3 ++
 2 files changed, 25 insertions(+), 24 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 f669235f1883..bc9eead77562 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,28 @@ 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 (pm_runtime_resume_and_get(qsdev->dev))
+		return;
 
-	thc_dma_unconfigure(qsdev->thc_hw);
+	thc_interrupt_enable(qsdev->thc_hw, false);
 
-	ret = thc_dma_configure(qsdev->thc_hw);
-	if (ret) {
-		dev_err(qsdev->dev, "Re-configure THC DMA failed, ret = %d\n", ret);
-		return ret;
-	}
+	if (thc_rxdma_reset(qsdev->thc_hw))
+		qsdev->state = QUICKSPI_DISABLED;
+	else
+		thc_interrupt_enable(qsdev->thc_hw, true);
 
-	return 0;
+	pm_runtime_put_autosuspend(qsdev->dev);
 }
 
 /**
@@ -337,11 +331,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 +378,7 @@ 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);
+	INIT_WORK(&qsdev->recover_work, try_recover);
 
 	/* thc hw init */
 	qsdev->thc_hw = thc_dev_init(qsdev->dev, qsdev->mem_addr);
@@ -710,6 +704,8 @@ static void quickspi_remove(struct pci_dev *pdev)
 	if (!qsdev)
 		return;
 
+	cancel_work_sync(&qsdev->recover_work);
+
 	quickspi_hid_remove(qsdev);
 	quickspi_dma_deinit(qsdev);
 
@@ -737,6 +733,8 @@ static void quickspi_shutdown(struct pci_dev *pdev)
 	if (!qsdev)
 		return;
 
+	cancel_work_sync(&qsdev->recover_work);
+
 	/* Must stop DMA before reboot to avoid DMA entering into unknown state */
 	quickspi_dma_deinit(qsdev);
 
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..363e589b0bde 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"
 
@@ -173,6 +174,8 @@ struct quickspi_device {
 
 	wait_queue_head_t set_report_cmpl_wq;
 	bool set_report_cmpl;
+
+	struct work_struct recover_work;
 };
 
 #endif /* _QUICKSPI_DEV_H_ */
-- 
2.43.0

Re: [PATCH v2 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:
- [High] `cancel_work_sync()` is called before interrupts are safely disabled during device teardown, and is missing entirely from the probe error path, allowing the work item to execute after the device structure is freed.
- [High] Non-fatal or spurious interrupts occurring during async recovery will prematurely re-enable hardware interrupts, racing with the DMA reset and potentially causing DMA corruption.
--

commit f745eb96a11ec6734e09736d8674beb349a74ab9
Author: Even Xu <even.xu@intel.com>

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

This commit shifts the QuickSPI recover flow from the IRQ handler to a
workqueue. It relies on thc_rxdma_reset() for recovery and keeps interrupts
disabled during the process.

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -337,11 +331,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);

[Severity: High]
If try_recover() disables device interrupts and is actively resetting DMA
engines, could a spurious or non-fatal interrupt cause
quickspi_irq_thread_handler() to evaluate err_recover == 0 and unconditionally
re-enable hardware interrupts here?

If so, a subsequent real interrupt could race with the ongoing DMA reset
since thc_rxdma_read() does not appear to hold thc_bus_lock.

>  
>  	pm_runtime_put_autosuspend(qsdev->dev);
>  
[ ... ]
> @@ -710,6 +704,8 @@ static void quickspi_remove(struct pci_dev *pdev)
>  	if (!qsdev)
>  		return;
>  
> +	cancel_work_sync(&qsdev->recover_work);
> +

[Severity: High]
Since quickspi_dev_deinit() disables hardware interrupts later in this function,
is there a race window here?

If a fatal error interrupt occurs after cancel_work_sync() but before
hardware interrupts are disabled, the IRQ thread handler could schedule
recover_work again. The qsdev structure would then be freed, leaving a
pending work item that dereferences freed memory.

Also, should cancel_work_sync() be called in the error paths of
quickspi_probe() after the work item is initialized and interrupts are
enabled?

>  	quickspi_hid_remove(qsdev);
>  	quickspi_dma_deinit(qsdev);
>  
[ ... ]
> @@ -737,6 +733,8 @@ static void quickspi_shutdown(struct pci_dev *pdev)
>  	if (!qsdev)
>  		return;
>  
> +	cancel_work_sync(&qsdev->recover_work);
> +

[Severity: High]
Could this have the same race window as quickspi_remove(), where a fatal
error interrupt might reschedule recover_work before quickspi_dev_deinit()
disables interrupts?

>  	/* Must stop DMA before reboot to avoid DMA entering into unknown state */
>  	quickspi_dma_deinit(qsdev);

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