Re: [PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)

On Sun, Sep 06, 2015 at 06:53:00PM +1000, Dave Airlie wrote:

There is a race where the reply could get processed by another work queue before we've updated the state.

Update the state before sending the msg to close it.

v2: reset value if return indicates we haven't send the msg.

Pointed out by Adam J Richter on

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91481 Signed-off-by: Dave Airlie airlied@redhat.com

Ok we see to have two callers of drm_dp_send_link_address: - drm_dp_add_port, but the only caller of that in turn is again drm_dp_send_link_address. This is just self-recursion. - drm_dp_check_and_send_link_address, which in turn is called only by itself and by drm_dp_mst_link_probe_work.

drm_dp_mst_link_probe_work is only called from the mgr->work function, and a single work item is never run concurrently.

I don't see any race here at all. A good cleanup though would be what Adam's original patch has done and move the link_address_sent = true into drm_dp_send_link_address.

Cheers, Daniel

---

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

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 294d904..831096b9 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1101,8 +1101,10 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,

ret = drm_dp_port_setup_pdt(port);
if (ret == true) {

• 	drm_dp_send_link_address(mstb->mgr, port->mstb);
  port->mstb->link_address_sent = true;
  

• 	ret = drm_dp_send_link_address(mstb->mgr, port->mstb);
  

• 	if (ret < 0)
  

• 		port->mstb->link_address_sent = false;
  

  } }

@@ -1198,8 +1200,11 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m struct drm_dp_mst_port *port; struct drm_dp_mst_branch *mstb_child; if (!mstb->link_address_sent) {

• drm_dp_send_link_address(mgr, mstb);
  

• int ret;
  

  mstb->link_address_sent = true;

• ret = drm_dp_send_link_address(mgr, mstb);
  

• if (ret < 0)
  

• 	mstb->link_address_sent = false;
  

  } list_for_each_entry(port, &mstb->ports, next) { if (port->input)

@@ -1493,11 +1498,12 @@ static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, } (*mgr->cbs->hotplug)(mgr); }

• ret = 0;
  

  } else DRM_DEBUG_KMS("link address failed %d\n", ret);

  kfree(txmsg);

• return 0;

• return ret;

}

static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,

2.4.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel