On Thu, Mar 14, 2024 at 09:30:57AM -0700, Abhinav Kumar wrote: > On 3/14/2024 8:38 AM, Johan Hovold wrote: > > On Wed, Mar 13, 2024 at 10:24:08AM -0700, Abhinav Kumar wrote: > > Perhaps I'm missing something in the race that you are trying to > > describe (and which I've asked you to describe in more detail so that I > > don't have to spend more time trying to come up with a reproducer > > myself). > The race condition is between the time we get disconnect event and set > link_ready to false, a commit can come in. Because setting link_ready to > false happens in the event thread so it could be slightly delayed. I get this part, just not why, or rather when, that becomes a problem. Once the disconnect event is processed, dp_hpd_unplug_handle() will update the state to ST_DISCONNECT_PENDING, and queue a notification event. link_ready is (before this patch) still set to 1. Here a commit comes in; what exactly are you suggesting would trigger that? And in such a way that it breaks the state machine? One way this could cause trouble is if you end up with a call to dp_bridge_atomic_post_disable() which updates the state to ST_DISCONNECTED. (1) This would then need to be followed by another call to dp_bridge_atomic_enable() which bails out early with the link clock disabled. (2) (And if link_ready were to be set to 0 sooner, the likelihood of this is reduced.) This in turn, would trigger a reset when dp_bridge_atomic_disable() is later called. This is the kind of description of the race I expect to see in the commit message, and I'm still not sure what would trigger the call to dp_bridge_atomic_post_disable() and dp_bridge_atomic_enable() (i.e. (1) and (2) above) and whether this is a real issue or not. Also note that the above scenario is quite different from the one I've hit and described earlier. > It will be hard to reproduce this. Only way I can think of is to delay > the EV_NOTIFICATION for sometime and see in dp_bridge_hpd_notify() > > else if (dp_display->link_ready && status == > connector_status_disconnected) > dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0); > > as dp_add_event() will add the event, then wakeup the event_q. Sure that would increase the race window with the current code, but that alone isn't enough to trigger the bug AFAICT. > Before the event thread wakes up and processes this unplug event, the > commit can come in. This is the race condition i was thinking of. Yes, but what triggers the commit? And why would it lead to a mode set that disables the bridge? Johan
