Re: [PATCH v3 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback
From: sashiko-bot@kernel.org
Date: 2026-07-13 03:17:50
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -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