[Public]
> -----Original Message-----
> From: Imre Deak <imre.deak@xxxxxxxxx>
> Sent: Monday, March 10, 2025 7:00 PM
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; Lyude Paul <lyude@xxxxxxxxxx>;
> stable@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> > > Sent: Saturday, March 8, 2025 2:32 AM
> > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> > > dri- devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Lin, Wayne <Wayne.Lin@xxxxxxx>; Lyude Paul <lyude@xxxxxxxxxx>;
> > > stable@xxxxxxxxxxxxxxx
> > > 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@xxxxxxx>
> > > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx # v6.6+
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxx>
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
