On Mon, Oct 21, 2019 at 10:36:00PM -0400, Lyude Paul wrote:
> Currently, MST lacks locking in a lot of places that really should have
> some sort of locking. Hotplugging and link address code paths are some
> of the offenders here, as there is actually nothing preventing us from
> running a link address probe while at the same time handling a
> connection status update request - something that's likely always been
> possible but never seen in the wild because hotplugging has been broken
> for ages now (with the exception of amdgpu, for reasons I don't think
> are worth digging into very far).
>
> Note: I'm going to start using the term "in-memory topology layout" here
> to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
>
> Locking in these places is a little tougher then it looks though.
> Generally we protect anything having to do with the in-memory topology
> layout under >lock. But this becomes nearly impossible to do from
> the context of link address probes due to the fact that >lock is
> usually grabbed under random various modesetting locks, meaning that
> there's no way we can just invert the >lock order and keep it
> locked throughout the whole process of updating the topology.
>
> Luckily there are only two workers which can modify the in-memory
> topology layout: drm_dp_mst_up_req_work() and
> drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
> workers from traveling the topology layout in parallel with the intent
> of updating it we don't need to worry about grabbing >lock in these
> workers for reads. We only need to grab >lock in these workers for
> writes, so that readers outside these two workers are still protected
> from the topology layout changing beneath them.
>
> So, add the new >probe_lock and use it in both
> drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
> add some more detailed explanations for how this locking is intended to
> work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
This seems like a good solution to me, thanks for working this through!
Any chance we could add a WARN_ON(!mutex_is_locked(>probe_lock)); somewhere
centrally called by all paths modifying the in-memory topology layout?
drm_dp_add_port perhaps? That way we have a fallback in case something else
starts mucking with the topology.
Other than that,
Reviewed-by: Sean Paul
>
> Signed-off-by: Lyude Paul
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Cc: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++-
> include/drm/drm_dp_mst_helper.h | 32 +++
> 2 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 08c316a727df..11d842f0bff5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct
> drm_dp_mst_topology_mgr *m
> struct drm_dp_mst_branch *mstb)
> {
> struct drm_dp_mst_port *port;
> - struct drm_dp_mst_branch *mstb_child;
> +
> if (!mstb->link_address_sent)
> drm_dp_send_link_address(mgr, mstb);
>
> list_for_each_entry(port, >ports, next) {
> - if (port->input)
> - continue;
> + struct drm_dp_mst_branch *mstb_child = NULL;
>
> - if (!port->ddps)
> + if (port->input || !port->ddps)
> continue;
>
> if (!port->available_pbn)
> drm_dp_send_enum_path_resources(mgr, mstb, port);
>
> - if (port->mstb) {
> + if (port->mstb)
> mstb_child = drm_dp_mst_topology_get_mstb_validated(
> mgr, port->mstb);
> - if (mstb_child) {
> - drm_dp_check_and_send_link_address(mgr,
> mstb_child);
> - drm_dp_mst_topology_put_mstb(mstb_child);
> - }
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
> }
> }
> }
>
> static void drm_dp_mst_link_probe_work(struct work_struct *work)
> {
> - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct
> drm_dp_mst_topology_mgr, work);
> + struct drm_dp_mst_topology_mgr *mgr =
> + container_of(work, struct drm_dp_mst_topology_mgr, work);
> + struct drm_device *dev = mgr->dev;
> struct drm_dp_mst_branch *mstb;
> int ret;
>
> + mutex_lock(>probe_lock);
> +
> mutex_lock(>lock);
> mstb = mgr->mst_primary;
> if (mstb) {
> @@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct
>