[Intel-gfx] [PATCH] drm: fix call_kern.cocci warnings v2

[Maarten Lankhorst]

Op 25-10-18 om 10:53 schreef Chunming Zhou:
> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL
>
>   Find functions that refer to GFP_KERNEL but are called with locks held.
>
> Semantic patch information:
>   The proposed change of converting the GFP_KERNEL is not necessarily the
>   correct one.  It may be desired to unlock the lock, or to not call the
>   function under the lock in the first place.
>
> Generated by: scripts/coccinelle/locks/call_kern.cocci
>
> v2:
> syncobj->timeline still needs protect.
>
> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Cc: Christian König <easy2remember.chk at googlemail.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b7eaa603f368..b843d4c2778c 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -111,15 +111,16 @@ static struct dma_fence
>  				      uint64_t point)
>  {
>  	struct drm_syncobj_signal_pt *signal_pt;
> +	struct dma_fence *f = NULL;
> +	struct drm_syncobj_stub_fence *fence =
> +		kzalloc(sizeof(struct drm_syncobj_stub_fence),
> +			GFP_KERNEL);
>  
> +	if (!fence)
> +		return NULL;
> +	spin_lock(&syncobj->pt_lock);
>  	if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
>  	    (point <= syncobj->timeline)) {
> -		struct drm_syncobj_stub_fence *fence =
> -			kzalloc(sizeof(struct drm_syncobj_stub_fence),
> -				GFP_KERNEL);
> -
> -		if (!fence)
> -			return NULL;
>  		spin_lock_init(&fence->lock);
>  		dma_fence_init(&fence->base,
>  			       &drm_syncobj_stub_fence_ops,
> @@ -128,6 +129,7 @@ static struct dma_fence
>  			       point);
>  
>  		dma_fence_signal(&fence->base);
> +		spin_unlock(&syncobj->pt_lock);
>  		return &fence->base;
>  	}
>  
> @@ -137,9 +139,12 @@ static struct dma_fence
>  		if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) &&
>  		    (point != signal_pt->value))
>  			continue;
> -		return dma_fence_get(&signal_pt->fence_array->base);
> +		f = dma_fence_get(&signal_pt->fence_array->base);
> +		break;
>  	}
> -	return NULL;
> +	spin_unlock(&syncobj->pt_lock);
> +	kfree(fence);
> +	return f;
>  }
>  
>  static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
> @@ -166,9 +171,7 @@ static void drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  	}
>  
>  	mutex_lock(&syncobj->cb_mutex);
> -	spin_lock(&syncobj->pt_lock);
>  	*fence = drm_syncobj_find_signal_pt_for_point(syncobj, pt_value);
> -	spin_unlock(&syncobj->pt_lock);
>  	if (!*fence)
>  		drm_syncobj_add_callback_locked(syncobj, cb, func);
>  	mutex_unlock(&syncobj->cb_mutex);
Maybe slightly offtopic, but seems that this function should return an error if fence == NULL,
at least when it's a result of -ENOMEM.
It would be nice if this was sent as a separate patch, instead of waiting indefinitely..


> @@ -379,11 +382,9 @@ drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags,
>  		if (ret < 0)
>  			return ret;
>  	}
> -	spin_lock(&syncobj->pt_lock);
>  	*fence = drm_syncobj_find_signal_pt_for_point(syncobj, point);
>  	if (!*fence)
>  		ret = -EINVAL;
There's no way to make a destinction between -ENOMEM and -EINVAL, I think it would be good to use ERR_PTR, and then
return PTR_ERR_OR_ZERO()..

I would also change the subject line to something more descriptive, rather than the generic one it's now. But otherwise looks good to me. :)