Re: [PATCH 1/4] reservation: sprinkle some WARN_ON()s

[Lucas Stach <l.stach@pengutronix.de>]

Am Donnerstag, den 31.03.2016, 16:26 -0400 schrieb Rob Clark:
> 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.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  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..0de3ea6 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(!ww_mutex_is_locked(&obj->lock));
> +
I don't really like those WARN_ONs to enforce the expected locking.

IMHO lockdep_assert_held is the better way, as it's as obvious as the
WARN_ONs to someone reading the code, blows up in the same way if you
are using a lockdep enabled build (which should be default when touching
locking code) and avoids the runtime overhead for production kernels.

It also checks that it's really the expected execution strand holding
the lock and not some other thread on another CPU.

I don't know if lockdep_assert_held works with ww_mutex currently, if
not we should definitely extend it to do so.

Regards,
Lucas

>  	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(!ww_mutex_is_locked(&obj->lock));
> +
>  	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(!ww_mutex_is_locked(&obj->lock));
> +
>  	old = reservation_object_get_list(obj);
>  	if (old)
>  		i = old->shared_count;


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