| Message ID | 1459456012-16248-2-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
Am 31.03.2016 um 22:26 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> Reviewed-by: Christian König <christian.koenig@amd.com> for this one. One item to purge from my todo list, thanks! Christian. > --- > 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)); > + > 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;
On Fri, Apr 1, 2016 at 4:48 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > 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. I guess it should, since there is reservation_object_held().. I suppose I should use this. I suppose all the WARN_ON(!drm_modeset_is_locked()) should someday get the same treatment.. BR, -R > 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; > >
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)); + 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;
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(+)