Re: [PATCH v2 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
From: sashiko-bot@kernel.org
Date: 2026-07-03 08:10:55
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [High] Use-After-Free due to premature `cancel_work_sync` during device removal and shutdown. Pre-existing issues: - [High] Missing `pci_free_irq_vectors()` in probe error paths and device removal leaks IRQ resources. - [High] The hard IRQ handler accesses hardware registers unconditionally, violating PM subsystem guidelines. -- commit e88da9d40562f5d0be755568ae60010a7ba925df Author: Even Xu [off-list ref] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback This commit refines the recover flow by offloading it to a workqueue instead of processing it within the IRQ handler. It simplifies the operation with a reset API and ensures interrupts are disabled 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 index 46d3e9a019995..11e0e129b44c3 100644 --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c@@ -245,28 +245,28 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id) }
[Severity: High] This is a pre-existing issue, but does quicki2c_irq_quick_handler() access hardware registers unconditionally? If this device shares an INTx interrupt line, a spurious interrupt from another device could cause quicki2c_irq_quick_handler() to execute. It then calls thc_interrupt_enable() which writes to memory-mapped registers. If the device is in a low-power state, this hardware access can trigger a bus fault. Should pm_runtime_get_if_active() be used to check if the device is active before touching the hardware registers? [ ... ]
quoted hunk ↗ jump to hunk
@@ -771,6 +771,8 @@ static void quicki2c_remove(struct pci_dev *pdev) if (!qcdev) return; + cancel_work_sync(&qcdev->recover_work);
[Severity: High]
Can this sequence lead to a use-after-free?
cancel_work_sync() is called here, but hardware interrupts are not yet
quiesced, and the state is still active. If an interrupt fires immediately
after cancel_work_sync(), quicki2c_irq_thread_handler() can execute and call:
schedule_work(&qcdev->recover_work);
This re-enqueues the work item. After quicki2c_remove() returns, devres
frees the qcdev structure. When the scheduled work later executes, it will
dereference the freed qcdev pointer.
Should the work be cancelled after interrupts are disabled and the device
state is set to QUICKI2C_DISABLED?
quicki2c_hid_remove(qcdev); quicki2c_dma_deinit(qcdev); pm_runtime_get_noresume(qcdev->dev); quicki2c_dev_deinit(qcdev); pci_clear_master(pdev); }
[Severity: High] This is a pre-existing issue, but does quicki2c_remove() leak IRQ resources? The remove path unbinds the device but doesn't appear to call pci_free_irq_vectors(). Similarly, if quicki2c_probe() fails after pci_alloc_irq_vectors(), the error path (disable_pci_device) does not free the vectors. Repeated unbind/bind operations or probe deferrals will continually leak these vectors, which can eventually exhaust system IRQ capacity. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260703075858.2780398-1-even.xu@intel.com?part=2