Re: [PATCH 07/11] drm/mcde: Use power domain for display power

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect return value check for pm_runtime_put_sync_suspend(), treating success as an error.
- [High] Resource leak of the main clock (mcde_clk) on the probe error path.
--

commit 1e2436e1c266a49af52c126fb77756c71e5e0e91
Author: Linus Walleij <linusw@kernel.org>

drm/mcde: Use power domain for display power
    
Replace explicit EPOD regulator handling with runtime PM.
Use the MCDE power domain and drop the regulator dependency.

> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 257a6e84dd583..52f071bb347c4 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c

[ ... ]

> @@ -1327,9 +1326,9 @@ static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
>  		spin_unlock_irq(&crtc->dev->event_lock);
>  	}
>  
> -	ret = regulator_disable(mcde->epod);
> +	ret = pm_runtime_put_sync_suspend(mcde->dev);
>  	if (ret)
> -		dev_err(drm->dev, "can't disable EPOD regulator\n");
> +		dev_err(drm->dev, "can't disable MCDE power domain\n");

[Severity: High]
According to the Runtime PM API contract, pm_runtime_put_sync_suspend()
returns 1 when the device is already in the RPM_SUSPENDED state. Is it
intentional to treat a positive return value as an error here? If the device
is already suspended, this will log a false-positive error.

>  	/* Make sure we are powered down (before we may power up again) */
>  	usleep_range(50000, 70000);
>  
> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> index 5f2c462bad7e1..3f966cccda5a3 100644
> --- a/drivers/gpu/drm/mcde/mcde_drv.c
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c

[ ... ]

> @@ -412,14 +392,15 @@ static int mcde_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * Perform an invasive reset of the MCDE and all blocks by
> -	 * cutting the power to the subsystem, then bring it back up
> +	 * powering down the subsystem, then bring it back up
>  	 * later when we enable the display as a result of
>  	 * component_master_add_with_match().
>  	 */
> -	ret = regulator_disable(mcde->epod);
> +	ret = pm_runtime_put_sync_suspend(dev);
>  	if (ret) {
> -		dev_err(dev, "can't disable EPOD regulator\n");
> -		return ret;
> +		dev_err(dev, "can't disable MCDE power domain\n");
> +		pm_runtime_get_noresume(dev);
> +		goto clk_disable;
>  	}

[Severity: High]
Does this incorrectly abort the probe sequence if the device is already
suspended? 

pm_runtime_put_sync_suspend() returns 1 if the device was already in the
RPM_SUSPENDED state, which is a success condition, but the if (ret) check
here treats it as a failure.

>  	/* Wait 50 ms so we are sure we cut the power */
>  	usleep_range(50000, 70000);

[ ... ]

> @@ -428,25 +409,18 @@ static int mcde_probe(struct platform_device *pdev)
>  					      match);
>  	if (ret) {
>  		dev_err(dev, "failed to add component master\n");
> -		/*
> -		 * The EPOD regulator is already disabled at this point so some
> -		 * special errorpath code is needed
> -		 */
> -		clk_disable_unprepare(mcde->mcde_clk);
> -		regulator_disable(mcde->vana);
> -		return ret;
> +		goto clk_disable_pm_disabled;
>  	}
>  
>  	return 0;
>  
>  clk_disable:
>  	clk_disable_unprepare(mcde->mcde_clk);
> -regulator_off:
> -	regulator_disable(mcde->vana);
> -regulator_epod_off:
> -	regulator_disable(mcde->epod);
> +pm_runtime_put:
> +	pm_runtime_put_sync_suspend(dev);
> +clk_disable_pm_disabled:
> +	pm_runtime_disable(dev);
>  	return ret;
> -
>  }

[Severity: High]
Does this code leak the main clock when component_master_add_with_match()
fails? 

The goto clk_disable_pm_disabled jump skips over the clk_disable label, so
clk_disable_unprepare(mcde->mcde_clk) is never called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-ux500-power-domains-v7-1-v1-0-eb5e50b1a588@kernel.org?part=7