[PATCH v3] crypto/amlogic: Use devm APIs for clock management

[PATCH v3] crypto/amlogic: Use devm APIs for clock management

[Mohamad Raizudeen]

The driver currently gets the core clock and manually enables it using
clk_prepare_enable(). This requires matching calls to
clk_disable_unprepare() in the error paths and remove function.

Switch to devm_clk_get_enabled() instead. This lets the kernel handle
enabling and disabling the clock automatically, which allow us to drop
the manual cleanup code and keeps the probe function simple.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202608310329.QgxCCMs1-lkp@intel.com/
Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>
---
Changes in v3:
- Moved IRQ requests after channel allocation to fix a null pointer
  derefernce and use-after-free.
- The IRQ return value issue is fixed in a separate patch [1].

Link to v2: https://lore.kernel.org/all/aqpmOzURNFOm78ls@gondor.apana.org.au/T/
[1] https://lore.kernel.org/all/20260908160120.4805-1-raizudeen.kerneldev@gmail.com/

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

diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
index 169c6eeb51e5..d2d23a443c53 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-core.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
@@ -243,13 +243,17 @@ 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_chainlist(mc);
+	if (err)
+		return err;
+
 	for (i = 0; i < MAXFLOW; i++) {
 		mc->irqs[i] = platform_get_irq(pdev, i);
 		if (mc->irqs[i] < 0)
@@ -261,16 +265,6 @@ static int meson_crypto_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	err = clk_prepare_enable(mc->busclk);
-	if (err != 0) {
-		dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
-		return err;
-	}
-
-	err = meson_allocate_chanlist(mc);
-	if (err)
-		goto error_flow;
-
 	err = meson_register_algs(mc);
 	if (err)
 		goto error_alg;
@@ -290,8 +284,6 @@ static int meson_crypto_probe(struct platform_device *pdev)
 error_alg:
 	meson_unregister_algs(mc);
 	meson_free_chanlist(mc, MAXFLOW - 1);
-error_flow:
-	clk_disable_unprepare(mc->busclk);
 	return err;
 }
 
@@ -306,8 +298,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 v3] crypto/amlogic: Use devm APIs for clock management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] crypto: amlogic: memory leak and UAF on probe error path
- [Low] crypto: amlogic: build failure due to misspelled meson_allocate_chainlist
--

commit 798e78c31473ca6be8ca9d38739f43b6653307b6
Author: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>

crypto/amlogic: Use devm APIs for clock management

This commit converts the driver to use devm_clk_get_enabled instead of
manually getting and enabling the core clock. This change allows the removal
of manual cleanup code in the error paths and simplifies the probe function.

Closes: https://lore.kernel.org/oe-kbuild-all/202608310329.QgxCCMs1-lkp@intel.com/

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> index 169c6eeb51e51..d2d23a443c53f 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
[ ... ]
> @@ -243,13 +243,17 @@ 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_chainlist(mc);
               ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is there a typo in the function name meson_allocate_chainlist?

> +	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];

[Severity: High]
Since the allocation of chanlist was moved before this loop, if
platform_get_irq fails, does returning directly here bypass the cleanup of
resources allocated by meson_allocate_chanlist?

If this cleanup is bypassed, could it cause a use-after-free when orphaned
kthreads attempt to access the devm-managed engine structures after they are
freed?

>  
>  		err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
>  				       "gxl-crypto", mc);
>  		if (err < 0)
>  			return err;

[Severity: High]
Similarly, if devm_request_irq fails, does returning directly here bypass
cleanup, leaking DMA coherent memory and leaving crypto_engine kthreads
running?

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

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