Re: [PATCH v5 3/4] dmaengine: fsl-edma: convert DMAMUX clock handling to bulk clock API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Switching to `devm_clk_bulk_get_optional_enable()` silently drops validation for missing DMAMUX clocks, leading to a system bus hang during probe on misconfigured device trees.
--

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

dmaengine: fsl-edma: convert DMAMUX clock handling to bulk clock API

This commit converts the DMAMUX clock management from individual clock
operations to the bulk clock API to simplify the code. This is in
preparation for adding runtime PM support.

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

[ ... ]

> @@ -747,23 +739,28 @@ static int fsl_edma_probe(struct platform_device *pdev)
>  		fsl_edma->chan_masked |= chan_mask[0];
>  	}
>  
> -	for (i = 0; i < fsl_edma->drvdata->dmamuxs; i++) {
> -		char clkname[32];
> -
> -		fsl_edma->muxbase[i] = devm_platform_ioremap_resource(pdev,
> -								      1 + i);
> -		if (IS_ERR(fsl_edma->muxbase[i])) {
> -			/* on error: disable all previously enabled clks */
> -			fsl_disable_clocks(fsl_edma, i);
> -			return PTR_ERR(fsl_edma->muxbase[i]);
> +	if (fsl_edma->drvdata->dmamuxs) {
> +		fsl_edma->muxclk = devm_kcalloc(&pdev->dev, fsl_edma->drvdata->dmamuxs,
> +						sizeof(*fsl_edma->muxclk), GFP_KERNEL);
> +		if (!fsl_edma->muxclk)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < fsl_edma->drvdata->dmamuxs; i++) {
> +			fsl_edma->muxbase[i] = devm_platform_ioremap_resource(pdev, 1 + i);
> +			if (IS_ERR(fsl_edma->muxbase[i]))
> +				return PTR_ERR(fsl_edma->muxbase[i]);
> +
> +			fsl_edma->muxclk[i].id = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +								"dmamux%d", i);
> +			if (!fsl_edma->muxclk[i].id)
> +				return -ENOMEM;
>  		}
>  
> -		sprintf(clkname, "dmamux%d", i);
> -		fsl_edma->muxclk[i] = devm_clk_get_enabled(&pdev->dev, clkname);
> -		if (IS_ERR(fsl_edma->muxclk[i]))
> -			return dev_err_probe(&pdev->dev,
> -					     PTR_ERR(fsl_edma->muxclk[i]),
> -					     "Missing DMAMUX block clock.\n");
> +		ret = devm_clk_bulk_get_optional_enable(&pdev->dev, fsl_edma->drvdata->dmamuxs,
> +							fsl_edma->muxclk);

Does using devm_clk_bulk_get_optional_enable() silently drop validation for
missing DMAMUX clocks?

The previous code used devm_clk_get_enabled(), which safely aborted the probe
if clocks were missing. Because the optional variant returns 0 when the clocks
are missing in the device tree, the probe will continue.

If the probe continues without the clocks enabled, the driver later calls
fsl_edma_chan_mux() which accesses the unclocked muxbase registers. On NXP
SoCs, accessing unclocked registers can trigger a system bus hang or
asynchronous external abort.

Should this use a mandatory bulk clock API to ensure the clocks are actually
present and enabled?

> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to enable DMAMUX block clock.\n");
>  	}
>

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