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

[Koenig, Christian]

Am 25.10.18 um 12:36 schrieb Maarten Lankhorst:
> Op 25-10-18 om 12:21 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.
>>
>> Generated by: scripts/coccinelle/locks/call_kern.cocci
>>
>> v2:
>> syncobj->timeline still needs protect.
>>
>> v3:
>> use a global signaled fence instead of re-allocation.
>>
>> 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_drv.c     |  2 ++
>>   drivers/gpu/drm/drm_syncobj.c | 52 +++++++++++++++++++++--------------
>>   include/drm/drm_syncobj.h     |  1 +
>>   3 files changed, 34 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 36e8e9cbec52..0a6f1023d6c3 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_client.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drmP.h>
>> +#include <drm/drm_syncobj.h>
>>   
>>   #include "drm_crtc_internal.h"
>>   #include "drm_legacy.h"
>> @@ -1003,6 +1004,7 @@ static int __init drm_core_init(void)
>>   	if (ret < 0)
>>   		goto error;
>>   
>> +	drm_syncobj_stub_fence_init();
>>   	drm_core_init_complete = true;
>>   
>>   	DRM_DEBUG("Initialized\n");
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index b7eaa603f368..6b3f5a06e4d3 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -80,6 +80,27 @@ struct drm_syncobj_signal_pt {
>>   	struct list_head list;
>>   };
>>   
>> +static struct drm_syncobj_stub_fence stub_signaled_fence;
>> +static void global_stub_fence_release(struct dma_fence *fence)
>> +{
>> +	/* it is impossible to come here */
>> +	BUG();
>> +}
> WARN_ON_ONCE(1)? No need to halt the machine.
>
>> +static const struct dma_fence_ops global_stub_fence_ops = {
>> +	.get_driver_name = drm_syncobj_stub_fence_get_name,
>> +	.get_timeline_name = drm_syncobj_stub_fence_get_name,
>> +	.release = global_stub_fence_release,
>> +};
>> +
>> +void drm_syncobj_stub_fence_init(void)
>> +{
>> +	spin_lock_init(&stub_signaled_fence.lock);
>> +	dma_fence_init(&stub_signaled_fence.base,
>> +		       &global_stub_fence_ops,
>> +		       &stub_signaled_fence.lock,
>> +		       0, 0);
>> +	dma_fence_signal(&stub_signaled_fence.base);
>> +}
>>   /**
>>    * drm_syncobj_find - lookup and reference a sync object.
>>    * @file_private: drm file private pointer
>> @@ -111,24 +132,14 @@ static struct dma_fence
>>   				      uint64_t point)
>>   {
>>   	struct drm_syncobj_signal_pt *signal_pt;
>> +	struct dma_fence *f = 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,
>> -			       &fence->lock,
>> -			       syncobj->timeline_context,
>> -			       point);
>> -
>> -		dma_fence_signal(&fence->base);
>> -		return &fence->base;
>> +		dma_fence_get(&stub_signaled_fence.base);
>> +		spin_unlock(&syncobj->pt_lock);
>> +		return &stub_signaled_fence.base;
>>   	}
>>   
>>   	list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
>> @@ -137,9 +148,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);
>> +
>> +	return f;
>>   }
>>   
>>   static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
>> @@ -166,9 +180,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);
>> @@ -379,11 +391,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;
>> -	spin_unlock(&syncobj->pt_lock);
>>   	return ret;
>>   }
>>   
>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>> index 29244cbcd05e..63cfd1540241 100644
>> --- a/include/drm/drm_syncobj.h
>> +++ b/include/drm/drm_syncobj.h
>> @@ -148,4 +148,5 @@ int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>   int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags,
>>   			     struct dma_fence **fence);
>>   
>> +void drm_syncobj_stub_fence_init(void);
>>   #endif
> Move it to drm_internal.h ? This is not for driver consumption. :)
>
> i would split up the changes at this point:
>
> 1. Replace memory allocation with a static fence, not necessarily changing any locking, add a fixes tag to this commit.
>
> 2. Move locking.
>
> 3. Fix drm_syncobj_fence_get_or_add_callback to return an error.

Actually we don't need to move the locking or change into returning an 
error code any more.

Without any allocation we don't have anything that could fail.

Christian.

>
> That way it should be nicely bisectable.
>
> ~Maarten
>