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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-after-free of qsdev during device removal due to uncanceled async work
- [High] Missing runtime PM protection during asynchronous hardware recovery
- [High] Unconditional interrupt unmasking causes severe interrupt storm
- [High] Data race on DMA configuration between IRQ thread and async recovery

Pre-existing issues:
- [High] Invalid runtime PM teardown causes unpowered hardware accesses during removal
--

commit b4ff70b4c528a251392ae3368aa0d2f80233ab2e
Author: Even Xu <even.xu@intel.com>
Subject: HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

This commit refines the recovery flow by using a workqueue to handle
recovery instead of processing it in the IRQ handler. It calls
thc_rxdma_reset() to simplify operations and disables interrupts during
the recovery 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
> index 4ae2e1718b30c..a2428db5f9414 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,23 @@ static irqreturn_t quickspi_irq_quick_handler(int irq, void *dev_id)
[ ... ]
> +static void try_recover(struct work_struct *work)
> +{
> +	struct quickspi_device *qsdev = container_of(work, struct quickspi_device, recover_work);
> +
> +	thc_interrupt_enable(qsdev->thc_hw, false);
> +

[Severity: High]
Does this asynchronously scheduled work need to acquire its own runtime PM
reference?

The IRQ thread drops its PM reference with pm_runtime_put_autosuspend()
immediately after scheduling this work. If the autosuspend delay expires
before this workqueue executes, the device may enter a suspended state.

Will calling thc_interrupt_enable() and thc_rxdma_reset() here cause a system
crash or silent failure by accessing unpowered hardware registers?

> +	if (thc_rxdma_reset(qsdev->thc_hw))

[Severity: High]
Is there a potential data race here with the IRQ thread?

The try_recover() function calls thc_rxdma_reset(), which holds the
thc_bus_lock while unconfiguring and reconfiguring the DMA hardware.
However, if a concurrent interrupt occurs, quickspi_irq_thread_handler()
calls thc_rxdma_read() without holding thc_bus_lock.

Could this allow the IRQ thread to read from DMA rings while they are
actively being torn down and rebuilt by the recovery work on another CPU?

> +		qsdev->state = QUICKSPI_DISABLED;
> +	else
> +		thc_interrupt_enable(qsdev->thc_hw, true);
>  }
[ ... ]
> @@ -340,8 +329,7 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
>  	thc_interrupt_enable(qsdev->thc_hw, true);
>  
>  	if (err_recover)
> -		if (try_recover(qsdev))
> -			qsdev->state = QUICKSPI_DISABLED;
> +		schedule_work(&qsdev->recover_work);

[Severity: High]
Could this unconditional interrupt unmasking cause an interrupt storm?

When the hardware signals a fatal error, the handler unmasks the interrupt
here before returning, but the hardware reset is deferred to the asynchronous
workqueue.

Since the hardware error condition hasn't been cleared yet, won't the
device immediately re-assert the interrupt and trap the CPU in an endless
hardirq-to-thread loop before the workqueue ever has a chance to execute?

>  
>  	pm_runtime_put_autosuspend(qsdev->dev);
>  
> @@ -385,6 +373,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]
Does this newly added work item need to be explicitly canceled during device
removal?

In the driver removal path in quickspi_remove(), there is no call to
cancel_work_sync().

Since qsdev is allocated with devm_kzalloc(), it will be freed shortly after
quickspi_remove() returns. If a fatal error interrupt schedules this work just
before or during driver unbind, could the workqueue execute after qsdev is
freed, causing a use-after-free when try_recover() dereferences it?

[Severity: High]
This is a pre-existing issue, but does quickspi_remove() properly wake the
device before accessing its hardware registers?

Currently, quickspi_remove() uses pm_runtime_get_noresume():

quickspi_remove() {
    ...
    pm_runtime_get_noresume(qsdev->dev);
    quickspi_dev_deinit(qsdev);
    ...
}

Calling pm_runtime_get_noresume() only increments the usage counter, it does
not resume the device. If the device was already auto-suspended,
quickspi_dev_deinit() will write to unpowered registers.

Could this lead to dropped writes or a PCIe bus error?

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