[PATCH 0/2] drm/dp_mst: Fix link address probing regressions

[PATCH 0/2] drm/dp_mst: Fix link address probing regressions

[Lyude Paul]

While fixing some regressions caused by introducing bandwidth checking
into the DP MST atomic helpers, I realized there was another much more
subtle regression that got introduced by a seemingly harmless patch to
fix unused variable errors while compiling with W=1 (mentioned in patch
2). Basically, this regression makes it so sometimes link address
appears to "hang". This patch series fixes it.

Lyude Paul (2):
  drm/dp_mst: Make drm_dp_mst_dpcd_write() consistent with
    drm_dp_dpcd_write()
  drm/dp_mst: Fix drm_dp_check_mstb_guid() return code

 drivers/gpu/drm/drm_dp_mst_topology.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

-- 
2.24.1

[PATCH 1/2] drm/dp_mst: Make drm_dp_mst_dpcd_write() consistent with drm_dp_dpcd_write()

[Lyude Paul]

Noticed this while having some problems with hubs sometimes not being
detected on the first plug. Every single dpcd read or write function
returns the number of bytes transferred on success or a negative error
code, except apparently for drm_dp_mst_dpcd_write() - which returns 0 on
success.

There's not really any good reason for this difference that I can tell,
and having the two functions give differing behavior means that
drm_dp_dpcd_write() will end up returning 0 on success for MST devices,
but the number of bytes transferred for everything else.

So, fix that and update the kernel doc.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 2f221a5efed4 ("drm/dp_mst: Add MST support to DP DPCD R/W functions")
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 6c62ad8f4414..e421c2d13267 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2063,7 +2063,7 @@ ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
  * sideband messaging as drm_dp_dpcd_write() does for local
  * devices via actual AUX CH.
  *
- * Return: 0 on success, negative error code on failure.
+ * Return: number of bytes written on success, negative error code on failure.
  */
 ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
 			      unsigned int offset, void *buffer, size_t size)
@@ -3428,12 +3428,9 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
 	drm_dp_queue_down_tx(mgr, txmsg);
 
 	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
-	if (ret > 0) {
-		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
-			ret = -EIO;
-		else
-			ret = 0;
-	}
+	if (ret > 0 && txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
+		ret = -EIO;
+
 	kfree(txmsg);
 fail_put:
 	drm_dp_mst_topology_put_mstb(mstb);
-- 
2.24.1

[PATCH 2/2] drm/dp_mst: Fix drm_dp_check_mstb_guid() return code

[Lyude Paul]

We actually expect this to return a 0 on success, or negative error code
on failure. In order to do that, we check whether or not we managed to
write the whole GUID and then return 0 if so, otherwise return a
negative error code. Also, let's add an error message here so it's a
little more obvious when this fails in the middle of a link address
probe.

This should fix issues with certain MST hubs seemingly stopping for no
reason in the middle of the link address probe process.

Fixes: cb897542c6d2 ("drm/dp_mst: Fix W=1 warnings")
Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index e421c2d13267..b2ec6e8634ed 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2092,7 +2092,10 @@ static int drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
 		}
 	}
 
-	return ret;
+	if (ret < 16 && ret > 0)
+		return -EPROTO;
+
+	return ret == 16 ? 0 : ret;
 }
 
 static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
@@ -2907,8 +2910,14 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
 	drm_dp_dump_link_address(reply);
 
 	ret = drm_dp_check_mstb_guid(mstb, reply->guid);
-	if (ret)
+	if (ret) {
+		char buf[64];
+
+		drm_dp_mst_rad_to_str(mstb->rad, mstb->lct, buf, sizeof(buf));
+		DRM_ERROR("GUID check on %s failed: %d\n",
+			  buf, ret);
 		goto out;
+	}
 
 	for (i = 0; i < reply->nports; i++) {
 		port_mask |= BIT(reply->ports[i].port_number);
-- 
2.24.1

Re: [PATCH 0/2] drm/dp_mst: Fix link address probing regressions

[Alex Deucher]

On Fri, Mar 6, 2020 at 6:49 PM Lyude Paul <lyude@redhat.com> wrote:
>
> While fixing some regressions caused by introducing bandwidth checking
> into the DP MST atomic helpers, I realized there was another much more
> subtle regression that got introduced by a seemingly harmless patch to
> fix unused variable errors while compiling with W=1 (mentioned in patch
> 2). Basically, this regression makes it so sometimes link address
> appears to "hang". This patch series fixes it.

Series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Lyude Paul (2):
>   drm/dp_mst: Make drm_dp_mst_dpcd_write() consistent with
>     drm_dp_dpcd_write()
>   drm/dp_mst: Fix drm_dp_check_mstb_guid() return code
>
>  drivers/gpu/drm/drm_dp_mst_topology.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel