Re: [PATCH v3 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:
- [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 <even.xu@intel.com>

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.

> 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
[ ... ]
> @@ -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?

[ ... ]
> @@ -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?

[ ... ]
> @@ -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