[PATCH] drm: fix call_kern.cocci warnings v3

[PATCH] drm: fix call_kern.cocci warnings v3

[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@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: Christian König <easy2remember.chk@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();
+}
+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
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Christian König <easy2remember.chk@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.

That way it should be nicely bisectable.

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@amd.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: Christian König <easy2remember.chk@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
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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@amd.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: Christian König <easy2remember.chk@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.
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.

Thanks,
David
>
> That way it should be nicely bisectable.
>
> ~Maarten
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

Give this a nice summary,

drm/syncobj: Avoid kmalloc(GFP_KERNEL) under spinlock

Quoting Chunming Zhou (2018-10-25 11:21:05)
> 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.
> 

Good practice, would be to add Testcase: (and as penance for the bug,
write it ;)

> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Christian König <easy2remember.chk@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();

BUG() is overkill, kasan will complain for us anyway, so we can just use
the default release function.

> +}
> +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)

I think we can avoid having this exposed by:

static DECLARE_SPINLOCK(signaled_fence_lock);
static dma_fence signaled_fence;

static struct dma_fence *signaled_fence_get(void)
{
	spin_lock(&signaled_fenced_lock);
	if (!signaled_fence.ops) {
		dma_fence_init(&signaled_fence,
			       &signaled_fence_ops,
			       &signaled_fence_lock,
			       0, 0);
		dma_fence_signal_locked(&signaled_fence.base);
	}
	spin_unlock(&signaled_fenced_lock);

	return dma_fence_get(&signaled_fence);
}

>  /**
>   * 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;

f = signaled_fence_get();
goto unlock;
>         }
>  
>         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;

unlock:
> +       spin_unlock(&syncobj->pt_lock);
> +
> +       return f;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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@amd.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: Christian König <easy2remember.chk@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.
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel