Re: [PATCH] drm/imx: dcss: fix autosuspend cleanup during teardown

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Double Hardware Suspend / Clock Disable caused by synchronous rpm_idle.

Pre-existing issues:
- [High] TOCTOU Race Condition during driver teardown.
- [High] Hardware register access while runtime suspended leads to system crash.
--

--- Patch [1]: [PATCH] drm/imx: dcss: fix autosuspend cleanup during teardown ---
commit 57aed202fd27fa66b9bc5448a0fe4df074c91e5c
Author: Guangshuo Li <[email protected]>

drm/imx: dcss: fix autosuspend cleanup during teardown

This patch adds a missing pm_runtime_dont_use_autosuspend() call during
driver teardown before disabling runtime PM. This ensures the usage count
remains balanced if the autosuspend delay is set to a negative value while
enabled.

> diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.c b/drivers/gpu/drm/imx/dcss/dcss-dev.c
> index 7fd0c4c14205d..51398852cdc2b 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-dev.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-dev.c
> @@ -234,6 +234,7 @@ void dcss_dev_destroy(struct dcss_dev *dcss)
>  {
>  	if (!pm_runtime_suspended(dcss->dev)) {

[Severity: High]
This isn't a bug introduced by this patch, but does this check have a race
condition with the PM worker?

If the autosuspend timer expires exactly when dcss_dev_destroy() is called,
is it possible for the PM worker thread to execute dcss_dev_runtime_suspend()
concurrently with this block? Both paths would call dcss_ctxld_suspend()
and dcss_clocks_disable() at the same time.

Should pm_runtime_disable() be called before this check to wait for pending
transitions and prevent new ones?

>  		dcss_ctxld_suspend(dcss->ctxld);
>  		dcss_clocks_disable(dcss);
>  	}
>  
>  	of_node_put(dcss->of_port);
>  
> +	pm_runtime_dont_use_autosuspend(dcss->dev);

[Severity: High]
Could this newly added call trigger a double hardware suspend and clock
disable?

If the device is active during teardown (e.g., the autosuspend delay hasn't
expired), the code above manually executes dcss_ctxld_suspend() and
dcss_clocks_disable(). When pm_runtime_dont_use_autosuspend() is then
called, it drops the autosuspend flag and synchronously calls rpm_idle().

Because the usage count is 0 and the device is still marked RPM_ACTIVE,
this will synchronously invoke the driver's runtime suspend callback:

dcss_dev_destroy()
  pm_runtime_dont_use_autosuspend()
    rpm_idle()
      dcss_dev_runtime_suspend()
        dcss_clocks_disable()

This causes a second clock disable and suspend. Should this call be moved
after pm_runtime_disable(), where disable_depth > 0 would safely prevent the
PM callbacks from executing?

>  	pm_runtime_disable(dcss->dev);
>  
>  	dcss_submodules_stop(dcss);

[Severity: High]
This is a pre-existing issue, but can this call cause a bus fault if the
device was already runtime suspended?

If dcss_dev_destroy() is called when the device is runtime suspended,
the manual dcss_clocks_disable() above is skipped. dcss_submodules_stop()
is then unconditionally called, which executes submodule exit functions like:

dcss_submodules_stop()
  dcss_scaler_exit()
    dcss_writel(0, ch->base_reg + DCSS_SCALER_CTRL);

Since dcss_submodules_stop() only calls dcss_clocks_enable() but doesn't
power on the PM domain, doesn't writing to these MMIO registers cause a
synchronous external abort?

>  
>  	dcss_clks_release(dcss);
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1