Re: [PATCH v2 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback

[sashiko-bot@kernel.org]

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 <even.xu@intel.com>

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.

> 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?

[ ... ]

> @@ -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