Re: [PATCH] drm: fix call_kern.cocci warnings v3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 25-10-18 om 14:01 schreef Chunming Zhou:
>
> 在 2018/10/25 18:36, 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@xxxxxxx>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> Cc: Christian König <easy2remember.chk@xxxxxxxxxxxxxx>
>>> ---
>>>   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.
> No necessary, why change it?
>
>> 3. Fix drm_syncobj_fence_get_or_add_callback to return an error.
> why does it still need a return value? Chris and I are trying to avoid that.
I just need a clarification on why it's ok to ignore to return fence not found. Could be a comment in the code as well. :)
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux