| Message ID | 1460076776-25023-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(+)