[Nouveau] [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking
This is a complicated one. Essentially, there's currently a problem in the MST core that hasn't really caused any issues that we're aware of (emphasis on "that we're aware of"): locking. When we go through and probe the link addresses and path resources in a topology, we hold no locks when updating ports with said information. The members I'm referring to in particular are: - ldps - ddps - mcs - pdt - dpcd_rev - num_sdp_streams - num_sdp_stream_sinks - available_pbn - input - connector Now that we're handling UP requests asynchronously and will be using some of the struct members mentioned above in atomic modesetting in the future for features such as PBN validation, this is going to become a lot more important. As well, the next few commits that prepare us for and introduce suspend/resume reprobing will also need clear locking in order to prevent from additional racing hilarities that we never could have hit in the past. So, let's solve this issue by using >base.lock, the modesetting lock which currently only protects >base.state. This works perfectly because it allows us to avoid blocking connection_mutex unnecessarily, and we can grab this in connector detection paths since it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this when updating ports. For drm_dp_mst_handle_link_address_port() things are a bit more complicated. As I've learned the hard way, we can grab >lock.base for everything except for port->connector. See, our normal driver probing paths end up generating this rather obvious lockdep chain: >mode_config.mutex -> crtc_ww_class_mutex/crtc_ww_class_acquire -> >mutex However, sysfs grabs >mode_config.mutex in order to protect itself from connector state changing under it. Because this entails grabbing kn->count, e.g. the lock that the kernel provides for protecting sysfs contexts, we end up grabbing kn->count followed by >mode_config.mutex. This ends up creating an extremely rude chain: >count -> >mode_config.mutex -> crtc_ww_class_mutex/crtc_ww_class_acquire -> >mutex I mean, look at that thing! It's just evil!!! This gross thing ends up making any calls to drm_connector_register()/drm_connector_unregister() impossible when holding any kind of modesetting lock. This is annoying because ideally, we always want to ensure that drm_dp_mst_port->connector never changes when doing an atomic commit or check that would affect the atomic topology state so that it can reliably and easily be used from future DRM DP MST helpers to assist with tasks such as scanning through the current VCPI allocations and adding connectors which need to have their allocations updated in response to a bandwidth change or the like. Being able to hold >base.lock throughout the entire link probe process would have been _great_, since we could prevent userspace from ever seeing any states in-between individual port changes and as a result likely end up with a much faster probe and more consistent results from said probes. But without some rework of how we handle connector probing in sysfs it's not at all currently possible. In the future, maybe we can try using the sysfs locks to protect updates to connector probing state and fix this mess. So for now, to protect everything other than port->connector under >base.lock and ensure that we still have the guarantee that atomic check/commit contexts will never see port->connector change we use a silly trick. See: port->connector only needs to change in order to ensure that input ports (see the MST spec) never have a ghost connector associated with them. But, there's nothing stopping us from simply throwing the entire port out and creating a new one in order to maintain that requirement while still keeping port->connector consistent across the lifetime of the port in atomic check/commit contexts. For all intended purposes this works fine, as we validate ports in any contexts we care about before using them and as such will end up reporting the connector as disconnected until it's port's destruction finalizes. So, we just do that in cases where we detect port->input has transitioned from true->false. We don't need to worry about the other direction, since a port without a connector isn't visible to userspace and as such doesn't need to be protected by >base.lock until we finish registering a connector for it. For updating members of drm_dp_mst_port other than port->connector, we simply grab >base.lock in drm_dp_mst_link_probe_work() for already registered ports, update said members and drop the lock before potentially registering a connector and probing the link address of it's children. Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock acquisition context in order to acquire >base.lock under _mutex and convert all it's users over to using the .detect_ctx probe hooks. With that, we finally have well defined locking. Changes since v4: * Get rid of port->mutex, stop using connection_mutex and just use our own
Re: [Nouveau] [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking
On Mon, Oct 21, 2019 at 10:36:01PM -0400, Lyude Paul wrote: > This is a complicated one. Essentially, there's currently a problem in the MST > core that hasn't really caused any issues that we're aware of (emphasis on > "that > we're aware of"): locking. > > When we go through and probe the link addresses and path resources in a > topology, we hold no locks when updating ports with said information. The > members I'm referring to in particular are: > > - ldps > - ddps > - mcs > - pdt > - dpcd_rev > - num_sdp_streams > - num_sdp_stream_sinks > - available_pbn > - input > - connector > > Now that we're handling UP requests asynchronously and will be using some of > the struct members mentioned above in atomic modesetting in the future for > features such as PBN validation, this is going to become a lot more important. > As well, the next few commits that prepare us for and introduce suspend/resume > reprobing will also need clear locking in order to prevent from additional > racing hilarities that we never could have hit in the past. > > So, let's solve this issue by using >base.lock, the modesetting > lock which currently only protects >base.state. This works > perfectly because it allows us to avoid blocking connection_mutex > unnecessarily, and we can grab this in connector detection paths since > it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this > when updating ports. For drm_dp_mst_handle_link_address_port() things > are a bit more complicated. As I've learned the hard way, we can grab > >lock.base for everything except for port->connector. See, our > normal driver probing paths end up generating this rather obvious > lockdep chain: > > >mode_config.mutex > -> crtc_ww_class_mutex/crtc_ww_class_acquire > -> >mutex > > However, sysfs grabs >mode_config.mutex in order to protect itself > from connector state changing under it. Because this entails grabbing > kn->count, e.g. the lock that the kernel provides for protecting sysfs > contexts, we end up grabbing kn->count followed by > >mode_config.mutex. This ends up creating an extremely rude chain: > > >count > -> >mode_config.mutex > -> crtc_ww_class_mutex/crtc_ww_class_acquire > -> >mutex > > I mean, look at that thing! It's just evil!!! This gross thing ends up > making any calls to drm_connector_register()/drm_connector_unregister() > impossible when holding any kind of modesetting lock. This is annoying > because ideally, we always want to ensure that > drm_dp_mst_port->connector never changes when doing an atomic commit or > check that would affect the atomic topology state so that it can > reliably and easily be used from future DRM DP MST helpers to assist > with tasks such as scanning through the current VCPI allocations and > adding connectors which need to have their allocations updated in > response to a bandwidth change or the like. > > Being able to hold >base.lock throughout the entire link probe > process would have been _great_, since we could prevent userspace from > ever seeing any states in-between individual port changes and as a > result likely end up with a much faster probe and more consistent > results from said probes. But without some rework of how we handle > connector probing in sysfs it's not at all currently possible. In the > future, maybe we can try using the sysfs locks to protect updates to > connector probing state and fix this mess. > > So for now, to protect everything other than port->connector under > >base.lock and ensure that we still have the guarantee that atomic > check/commit contexts will never see port->connector change we use a > silly trick. See: port->connector only needs to change in order to > ensure that input ports (see the MST spec) never have a ghost connector > associated with them. But, there's nothing stopping us from simply > throwing the entire port out and creating a new one in order to maintain > that requirement while still keeping port->connector consistent across > the lifetime of the port in atomic check/commit contexts. For all > intended purposes this works fine, as we validate ports in any contexts > we care about before using them and as such will end up reporting the > connector as disconnected until it's port's destruction finalizes. So, > we just do that in cases where we detect port->input has transitioned > from true->false. We don't need to worry about the other direction, > since a port without a connector isn't visible to userspace and as such > doesn't need to be protected by >base.lock until we finish > registering a connector for it. > > For updating members of drm_dp_mst_port other than port->connector, we > simply grab >base.lock in drm_dp_mst_link_probe_work() for already > registered ports, update said members and drop the lock before > potentially registering a connector and probing the link address of it's > children. > > Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock > acquisition context in
