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 <[email protected]> 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/[email protected]?part=20
