lkml.org 
[lkml]   [2020]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train
From
Date
Quoting khsieh@codeaurora.org (2020-10-05 11:02:10)
> >> + dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >> +
> >> dp_display_disable(dp_display, 0);
> >>
> >> rc = dp_display_unprepare(dp);
> >> if (rc)
> >> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
> >>
> >> - dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >> -
> >> state = atomic_read(&dp_display->hpd_state);
> >> if (state == ST_DISCONNECT_PENDING) {
> >
> > I don't understand the atomic nature of this hpd_state variable. Why is
> > it an atomic variable? Is taking a spinlock bad? What is to prevent the
> > atomic read here to not be interrupted and then this if condition check
> > be invalid because the variable has been updated somewhere else?
> hpd_state variable updated by multiple threads. however it was protected
> by mutex.
> in theory, it should also work as u32. since it was declared as atomic
> from beginning
> and it does not cause any negative effects, can we keep it as it is?
>

It does cause negative effects by generating worse code for something
that is already protected from concurrency by a mutex. Can we make it an
enum and name the enum and then add a comment indicating that the
'event_mutex' lock protects this variable?

\
 
 \ /
  Last update: 2020-10-06 02:54    [W:1.570 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site