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(-)
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);
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);
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
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Lyude Paul