On Mon March 11 2013 12:45:39 Hans Verkuil wrote:
> From: Andy Walls <awalls at>
Oops, somehow I messed up Andy's email. I've corrected it in my git tree.
Regards,
Hans
>
> When calling v4l2_ctrl_add_handler(), lockdep would detect a potential
> recursive locking problem on a situation that is by design intended and
> not a recursive lock. This happened because all struct
> v4l2_ctrl_handler.lock mutexes were created as members of the same lock
> class in v4l2_ctrl_handler_init(), and v4l2_ctrl_add_handler() takes the
> hdl->lock on two different v4l2_ctrl_handler instances.
>
> This change breaks the large lockdep lock class for struct
> v4l2_ctrl_handler.lock and breaks it into v4l2_ctrl_handler
> instantiation specific lock classes with meaningful class names.
>
> This will validly eliminate lockdep alarms for v4l2_ctrl_handler locking
> validation, as long as the relationships between drivers adding v4l2
> controls to their own handler from other v4l2 drivers' control handlers
> remains straightforward.
>
> struct v4l2_ctrl_handler.lock lock classes are created with names such
> that the output of cat /proc/lockdep indicates where in the v4l2 driver
> code v4l2_ctrl_handle_init() is being called on instantiations:
>
> ffffffffa045f490 FD: 10 BD: 8 +.+...: cx2341x:1534:(hdl)->lock
> ffffffffa0497d20 FD: 12 BD: 2 +.+.+.: saa7115:1581:(hdl)->lock
> ffffffffa04ac660 FD: 14 BD: 2 +.+.+.: msp3400_driver:756:(hdl)->lock
> ffffffffa0484b90 FD: 12 BD: 1 +.+.+.: ivtv_gpio:366:(&itv->hdl_gpio)->lock
> ffffffffa04eb530 FD: 11 BD: 2 +.+.+.: cx25840_core:1982:(&state->hdl)->lock
> ffffffffa04fbc80 FD: 11 BD: 3 +.+.+.: wm8775:246:(&state->hdl)->lock
>
> Some lock chains, that were previously causing the recursion alarms, are
> now visible in the output of cat /proc/lockdep_chains:
>
> irq_context: 0
> [ffffffffa0497d20] saa7115:1581:(hdl)->lock
> [ffffffffa045f490] cx2341x:1534:(hdl)->lock
>
> irq_context: 0
> [ffffffffa04ac660] msp3400_driver:756:(hdl)->lock
> [ffffffffa045f490] cx2341x:1534:(hdl)->lock
>
> irq_context: 0
> [ffffffffa0484b90] ivtv_gpio:366:(&itv->hdl_gpio)->lock
> [ffffffffa045f490] cx2341x:1534:(hdl)->lock
>
> irq_context: 0
> [ffffffffa04eb530] cx25840_core:1982:(&state->hdl)->lock
> [ffffffffa045f490] cx2341x:1534:(hdl)->lock
>
> irq_context: 0
> [ffffffffa04fbc80] wm8775:246:(&state->hdl)->lock
> [ffffffffa045f490] cx2341x:1534:(hdl)->lock
>
> Signed-off-by: Andy Walls <awalls <at> md.metrocast.net>
> [hans.verkuil@xxxxxxxxx: keep mutex_init in v4l2_ctrl_handler_init_class]
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 8 +++++---
> include/media/v4l2-ctrls.h | 29 ++++++++++++++++++++++++++---
> 2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 6b28b58..b36d1ec 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1362,11 +1362,13 @@ static inline int handler_set_err(struct v4l2_ctrl_handler *hdl, int err)
> }
>
> /* Initialize the handler */
> -int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
> - unsigned nr_of_controls_hint)
> +int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
> + unsigned nr_of_controls_hint,
> + struct lock_class_key *key, const char *name)
> {
> hdl->lock = &hdl->_lock;
> mutex_init(hdl->lock);
> + lockdep_set_class_and_name(hdl->lock, key, name);
> INIT_LIST_HEAD(&hdl->ctrls);
> INIT_LIST_HEAD(&hdl->ctrl_refs);
> hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
> @@ -1375,7 +1377,7 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
> hdl->error = hdl->buckets ? 0 : -ENOMEM;
> return hdl->error;
> }
> -EXPORT_SYMBOL(v4l2_ctrl_handler_init);
> +EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
>
> /* Free all controls and control refs */
> void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f00d42b..7343a27 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -259,7 +259,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> s32 *min, s32 *max, s32 *step, s32 *def, u32 *flags);
>
>
> -/** v4l2_ctrl_handler_init() - Initialize the control handler.
> +/** v4l2_ctrl_handler_init_class() - Initialize the control handler.
> * @hdl: The control handler.
> * @nr_of_controls_hint: A hint of how many controls this handler is
> * expected to refer to. This is the total number, so including
> @@ -268,12 +268,35 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> * are allocated) or the control lookup becomes slower (not enough
> * buckets are allocated, so there are more slow list lookups).
> * It will always work, though.
> + * @key: Used by the lock validator if CONFIG_LOCKDEP is set.
> + * @name: Used by the lock validator if CONFIG_LOCKDEP is set.
> *
> * Returns an error if the buckets could not be allocated. This error will
> * also be stored in @hdl->error.
> + *
> + * Never use this call directly, always use the v4l2_ctrl_handler_init
> + * macro that hides the @key and @name arguments.
> */
> -int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
> - unsigned nr_of_controls_hint);
> +int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
> + unsigned nr_of_controls_hint,
> + struct lock_class_key *key, const char *name);
> +
> +#ifdef CONFIG_LOCKDEP
> +#define v4l2_ctrl_handler_init(hdl, nr_of_controls_hint) \
> +( \
> + ({ \
> + static struct lock_class_key _key; \
> + v4l2_ctrl_handler_init_class(hdl, nr_of_controls_hint, \
> + &_key, \
> + KBUILD_BASENAME ":" \
> + __stringify(__LINE__) ":" \
> + "(" #hdl ")->_lock"); \
> + }) \
> +)
> +#else
> +#define v4l2_ctrl_handler_init(hdl, nr_of_controls_hint) \
> + v4l2_ctrl_handler_init_class(hdl, nr_of_controls_hint, NULL, NULL)
> +#endif
>
> /** v4l2_ctrl_handler_free() - Free all controls owned by the handler and free
> * the control list.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
