Re: [PATCH v6 15/29] drm/msm/dp: factor out _helper variants of bridge ops accepting a panel

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unclocked register access and system crash in atomic_disable if link preparation fails.
- [High] Sleeping function called from atomic context in the atomic_post_disable path.
--

commit e10b321045ec4e6478af1b4a9b441fca4e712881
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
Subject: drm/msm/dp: factor out _helper variants of bridge ops accepting a panel

This commit factors out helper variants of DP bridge operations to accept
arbitrary panels, facilitating MST support. It updates the atomic callbacks
to act as thin wrappers around these helpers and modifies prepare/unprepare
functions to use the generic msm_dp structure.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 9c5563dcb856e..6103fa67f3ee9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> +void msm_dp_display_disable_helper(struct msm_dp *msm_dp_display,
> +				   struct msm_dp_panel *msm_dp_panel)
> +{
> +	struct msm_dp_display_private *dp;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	msm_dp_ctrl_push_vcpf(dp->ctrl, msm_dp_panel);
> +	msm_dp_ctrl_mst_timeslot_setup(dp->ctrl);
> +	msm_dp_ctrl_mst_send_act(dp->ctrl, msm_dp_panel);
> +}

[Severity: Critical]
This isn't a bug introduced by this patch, but does this sequence
unconditionally write to hardware registers without ensuring the link
is prepared?

If msm_dp_display_prepare_link() fails earlier in the atomic commit path,
the hardware clocks and power domains are not enabled. Because the
atomic_pre_enable() callback returns void, the DRM core still considers
the display enabled.

On a subsequent disable, msm_dp_display_atomic_disable() unconditionally
calls this helper. Accessing hardware MMIO registers like REG_DP_STATE_CTRL
via msm_dp_ctrl_push_vcpf() when the clocks are disabled causes a
synchronous external abort (kernel panic).

Could we add a link readiness check to safely skip these MMIO writes if the
display is not powered on?

[ ... ]
> -static void msm_dp_display_unprepare(struct msm_dp_display_private *dp)
> +void msm_dp_display_unprepare(struct msm_dp *msm_dp_display)
>  {
[ ... ]
>  	if (msm_dp_display->active_stream_cnt) {
>  		drm_dbg_dp(dp->drm_dev, "stream still active, return\n");
> @@ -1530,11 +1561,9 @@ static void msm_dp_display_unprepare(struct msm_dp_display_private *dp)
>  		msm_dp_display_host_phy_exit(dp);
>  
>  	pm_runtime_put_sync(&msm_dp_display->pdev->dev);

[Severity: High]
This is a pre-existing issue, but does this call chain execute sleeping
operations from an atomic context?

The atomic_post_disable() callback executes in an atomic context where
sleeping is explicitly forbidden. However, it calls
msm_dp_display_unprepare(), which in turn calls pm_runtime_put_sync().

This synchronous put operation suspends the device via:
pm_runtime_put_sync() -> msm_dp_pm_runtime_suspend() -> disable_irq()

Since disable_irq() can sleep, this causes kernel warnings and potential
deadlocks during the atomic modeset. 

Should this synchronous power management operation be deferred or handled
outside of the atomic disable path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=15