Re: [PATCH 09/10] drm/tidss: Drop WARN_ON() from tidss_runtime_put()

[sashiko-bot@kernel.org]

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 <tomi.valkeinen@ideasonboard.com>

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/20260903-tidss-pre-beagley-v1-0-93928baa8b12@ideasonboard.com?part=9