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

[sashiko-bot@kernel.org]

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