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