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().
> > +
> > + 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;
> > +}
