Reviewed-by: Lyude Paul lyude@redhat.com
And yes - feel free to push this change!
On Mon, 2025-03-10 at 19:24 +0200, Imre Deak wrote:
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