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?