[PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)
On 30 September 2015 at 17:44, Daniel Vetter wrote: > 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 > > 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. I think we were being paranoid about the work queue vs the send_link_address if you can't have multiple then it matters not. But I'll repost the cleaned up version as it makes things easier to grok. Dave.
[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
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, >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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 2/2] drm/mst: update the link_address_sent before sending the link address (v2)
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
---
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, >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
