Re: [PATCH v2 09/20] drm/msm/dp: Unwind resources when enabling a stream fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: pm_runtime reference leak in msm_dp_bridge_detect()
- [Medium] drm/msm/dp: dead code in stream enable abort path due to sink_count invariant
--

commit aeec7eb4a5527629640011cba89d7bbaaf90341c
Author: Xilin Wu <sophon@radxa.com>
Subject: drm/msm/dp: Unwind resources when enabling a stream fails

This patch introduces resource unwinding for stream enable failures by tracking
the stream PM reference and mainlink startup status. It unifies the rollback
paths and propagates errors from PHY initialization.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 787efbcb6c124..b076d44b5118c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -988,6 +1014,7 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
>  
>  		put = true;
>  	}
> +unlock:
>  	mutex_unlock(&priv->plugged_lock);
>  
>  	if (put)
>  		pm_runtime_put_sync(&dp->pdev->dev);

[Severity: High]
Does msm_dp_bridge_detect() leak a PM runtime reference here?

msm_dp_bridge_detect() unconditionally calls pm_runtime_resume_and_get()
earlier in the function. If the display is already connected (priv->plugged
is true), the exit path skips setting put = true. 

As a result, pm_runtime_put_sync() is never called, which leaks the PM
reference. Since userspace can poll the detect callback repeatedly
(e.g., via DRM_IOCTL_MODE_GETCONNECTOR), could this leak the PM reference
indefinitely and permanently prevent the device from suspending?

[ ... ]

> @@ -1460,6 +1493,52 @@ void msm_dp_display_atomic_pre_enable(struct msm_dp *msm_dp_display,
>  	msm_dp_display_set_mode(msm_dp_display, &crtc_state->adjusted_mode, dp->panel);
>  }
>  
> +static void msm_dp_display_abort_enable_locked(struct msm_dp_display_private *dp)
> +{
> +	bool keep_aux;
> +	int ret;
> +
> +	lockdep_assert_held(&dp->plugged_lock);
> +
> +	keep_aux = !dp->msm_dp_display.is_edp && dp->plugged &&
> +		msm_dp_aux_is_link_connected(dp->aux);
> +
> +	if (dp->stream_link_attempted) {
> +		/* The AUX peer is the remaining dongle, not its absent sink. */
> +		if (keep_aux && !dp->link->sink_count && dp->phy_initialized)
> +			msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);

[Severity: Medium]
Is this Power Save Mode (PSM) configuration block unreachable?

The condition !dp->link->sink_count is evaluated inside the
if (dp->stream_link_attempted) block. However, dp->stream_link_attempted
is only set to true in msm_dp_display_prepare_link_locked() if
dp->link->sink_count is non-zero. 

Because both msm_dp_display_prepare_link_locked() and 
msm_dp_display_abort_enable_locked() execute sequentially within the same 
plugged_lock critical section in msm_dp_display_atomic_enable(), concurrent
HPD events cannot modify sink_count.

Doesn't this mean the inner check !dp->link->sink_count will always evaluate
to false, rendering this fallback dead code?

> +
> +		msm_dp_ctrl_off_pixel_clk(dp->ctrl);
> +		msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
> +		dp->stream_link_attempted = false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-msm-dp-hdr10-v2-0-1835d4966da3@radxa.com?part=9

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic