Re: [PATCH] drm/dp_mst: Fix locking when skipping CSN before topology probing

[Imre Deak <imre.deak@intel.com>]

On Mon, Mar 10, 2025 at 08:59:51AM +0000, Lin, Wayne wrote:
> 
> > -----Original Message-----
> > From: Imre Deak <imre.deak@intel.com>
> > Sent: Saturday, March 8, 2025 2:32 AM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org
> > Cc: Lin, Wayne <Wayne.Lin@amd.com>; Lyude Paul <lyude@redhat.com>;
> > stable@vger.kernel.org
> > Subject: [PATCH] drm/dp_mst: Fix locking when skipping CSN before topology
> > probing
> >
> > The handling of the MST Connection Status Notify message is skipped if the probing
> > of the topology is still pending. Acquiring the drm_dp_mst_topology_mgr::probe_lock
> > for this in
> > drm_dp_mst_handle_up_req() is problematic: the task/work this function is called
> > from is also responsible for handling MST down-request replies (in
> > drm_dp_mst_handle_down_rep()). Thus drm_dp_mst_link_probe_work() - holding
> > already probe_lock - could be blocked waiting for an MST down-request reply while
> > drm_dp_mst_handle_up_req() is waiting for probe_lock while processing a CSN
> > message. This leads to the probe work's down-request message timing out.
> >
> > A scenario similar to the above leading to a down-request timeout is handling a CSN
> > message in drm_dp_mst_handle_conn_stat(), holding the probe_lock and sending
> > down-request messages while a second CSN message sent by the sink
> > subsequently is handled by drm_dp_mst_handle_up_req().
> >
> > Fix the above by moving the logic to skip the CSN handling to
> > drm_dp_mst_process_up_req(). This function is called from a work (separate from
> > the task/work handling new up/down messages), already holding probe_lock. This
> > solves the above timeout issue, since handling of down-request replies won't be
> > blocked by probe_lock.
> >
> > Fixes: ddf983488c3e ("drm/dp_mst: Skip CSN if topology probing is not done yet")
> > Cc: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org # v6.6+
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 40 +++++++++++--------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 8b68bb3fbffb0..3a1f1ffc7b552 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4036,6 +4036,22 @@ static int drm_dp_mst_handle_down_rep(struct
> > drm_dp_mst_topology_mgr *mgr)
> >       return 0;
> >  }
> >
> > +static bool primary_mstb_probing_is_done(struct drm_dp_mst_topology_mgr
> > +*mgr) {
> > +     bool probing_done = false;
> > +
> > +     mutex_lock(&mgr->lock);
> 
> Thanks for catching this, Imre!
>
> Here I think using mgr->lock is not sufficient for determining probing
> is done or not by mst_primary->link_address_sent. Since it might still
> be probing the rest of the topology with mst_primary probed. Use
> probe_lock instead? Thanks!

mgr->lock is taken here to guard the mgr->mst_primary access.

probe_lock is also held, taken already by the caller in
drm_dp_mst_up_req_work().

> > +
> > +     if (mgr->mst_primary && drm_dp_mst_topology_try_get_mstb(mgr-> >mst_primary)) {
> > +             probing_done = mgr->mst_primary->link_address_sent;
> > +             drm_dp_mst_topology_put_mstb(mgr->mst_primary);
> > +     }
> > +
> > +     mutex_unlock(&mgr->lock);
> > +
> > +     return probing_done;
> > +}