Re: [PATCH v5 2/4] dmaengine: fsl-edma: use devm_clk_get_optional_enabled() for DMA engine clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removing mandatory clock validation risks kernel crashes on misconfigured device trees.
- [Medium] API Mismatch: `devm_clk_get_optional_enabled()` conflicts with the stated runtime PM intent, leading to unbalanced clock disables.
--

commit 571d7197ad2ed67c4508436798804c62669d46f2
Author: Joy Zou <joy.zou@nxp.com>

dmaengine: fsl-edma: use devm_clk_get_optional_enabled() for DMA engine clock

This commit replaces devm_clk_get_enabled() with
devm_clk_get_optional_enabled() for the eDMA engine clock and removes the
FSL_EDMA_DRV_HAS_DMACLK flag. The stated goal is to simplify clock handling
and prepare for adding channel runtime PM support.

>     Prepare to add channel runtime pm support.

If the intention is to manage this clock via runtime PM, does using
devm_clk_get_optional_enabled() conflict with that?

The devm_clk_get_optional_enabled() helper registers a devres action that
automatically calls clk_disable_unprepare() upon driver unbind. If the device
is runtime suspended and the clock is already disabled when the driver is
unbound, the devres cleanup will attempt to disable the clock again.

Can this trigger a WARN_ON in the clock core for an unbalanced enable count?

Should the driver use devm_clk_get_optional() and manually handle
clk_prepare_enable() instead?

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index 87f575d6ccaff..ecd14967bfbc0 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c

[ ... ]

> @@ -733,13 +733,11 @@ static int fsl_edma_probe(struct platform_device *pdev)
>  		regs = &fsl_edma->regs;
>  	}
>  
> -	if (drvdata->flags & FSL_EDMA_DRV_HAS_DMACLK) {
> -		fsl_edma->dmaclk = devm_clk_get_enabled(&pdev->dev, "dma");
> -		if (IS_ERR(fsl_edma->dmaclk))
> -			return dev_err_probe(&pdev->dev,
> -					     PTR_ERR(fsl_edma->dmaclk),
> -					     "Missing DMA block clock.\n");
> -	}
> +	fsl_edma->dmaclk = devm_clk_get_optional_enabled(&pdev->dev, "dma");
> +	if (IS_ERR(fsl_edma->dmaclk))
> +		return dev_err_probe(&pdev->dev,
> +				     PTR_ERR(fsl_edma->dmaclk),
> +				     "Failed to get/enable DMA clock.\n");

Does removing the driver-level validation for this clock risk kernel crashes
on misconfigured device trees?

On platforms where the eDMA engine clock is physically required (e.g.,
i.MX7ULP, i.MX8ULP, i.MX93), a missing "dma" clock in the device tree
previously caused a graceful probe failure. Now,
devm_clk_get_optional_enabled() will silently return NULL and probing will
continue.

If the driver subsequently writes to unclocked eDMA registers, such as during
edma_write_tcdreg(), can this trigger a synchronous external abort (bus fault)
and panic the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-b4-b4-edma-runtime-opt-v5-0-1e595bfb8423@nxp.com?part=2