Skip to contentSkip to conversation

crypto: Remove redundant error messages on IRQ request failure

PATCH In review Pan Chuang 6 participants 20 messages Jul 17 – Jul 27

Subsystems (21)
lore
Share & tools
lore

Pan Chuang

author
#0 lore raw

cover crypto: Remove redundant error messages on IRQ request failure

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 crypto drivers that follow these devm_request_*_irq() functions, as the core now provides more detailed diagnostic information on failure.

Pan Chuang (12):
crypto: allwinner - Remove redundant dev_err()
crypto: amlogic - Remove redundant dev_err()
crypto: aspeed - Remove redundant dev_err()
crypto: Remove redundant dev_err()/dev_err_probe()
crypto: caam - Remove redundant dev_err()
crypto: ccree - Remove redundant dev_err()
crypto: sl3516 - Remove redundant dev_err()
crypto: safexcel - Remove redundant dev_err()
crypto: keembay - Remove redundant dev_err()
crypto: octeontx2 - Remove redundant dev_err()
crypto: rockchip - Remove redundant dev_err()
crypto: stm32 - Remove redundant dev_err()

 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   | 4 +---
 drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c   | 4 +---
 drivers/crypto/amlogic/amlogic-gxl-core.c           | 4 +---
 drivers/crypto/aspeed/aspeed-acry.c                 | 4 +---
 drivers/crypto/aspeed/aspeed-hace.c                 | 4 +---
 drivers/crypto/atmel-aes.c                          | 4 +---
 drivers/crypto/atmel-sha.c                          | 4 +---
 drivers/crypto/atmel-tdes.c                         | 4 +---
 drivers/crypto/caam/jr.c                            | 2 --
 drivers/crypto/ccree/cc_driver.c                    | 4 +---
 drivers/crypto/gemini/sl3516-ce-core.c              | 4 +---
 drivers/crypto/img-hash.c                           | 4 +---
 drivers/crypto/inside-secure/safexcel.c             | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-aes-core.c | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-ecc.c      | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-hcu-core.c | 4 +---
 drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c  | 5 +----
 drivers/crypto/mxs-dcp.c                            | 8 ++------
 drivers/crypto/omap-aes.c                           | 4 +---
 drivers/crypto/omap-des.c                           | 4 +---
 drivers/crypto/omap-sham.c                          | 5 +----
 drivers/crypto/rockchip/rk3288_crypto.c             | 4 +---
 drivers/crypto/sahara.c                             | 3 +--
 drivers/crypto/stm32/stm32-cryp.c                   | 4 +---
 drivers/crypto/stm32/stm32-hash.c                   | 4 +---
 25 files changed, 25 insertions(+), 78 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/12 crypto: allwinner - Remove redundant dev_err()

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.

---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 4 +---
 drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c +1 -3
index c6402e87f8a0..21762bd408e5 100644
@@ -976,10 +976,8 @@ static int sun8i_ce_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0,
 			       "sun8i-ce-ns", ce);
-	if (err) {
-		dev_err(ce->dev, "Cannot request CryptoEngine Non-secure IRQ (err=%d)\n", err);
+	if (err)
 		goto error_pm;
-	}
 
 	err = sun8i_ce_register_algs(ce);
 	if (err)
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c +1 -3
index 0b22fcddb882..2167dd9f44c7 100644
@@ -824,10 +824,8 @@ static int sun8i_ss_probe(struct platform_device *pdev)
 		goto error_pm;
 
 	err = devm_request_irq(&pdev->dev, irq, ss_irq_handler, 0, "sun8i-ss", ss);
-	if (err) {
-		dev_err(ss->dev, "Cannot request SecuritySystem IRQ (err=%d)\n", err);
+	if (err)
 		goto error_irq;
-	}
 
 	err = sun8i_ss_register_algs(ss);
 	if (err)
Signature · 1 line
2.34.1
#2 lore raw

Re: [PATCH 01/12] crypto: allwinner - Remove redundant dev_err()

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

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

Pre-existing issues:
- [High] Uninitialized list dereference (kernel panic) due to ignored TRNG registration failure.
- [High] List corruption and kernel panic due to dangling pointers in global algorithm arrays during failed re-binds. --

--- Patch [1]: [PATCH 01/12] crypto: allwinner - Remove redundant dev_err() --- commit 19d9808368b2fa17e987058da2afc72772963840
Author: Pan Chuang <[email protected]>
Subject: crypto: allwinner - Remove redundant dev_err()

This patch removes redundant driver-specific dev_err() messages after devm_request_irq() calls. Since commit 55b48e23f5c4, devm_request_irq() automatically logs detailed error messages on failure, making these additional prints unnecessary.

Quoted 6 lines
drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c +0 -0
index f3b58ed6aed0d..a1cc59a2ce3f7 100644
@@ -1039,10 +1039,8 @@ static int sun8i_ce_probe(struct platform_device *pdev)
 

drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c static int sun8i_ce_probe(struct platform_device *pdev) · line 1046

Quoted 10 lines
drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c +1 -3
 	err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0,
 			       "sun8i-ce-ns", ce);
-	if (err) {
-		dev_err(ce->dev, "Cannot request CryptoEngine Non-secure IRQ (err=%d)\n", err);
+	if (err)
 		goto error_pm;
-	}
 
 	err = sun8i_ce_register_algs(ce);
 	if (err)

[Severity: High]
This is a pre-existing issue, but does probe proceed successfully even if hardware RNG registration fails?

A bit further down in sun8i_ce_probe() in drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c, the return value of sun8i_ce_hwrng_register() is ignored:

#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_TRNG
	sun8i_ce_hwrng_register(ce);

#endif

If registration fails (e.g., due to a duplicate name via -EEXIST from a mock device, or sysfs unbind/bind races), probe proceeds. Later, upon unbinding or module removal, sun8i_ce_remove() unconditionally calls sun8i_ce_hwrng_unregister() which triggers:

drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c:sun8i_ce_hwrng_unregister() {
    ...
	if (ce->variant->trng == CE_ID_NOTSUPP)
		return;
	hwrng_unregister(&ce->trng);
    ...
}

Because the device was never successfully added to the core list, its list_head remains zero-initialized (from devm_kzalloc). Calling hwrng_unregister() executes list_del(), which attempts to dereference the NULL prev and next pointers, causing an immediate kernel oops.

Could the return value of sun8i_ce_hwrng_register() be checked and handled appropriately?

[Severity: High]
This is another pre-existing issue, but does sun8i_ce_unregister_algs() leave dangling pointers in the global ce_algs array?

When algorithms are unregistered, the device instance pointers are not cleared:

drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:sun8i_ce_unregister_algs() {
    ...
		case CRYPTO_ALG_TYPE_SKCIPHER:
			dev_info(ce->dev, "Unregister %d %s\n", i,
				 ce_algs[i].alg.skcipher.base.base.cra_name);
			crypto_engine_unregister_skcipher(&ce_algs[i].alg.skcipher);
			break;
    ...
}

If a device is successfully bound and then unbound, these global arrays are left with dangling pointers to the freed device structure. On a subsequent probe of the device, if algorithm registration fails partway through (such as if sun8i_ce_register_algs() fails as seen in the diff context above), the error path calls the unregister function.

The loop sees the dangling pointers from the previous successful bind and attempts to call crypto_engine_unregister_skcipher() on them again. This causes a double-unregister of the same algorithm template, resulting in list corruption.

Could ce_algs[i].ce be set to NULL after unregistering each algorithm?

Quoted 6 lines
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c +0 -0
index 59c9bc45ec0f2..c994e023afbc4 100644
@@ -869,10 +869,8 @@ static int sun8i_ss_probe(struct platform_device *pdev)
 		goto error_pm;

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c static int sun8i_ss_probe(struct platform_device *pdev) · line 876

Quoted 10 lines
drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c +1 -3
 
 	err = devm_request_irq(&pdev->dev, irq, ss_irq_handler, 0, "sun8i-ss", ss);
-	if (err) {
-		dev_err(ss->dev, "Cannot request SecuritySystem IRQ (err=%d)\n", err);
+	if (err)
 		goto error_irq;
-	}
 
 	err = sun8i_ss_register_algs(ss);
 	if (err)

[Severity: High]
This is a pre-existing issue, but similar to the sun8i-ce driver above, can sun8i_ss_unregister_algs() cause a double-free?

drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c:sun8i_ss_unregister_algs() {
    ...
		case CRYPTO_ALG_TYPE_SKCIPHER:
			dev_info(ss->dev, "Unregister %d %s\n", i,
				 ss_algs[i].alg.skcipher.base.base.cra_name);
			crypto_engine_unregister_skcipher(&ss_algs[i].alg.skcipher);
			break;
    ...
}

Failing to clear ss_algs[i].ss to NULL after unregistering the algorithm leaves a dangling pointer for future probes, which can lead to list corruption and a double-free in the crypto core during failed re-binds.

Could ss_algs[i].ss be set to NULL after unregistering each algorithm here as well?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author
#3 lore raw

PATCH 2/12 crypto: amlogic - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/amlogic/amlogic-gxl-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/crypto/amlogic/amlogic-gxl-core.c +1 -3
index 6cb33949915f..169c6eeb51e5 100644
@@ -257,10 +257,8 @@ static int meson_crypto_probe(struct platform_device *pdev)
 
 		err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
 				       "gxl-crypto", mc);
-		if (err < 0) {
-			dev_err(mc->dev, "Cannot request IRQ for flow %d\n", i);
+		if (err < 0)
 			return err;
-		}
 	}
 
 	err = clk_prepare_enable(mc->busclk);
Signature · 1 line
2.34.1
#4 lore raw

Re: [PATCH 02/12] crypto: amlogic - Remove redundant dev_err()

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

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

Pre-existing issues:
- [High] IRQs are requested via devm_request_irq before the device clock is enabled and the channel list is allocated.
- [High] The driver modifies a shared static global array of algorithm templates during probe, preventing multiple device instances and corrupting crypto API state.
- [High] DMA mappings are leaked in meson_cipher when scatterlist mapping fails or exceeds the maximum descriptor count.
- [Medium] The IRQ handler unconditionally returns IRQ_HANDLED even for spurious interrupts.
--

--- Patch [2]: [PATCH 02/12] crypto: amlogic - Remove redundant dev_err() --- commit 82c5002d2a0028a21e2688dd2106e4d410487f9e
Author: Pan Chuang <[email protected]>

crypto: amlogic - Remove redundant dev_err()

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.

drivers/crypto/amlogic/amlogic-gxl-core.c static int meson_crypto_probe(struct platform_device *pdev) · line 261

Quoted 12 lines
drivers/crypto/amlogic/amlogic-gxl-core.c +1 -2
index 6cb33949915fe..169c6eeb51e51 100644
@@ -257,10 +257,8 @@ static int meson_crypto_probe(struct platform_device *pdev)
 
 		err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
 				       "gxl-crypto", mc);
-		if (err < 0) {
-			dev_err(mc->dev, "Cannot request IRQ for flow %d\n", i);
+		if (err < 0)
 			return err;

[Severity: High]
This is a pre-existing issue, but does requesting the interrupt at this point risk a crash if an interrupt fires immediately?

If an interrupt fires between devm_request_irq() and the subsequent calls to clk_prepare_enable() or meson_allocate_chanlist(), the handler will run before resources are initialized.

In drivers/crypto/amlogic/amlogic-gxl-core.c:meson_irq_handler():

    p = readl(mc->base + ((0x04 + flow) << 2));
    if (p) {
        writel_relaxed(0xF, mc->base + ((0x4 + flow) << 2));
        mc->chanlist[flow].status = 1;

The handler performs a readl() on unclocked device registers, which can cause a synchronous external abort or bus hang. It also accesses mc->chanlist without checking if it is allocated, which could cause a null pointer dereference.

[Severity: Medium]
This is a pre-existing issue, but does meson_irq_handler() unconditionally return IRQ_HANDLED even for spurious interrupts?

Looking at drivers/crypto/amlogic/amlogic-gxl-core.c:meson_irq_handler():

    dev_err(mc->dev, "%s %d from unknown irq\n", __func__, irq);
    return IRQ_HANDLED;

If an unhandled interrupt occurs, returning IRQ_HANDLED prevents the core interrupt subsystem's "nobody cared" logic from disabling a stuck interrupt line. Could this result in an interrupt storm locking up a processor if the hardware malfunctions?

drivers/crypto/amlogic/amlogic-gxl-core.c static int meson_crypto_probe(struct platform_device *pdev) · line 264

Quoted 4 lines
drivers/crypto/amlogic/amlogic-gxl-core.c +0 -1
-		}
 	}
 
 	err = clk_prepare_enable(mc->busclk);

[Severity: High]
This is a pre-existing issue, but does the driver modify a shared static global array during probe?

Looking at meson_register_algs() which is called shortly after in meson_crypto_probe():

In drivers/crypto/amlogic/amlogic-gxl-core.c:meson_register_algs():

    mc_algs[i].mc = mc;
    switch (mc_algs[i].type) {
    case CRYPTO_ALG_TYPE_SKCIPHER:
        err = crypto_engine_register_skcipher(&mc_algs[i].alg.skcipher);

Modifying the shared global static array mc_algs with device-specific pointers prevents multiple device instances from functioning correctly. Probing multiple instances or unbinding and rebinding the device will mutate this shared state, which might corrupt the linked lists within the kernel's crypto subsystem.

[Severity: High]
This is a pre-existing issue, but does meson_cipher() leak direct memory access mappings when scatterlist mapping fails?

In drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher():

    if (!nr_sgs || nr_sgs > MAXDESC - 3) {
        dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
        err = -EINVAL;
        goto theend;
    }

And in the error path:

    theend:
        kfree_sensitive(bkeyiv);
        kfree_sensitive(backup_iv);

        return err;

The error path frees memory but does not call dma_unmap_single() for the previously mapped phykeyiv, nor dma_unmap_sg() for the already mapped areq->src. Can this permanently leak mapping resources until exhaustion if fragmented scatterlists are submitted?

Signature · 6 lines
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic
author
#5 lore raw

PATCH 3/12 crypto: aspeed - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/aspeed/aspeed-acry.c | 4 +---
 drivers/crypto/aspeed/aspeed-hace.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/crypto/aspeed/aspeed-acry.c +1 -3
index 5993bcba9716..8d13f01e7a37 100644
@@ -732,10 +732,8 @@ static int aspeed_acry_probe(struct platform_device *pdev)
 
 	rc = devm_request_irq(dev, acry_dev->irq, aspeed_acry_irq, 0,
 			      dev_name(dev), acry_dev);
-	if (rc) {
-		dev_err(dev, "Failed to request irq.\n");
+	if (rc)
 		return rc;
-	}
 
 	acry_dev->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(acry_dev->clk)) {
drivers/crypto/aspeed/aspeed-hace.c +1 -3
index 3fe644bfe037..6677cba666d7 100644
@@ -131,10 +131,8 @@ static int aspeed_hace_probe(struct platform_device *pdev)
 
 	rc = devm_request_irq(&pdev->dev, hace_dev->irq, aspeed_hace_irq, 0,
 			      dev_name(&pdev->dev), hace_dev);
-	if (rc) {
-		dev_err(&pdev->dev, "Failed to request interrupt\n");
+	if (rc)
 		return rc;
-	}
 
 	/* Get clk and enable it */
 	hace_dev->clk = devm_clk_get(&pdev->dev, NULL);
Signature · 1 line
2.34.1
maintainer Reviewed-by on v1 3/12
#6 lore raw

Re: [PATCH 03/12] crypto: aspeed - Remove redundant dev_err()

Quoted 6 lines · 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.

author
#7 lore raw

PATCH 4/12 crypto: Remove redundant dev_err()/dev_err_probe()

In reply to 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.

---
 drivers/crypto/atmel-aes.c  | 4 +---
 drivers/crypto/atmel-sha.c  | 4 +---
 drivers/crypto/atmel-tdes.c | 4 +---
 drivers/crypto/img-hash.c   | 4 +---
 drivers/crypto/mxs-dcp.c    | 8 ++------
 drivers/crypto/omap-aes.c   | 4 +---
 drivers/crypto/omap-des.c   | 4 +---
 drivers/crypto/omap-sham.c  | 5 +----
 drivers/crypto/sahara.c     | 3 +--
 9 files changed, 10 insertions(+), 30 deletions(-)
drivers/crypto/atmel-aes.c +1 -3
index 30acfd937d54..50d97cef75b8 100644
@@ -2364,10 +2364,8 @@ static int atmel_aes_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, aes_dd->irq, atmel_aes_irq,
 			       IRQF_SHARED, "atmel-aes", aes_dd);
-	if (err) {
-		dev_err(dev, "unable to request aes irq.\n");
+	if (err)
 		goto err_tasklet_kill;
-	}
 
 	/* Initializing the clock */
 	aes_dd->iclk = devm_clk_get_prepared(&pdev->dev, "aes_clk");
drivers/crypto/atmel-sha.c +1 -3
index 66323ac63406..48f54b615504 100644
@@ -2604,10 +2604,8 @@ static int atmel_sha_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, sha_dd->irq, atmel_sha_irq,
 			       IRQF_SHARED, "atmel-sha", sha_dd);
-	if (err) {
-		dev_err(dev, "unable to request sha irq.\n");
+	if (err)
 		goto err_tasklet_kill;
-	}
 
 	/* Initializing the clock */
 	sha_dd->iclk = devm_clk_get_prepared(&pdev->dev, "sha_clk");
drivers/crypto/atmel-tdes.c +1 -3
index 48cd537f001e..4511166fd74d 100644
@@ -978,10 +978,8 @@ static int atmel_tdes_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, tdes_dd->irq, atmel_tdes_irq,
 			       IRQF_SHARED, "atmel-tdes", tdes_dd);
-	if (err) {
-		dev_err(dev, "unable to request tdes irq.\n");
+	if (err)
 		goto err_tasklet_kill;
-	}
 
 	/* Initializing the clock */
 	tdes_dd->iclk = devm_clk_get(&pdev->dev, "tdes_clk");
drivers/crypto/img-hash.c +1 -3
index 0f19dcc2f388..1ccfc0eccc73 100644
@@ -966,10 +966,8 @@ static int img_hash_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(dev, irq, img_irq_handler, 0,
 			       dev_name(dev), hdev);
-	if (err) {
-		dev_err(dev, "unable to request irq\n");
+	if (err)
 		goto res_err;
-	}
 	dev_dbg(dev, "using IRQ channel %d\n", irq);
 
 	hdev->hash_clk = devm_clk_get_enabled(&pdev->dev, "hash");
drivers/crypto/mxs-dcp.c +2 -6
index 595b2fd84667..c75a06999787 100644
@@ -1085,17 +1085,13 @@ static int mxs_dcp_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, dcp_vmi_irq, mxs_dcp_irq, 0,
 			       "dcp-vmi-irq", sdcp);
-	if (ret) {
-		dev_err(dev, "Failed to claim DCP VMI IRQ!\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = devm_request_irq(dev, dcp_irq, mxs_dcp_irq, 0,
 			       "dcp-irq", sdcp);
-	if (ret) {
-		dev_err(dev, "Failed to claim DCP IRQ!\n");
+	if (ret)
 		return ret;
-	}
 
 	/* Allocate coherent helper block. */
 	sdcp->coh = devm_kzalloc(dev, sizeof(*sdcp->coh) + DCP_ALIGNMENT,
drivers/crypto/omap-aes.c +1 -3
index f31555c0d715..711909de2013 100644
@@ -1172,10 +1172,8 @@ static int omap_aes_probe(struct platform_device *pdev)
 
 		err = devm_request_irq(dev, irq, omap_aes_irq, 0,
 				dev_name(dev), dd);
-		if (err) {
-			dev_err(dev, "Unable to grab omap-aes IRQ\n");
+		if (err)
 			goto err_irq;
-		}
 	}
 
 	spin_lock_init(&dd->lock);
drivers/crypto/omap-des.c +1 -3
index dfaf831dd8ec..3b7104cb4487 100644
@@ -1017,10 +1017,8 @@ static int omap_des_probe(struct platform_device *pdev)
 
 		err = devm_request_irq(dev, irq, omap_des_irq, 0,
 				dev_name(dev), dd);
-		if (err) {
-			dev_err(dev, "Unable to grab omap-des IRQ\n");
+		if (err)
 			goto err_irq;
-		}
 	}
 
 
drivers/crypto/omap-sham.c +1 -4
index be1ac640ee59..8f95c381f4c8 100644
@@ -2092,11 +2092,8 @@ static int omap_sham_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(dev, dd->irq, dd->pdata->intr_hdlr,
 			       IRQF_TRIGGER_NONE, dev_name(dev), dd);
-	if (err) {
-		dev_err(dev, "unable to request irq %d, err = %d\n",
-			dd->irq, err);
+	if (err)
 		goto data_err;
-	}
 
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_SLAVE, mask);
drivers/crypto/sahara.c +1 -2
index 533080b0cddc..d586f1315de1 100644
@@ -1305,8 +1305,7 @@ static int sahara_probe(struct platform_device *pdev)
 	err = devm_request_irq(&pdev->dev, irq, sahara_irq_handler,
 			       0, dev_name(&pdev->dev), dev);
 	if (err)
-		return dev_err_probe(&pdev->dev, err,
-				     "failed to request irq\n");
+		return err;
 
 	/* clocks */
 	dev->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
Signature · 1 line
2.34.1
#8 lore raw

Re: [PATCH 04/12] crypto: Remove redundant dev_err()/dev_err_probe()

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

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

Pre-existing issues:
- [High] Premature IRQ registration before clocks and driver state are initialized --

--- Patch [4]: [PATCH 04/12] crypto: Remove redundant dev_err()/dev_err_probe() --- commit 1a1461364ed2dcfb8ce0bfe3945b90a7c3d3835d
Author: Pan Chuang <[email protected]>
Subject: crypto: Remove redundant dev_err()/dev_err_probe()

Since devm_request_irq() automatically logs detailed error messages on failure, this patch removes the now-redundant driver-specific dev_err() and dev_err_probe() calls across multiple crypto drivers.

drivers/crypto/atmel-aes.c static int atmel_aes_probe(struct platform_device *pdev) · line 2371

Quoted 8 lines
drivers/crypto/atmel-aes.c +0 -0
index b393689400b4c..1bf00df864e8f 100644
@@ -2369,10 +2369,8 @@ static int atmel_aes_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, aes_dd->irq, atmel_aes_irq,
 			       IRQF_SHARED, "atmel-aes", aes_dd);

[Severity: High]
This is a pre-existing issue, but can this premature IRQ request cause a crash?

Since IRQF_SHARED is used here, if another device sharing this IRQ line triggers an interrupt before the aes_clk is enabled, atmel_aes_irq() will unconditionally read from the AES_ISR register:

    atmel_aes_irq()
       reg = atmel_aes_read(aes_dd, AES_ISR);

Could this result in a bus fault because the hardware is still unclocked?

Quoted 8 lines
drivers/crypto/atmel-aes.c +1 -3
-	if (err) {
-		dev_err(dev, "unable to request aes irq.\n");
+	if (err)
 		goto err_tasklet_kill;
-	}
 
 	/* Initializing the clock */
 	aes_dd->iclk = devm_clk_get_prepared(&pdev->dev, "aes_clk");

[ ... ]

drivers/crypto/mxs-dcp.c static int mxs_dcp_probe(struct platform_device *pdev) · line 1087

Quoted 8 lines
drivers/crypto/mxs-dcp.c +0 -0
index 133ebc9982362..49cd6b036a86c 100644
@@ -1085,17 +1085,13 @@ static int mxs_dcp_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, dcp_vmi_irq, mxs_dcp_irq, 0,
 			       "dcp-vmi-irq", sdcp);

[Severity: High]
This is a pre-existing issue, but does this request the IRQ too early?

If a pending interrupt from the bootloader fires immediately here, mxs_dcp_irq() might access uninitialized data structures, as the locks in sdcp->completion aren't initialized until later in the mxs_dcp_probe() function.

Quoted 5 lines
drivers/crypto/mxs-dcp.c +1 -3
-	if (ret) {
-		dev_err(dev, "Failed to claim DCP VMI IRQ!\n");
+	if (ret)
 		return ret;
-	}

[ ... ]

drivers/crypto/omap-aes.c static int omap_aes_probe(struct platform_device *pdev) · line 1174

Quoted 8 lines
drivers/crypto/omap-aes.c +0 -0
index f31555c0d715b..711909de20130 100644
@@ -1172,10 +1172,8 @@ static int omap_aes_probe(struct platform_device *pdev)
 
 		err = devm_request_irq(dev, irq, omap_aes_irq, 0,
 				dev_name(dev), dd);

[Severity: High]
This is a pre-existing issue, but could this lead to a BUG_ON crash?

The IRQ is requested after the device is suspended via pm_runtime_put_sync(), and dd->in_sg is still NULL. If a pending interrupt fires right after this registration, could the handler try to access the suspended device or trip a BUG_ON over the NULL scatterlist?

Quoted 6 lines
drivers/crypto/omap-aes.c +1 -3
-		if (err) {
-			dev_err(dev, "Unable to grab omap-aes IRQ\n");
+		if (err)
 			goto err_irq;
-		}
 	}

[ ... ]

drivers/crypto/sahara.c static int sahara_probe(struct platform_device *pdev) · line 1306

Quoted 7 lines
drivers/crypto/sahara.c +0 -0
index 533080b0cddc9..d586f1315de1f 100644
@@ -1305,8 +1305,7 @@ static int sahara_probe(struct platform_device *pdev)
 	err = devm_request_irq(&pdev->dev, irq, sahara_irq_handler,
 			       0, dev_name(&pdev->dev), dev);

[Severity: High]
This is a pre-existing issue, but can the handler execute before the clocks are ready?

The ipg and ahb clocks are retrieved and enabled after this IRQ registration. If a bootloader left an interrupt pending, could sahara_irq_handler() attempt to access unclocked hardware registers?

Quoted 7 lines
drivers/crypto/sahara.c +1 -2
 	if (err)
-		return dev_err_probe(&pdev->dev, err,
-				     "failed to request irq\n");
+		return err;
 
 	/* clocks */
 	dev->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
author
#9 lore raw

PATCH 5/12 crypto: caam - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/caam/jr.c | 2 --
 1 file changed, 2 deletions(-)
drivers/crypto/caam/jr.c +0 -2
index bddeaaaca487..e4fab3a3112e 100644
@@ -566,8 +566,6 @@ static int caam_jr_init(struct device *dev)
 	error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
 				 dev_name(dev), dev);
 	if (error) {
-		dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
-			jrp->ridx, jrp->irq);
 		tasklet_kill(&jrp->irqtask);
 	}
 
Signature · 1 line
2.34.1
author
#10 lore raw

PATCH 6/12 crypto: ccree - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/ccree/cc_driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/crypto/ccree/cc_driver.c +1 -3
index 061e68a31c36..28a23cd572d6 100644
@@ -471,10 +471,8 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	/* register the driver isr function */
 	rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "ccree",
 			      new_drvdata);
-	if (rc) {
-		dev_err(dev, "Could not register to interrupt %d\n", irq);
+	if (rc)
 		goto post_pm_err;
-	}
 	dev_dbg(dev, "Registered to IRQ: %d\n", irq);
 
 	init_cc_cache_params(new_drvdata);
Signature · 1 line
2.34.1
author
#11 lore raw

PATCH 7/12 crypto: sl3516 - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/gemini/sl3516-ce-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/crypto/gemini/sl3516-ce-core.c +1 -3
index f7e0e3fea15c..c1df5f96ea42 100644
@@ -414,10 +414,8 @@ static int sl3516_ce_probe(struct platform_device *pdev)
 		return irq;
 
 	err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, "crypto", ce);
-	if (err) {
-		dev_err(ce->dev, "Cannot request Crypto Engine IRQ (err=%d)\n", err);
+	if (err)
 		return err;
-	}
 
 	ce->reset = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(ce->reset))
Signature · 1 line
2.34.1
maintainer Reviewed-by on v1 7/12
#12 lore raw

Re: [PATCH 07/12] crypto: sl3516 - Remove redundant dev_err()

Quoted 6 lines · 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.

Yours,
Linus Walleij

author
#13 lore raw

PATCH 8/12 crypto: safexcel - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/inside-secure/safexcel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/crypto/inside-secure/safexcel.c +1 -3
index 52809e57361a..71b920e17e98 100644
@@ -1172,10 +1172,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid,
 	ret = devm_request_threaded_irq(dev, irq, handler,
 					threaded_handler, IRQF_ONESHOT,
 					dev_name(dev), ring_irq_priv);
-	if (ret) {
-		dev_err(dev, "unable to request IRQ %d\n", irq);
+	if (ret)
 		return ret;
-	}
 
 	/* Set affinity */
 	cpu = cpumask_local_spread(ring_id, NUMA_NO_NODE);
Signature · 1 line
2.34.1
author
#14 lore raw

PATCH 9/12 crypto: keembay - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/intel/keembay/keembay-ocs-aes-core.c | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-ecc.c      | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-hcu-core.c | 4 +---
 3 files changed, 3 insertions(+), 9 deletions(-)
drivers/crypto/intel/keembay/keembay-ocs-aes-core.c +1 -3
index 0e424024224e..6de6dbdcdbd0 100644
@@ -1609,10 +1609,8 @@ static int kmb_ocs_aes_probe(struct platform_device *pdev)
 
 	rc = devm_request_threaded_irq(dev, aes_dev->irq, ocs_aes_irq_handler,
 				       NULL, 0, "keembay-ocs-aes", aes_dev);
-	if (rc < 0) {
-		dev_err(dev, "Could not request IRQ\n");
+	if (rc < 0)
 		return rc;
-	}
 
 	INIT_LIST_HEAD(&aes_dev->list);
 	spin_lock(&ocs_aes.lock);
drivers/crypto/intel/keembay/keembay-ocs-ecc.c +1 -3
index 9e555b02086c..9bdee39dc4e1 100644
@@ -900,10 +900,8 @@ static int kmb_ocs_ecc_probe(struct platform_device *pdev)
 
 	rc = devm_request_threaded_irq(dev, ecc_dev->irq, ocs_ecc_irq_handler,
 				       NULL, 0, "keembay-ocs-ecc", ecc_dev);
-	if (rc < 0) {
-		dev_err(dev, "Could not request IRQ\n");
+	if (rc < 0)
 		goto list_del;
-	}
 
 	/* Add device to the list of OCS ECC devices. */
 	spin_lock(&ocs_ecc.lock);
drivers/crypto/intel/keembay/keembay-ocs-hcu-core.c +1 -3
index 87b67060c77b..9a67bb4ecc82 100644
@@ -1204,10 +1204,8 @@ static int kmb_ocs_hcu_probe(struct platform_device *pdev)
 	rc = devm_request_threaded_irq(&pdev->dev, hcu_dev->irq,
 				       ocs_hcu_irq_handler, NULL, 0,
 				       "keembay-ocs-hcu", hcu_dev);
-	if (rc < 0) {
-		dev_err(dev, "Could not request IRQ.\n");
+	if (rc < 0)
 		return rc;
-	}
 
 	INIT_LIST_HEAD(&hcu_dev->list);
 
Signature · 1 line
2.34.1
author
#15 lore raw

PATCH 10/12 crypto: octeontx2 - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c +1 -4
index f6f47f4e5d83..79f6c2bb9d8b 100644
@@ -430,11 +430,8 @@ static int cptpf_register_afpf_mbox_intr(struct otx2_cptpf_dev *cptpf)
 	/* Register AF-PF mailbox interrupt handler */
 	ret = devm_request_irq(dev, irq, otx2_cptpf_afpf_mbox_intr, 0,
 			       "CPTAFPF Mbox", cptpf);
-	if (ret) {
-		dev_err(dev,
-			"IRQ registration failed for PFAF mbox irq\n");
+	if (ret)
 		return ret;
-	}
 	/* Clear interrupt if any, to avoid spurious interrupts */
 	otx2_cpt_write64(cptpf->reg_base, BLKADDR_RVUM, 0, RVU_PF_INT, 0x1ULL);
 	/* Enable AF-PF interrupt */
Signature · 1 line
2.34.1
author
#16 lore raw

PATCH 11/12 crypto: rockchip - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/rockchip/rk3288_crypto.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/crypto/rockchip/rk3288_crypto.c +1 -3
index b77bdce8e7fc..7cb1316b6f18 100644
@@ -365,10 +365,8 @@ static int rk_crypto_probe(struct platform_device *pdev)
 			       rk_crypto_irq_handle, IRQF_SHARED,
 			       "rk-crypto", pdev);
 
-	if (err) {
-		dev_err(&pdev->dev, "irq request failed.\n");
+	if (err)
 		goto err_crypto;
-	}
 
 	crypto_info->engine = crypto_engine_alloc_init(&pdev->dev, true);
 	if (!crypto_info->engine) {
Signature · 1 line
2.34.1
author
#17 lore raw

PATCH 12/12 crypto: stm32 - Remove redundant dev_err()

In reply to 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.

---
 drivers/crypto/stm32/stm32-cryp.c | 4 +---
 drivers/crypto/stm32/stm32-hash.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/crypto/stm32/stm32-cryp.c +1 -3
index b79877099942..3fc2db290b0f 100644
@@ -2590,10 +2590,8 @@ static int stm32_cryp_probe(struct platform_device *pdev)
 	ret = devm_request_threaded_irq(dev, irq, stm32_cryp_irq,
 					stm32_cryp_irq_thread, IRQF_ONESHOT,
 					dev_name(dev), cryp);
-	if (ret) {
-		dev_err(dev, "Cannot grab IRQ\n");
+	if (ret)
 		return ret;
-	}
 
 	cryp->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(cryp->clk)) {
drivers/crypto/stm32/stm32-hash.c +1 -3
index dada5951082c..e6a1e9b9f88d 100644
@@ -2356,10 +2356,8 @@ static int stm32_hash_probe(struct platform_device *pdev)
 						stm32_hash_irq_thread,
 						IRQF_ONESHOT,
 						dev_name(dev), hdev);
-		if (ret) {
-			dev_err(dev, "Cannot grab IRQ\n");
+		if (ret)
 			return ret;
-		}
 	} else {
 		dev_info(dev, "No IRQ, use polling mode\n");
 		hdev->polled = true;
Signature · 1 line
2.34.1
Acked-by on v1 12/12
#18 lore raw

Re: [PATCH 12/12] crypto: stm32 - Remove redundant dev_err()

Quoted 43 lines · 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.

---
  drivers/crypto/stm32/stm32-cryp.c | 4 +---
  drivers/crypto/stm32/stm32-hash.c | 4 +---
  2 files changed, 2 insertions(+), 6 deletions(-)
drivers/crypto/stm32/stm32-cryp.c +1 -3
index b79877099942..3fc2db290b0f 100644
@@ -2590,10 +2590,8 @@ static int stm32_cryp_probe(struct platform_device *pdev)
  	ret = devm_request_threaded_irq(dev, irq, stm32_cryp_irq,
  					stm32_cryp_irq_thread, IRQF_ONESHOT,
  					dev_name(dev), cryp);
-	if (ret) {
-		dev_err(dev, "Cannot grab IRQ\n");
+	if (ret)
  		return ret;
-	}
 
  	cryp->clk = devm_clk_get(dev, NULL);
  	if (IS_ERR(cryp->clk)) {
drivers/crypto/stm32/stm32-hash.c +1 -3
index dada5951082c..e6a1e9b9f88d 100644
@@ -2356,10 +2356,8 @@ static int stm32_hash_probe(struct platform_device *pdev)
  						stm32_hash_irq_thread,
  						IRQF_ONESHOT,
  						dev_name(dev), hdev);
-		if (ret) {
-			dev_err(dev, "Cannot grab IRQ\n");
+		if (ret)
  			return ret;
-		}
  	} else {
  		dev_info(dev, "No IRQ, use polling mode\n");
  		hdev->polled = true;
maintainer
#19 lore raw
In reply to Pan Chuang
Quoted 47 lines · 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 crypto drivers that follow these devm_request_*_irq() functions, as the core now provides more detailed diagnostic information on failure.

Pan Chuang (12):
crypto: allwinner - Remove redundant dev_err()
crypto: amlogic - Remove redundant dev_err()
crypto: aspeed - Remove redundant dev_err()
crypto: Remove redundant dev_err()/dev_err_probe()
crypto: caam - Remove redundant dev_err()
crypto: ccree - Remove redundant dev_err()
crypto: sl3516 - Remove redundant dev_err()
crypto: safexcel - Remove redundant dev_err()
crypto: keembay - Remove redundant dev_err()
crypto: octeontx2 - Remove redundant dev_err()
crypto: rockchip - Remove redundant dev_err()
crypto: stm32 - Remove redundant dev_err()

 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   | 4 +---
 drivers/crypto/allwinner/sun8i-ss/sun8i-ss-core.c   | 4 +---
 drivers/crypto/amlogic/amlogic-gxl-core.c           | 4 +---
 drivers/crypto/aspeed/aspeed-acry.c                 | 4 +---
 drivers/crypto/aspeed/aspeed-hace.c                 | 4 +---
 drivers/crypto/atmel-aes.c                          | 4 +---
 drivers/crypto/atmel-sha.c                          | 4 +---
 drivers/crypto/atmel-tdes.c                         | 4 +---
 drivers/crypto/caam/jr.c                            | 2 --
 drivers/crypto/ccree/cc_driver.c                    | 4 +---
 drivers/crypto/gemini/sl3516-ce-core.c              | 4 +---
 drivers/crypto/img-hash.c                           | 4 +---
 drivers/crypto/inside-secure/safexcel.c             | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-aes-core.c | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-ecc.c      | 4 +---
 drivers/crypto/intel/keembay/keembay-ocs-hcu-core.c | 4 +---
 drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c  | 5 +----
 drivers/crypto/mxs-dcp.c                            | 8 ++------
Quoted 11 lines
 drivers/crypto/omap-aes.c                           | 4 +---
 drivers/crypto/omap-des.c                           | 4 +---
 drivers/crypto/omap-sham.c                          | 5 +----
 drivers/crypto/rockchip/rk3288_crypto.c             | 4 +---
 drivers/crypto/sahara.c                             | 3 +--
 drivers/crypto/stm32/stm32-cryp.c                   | 4 +---
 drivers/crypto/stm32/stm32-hash.c                   | 4 +---
 25 files changed, 25 insertions(+), 78 deletions(-)
Signature · 1 line
2.34.1

All applied. Thanks.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt