Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Callers of `tidss_runtime_get()` ignore its return value, proceeding to access hardware even if the device fails to resume. - [High] Pending asynchronous suspend is cancelled by `pm_runtime_disable()` during device removal, leaving hardware powered on. -- commit 1177f415f04c015bad856d54b014b7b1a387e7b9 Author: Tomi Valkeinen <[email protected]> drm/tidss: Drop WARN_ON() from tidss_runtime_put() This commit removes the WARN_ON() check for the return value of pm_runtime_put_autosuspend() in tidss_runtime_put(). The change is made to avoid spurious warnings when an OLDI auxiliary device is active, which legitimately causes the autosuspend to return -EBUSY due to a non-zero child count. > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c > b/drivers/gpu/drm/tidss/tidss_drv.c > index 5cb3e746aeb37..44b0b009c29a9 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -40,10 +40,7 @@ int tidss_runtime_get(struct tidss_device *tidss) > > void tidss_runtime_put(struct tidss_device *tidss) > { [Severity: High] This isn't a bug introduced by this patch, but since we are modifying the PM handling here, is it safe for callers of tidss_runtime_get() to ignore its return value? If pm_runtime_resume_and_get() fails (e.g., due to power domain or clock issues), the error is ignored, and the driver immediately accesses display hardware registers which could cause a kernel panic (synchronous external abort). For example, in tidss_crtc_atomic_enable(): drivers/gpu/drm/tidss/tidss_crtc.c:tidss_crtc_atomic_enable() { ... tidss_runtime_get(tidss); r = dispc_vp_set_clk_rate(tidss->dispc, tcrtc->hw_videoport, mode->crtc_clock * 1000); ... } The same happens in tidss_atomic_commit_tail() and tidss_crtc_enable_vblank(). Should these callers check the return value of tidss_runtime_get() before proceeding to access hardware? > - int r; > - > - r = pm_runtime_put_autosuspend(tidss->dev); > - WARN_ON(r < 0); > + pm_runtime_put_autosuspend(tidss->dev); > } [Severity: High] This is a pre-existing issue, but does tidss_remove() safely handle the asynchronous suspend scheduled here? During module unload, tidss_remove() calls drm_atomic_helper_shutdown(), which queues an async suspend via tidss_runtime_put(). Immediately after, it calls pm_runtime_disable(): drivers/gpu/drm/tidss/tidss_drv.c:tidss_remove() { ... drm_atomic_helper_shutdown(ddev); ... pm_runtime_dont_use_autosuspend(dev); pm_runtime_disable(dev); ... } Calling pm_runtime_disable() cancels the pending autosuspend timer. Does this leave the hardware powered on after driver removal, causing a power leak and leaving the PM core state machine in RPM_ACTIVE? If the device is left in RPM_ACTIVE, could its parent power domain be pinned active forever, preventing the entire SoC from entering deep sleep states? Should this teardown sequence ensure the device is synchronously suspended before disabling runtime PM? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
