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.z...@amd.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: intel-...@lists.freedesktop.org
> Cc: Christian König <easy2remember....@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. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
