reservation: sprinkle some WARN_ON()s

Submitted by Rob Clark on April 8, 2016, 12:52 a.m.

Details

Message ID 1460076776-25023-1-git-send-email-robdclark@gmail.com
State New
Headers show
Series "reservation: sprinkle some WARN_ON()s" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Rob Clark April 8, 2016, 12:52 a.m.
A bit overkill since, for example, the rcu_dereference_protected() in
reservation_object_get_list() will WARN.  But this is much less subtle
for folks reading the code.

v2: use reservation_object_held() instead of ww_mutex_is_locked()

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/dma-buf/reservation.c | 6 ++++++
 1 file changed, 6 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index c0bd572..439af82 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -52,6 +52,8 @@  int reservation_object_reserve_shared(struct reservation_object *obj)
 	struct reservation_object_list *fobj, *old;
 	u32 max;
 
+	WARN_ON(!reservation_object_held(obj));
+
 	old = reservation_object_get_list(obj);
 
 	if (old && old->shared_max) {
@@ -189,6 +191,8 @@  void reservation_object_add_shared_fence(struct reservation_object *obj,
 {
 	struct reservation_object_list *old, *fobj = obj->staged;
 
+	WARN_ON(!reservation_object_held(obj));
+
 	old = reservation_object_get_list(obj);
 	obj->staged = NULL;
 
@@ -207,6 +211,8 @@  void reservation_object_add_excl_fence(struct reservation_object *obj,
 	struct reservation_object_list *old;
 	u32 i = 0;
 
+	WARN_ON(!reservation_object_held(obj));
+
 	old = reservation_object_get_list(obj);
 	if (old)
 		i = old->shared_count;

Comments

On Thu, Apr 07, 2016 at 08:52:56PM -0400, Rob Clark wrote:
> A bit overkill since, for example, the rcu_dereference_protected() in
> reservation_object_get_list() will WARN.  But this is much less subtle
> for folks reading the code.
> 
> v2: use reservation_object_held() instead of ww_mutex_is_locked()
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Doesn't seem to build here:


In file included from ./arch/arm/include/asm/bug.h:59:0,
                 from include/linux/bug.h:4,
                 from include/linux/thread_info.h:11,
                 from include/asm-generic/current.h:4,
                 from arch/arm/include/generated/asm/current.h:1,
                 from include/linux/mutex.h:13,
                 from include/linux/ww_mutex.h:17,
                 from include/linux/reservation.h:42,
                 from drivers/dma-buf/reservation.c:35:
drivers/dma-buf/reservation.c: In function ‘reservation_object_reserve_shared’:
include/linux/reservation.h:82:38: error: implicit declaration of function ‘lockdep_is_held’ [-Werror=implicit-function-declaration]
 #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base)
                                      ^
include/asm-generic/bug.h:92:25: note: in definition of macro ‘WARN_ON’
  int __ret_warn_on = !!(condition);    \
                         ^
drivers/dma-buf/reservation.c:74:11: note: in expansion of macro ‘reservation_object_held’
  WARN_ON(!reservation_object_held(obj));

Sounds like we should include some headers from reservation.h.
-Daniel

> ---
>  drivers/dma-buf/reservation.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index c0bd572..439af82 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -52,6 +52,8 @@ int reservation_object_reserve_shared(struct reservation_object *obj)
>  	struct reservation_object_list *fobj, *old;
>  	u32 max;
>  
> +	WARN_ON(!reservation_object_held(obj));
> +
>  	old = reservation_object_get_list(obj);
>  
>  	if (old && old->shared_max) {
> @@ -189,6 +191,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
>  {
>  	struct reservation_object_list *old, *fobj = obj->staged;
>  
> +	WARN_ON(!reservation_object_held(obj));
> +
>  	old = reservation_object_get_list(obj);
>  	obj->staged = NULL;
>  
> @@ -207,6 +211,8 @@ void reservation_object_add_excl_fence(struct reservation_object *obj,
>  	struct reservation_object_list *old;
>  	u32 i = 0;
>  
> +	WARN_ON(!reservation_object_held(obj));
> +
>  	old = reservation_object_get_list(obj);
>  	if (old)
>  		i = old->shared_count;
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Jun 4, 2016 at 8:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 07, 2016 at 08:52:56PM -0400, Rob Clark wrote:
>> A bit overkill since, for example, the rcu_dereference_protected() in
>> reservation_object_get_list() will WARN.  But this is much less subtle
>> for folks reading the code.
>>
>> v2: use reservation_object_held() instead of ww_mutex_is_locked()
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
> Doesn't seem to build here:
>
>
> In file included from ./arch/arm/include/asm/bug.h:59:0,
>                  from include/linux/bug.h:4,
>                  from include/linux/thread_info.h:11,
>                  from include/asm-generic/current.h:4,
>                  from arch/arm/include/generated/asm/current.h:1,
>                  from include/linux/mutex.h:13,
>                  from include/linux/ww_mutex.h:17,
>                  from include/linux/reservation.h:42,
>                  from drivers/dma-buf/reservation.c:35:
> drivers/dma-buf/reservation.c: In function ‘reservation_object_reserve_shared’:
> include/linux/reservation.h:82:38: error: implicit declaration of function ‘lockdep_is_held’ [-Werror=implicit-function-declaration]
>  #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base)
>                                       ^
> include/asm-generic/bug.h:92:25: note: in definition of macro ‘WARN_ON’
>   int __ret_warn_on = !!(condition);    \
>                          ^
> drivers/dma-buf/reservation.c:74:11: note: in expansion of macro ‘reservation_object_held’
>   WARN_ON(!reservation_object_held(obj));
>
> Sounds like we should include some headers from reservation.h.


Sumit hit something similar.. not sure if he figured out the problem.
But seemed to only work if lockdep was enabled, iirc?  It is probably
true that I didn't try building without lockdep..  although I don't
remember kbuild robot complaining.  I'm a bit confused since
reservation_object_held() was used before.

Hmm, on second thought, without lockdep, rcu_lockdep_assert() macro is
a no-op, so the use of reservation_object_held() goes away..  :-/

Maybe we should just go back to earlier version of the patch which
used mutex_is_locked() instead?

BR,
-R

> -Daniel
>
>> ---
>>  drivers/dma-buf/reservation.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>> index c0bd572..439af82 100644
>> --- a/drivers/dma-buf/reservation.c
>> +++ b/drivers/dma-buf/reservation.c
>> @@ -52,6 +52,8 @@ int reservation_object_reserve_shared(struct reservation_object *obj)
>>       struct reservation_object_list *fobj, *old;
>>       u32 max;
>>
>> +     WARN_ON(!reservation_object_held(obj));
>> +
>>       old = reservation_object_get_list(obj);
>>
>>       if (old && old->shared_max) {
>> @@ -189,6 +191,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
>>  {
>>       struct reservation_object_list *old, *fobj = obj->staged;
>>
>> +     WARN_ON(!reservation_object_held(obj));
>> +
>>       old = reservation_object_get_list(obj);
>>       obj->staged = NULL;
>>
>> @@ -207,6 +211,8 @@ void reservation_object_add_excl_fence(struct reservation_object *obj,
>>       struct reservation_object_list *old;
>>       u32 i = 0;
>>
>> +     WARN_ON(!reservation_object_held(obj));
>> +
>>       old = reservation_object_get_list(obj);
>>       if (old)
>>               i = old->shared_count;
>> --
>> 2.5.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch