[PATCH 0/3] char: Remove redundant error messages on IRQ request failure

[PATCH 0/3] char: 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 char drivers that follow these devm_request_*_irq()
functions, as the core now provides more detailed diagnostic
information on failure.

Pan Chuang (3):
  hwrng: drivers - Remove redundant dev_err()/dev_err_probe()
  tpm: Remove redundant dev_err()
  char: xillybus: Remove redundant dev_err()

 drivers/char/hw_random/airoha-trng.c  | 4 +---
 drivers/char/hw_random/cctrng.c       | 2 +-
 drivers/char/hw_random/imx-rngc.c     | 2 +-
 drivers/char/hw_random/jh7110-trng.c  | 3 +--
 drivers/char/hw_random/omap-rng.c     | 5 +----
 drivers/char/hw_random/xgene-rng.c    | 2 +-
 drivers/char/tpm/st33zp24/st33zp24.c  | 5 +----
 drivers/char/tpm/tpm_i2c_nuvoton.c    | 2 --
 drivers/char/tpm/tpm_tis_i2c_cr50.c   | 4 +---
 drivers/char/xillybus/xillybus_of.c   | 5 +----
 drivers/char/xillybus/xillybus_pcie.c | 5 +----
 11 files changed, 10 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH 1/3] hwrng: drivers - Remove redundant dev_err()/dev_err_probe()

[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() and dev_err_probe() calls.

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/char/hw_random/airoha-trng.c | 4 +---
 drivers/char/hw_random/cctrng.c      | 2 +-
 drivers/char/hw_random/imx-rngc.c    | 2 +-
 drivers/char/hw_random/jh7110-trng.c | 3 +--
 drivers/char/hw_random/omap-rng.c    | 5 +----
 drivers/char/hw_random/xgene-rng.c   | 2 +-
 6 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/char/hw_random/airoha-trng.c b/drivers/char/hw_random/airoha-trng.c
index 076519a2f100..98c131ee9891 100644
--- a/drivers/char/hw_random/airoha-trng.c
+++ b/drivers/char/hw_random/airoha-trng.c
@@ -186,10 +186,8 @@ static int airoha_trng_probe(struct platform_device *pdev)
 	airoha_trng_irq_mask(trng);
 	ret = devm_request_irq(&pdev->dev, irq, airoha_trng_irq, 0,
 			       pdev->name, (void *)trng);
-	if (ret) {
-		dev_err(dev, "Can't get interrupt working.\n");
+	if (ret)
 		return ret;
-	}
 
 	init_completion(&trng->rng_op_done);
 
diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
index a5be9258037f..a6925211c3b5 100644
--- a/drivers/char/hw_random/cctrng.c
+++ b/drivers/char/hw_random/cctrng.c
@@ -509,7 +509,7 @@ static int cctrng_probe(struct platform_device *pdev)
 	/* register the driver isr function */
 	rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng", drvdata);
 	if (rc)
-		return dev_err_probe(dev, rc, "Could not register to interrupt %d\n", irq);
+		return rc;
 	dev_dbg(dev, "Registered to IRQ: %d\n", irq);
 
 	/* Clear all pending interrupts */
diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index 28c56c2d1bf6..bae8cdca13fe 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -296,7 +296,7 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
 			irq, imx_rngc_irq, 0, pdev->name, (void *)rngc);
 	if (ret) {
 		clk_disable_unprepare(rngc->clk);
-		return dev_err_probe(&pdev->dev, ret, "Can't get interrupt working.\n");
+		return ret;
 	}
 
 	if (self_test) {
diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
index 4712c3c530e4..aee12caab578 100644
--- a/drivers/char/hw_random/jh7110-trng.c
+++ b/drivers/char/hw_random/jh7110-trng.c
@@ -303,8 +303,7 @@ static int starfive_trng_probe(struct platform_device *pdev)
 	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
 			       (void *)trng);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "Failed to register interrupt handler\n");
+		return ret;
 
 	trng->hclk = devm_clk_get(&pdev->dev, "hclk");
 	if (IS_ERR(trng->hclk))
diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
index 5e8b50f15db7..327643ba971c 100644
--- a/drivers/char/hw_random/omap-rng.c
+++ b/drivers/char/hw_random/omap-rng.c
@@ -391,11 +391,8 @@ static int of_get_omap_rng_device_details(struct omap_rng_dev *priv,
 
 		err = devm_request_irq(dev, irq, omap4_rng_irq,
 				       IRQF_TRIGGER_NONE, dev_name(dev), priv);
-		if (err) {
-			dev_err(dev, "unable to request irq %d, err = %d\n",
-				irq, err);
+		if (err)
 			return err;
-		}
 
 		/*
 		 * On OMAP4, enabling the shutdown_oflo interrupt is
diff --git a/drivers/char/hw_random/xgene-rng.c b/drivers/char/hw_random/xgene-rng.c
index 1f4b95341c2e..629dc85c3741 100644
--- a/drivers/char/hw_random/xgene-rng.c
+++ b/drivers/char/hw_random/xgene-rng.c
@@ -336,7 +336,7 @@ static int xgene_rng_probe(struct platform_device *pdev)
 	rc = devm_request_irq(&pdev->dev, ctx->irq, xgene_rng_irq_handler, 0,
 				dev_name(&pdev->dev), ctx);
 	if (rc)
-		return dev_err_probe(&pdev->dev, rc, "Could not request RNG alarm IRQ\n");
+		return rc;
 
 	/* Enable IP clock */
 	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
-- 
2.34.1

Re: [PATCH 1/3] hwrng: drivers - Remove redundant dev_err()/dev_err_probe()

[sashiko-bot]

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

Pre-existing issues:
- [High] PM runtime and clock reference leak when the second clock defers probe.
- [High] Device IRQ is requested before hardware clocks and resets are ready.
- [High] Device IRQ is requested before the IP clock is enabled.
- [High] The driver leaks its PM runtime enablement, reset control deassertion, and clock enablements upon driver unbind because it lacks a `remove` function.
- [High] The driver queues background works (`compwork` and `startwork`) but fails to cancel them during driver removal, leading to a Use-After-Free.
- [High] The driver starts a failure recovery timer but fails to delete it when the driver is removed, leading to a Use-After-Free.
- [Medium] Clock prepare/enable leak on error path during probe.
- [Medium] PM runtime reference leak on error path during probe.
--

--- Patch [1]: [PATCH 1/3] hwrng: drivers - Remove redundant dev_err()/dev_err_probe() ---
commit 5b0e3802decb80a4bd7c81e612150dfe5af3cc8a
Author: Pan Chuang <panchuang@vivo.com>

hwrng: drivers - Remove redundant dev_err()/dev_err_probe()
    
This patch removes driver-specific dev_err() and dev_err_probe() calls
following devm_request_irq() since the core now automatically logs these
errors.

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
> @@ -509,7 +509,7 @@ static int cctrng_probe(struct platform_device *pdev)
>  	/* register the driver isr function */
>  	rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng", drvdata);
>  	if (rc)
> -		return dev_err_probe(dev, rc, "Could not register to interrupt %d\n", irq);
> +		return rc;

[Severity: Medium]
This is a pre-existing issue, but does this error path leave the PM runtime
reference count elevated?

In cctrng_probe(), cc_trng_pm_get() increments the PM runtime usage count.
If devm_hwrng_register() subsequently fails, the code jumps to post_pm_err:

drivers/char/hw_random/cctrng.c:cctrng_probe() {
    ...
    rc = devm_hwrng_register(dev, &drvdata->rng);
    if (rc) {
        dev_err(dev, "Could not register hwrng device.\n");
        goto post_pm_err;
    }
    ...
post_pm_err:
    cc_trng_pm_fini(drvdata);

    return rc;
}

Since cc_trng_pm_fini() only disables PM runtime but doesn't drop the usage
count, does this permanently prevent the device's power domain from ever
suspending?

[Severity: High]
This is a pre-existing issue, but does the driver queue background works that
fail to cancel during driver removal?

The driver schedules asynchronous works (compwork and startwork) during
hardware operation. The teardown function only disables PM runtime:

drivers/char/hw_random/cctrng.c:cctrng_remove() {
    ...
    cc_trng_pm_fini(drvdata);

    dev_info(dev, "ARM cctrng device terminated\n");
}

Does this lead to a Use-After-Free if a work item is pending or executing
when the driver is unbound, since the devres framework will automatically
free the drvdata context memory?

> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -297,7 +297,7 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
>  			irq, imx_rngc_irq, 0, pdev->name, (void *)rngc);
>  	if (ret) {
>  		clk_disable_unprepare(rngc->clk);
> -		return dev_err_probe(&pdev->dev, ret, "Can't get interrupt working.\n");
> +		return ret;
>  	}

[Severity: Medium]
This is a pre-existing issue, but does this error path fail to undo the
previous manual clock enablement?

In imx_rngc_probe(), rngc->clk is explicitly enabled via clk_prepare_enable().
If the subsequent call to devm_hwrng_register() fails, the function directly
returns the error:

drivers/char/hw_random/imx-rngc.c:imx_rngc_probe() {
    ...
    ret = devm_hwrng_register(&pdev->dev, &rngc->rng);
    if (ret)
        return dev_err_probe(&pdev->dev, ret, "hwrng registration failed\n");
    ...
}

Does this leave the hardware clock perpetually enabled without calling
clk_disable_unprepare(rngc->clk)?

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c
> @@ -303,8 +303,7 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
>  			       (void *)trng);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "Failed to register interrupt handler\n");
> +		return ret;
>  
>  	trng->hclk = devm_clk_get(&pdev->dev, "hclk");

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ before the
hardware clocks and resets are ready?

If a shared or pending interrupt triggers immediately upon registration,
starfive_trng_irq() could execute and read STARFIVE_ISTAT while the hardware
is unpowered and in reset.

Could this lead to a synchronous external abort when the unclocked device
registers are accessed?

[Severity: High]
This is a pre-existing issue, but does the platform driver leak resources
upon driver unbind?

The driver registers with module_platform_driver() without a remove callback:

drivers/char/hw_random/jh7110-trng.c {
static struct platform_driver starfive_trng_driver = {
    .probe = starfive_trng_probe,
    .driver = {
        .name = "jh7110-trng",
...
}

During starfive_trng_probe(), the driver manually enables clocks, deasserts
reset, and enables PM runtime.

Since there is no remove callback, does devm automatically unregister the
hwrng while leaving the manually acquired clocks, reset, and PM runtime
states permanently active?

> diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
> --- a/drivers/char/hw_random/omap-rng.c
> +++ b/drivers/char/hw_random/omap-rng.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the immediate return on -EPROBE_DEFER
bypass manual resource cleanup?

In omap_rng_probe(), PM runtime is enabled and priv->clk is manually enabled.
The driver then attempts to get the second clock:

drivers/char/hw_random/omap-rng.c:omap_rng_probe() {
    ...
    priv->clk_reg = devm_clk_get(&pdev->dev, "reg");
    if (PTR_ERR(priv->clk_reg) == -EPROBE_DEFER)
        return -EPROBE_DEFER;
    ...
}

Does this early return bypass the err_register and err_ioremap cleanup labels,
leaving pm_runtime_put_sync(), pm_runtime_disable(), and
clk_disable_unprepare(priv->clk) uncalled?

> diff --git a/drivers/char/hw_random/xgene-rng.c b/drivers/char/hw_random/xgene-rng.c
> --- a/drivers/char/hw_random/xgene-rng.c
> +++ b/drivers/char/hw_random/xgene-rng.c
> @@ -337,7 +337,7 @@ static int xgene_rng_probe(struct platform_device *pdev)
>  	rc = devm_request_irq(&pdev->dev, ctx->irq, xgene_rng_irq_handler, 0,
>  				dev_name(&pdev->dev), ctx);
>  	if (rc)
> -		return dev_err_probe(&pdev->dev, rc, "Could not request RNG alarm IRQ\n");
> +		return rc;
>  
>  	/* Enable IP clock */
>  	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);

[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the IP clock is
enabled?

An early interrupt execution during this window could cause the handler to
read the RNG_INTR_STS_ACK register.

Could accessing an unclocked APB/AHB register lead to a system crash or bus
error?

[Severity: High]
This is a pre-existing issue, but does the driver start a failure recovery
timer that fails to get deleted when the driver is removed?

The teardown function returns without canceling the asynchronous
failure_timer:

drivers/char/hw_random/xgene-rng.c:xgene_rng_remove() {
    ...
    rc = device_init_wakeup(&pdev->dev, 0);
    if (rc)
        dev_err(&pdev->dev, "RNG init wakeup failed error %d\n", rc);
}

After remove() completes, the devres framework automatically frees the
xgene_rng_dev context. If the timer later expires, could
xgene_rng_expired_timer() execute and dereference the freed memory?

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