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?
