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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel