Re: [PATCH v2 20/20] drm/msm/dp: Add static HDR metadata support for DP and eDP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: unclocked access crash in msm_dp_display_disable() after failed PHY test
- [Medium] cleanup: mixed goto and scoped_guard in msm_dp_display_atomic_enable
--

commit 3c44a9bdc2d3792773d111df209f6cd19e02a7d6
Author: Xilin Wu <sophon@radxa.com>
Subject: drm/msm/dp: Add static HDR metadata support for DP and eDP

Expose HDR_OUTPUT_METADATA and validate and pack it during atomic check.
Program Generic2 from the checked bridge state on every stream enable.
Reuse the DRM metadata conversion and infoframe packing helpers.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c51d033598a5b..7104822b03e05 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -880,10 +903,13 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp,
>  
>  	lockdep_assert_held(&dp->plugged_lock);
>  
> +	dp->video_test_blocked = false;
> +
>  	if (!msm_dp_display->power_on)
>  		return 0;
>  
>  	msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
> +	msm_dp_panel_disable_hdr_sdp(msm_dp_panel);

[Severity: High]
Can this unconditionally access hardware registers when the DP link clocks
are disabled?

If a DP sink requests a PHY test pattern,
msm_dp_ctrl_process_phy_test_request() intentionally disables the link
clocks via msm_dp_ctrl_off_link().

If msm_dp_ctrl_on_link() subsequently fails, it returns early leaving the
link clocks disabled, but the display power_on flag remains true.

When msm_dp_display_disable() is then called (e.g. on monitor unplug), it
proceeds because power_on is true, and calls msm_dp_panel_disable_hdr_sdp()
and msm_dp_panel_disable_vsc_sdp(). These functions write to hardware link
registers (like MMSS_DP_SDP_CFG).

Accessing these registers while the link clocks are off can cause a
synchronous external abort (kernel panic) which is reachable via a faulty
or malicious DP sink.

[ ... ]
> @@ -1774,6 +1800,7 @@ void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
>  		msm_dp_hpd_unplug_handle(dp);
>  	msm_dp_display_unprepare(dp);
>  	scoped_guard(mutex, &dp->plugged_lock) {
> +		dp->video_test_blocked = false;
>  		dp->link_failed = true;
>  		/* Keep reporting BAD, but notify only once until recovery. */
>  		schedule_work(&dp->link_status_work);

[Severity: Medium]
Does this mix goto-based error handling and scope-based cleanup in the same
function?

The msm_dp_display_atomic_enable() function uses traditional goto-based error
handling (goto err; goto err_prepare;) but also includes a scoped_guard()
block at the end.

According to kernel cleanup guidelines (include/linux/cleanup.h), mixing
goto and scope-based cleanup within the same function is explicitly
prohibited because it creates confusing ownership semantics and increases
the risk of double-free or locking bugs during maintenance.

Could this be converted to use scope-based cleanup entirely, or use
traditional locking for this block to match the rest of the function?

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

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