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

[sashiko-bot@kernel.org]

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 <panchuang@vivo.com>
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.

> diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
> index b393689400b4c..1bf00df864e8f 100644
> --- a/drivers/crypto/atmel-aes.c
> +++ b/drivers/crypto/atmel-aes.c
> @@ -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?

> -	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");

[ ... ]

> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index 133ebc9982362..49cd6b036a86c 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -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.

> -	if (ret) {
> -		dev_err(dev, "Failed to claim DCP VMI IRQ!\n");
> +	if (ret)
>  		return ret;
> -	}

[ ... ]

> diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
> index f31555c0d715b..711909de20130 100644
> --- a/drivers/crypto/omap-aes.c
> +++ b/drivers/crypto/omap-aes.c
> @@ -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?

> -		if (err) {
> -			dev_err(dev, "Unable to grab omap-aes IRQ\n");
> +		if (err)
>  			goto err_irq;
> -		}
>  	}

[ ... ]

> diff --git a/drivers/crypto/sahara.c b/drivers/crypto/sahara.c
> index 533080b0cddc9..d586f1315de1f 100644
> --- a/drivers/crypto/sahara.c
> +++ b/drivers/crypto/sahara.c
> @@ -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?

>  	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");

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