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

[sashiko-bot@kernel.org]

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