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

[PATCH v3 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:
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             | 49 ++++++++++-------
 .../intel-quicki2c/quicki2c-dev.h             |  2 +
 .../intel-quickspi/pci-quickspi.c             | 53 ++++++++++---------
 .../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, 116 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH v3 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 v3 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 2 potential issue(s) to consider:
- [Critical] `thc_rxdma_reset` improperly clears descriptor addresses for all DMA channels without pausing them first, potentially causing active TX/SW DMA to access physical address 0.
- [High] Error paths in `thc_rxdma_reset` fail to restore the interrupt quiesce state, permanently disabling device interrupts.
--

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

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

This commit adds a new helper function thc_rxdma_reset() to perform a read DMA
reset. It is intended to be called for RxDMA recovery when fatal DMA errors
occur.

> 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
[ ... ]
> +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 skip unquiescing the interrupts?  If thc_wait_for_dma_pause() fails,
we jump to the end label and return without calling 
thc_interrupt_quiesce(dev, false).  Will this leave device interrupts 
permanently disabled?

> +	}
> +
> +	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]
Can this cause active TX or SW DMA engines to access physical address 0?

thc_rxdma_reset() pauses THC_RXDMA1 and THC_RXDMA2, but thc_dma_unconfigure() 
unconditionally iterates over MAX_THC_DMA_CHANNEL and sets the PRD base
address to 0 for all channels, including THC_TXDMA and THC_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]);
	}
        ...
}

If a previous thc_dma_write() or thc_swdma_read() timed out, it unlocks 
thc_bus_lock but leaves the TX/SW DMA engine running in hardware. Overwriting 
the PRD base address to 0 while the engine is active could result in memory 
corruption or an IOMMU fault.

> +
> +	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;
> +}

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

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

[Xu, Even]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Monday, July 13, 2026 11:18 AM
> To: Xu, Even <even.xu@intel.com>
> Cc: linux-input@vger.kernel.org; dmitry.torokhov@gmail.com
> Subject: Re: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read
> DMA
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to
> consider:
> - [Critical] `thc_rxdma_reset` improperly clears descriptor addresses for all DMA
> channels without pausing them first, potentially causing active TX/SW DMA to
> access physical address 0.
> - [High] Error paths in `thc_rxdma_reset` fail to restore the interrupt quiesce
> state, permanently disabling device interrupts.
> --
> 
> --- Patch [1]: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read
> DMA --- commit 64b22c0d882751a67b709351d2c2e3daf211bb52
> Author: Even Xu <even.xu@intel.com>
> 
> HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
> 
> This commit adds a new helper function thc_rxdma_reset() to perform a read
> DMA reset. It is intended to be called for RxDMA recovery when fatal DMA errors
> occur.
> 
> > 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
> [ ... ]
> > +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 skip unquiescing the interrupts?  If thc_wait_for_dma_pause() fails, we
> jump to the end label and return without calling thc_interrupt_quiesce(dev, false).
> Will this leave device interrupts permanently disabled?
> 
> > +	}
> > +
> > +	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);
> 

This is expected, thc_rxdma_reset() is designed only be called in fatal DMA error. In this case, if pause RxDMA failed, it means DMA engine cannot be recovered.

> [Severity: Critical]
> Can this cause active TX or SW DMA engines to access physical address 0?
> 
> thc_rxdma_reset() pauses THC_RXDMA1 and THC_RXDMA2, but
> thc_dma_unconfigure() unconditionally iterates over MAX_THC_DMA_CHANNEL
> and sets the PRD base address to 0 for all channels, including THC_TXDMA and
> THC_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]);
> 	}
>         ...
> }
> 
> If a previous thc_dma_write() or thc_swdma_read() timed out, it unlocks
> thc_bus_lock but leaves the TX/SW DMA engine running in hardware. Overwriting
> the PRD base address to 0 while the engine is active could result in memory
> corruption or an IOMMU fault.

This is not a problem, All DMA operations were protected by THC bus mutex lock.
There is no Tx/SW DMA operation when calls into this thc_rxdma_reset().

> 
> > +
> > +	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;
> > +}
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713030412.2531933-1-
> even.xu@intel.com?part=1

[PATCH v3 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             | 49 ++++++++++++-------
 .../intel-quicki2c/quicki2c-dev.h             |  2 +
 2 files changed, 32 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..2b586d0635b6 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);
@@ -772,6 +772,12 @@ static void quicki2c_remove(struct pci_dev *pdev)
 		return;
 
 	quicki2c_hid_remove(qcdev);
+
+	thc_interrupt_quiesce(qcdev->thc_hw, true);
+	thc_interrupt_enable(qcdev->thc_hw, false);
+
+	cancel_work_sync(&qcdev->recover_work);
+
 	quicki2c_dma_deinit(qcdev);
 
 	pm_runtime_get_noresume(qcdev->dev);
@@ -796,6 +802,11 @@ static void quicki2c_shutdown(struct pci_dev *pdev)
 	if (!qcdev)
 		return;
 
+	thc_interrupt_quiesce(qcdev->thc_hw, true);
+	thc_interrupt_enable(qcdev->thc_hw, false);
+
+	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 v3 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:
- [High] The recovery workqueue can be scheduled by the threaded IRQ handler after `cancel_work_sync()` returns, leading to a Use-After-Free during device removal or shutdown.
- [High] Missing cancellation of `recover_work` in the probe error path leads to a Use-After-Free if initialization fails.
- [High] Missing cancellation of `recover_work` during system suspend leaves the hardware in a corrupted state.
--

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

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

This commit modifies the recovery flow for Intel-quicki2c by using a
workqueue to process device resets instead of handling them directly
within the IRQ handler. It introduces the recover_work work_struct and
manages hardware interrupt states 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
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
[ ... ]
> @@ -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 */

[Severity: High]
Does this work need to be cancelled during the probe error paths and
system suspend transitions?

If a failure occurs during quicki2c_probe() after the hardware interrupts are
enabled, the devres framework will free qcdev. If an interrupt fires and
schedules this work before the error handler returns, could the background
workqueue dereference the freed qcdev?

Similarly, quicki2c_suspend() and quicki2c_freeze() do not appear to cancel
this work. If an interrupt schedules recover_work just before the system
suspends, could try_recover() run concurrently and re-enable hardware
interrupts, overriding the suspended hardware state?

[ ... ]
> @@ -772,6 +772,12 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quicki2c_hid_remove(qcdev);
> +
> +	thc_interrupt_quiesce(qcdev->thc_hw, true);
> +	thc_interrupt_enable(qcdev->thc_hw, false);
> +
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	quicki2c_dma_deinit(qcdev);

[Severity: High]
Can the threaded IRQ handler race with this removal and re-queue the work?

The IRQ handler is managed by devm_request_threaded_irq() and remains active
until devres cleanup occurs after quicki2c_remove() returns.

If an interrupt fires right as cancel_work_sync() completes, couldn't the
IRQ thread call schedule_work() again?

If try_recover() then executes concurrently, it overrides the hardware
teardown by calling thc_interrupt_enable(..., true). When devres subsequently
frees the IRQ and qcdev, will this result in a use-after-free when the
re-queued work executes?

[ ... ]
> @@ -796,6 +802,11 @@ static void quicki2c_shutdown(struct pci_dev *pdev)
>  	if (!qcdev)
>  		return;
>  
> +	thc_interrupt_quiesce(qcdev->thc_hw, true);
> +	thc_interrupt_enable(qcdev->thc_hw, false);
> +
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	/* Must stop DMA before reboot to avoid DMA entering into unknown state */

[Severity: High]
Does this cancellation share the same race condition with the threaded IRQ
handler as quicki2c_remove()?

Without disabling the Linux IRQ line before cancelling the work, could a
concurrent hardware interrupt allow quicki2c_irq_thread_handler() to
re-queue recover_work during system shutdown?

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

[PATCH v3 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             | 53 ++++++++++---------
 .../intel-quickspi/quickspi-dev.h             |  3 ++
 2 files changed, 32 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..3690ddd061e1 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);
@@ -711,6 +705,12 @@ static void quickspi_remove(struct pci_dev *pdev)
 		return;
 
 	quickspi_hid_remove(qsdev);
+
+	thc_interrupt_quiesce(qsdev->thc_hw, true);
+	thc_interrupt_enable(qsdev->thc_hw, false);
+
+	cancel_work_sync(&qsdev->recover_work);
+
 	quickspi_dma_deinit(qsdev);
 
 	pm_runtime_get_noresume(qsdev->dev);
@@ -737,6 +737,11 @@ static void quickspi_shutdown(struct pci_dev *pdev)
 	if (!qsdev)
 		return;
 
+	thc_interrupt_quiesce(qsdev->thc_hw, true);
+	thc_interrupt_enable(qsdev->thc_hw, false);
+
+	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 v3 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Teardown race in `quickspi_remove()` and `quickspi_shutdown()` against `try_recover()` allows hardware interrupts to be incorrectly re-enabled or work to be scheduled after cancellation, leading to Use-After-Free.
- [High] Missing `cancel_work_sync` in the probe error path leads to a Use-After-Free if recovery work is scheduled during a failed probe.
- [High] The recovery work is not cancelled during system suspend, potentially corrupting hardware state.
--

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

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

This commit refines the quickspi device recovery flow. It moves the
recovery process from the IRQ handler to a workqueue, uses the
thc_rxdma_reset() API to simplify operations, and manages interrupt
states throughout the recovery sequence.

> 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 4ae2e1718b30c..83ed5516b8097 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c

[ ... ]

> @@ -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);

[Severity: High]
Since recover_work is being initialized here, what happens if
quickspi_probe() fails after the IRQ is registered?

If a hardware error interrupt occurs and schedules recover_work, and a
subsequent step in the probe process fails, the error path jumps to
the dev_deinit label which cleans up the device but never calls
cancel_work_sync().

Could this result in the devres framework freeing the IRQ and qsdev
while the recovery work is still pending, leading to a use-after-free
crash when the work runs?

[ ... ]

> @@ -711,6 +705,12 @@ static void quickspi_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quickspi_hid_remove(qsdev);
> +
> +	thc_interrupt_quiesce(qsdev->thc_hw, true);
> +	thc_interrupt_enable(qsdev->thc_hw, false);
> +
> +	cancel_work_sync(&qsdev->recover_work);
> +

[Severity: High]
Is there a race condition here against the threaded IRQ handler during
device teardown?

Since qsdev->state is not set to QUICKSPI_DISABLED and the OS-level
IRQ is not disabled before cancel_work_sync() is called, could a
concurrent execution of quickspi_irq_thread_handler() schedule the
work again immediately after cancel_work_sync() returns?

Also, if try_recover() is running concurrently during this teardown, it
unconditionally calls thc_interrupt_enable(qsdev->thc_hw, true) on exit,
which could re-enable hardware interrupts after they were just disabled
here. Could this lead to a use-after-free when the devres framework
later frees the qsdev structure?

[Severity: High]
Does the quickspi_suspend() callback need to cancel this work as well?

Currently, quickspi_suspend() transitions the device to a suspended
state but lacks a call to cancel_work_sync(&qsdev->recover_work).
Because schedule_work() queues onto the non-freezable system_wq, the
try_recover() work could execute concurrently with or after the
suspend process.

Since try_recover() calls pm_runtime_resume_and_get(), resets the
RxDMA, and re-enables hardware interrupts, could this leave the device
in an inconsistent active state across system suspend, potentially
causing resume failures or power leaks?

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