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

[sashiko-bot@kernel.org]

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