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 01:01:25PM +0000, Lin, Wayne wrote:
> [Public]
> 
> > -----Original Message-----
> > From: Imre Deak <imre.deak@intel.com>
> > Sent: Monday, March 10, 2025 7:00 PM
> > To: Lin, Wayne <Wayne.Lin@amd.com>
> > Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; Lyude Paul <lyude@redhat.com>;
> > stable@vger.kernel.org
> > Subject: Re: [PATCH] drm/dp_mst: Fix locking when skipping CSN before topology
> > probing
> >
> > 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().
> 
> Oh I see. It looks good to me. Feel free to add:
> 
> Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>

Thanks for the review.

Lyude, are you ok with the change and if I push it to drm-misc-fixes?

> 
> Thanks!
> >
> > > > +
> > > > +     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;
> > > > +}
> --
> Regards,
> Wayne Lin