Re: [PATCH 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback
From: sashiko-bot@kernel.org
Date: 2026-07-01 02:15:49
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing cancellation of the scheduled asynchronous recovery work during device removal and shutdown leads to a Use-After-Free. - [High] Unconditionally re-enabling hardware interrupts before the asynchronous recovery completes causes an immediate interrupt storm. - [High] The deferred `try_recover` work function accesses hardware registers directly without acquiring a Runtime PM reference. -- commit 8f5c7d640eb5d690c93bdf294d89a5d9087edae5 Author: Even Xu [off-list ref] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback This commit refines the recover flow by moving the hardware recovery operation to an asynchronous workqueue. It replaces the inline recover operation with a call to schedule_work() and simplifies the recover callback using thc_rxdma_reset().
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..411d7a0ab90ef 100644 --- 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
@@ -245,28 +245,23 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id) } -static int try_recover(struct quicki2c_device *qcdev) +static void try_recover(struct work_struct *work) { + struct quicki2c_device *qcdev = container_of(work, struct quicki2c_device, recover_work); + thc_interrupt_enable(qcdev->thc_hw, false);
[Severity: High] Is it safe to access the hardware registers in this work function without holding a runtime PM reference? Since the IRQ handler calls pm_runtime_put_autosuspend() right after scheduling the work, could the device transition to a suspended state before try_recover() executes, resulting in invalid MMIO accesses when trying to manipulate the THC registers?
- thc_dma_unconfigure(qcdev->thc_hw);
-
- ret = thc_dma_configure(qcdev->thc_hw);
- if (ret) {
- dev_err(qcdev->dev, "Reconfig DMA failed\n");
- return ret;
- }
+ if (thc_rxdma_reset(qcdev->thc_hw))
+ qcdev->state = QUICKI2C_DISABLED;
+ else
+ thc_interrupt_enable(qcdev->thc_hw, true);
}[ ... ]
quoted hunk ↗ jump to hunk
@@ -346,8 +341,7 @@ static irqreturn_t quicki2c_irq_thread_handler(int irq, void *dev_id) thc_interrupt_enable(qcdev->thc_hw, true);
[Severity: High] Does re-enabling the interrupt here before the recovery work executes cause an interrupt storm? Because the actual hardware error state isn't cleared until thc_rxdma_reset() runs asynchronously in the workqueue, unmasking the interrupt here might cause the hardware to instantly re-assert the interrupt. Could this lead to an infinite loop between the hard IRQ and threaded IRQ handlers, thereby starving the system workqueue from ever executing try_recover()?
if (err_recover) - if (try_recover(qcdev)) - qcdev->state = QUICKI2C_DISABLED; + schedule_work(&qcdev->recover_work); pm_runtime_put_autosuspend(qcdev->dev);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -386,6 +380,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);
[Severity: High] When the device is removed, could the scheduled work run after the device context is freed? Looking at quicki2c_remove(), there is no call to cancel_work_sync() before tearing down the device. Since qcdev is allocated with devres, might this lead to a use-after-free if the recovery work is still queued on the system workqueue when the removal path finishes?
/* THC hardware init */ qcdev->thc_hw = thc_dev_init(qcdev->dev, qcdev->mem_addr);
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260701020432.3774514-1-even.xu@intel.com?part=2