Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The refactored code introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition by evaluating `dp->link->sink_count == 0` twice. --
--- Patch [12]: [PATCH v8 12/15] drm/msm/dp: simplify link and clock disable sequence --- commit 84b41be14edf75da081c987f79e9f547234b2511 Author: Yongxing Mou <[email protected]> drm/msm/dp: simplify link and clock disable sequence This commit moves common disable steps out of the sink_count check to make the flow easier to follow. No functional change is intended. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 1ecd700a474fe..d1a3a6f6c303d 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -718,27 +718,19 @@ static int msm_dp_display_disable(struct > msm_dp_display_private *dp) > > msm_dp_panel_disable_vsc_sdp(dp->panel); > > - /* dongle is still connected but sinks are disconnected */ > - if (dp->link->sink_count == 0) { > - /* > - * irq_hpd with sink_count = 0 > - * hdmi unplugged out of dongle > - */ > + msm_dp_ctrl_off_pixel_clk(dp->ctrl); > > - /* set dongle to D3 (power off) mode */ > + /* dongle is still connected but sinks are disconnected */ > + if (dp->link->sink_count == 0) > msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true); > - msm_dp_ctrl_off_pixel_clk(dp->ctrl); > - msm_dp_ctrl_off_link(dp->ctrl, dp->panel); > + > + msm_dp_ctrl_off_link(dp->ctrl, dp->panel); > + > + if (dp->link->sink_count == 0) [Severity: High] Can checking dp->link->sink_count twice in this function introduce a race condition? If a hotplug or unplug event occurs exactly between these two checks, the IRQ thread could dynamically update dp->link->sink_count via msm_dp_irq_hpd_handle() -> msm_dp_link_process_request() while msm_dp_display_disable() is executing. Since there are no locks serializing the IRQ thread update against this atomic disable sequence, could this result in an inconsistent hardware state? For example, the dongle might be put into D3 power-off mode during the first check, but if sink_count changes before the second check, the host PHY might be exited instead of re-initialized. The original code evaluated sink_count once and used an if/else block, which guaranteed a mutually exclusive and consistent teardown sequence. Would it be safer to cache the value of sink_count in a local variable prior to the first check and use that local variable for both conditions? > + /* re-init the PHY so that we can listen to Dongle disconnect */ > msm_dp_ctrl_reinit_phy(dp->ctrl); > - } else { > - /* > - * unplugged interrupt > - * dongle unplugged out of DUT > - */ > - msm_dp_ctrl_off_pixel_clk(dp->ctrl); > - msm_dp_ctrl_off_link(dp->ctrl, dp->panel); > + else > msm_dp_display_host_phy_exit(dp); > - } > > msm_dp_display->power_on = false; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
