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

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().

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