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
