Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train

[Stephen Boyd]

Quoting khsieh@xxxxxxxxxxxxxx (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?