[PATCH v2] crypto: amlogic: Fix IRQ handler return value and fallthrough logic

[PATCH v2] crypto: amlogic: Fix IRQ handler return value and fallthrough logic

[Mohamad Raizudeen]

In meson_irq_handler(), when an interrupt matches a flow but the status
register is empty, the driver prints an error but doesn't return. It
falls through the loop and incorrectly prints an `unknown irq` message.

Fix this by returning immediately. For empty status registers, return
IRQ_NONE instead of IRQ_HANDLED to avoid masking spurious interrupts and
allow the kernel to detect interrupt storms. Also use
dev_err_ratelimited() to prevent console lockups during an interrupt
storm.

Additionally, the handler return IRQ_HANDLED for genuinely unknown
interrupts. Return IRQ_NONE instead for unhandled interrupts.

Cc: stable@vger.kernel.org
Fixes: 48fe583fe5417 ("crypto: amlogic - Add crypto accelerator for amlogic GXL")
Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>
---
I have addressed the IRQ specific issues in this v2. The other
pre-existing issues mentioned by Sashiko are already addressed in my
separate patch.

Changes in v2:
- Return IRQ_NONE instead of IRQ_HANDLED when status register is empty
  to avoid masking spurious interrupts.
- Use dev_err_ratelimited() to prevent console lockups.

 drivers/crypto/amlogic/amlogic-gxl-core.c | 38 ++++++++++-------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
index 169c6eeb51e5..d5d84524c110 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-core.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
@@ -37,12 +37,13 @@ static irqreturn_t meson_irq_handler(int irq, void *data)
 				complete(&mc->chanlist[flow].complete);
 				return IRQ_HANDLED;
 			}
-			dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", __func__, irq, flow);
+			dev_err_ratelimited(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", __func__, irq, flow);
+			return IRQ_NONE;
 		}
 	}
 
-	dev_err(mc->dev, "%s %d from unknown irq\n", __func__, irq);
-	return IRQ_HANDLED;
+	dev_err_ratelimited(mc->dev, "%s %d from unknown irq\n", __func__, irq);
+	return IRQ_NONE;
 }
 
 static struct meson_alg_template mc_algs[] = {
@@ -243,34 +244,30 @@ static int meson_crypto_probe(struct platform_device *pdev)
 	if (IS_ERR(mc->base))
 		return PTR_ERR(mc->base);
 
-	mc->busclk = devm_clk_get(&pdev->dev, "blkmv");
+	mc->busclk = devm_clk_get_enabled(&pdev->dev, "blkmv");
 	if (IS_ERR(mc->busclk)) {
 		err = PTR_ERR(mc->busclk);
-		dev_err(&pdev->dev, "Cannot get core clock err=%d\n", err);
+		dev_err(&pdev->dev, "Cannot get/enable core clock err=%d\n", err);
 		return err;
 	}
 
+	err = meson_allocate_chanlist(mc);
+	if (err)
+		return err;
+
 	for (i = 0; i < MAXFLOW; i++) {
 		mc->irqs[i] = platform_get_irq(pdev, i);
-		if (mc->irqs[i] < 0)
-			return mc->irqs[i];
+		if (mc->irqs[i] < 0) {
+			err = mc->irqs[i];
+			goto error_chanlist;
+		}
 
 		err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
 				       "gxl-crypto", mc);
 		if (err < 0)
-			return err;
-	}
-
-	err = clk_prepare_enable(mc->busclk);
-	if (err != 0) {
-		dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
-		return err;
+			goto error_chanlist;
 	}
 
-	err = meson_allocate_chanlist(mc);
-	if (err)
-		goto error_flow;
-
 	err = meson_register_algs(mc);
 	if (err)
 		goto error_alg;
@@ -289,9 +286,8 @@ static int meson_crypto_probe(struct platform_device *pdev)
 	return 0;
 error_alg:
 	meson_unregister_algs(mc);
+error_chanlist:
 	meson_free_chanlist(mc, MAXFLOW - 1);
-error_flow:
-	clk_disable_unprepare(mc->busclk);
 	return err;
 }
 
@@ -306,8 +302,6 @@ static void meson_crypto_remove(struct platform_device *pdev)
 	meson_unregister_algs(mc);
 
 	meson_free_chanlist(mc, MAXFLOW - 1);
-
-	clk_disable_unprepare(mc->busclk);
 }
 
 static const struct of_device_id meson_crypto_of_match_table[] = {
-- 
2.53.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] crypto: amlogic: Fix IRQ handler return value and fallthrough logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] crypto: amlogic: undocumented unrelated changes squashed into patch
- [Low] crypto: amlogic: inefficient probe resource allocation order
--

commit 67eeae529cef60f91d2c385d9cec2d060aabf9ac
Author: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>

crypto: amlogic: Fix IRQ handler return value and fallthrough logic

This commit fixes the IRQ handler in the amlogic crypto driver by returning
immediately and providing IRQ_NONE when the status register is empty to avoid
masking spurious interrupts. It also uses ratelimited error logging to
prevent console lockups.

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> index 169c6eeb51e51..d5d84524c1104 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
[ ... ]
> @@ -243,34 +244,30 @@ static int meson_crypto_probe(struct platform_device *pdev)
>  	if (IS_ERR(mc->base))
>  		return PTR_ERR(mc->base);
>  
> -	mc->busclk = devm_clk_get(&pdev->dev, "blkmv");
> +	mc->busclk = devm_clk_get_enabled(&pdev->dev, "blkmv");

[Severity: Medium]
The commit message claims to only fix the IRQ handler return value and
fallthrough logic, but is this change to use devm_clk_get_enabled() related?

The changelog mentions that other pre-existing issues were meant for a
separate patch. Was this change, along with the channel list allocation
reordering below, squashed into this patch accidentally?

>  	if (IS_ERR(mc->busclk)) {
>  		err = PTR_ERR(mc->busclk);
> -		dev_err(&pdev->dev, "Cannot get core clock err=%d\n", err);
> +		dev_err(&pdev->dev, "Cannot get/enable core clock err=%d\n", err);
>  		return err;
>  	}
>  
> +	err = meson_allocate_chanlist(mc);
> +	if (err)
> +		return err;
> +
>  	for (i = 0; i < MAXFLOW; i++) {
>  		mc->irqs[i] = platform_get_irq(pdev, i);

[Severity: Low]
Is this order of initialization efficient when dealing with probe deferrals?

The patch moves the meson_allocate_chanlist() call before the loop that calls
platform_get_irq(). The allocation function performs heavy allocations like DMA
memory and crypto engines.

If platform_get_irq() returns -EPROBE_DEFER, these heavy resources will be
allocated and immediately destroyed.

Would it be better to keep the IRQ lookups before the channel list allocation
to avoid unnecessary probe deferral overhead, while leaving devm_request_irq()
after the allocation to prevent the NULL dereference race?

> -		if (mc->irqs[i] < 0)
> -			return mc->irqs[i];
> +		if (mc->irqs[i] < 0) {
> +			err = mc->irqs[i];
> +			goto error_chanlist;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918110504.7207-1-raizudeen.kerneldev@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic