[PATCH v3 0/4] HID: Remove redundant error messages on IRQ request failure

[PATCH v3 0/4] HID: Remove redundant error messages on IRQ request failure

[Pan Chuang]

Commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()") added automatic error logging to
devm_request_threaded_irq() and devm_request_any_context_irq()
via the new devm_request_result() helper, which prints device
name, IRQ number, handler functions, and error code on failure.

Since devm_request_irq() is a static inline wrapper around
devm_request_threaded_irq(), it also benefits from this
automatic logging.

Remove the now-redundant dev_err() and dev_err_probe() calls
in hid drivers that follow these devm_request_*_irq()
functions, as the core now provides more detailed diagnostic
information on failure.

v1 -> v2:
- Added the same change for the intel-quicki2c driver

v2 -> v3:
- amd_sfh: directly return devm_request_irq() result

Pan Chuang (4):
  HID: amd_sfh: Remove redundant dev_err()
  HID: hid-goodix: Remove redundant dev_err()
  HID: intel-ish-hid: ipc: Remove redundant dev_err()
  HID: Intel-thc-hid: Remove redundant dev_err()

 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c              | 13 ++-----------
 drivers/hid/hid-goodix-spi.c                        |  5 +----
 drivers/hid/intel-ish-hid/ipc/pci-ish.c             |  4 +---
 .../hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c |  5 +----
 .../hid/intel-thc-hid/intel-quickspi/pci-quickspi.c |  5 +----
 5 files changed, 6 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] HID: amd_sfh: Remove redundant dev_err()

[Pan Chuang]

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

Signed-off-by: Pan Chuang <panchuang@vivo.com>
Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 4b81cebdc335..4d0a95fbc4e4 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -122,19 +122,10 @@ static irqreturn_t amd_sfh_irq_handler(int irq, void *data)
 
 int amd_sfh_irq_init_v2(struct amd_mp2_dev *privdata)
 {
-	int rc;
-
 	pcim_intx(privdata->pdev, true);
 
-	rc = devm_request_irq(&privdata->pdev->dev, privdata->pdev->irq,
-			      amd_sfh_irq_handler, 0, DRIVER_NAME, privdata);
-	if (rc) {
-		dev_err(&privdata->pdev->dev, "failed to request irq %d err=%d\n",
-			privdata->pdev->irq, rc);
-		return rc;
-	}
-
-	return 0;
+	return devm_request_irq(&privdata->pdev->dev, privdata->pdev->irq,
+				amd_sfh_irq_handler, 0, DRIVER_NAME, privdata);
 }
 
 static int amd_sfh_dis_sts_v2(struct amd_mp2_dev *privdata)
-- 
2.34.1

[PATCH v3 2/4] HID: hid-goodix: Remove redundant dev_err()

[Pan Chuang]

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_threaded_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/hid/hid-goodix-spi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
index 288cb827e9d6..03d549efbdce 100644
--- a/drivers/hid/hid-goodix-spi.c
+++ b/drivers/hid/hid-goodix-spi.c
@@ -722,11 +722,8 @@ static int goodix_spi_probe(struct spi_device *spi)
 	error = devm_request_threaded_irq(&ts->spi->dev, ts->spi->irq,
 					  NULL, goodix_hid_irq, IRQF_ONESHOT,
 					  "goodix_spi_hid", ts);
-	if (error) {
-		dev_err(ts->dev, "could not register interrupt, irq = %d, %d",
-			ts->spi->irq, error);
+	if (error)
 		goto err_destroy_hid;
-	}
 
 	return 0;
 
-- 
2.34.1

[PATCH v3 3/4] HID: intel-ish-hid: ipc: Remove redundant dev_err()

[Pan Chuang]

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

Signed-off-by: Pan Chuang <panchuang@vivo.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 8d36ae96a3ee..e7196e429c8b 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -232,10 +232,8 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ret = devm_request_irq(dev, pdev->irq, ish_irq_handler,
 			       irq_flag, KBUILD_MODNAME, ishtp);
-	if (ret) {
-		dev_err(dev, "ISH: request IRQ %d failed\n", pdev->irq);
+	if (ret)
 		return ret;
-	}
 
 	dev_set_drvdata(ishtp->devc, ishtp);
 
-- 
2.34.1

[PATCH v3 4/4] HID: Intel-thc-hid: Remove redundant dev_err()

[Pan Chuang]

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_threaded_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

Signed-off-by: Pan Chuang <panchuang@vivo.com>
Reviewed-by: Even Xu <even.xu@intel.com>
---
 drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c | 5 +----
 drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c | 5 +----
 2 files changed, 2 insertions(+), 8 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 46d3e9a01999..59f500345acb 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
@@ -682,11 +682,8 @@ static int quicki2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 					quicki2c_irq_thread_handler,
 					IRQF_ONESHOT, KBUILD_MODNAME,
 					qcdev);
-	if (ret) {
-		dev_err_once(&pdev->dev,
-			     "Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
+	if (ret)
 		goto dev_deinit;
-	}
 
 	ret = quicki2c_get_device_descriptor(qcdev);
 	if (ret) {
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 4ae2e1718b30..504ef3c842ab 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -636,11 +636,8 @@ static int quickspi_probe(struct pci_dev *pdev,
 					quickspi_irq_thread_handler,
 					IRQF_ONESHOT, KBUILD_MODNAME,
 					qsdev);
-	if (ret) {
-		dev_err(&pdev->dev,
-			"Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
+	if (ret)
 		goto dev_deinit;
-	}
 
 	ret = reset_tic(qsdev);
 	if (ret) {
-- 
2.34.1

Re: [PATCH v3 4/4] HID: Intel-thc-hid: Remove redundant dev_err()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Requesting the IRQ before allocating report buffers and initializing DMA causes an infinite interrupt storm that hangs the CPU.
- [High] Bypassing `quicki2c_dma_deinit()` on the error path causes a memory leak of non-managed DMA buffers and mappings.
--

--- Patch [4]: [PATCH v3 4/4] HID: Intel-thc-hid: Remove redundant dev_err() ---
commit 7eb5c91c4d9eb3ab7af0dabaab924561ec278776
Author: Pan Chuang <panchuang@vivo.com>

HID: Intel-thc-hid: Remove redundant dev_err()

This patch removes redundant driver-specific dev_err() calls after
devm_request_threaded_irq(). The core genirq/devres code automatically
logs detailed error messages on failure now.

> 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..59f500345acbf 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -682,11 +682,8 @@ static int quicki2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  					quicki2c_irq_thread_handler,
>  					IRQF_ONESHOT, KBUILD_MODNAME,
>  					qcdev);
> -	if (ret) {
> -		dev_err_once(&pdev->dev,
> -			     "Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
> +	if (ret)
>  		goto dev_deinit;
> -	}

[Severity: Critical]
This is a pre-existing issue, but does requesting the IRQ here before allocating
report buffers and initializing DMA cause an interrupt loop?

If a pending or spurious interrupt occurs right after devm_request_threaded_irq()
is called but before quicki2c_alloc_report_buf() and quicki2c_dma_init(), the
RT-priority threaded handler could execute.

The handler would find input_buf is NULL and fail to read the DMA buffer. Any
attempt to recover would fail because dma_init() hasn't run. The handler would
then unmask the interrupt. Because the hardware interrupt condition was never
cleared, it could immediately fire again, preempting the probe thread and
hanging the CPU.

>  
>  	ret = quicki2c_get_device_descriptor(qcdev);
>  	if (ret) {

[Severity: High]
This isn't a bug introduced by this patch, but could the error handling further
down in this function leak unmanaged DMA memory?

After quicki2c_dma_init() successfully allocates DMA buffers, subsequent
failures in thc_interrupt_quiesce(), quicki2c_set_power(), or
quicki2c_reset() jump to the dev_deinit label:

	ret = quicki2c_set_power(qcdev, HIDI2C_ON);
	if (ret) {
		dev_err(&pdev->dev, "Set Power On command failed, ret= %d\n", ret);
		goto dev_deinit;
	}

Jumping to dev_deinit bypasses the dma_deinit label and quicki2c_dma_deinit(),
which leaves the DMA resources allocated.

[ ... ]

> 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 4ae2e1718b30c..504ef3c842ab6 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -636,11 +636,8 @@ static int quickspi_probe(struct pci_dev *pdev,
>  					quickspi_irq_thread_handler,
>  					IRQF_ONESHOT, KBUILD_MODNAME,
>  					qsdev);
> -	if (ret) {
> -		dev_err(&pdev->dev,
> -			"Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
> +	if (ret)
>  		goto dev_deinit;
> -	}

[Severity: Critical]
This is a pre-existing issue, but might requesting the IRQ here before allocating
report buffers and initializing DMA lead to a similar interrupt loop?

Just like in quicki2c_probe(), if an interrupt fires right after
devm_request_threaded_irq() is called but before quickspi_alloc_report_buf()
and quickspi_dma_init(), the handler could fail to recover DMA state. It would
then unmask the interrupt, which immediately fires again, hanging the CPU in
an RT-priority interrupt loop.

>  
>  	ret = reset_tic(qsdev);
>  	if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720084310.210976-1-panchuang@vivo.com?part=4