Re: [PATCH v3 2/2] HID: intel-thc-hid: quicki2c: quiesce IRQs during teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] HID: intel-thc-hid: quicki2c: premature IRQ/dev teardown in
  quicki2c_remove()
--

commit cd8b528b30521f872e42d4b36fda65f3e1f39085
Author: Myeonghun Pak <mhun512@gmail.com>

HID: intel-thc-hid: quicki2c: quiesce IRQs during teardown

This commit disables and frees the device interrupt and deinitializes the
controller before tearing down the hid and dma resources during probe
unwind, remove, and shutdown.

> 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 29319b6227f3f..84e458fdd69cc 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c

[ ... ]

> @@ -788,9 +802,12 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  	if (!qcdev)
>  		return;
>  
> -	quicki2c_hid_remove(qcdev);
> -
> +	WRITE_ONCE(qcdev->recovery_disabled, true);
> +	disable_irq(pdev->irq);
>  	quicki2c_dev_deinit(qcdev);
> +	devm_free_irq(&pdev->dev, pdev->irq, qcdev);
> +
> +	quicki2c_hid_remove(qcdev);
>  
>  	quicki2c_dma_deinit(qcdev);

[Severity: High]
Does tearing down pdev->irq and calling quicki2c_dev_deinit() before
quicki2c_hid_remove() break synchronous transfers issued while
hid_destroy_device() disconnects subdrivers and hidraw?

When quicki2c_hid_remove() runs hid_destroy_device(), bound subdrivers such
as hid-multitouch close the device (mt_remove() -> hid_hw_stop() -> ... ->
hid_hw_close() -> mt_on_hid_hw_close() -> mt_set_modes()) and send
synchronous requests through quicki2c_hid_raw_request().  Because
quicki2c_set_report() and quicki2c_get_report() rely on pdev->irq via
thc_dma_write() and thc_swdma_read() to signal completion on
dev->write_complete_wait and dev->swdma_complete_wait, starting those
transfers after pdev->irq is disabled and freed stalls each request for its
1-second wait timeout.

On timeout, thc_swdma_read() skips thc_swdma_read_completion(), leaving the
software dma engine started when quicki2c_dma_deinit() unmaps and frees the
scatterlists and prd tables (as thc_dma_unconfigure() only stops the two
receive dma engines):

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:thc_swdma_read() {
    ...
	ret = thc_swdma_read_start(dev, write_buff, write_len, prd_tbl_len);
	if (ret)
		goto end;

	ret = wait_event_interruptible_timeout(dev->swdma_complete_wait, dev->swdma_done, 1 * HZ);
	if (ret <= 0 || !dev->swdma_done) {
		dev_err_once(dev->dev, "timeout for waiting SWDMA completion\n");
		ret = -ETIMEDOUT;
		goto end;
	}
    ...
	ret = thc_swdma_read_completion(dev);
    ...
}

In addition, if an in-flight hidraw request in thc_swdma_read() receives
its completion interrupt just before disable_irq(),
thc_swdma_read_completion() can run after quicki2c_dev_deinit() while
quicki2c_hid_remove() drains hidraw, restarting the receive dma engine and
unquiescing interrupts right before quicki2c_dma_deinit() frees the dma
buffers.

Keeping quicki2c_hid_remove(qcdev) before disabling pdev->irq and calling
quicki2c_dev_deinit(qcdev) would let hid teardown finish while interrupts
are still active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112T063704Z-thc-v3-0-mhun512@gmail.com?part=2