Re: [PATCH v2 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 91b9650f43556a025a9bb311fa9a30624bcb700a
Author: Myeonghun Pak <mhun512@gmail.com>

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

This commit disables and frees the device IRQ, cancels recovery work, and
quiesces THC interrupts during probe error unwind, removal, 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..a3e98e0faff83 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 +798,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 disabling and freeing pdev->irq and calling quicki2c_dev_deinit()
before quicki2c_hid_remove() break synchronous HID report transfers issued
while unregistering qcdev->hid_dev?

While hid_destroy_device() runs, bound HID drivers and open hidraw clients
still invoke quicki2c_hid_raw_request() or quicki2c_hid_output_report().
For example, when an input device is open during removal, hid-multitouch
calls hid_hw_close() -> mt_on_hid_hw_close() -> mt_set_modes(), which sends
HID_REQ_SET_REPORT and HID_REQ_GET_REPORT via thc_dma_write() and
thc_swdma_read().

Because pdev->irq is already disabled and freed and THC interrupts are
masked, wait_event_interruptible_timeout() on dev->write_complete_wait and
dev->swdma_complete_wait can never be woken, stalling each transfer for 1
second until it fails with -ETIMEDOUT.

On timeout, thc_swdma_read() also skips thc_swdma_read_completion(), and
thc_dma_unconfigure() only clears THC_M_PRT_READ_DMA_CNTRL_START on
THC_RXDMA1 and THC_RXDMA2, leaving THC_SWDMA or THC_TXDMA started when
thc_dma_release() unmaps and frees the DMA buffers:

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

Conversely, if an in-flight hidraw GET_REPORT receives its THC_SWDMA_INT
wake-up just before disable_irq(), thc_swdma_read_completion() runs without
serialization against quicki2c_dev_deinit() and restarts THC_RXDMA2 and
unquiesces device interrupts after quicki2c_dev_deinit() has completed.

Calling quicki2c_hid_remove() before disabling the IRQ and running
quicki2c_dev_deinit() avoids starting or completing HID transfers after the
interrupt and hardware have been torn down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-thc-rebase-v2-0-mhun512@gmail.com?part=2