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

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

[Myeonghun Pak]

This resends v2 with the probe cleanup formatting requested by Even.
There are no functional changes from v2.

Changes in v3:
- Add blank lines in the probe error paths of both drivers (Even).

v2: https://lore.kernel.org/all/20261109-thc-rebase-v2-0-mhun512@gmail.com/

No hardware testing.

Myeonghun Pak (2):
  HID: intel-thc-hid: quickspi: quiesce IRQs during teardown
  HID: intel-thc-hid: quicki2c: quiesce IRQs during teardown

 .../intel-quicki2c/pci-quicki2c.c | 32 ++++++++++++++++++++++++++------
 .../intel-quickspi/pci-quickspi.c | 32 ++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 12 deletions(-)

base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9

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

[Myeonghun Pak]

The managed IRQ can access HID/DMA buffers or queue recovery work while
remove or probe unwind tears down those resources.

Disable and synchronize the IRQ, drain recovery work, then mask THC
interrupts before freeing the IRQ and releasing HID/DMA resources.
Track IRQ/DMA initialization for conditional probe cleanup.

Disable the IRQ before shutdown deinit as well. Keep IRQ disabling and
release outside the shared hibernation helper.

Found by a static analysis tool under development.

Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI driver")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---

Changes in v3:
- Add blank lines in the probe error path (Even).

 drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index bf49d8ce5f07..3aa0ae81053e 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -600,6 +600,8 @@
 			  const struct pci_device_id *id)
 {
 	struct quickspi_device *qsdev;
+	bool dma_initialized = false;
+	bool irq_requested = false;
 	void __iomem *mem_addr;
 	int ret;
 
@@ -652,6 +654,7 @@
 					qsdev);
 	if (ret)
 		goto dev_deinit;
+	irq_requested = true;
 
 	ret = reset_tic(qsdev);
 	if (ret) {
@@ -670,17 +673,18 @@
 		dev_err(&pdev->dev, "Setup THC DMA failed, ret= %d\n", ret);
 		goto dev_deinit;
 	}
+	dma_initialized = true;
 
 	ret = quickspi_get_report_descriptor(qsdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Get report descriptor failed, ret = %d\n", ret);
-		goto dma_deinit;
+		goto dev_deinit;
 	}
 
 	ret = quickspi_hid_probe(qsdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register HID device, ret = %d\n", ret);
-		goto dma_deinit;
+		goto dev_deinit;
 	}
 
 	qsdev->state = QUICKSPI_ENABLED;
@@ -695,10 +699,20 @@
 
 	return 0;
 
-dma_deinit:
-	quickspi_dma_deinit(qsdev);
 dev_deinit:
+	if (irq_requested) {
+		WRITE_ONCE(qsdev->recovery_disabled, true);
+		disable_irq(pdev->irq);
+	}
+
 	quickspi_dev_deinit(qsdev);
+
+	if (irq_requested)
+		devm_free_irq(&pdev->dev, pdev->irq, qsdev);
+
+	if (dma_initialized)
+		quickspi_dma_deinit(qsdev);
+
 disable_pci_device:
 	pci_clear_master(pdev);
 
@@ -721,9 +735,12 @@
 	if (!qsdev)
 		return;
 
+	WRITE_ONCE(qsdev->recovery_disabled, true);
+	disable_irq(pdev->irq);
+	quickspi_dev_deinit(qsdev);
+	devm_free_irq(&pdev->dev, pdev->irq, qsdev);
+
 	quickspi_hid_remove(qsdev);
-
-	quickspi_dev_deinit(qsdev);
 
 	quickspi_dma_deinit(qsdev);
 
@@ -749,6 +766,9 @@
 	qsdev = pci_get_drvdata(pdev);
 	if (!qsdev)
 		return;
+
+	WRITE_ONCE(qsdev->recovery_disabled, true);
+	disable_irq(pdev->irq);
 
 	quickspi_dev_deinit(qsdev);
 

base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
-- 
2.53.0

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

[Myeonghun Pak]

The managed IRQ can access HID/DMA buffers or queue recovery work while
remove or probe unwind tears down those resources.

Disable and synchronize the IRQ, drain recovery work, then mask THC
interrupts before freeing the IRQ and releasing HID/DMA resources.
Track IRQ/DMA initialization for conditional probe cleanup.

Disable the IRQ before shutdown deinit as well. Keep IRQ disabling and
release outside the shared hibernation helper.

Found by a static analysis tool under development.

Fixes: 66b59bfce6d9 ("HID: intel-thc-hid: intel-quicki2c: Complete THC QuickI2C driver")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---

Changes in v3:
- Add blank lines in the probe error path (Even).

 drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

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 29319b6227f3..84e458fdd69c 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
@@ -652,6 +652,8 @@
 {
 	const struct quicki2c_ddata *ddata = (const struct quicki2c_ddata *)id->driver_data;
 	struct quicki2c_device *qcdev;
+	bool dma_initialized = false;
+	bool irq_requested = false;
 	void __iomem *mem_addr;
 	int ret;
 
@@ -704,6 +706,7 @@
 					qcdev);
 	if (ret)
 		goto dev_deinit;
+	irq_requested = true;
 
 	ret = quicki2c_get_device_descriptor(qcdev);
 	if (ret) {
@@ -722,6 +725,7 @@
 		dev_err(&pdev->dev, "Setup THC DMA failed, ret= %d\n", ret);
 		goto dev_deinit;
 	}
+	dma_initialized = true;
 
 	ret = thc_interrupt_quiesce(qcdev->thc_hw, false);
 	if (ret)
@@ -742,13 +746,13 @@
 	ret = quicki2c_get_report_descriptor(qcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Get report descriptor failed, ret = %d\n", ret);
-		goto dma_deinit;
+		goto dev_deinit;
 	}
 
 	ret = quicki2c_hid_probe(qcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register HID device, ret = %d\n", ret);
-		goto dma_deinit;
+		goto dev_deinit;
 	}
 
 	qcdev->state = QUICKI2C_ENABLED;
@@ -763,10 +767,20 @@
 
 	return 0;
 
-dma_deinit:
-	quicki2c_dma_deinit(qcdev);
 dev_deinit:
+	if (irq_requested) {
+		WRITE_ONCE(qcdev->recovery_disabled, true);
+		disable_irq(pdev->irq);
+	}
+
 	quicki2c_dev_deinit(qcdev);
+
+	if (irq_requested)
+		devm_free_irq(&pdev->dev, pdev->irq, qcdev);
+
+	if (dma_initialized)
+		quicki2c_dma_deinit(qcdev);
+
 disable_pci_device:
 	pci_clear_master(pdev);
 
@@ -788,9 +802,12 @@
 	if (!qcdev)
 		return;
 
+	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_dev_deinit(qcdev);
 
 	quicki2c_dma_deinit(qcdev);
 
@@ -814,6 +831,9 @@
 	qcdev = pci_get_drvdata(pdev);
 	if (!qcdev)
 		return;
+
+	WRITE_ONCE(qcdev->recovery_disabled, true);
+	disable_irq(pdev->irq);
 
 	quicki2c_dev_deinit(qcdev);
 

base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: quickspi: broken HID teardown in
  quickspi_remove()
--

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

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

This commit disables and frees the device IRQ and drains recovery work
during probe failure, driver removal, and shutdown before releasing HID
and DMA resources.

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index bf49d8ce5f07b..3aa0ae81053e8 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c

[ ... ]

> @@ -721,9 +735,12 @@ static void quickspi_remove(struct pci_dev *pdev)
>  	if (!qsdev)
>  		return;
>  
> -	quickspi_hid_remove(qsdev);
> -
> +	WRITE_ONCE(qsdev->recovery_disabled, true);
> +	disable_irq(pdev->irq);
>  	quickspi_dev_deinit(qsdev);
> +	devm_free_irq(&pdev->dev, pdev->irq, qsdev);
> +
> +	quickspi_hid_remove(qsdev);
>  
>  	quickspi_dma_deinit(qsdev);

[Severity: Medium]
Does moving disable_irq(), quickspi_dev_deinit(), and devm_free_irq()
ahead of quickspi_hid_remove() break HID teardown?

When a hid-multitouch input device is open during unbind,
quickspi_hid_remove() -> hid_destroy_device() -> ... -> hid_hw_close() ->
mt_on_hid_hw_close() -> mt_set_modes() synchronously calls
quickspi_hid_raw_request() -> quickspi_set_report() ->
write_cmd_to_txdma() -> thc_dma_write(). Because quickspi_hid_raw_request()
does not check qsdev->state and THC_TXDMA remains enabled until
quickspi_dma_deinit(), thc_dma_write() starts hardware TxDMA and blocks in
wait_event_interruptible_timeout(dev->write_complete_wait, ...) waiting
for a THC_TXDMA_INT completion interrupt that can no longer arrive. When
thc_dma_write() times out or is interrupted by a signal,
quickspi_dma_deinit() then unmaps and frees the TxDMA scatterlist and PRD
table without clearing THC_M_PRT_WRITE_DMA_CNTRL_THC_WRDMA_START.

Calling quickspi_hid_remove() before disabling the IRQ and deinitializing
the controller would avoid starting new transfers after interrupts are
disabled.

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

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

[sashiko-bot]

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