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 <[email protected]> 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/[email protected]?part=7
