On Wed, 6 Mar 2024 at 21:59, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 3/6/2024 11:52 AM, Dmitry Baryshkov wrote: > > On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > >> > >> There are cases where the userspace might still send another > >> frame after the HPD disconnect causing a modeset cycle after > >> a disconnect. This messes the internal state machine of MSM DP driver > >> and can lead to a crash as there can be an imbalance between > >> bridge_disable() and bridge_enable(). > >> > >> This was also previously reported on [1] for which [2] was posted > >> and helped resolve the issue by rejecting commits if the DP is not > >> in connected state. > >> > >> The change resolved the bug but there can also be another race condition. > >> If hpd_event_thread does not pick up the EV_USER_NOTIFICATION and process it > >> link_ready will also not be set to false allowing the frame to sneak in. > >> > >> Lets move setting link_ready outside of hpd_event_thread() processing to > >> eliminate a window of race condition. > >> > >> [1] : https://gitlab.freedesktop.org/drm/msm/-/issues/17 > >> [2] : https://lore.kernel.org/all/1664408211-25314-1-git-send-email-quic_khsieh@xxxxxxxxxxx/ > >> > >> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > >> index 068d44eeaa07..e00092904ccc 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >> @@ -345,8 +345,6 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp, > >> dp->panel->downstream_ports); > >> } > >> > >> - dp->dp_display.link_ready = hpd; > >> - > >> drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n", > >> dp->dp_display.connector_type, hpd); > >> drm_bridge_hpd_notify(bridge, dp->dp_display.link_ready); > >> @@ -399,6 +397,8 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) > >> goto end; > >> } > >> > >> + dp->dp_display.link_ready = true; > > > > Do we need any kind of locking now? > > > > hmm ... correct me if I have missed any flows but I think all paths > where we will set link_ready are already protected by event_mutex? I didn't check the source code, that's why I was asking. If it's protected by event_mutex, then it should be fine. > > >> + > >> dp_add_event(dp, EV_USER_NOTIFICATION, true, 0); > >> > >> end: > >> @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device *dev) > >> { > >> struct dp_display_private *dp = dev_get_dp_display_private(dev); > >> > >> + dp->dp_display.link_ready = false; > >> + > >> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0); > >> > >> return 0; > >> @@ -487,6 +489,7 @@ static int dp_display_handle_port_status_changed(struct dp_display_private *dp) > >> drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to do\n"); > >> if (dp->hpd_state != ST_DISCONNECTED) { > >> dp->hpd_state = ST_DISCONNECT_PENDING; > >> + dp->dp_display.link_ready = false; > >> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0); > >> } > >> } else { > >> -- > >> 2.34.1 > >> > > > > -- With best wishes Dmitry
