Hi Tomi,
Thank you for the patch.
On Mon, Aug 30, 2021 at 02:00:45PM +0300, Tomi Valkeinen wrote:
> The V4L2 subdevs have managed without centralized locking for the state
> (previously pad_config), as the TRY state is supposedly safe (although I
> believe two TRY ioctls for the same fd would race), and the ACTIVE
> state, and its locking, is managed by the drivers internally.
>
> We now have ACTIVE state in a centralized position, and need locking.
> Strictly speaking the locking is only needed for new drivers that use
> the new state, as the current drivers continue behaving as they used to.
>
> Add a mutex to the struct v4l2_subdev_state, along with a few helper
> functions for locking/unlocking.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 43 +++++++++++++++++----
> include/media/v4l2-subdev.h | 55 +++++++++++++++++++++++++--
> 2 files changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b3637cddca58..b1e65488210d 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -26,9 +26,11 @@
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> {
> + static struct lock_class_key __key;
> struct v4l2_subdev_state *state;
>
> - state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY);
> + state = __v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY,
> + "v4l2_subdev_fh->state", &__key);
What's the reason for not using the v4l2_alloc_subdev_state() macro here
?
> if (IS_ERR(state))
> return PTR_ERR(state);
>
> @@ -924,8 +926,10 @@ int v4l2_subdev_link_validate(struct media_link *link)
> EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>
> struct v4l2_subdev_state *
> -v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
> - enum v4l2_subdev_format_whence which)
> +__v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
> + enum v4l2_subdev_format_whence which,
> + const char *lock_name,
> + struct lock_class_key *lock_key)
> {
> struct v4l2_subdev_state *state;
> int ret;
> @@ -934,6 +938,8 @@ v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
> if (!state)
> return ERR_PTR(-ENOMEM);
>
> + __mutex_init(&state->lock, lock_name, lock_key);
> +
> state->which = which;
>
> if (sd->entity.num_pads) {
> @@ -960,13 +966,15 @@ v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
>
> return ERR_PTR(ret);
> }
> -EXPORT_SYMBOL_GPL(v4l2_alloc_subdev_state);
> +EXPORT_SYMBOL_GPL(__v4l2_alloc_subdev_state);
>
> void v4l2_free_subdev_state(struct v4l2_subdev_state *state)
> {
> if (!state)
> return;
>
> + mutex_destroy(&state->lock);
> +
> kvfree(state->pads);
> kfree(state);
> }
> @@ -1001,11 +1009,12 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
>
> -int v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
> +int __v4l2_subdev_alloc_state(struct v4l2_subdev *sd, const char *name,
> + struct lock_class_key *key)
> {
> struct v4l2_subdev_state *state;
>
> - state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE);
> + state = __v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE, name, key);
I already know that Sakari will ask for a line wrap at 80 columns, and
that would be my preference as well :-) I won't repeat the comment in
the rest of the series. Going over 80 columns is fine when it improves
readability, but in many places keeping lines short enough would be
nicer.
> if (IS_ERR(state))
> return PTR_ERR(state);
>
> @@ -1013,7 +1022,7 @@ int v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
> +EXPORT_SYMBOL_GPL(__v4l2_subdev_alloc_state);
>
> void v4l2_subdev_free_state(struct v4l2_subdev *sd)
> {
> @@ -1021,3 +1030,23 @@ void v4l2_subdev_free_state(struct v4l2_subdev *sd)
> sd->state = NULL;
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
> +
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> +{
> + mutex_lock(&sd->state->lock);
> +
> + return sd->state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
> +
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> + mutex_lock(&state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
> +
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> + mutex_unlock(&state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 5ec78ffda4f5..52a725281b23 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -655,6 +655,7 @@ struct v4l2_subdev_pad_config {
> /**
> * struct v4l2_subdev_state - Used for storing subdev state information.
> *
> + * @lock: mutex for the state
> * @which: state type (from enum v4l2_subdev_format_whence)
> * @pads: &struct v4l2_subdev_pad_config array
> *
> @@ -663,6 +664,7 @@ struct v4l2_subdev_pad_config {
> * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
> */
> struct v4l2_subdev_state {
> + struct mutex lock;
> u32 which;
> struct v4l2_subdev_pad_config *pads;
> };
> @@ -1147,9 +1149,18 @@ int v4l2_subdev_link_validate(struct media_link *link);
> *
> * Must call v4l2_free_subdev_state() when state is no longer needed.
> */
> +#define v4l2_alloc_subdev_state(sd, which) \
> + ({ \
> + static struct lock_class_key __key; \
> + const char *name = KBUILD_BASENAME \
> + ":" __stringify(__LINE__) ":sd->state->lock"; \
> + __v4l2_alloc_subdev_state(sd, which, name, &__key); \
> + })
> +
> struct v4l2_subdev_state *
> -v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
> - enum v4l2_subdev_format_whence which);
> +__v4l2_alloc_subdev_state(struct v4l2_subdev *sd,
> + enum v4l2_subdev_format_whence which,
> + const char *lock_name, struct lock_class_key *key);
>
> /**
> * v4l2_free_subdev_state - free a v4l2_subdev_state
> @@ -1234,7 +1245,16 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> *
> * Must call v4l2_subdev_free_state() when the state is no longer needed.
> */
> -int v4l2_subdev_alloc_state(struct v4l2_subdev *sd);
> +#define v4l2_subdev_alloc_state(sd) \
> + ({ \
> + static struct lock_class_key __key; \
> + const char *name = KBUILD_BASENAME \
> + ":" __stringify(__LINE__) ":sd->state->lock"; \
> + __v4l2_subdev_alloc_state(sd, name, &__key); \
> + })
> +
> +int __v4l2_subdev_alloc_state(struct v4l2_subdev *sd, const char *name,
> + struct lock_class_key *key);
>
> /**
> * v4l2_subdev_free_state() - Free the active subdev state for subdevice
> @@ -1258,4 +1278,33 @@ v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> return sd->state;
> }
>
> +/**
> + * v4l2_subdev_lock_active_state() - Lock and return the active subdev state for subdevice
> + * @sd: The subdevice
> + *
> + * Return the locked active state for the subdevice, or NULL if the subdev
> + * does not support active state.
> + *
> + * Must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_lock_state() - Lock the subdev state
> + * @state: The subdevice state
> + *
> + * Lock the given subdev state.
> + *
> + * Must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
This seems to be used only to lock the state passed to the subdev
operation by the caller. Could the caller lock the state instead ? This
could possibly be done by wrapping the v4l2_subdev_call() calls in
dedicated helper functions.
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlock the subdev state
> + * @state: The subdevice state
> + *
> + * Unlock the given subdev state.
> + */
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> +
> #endif
--
Regards,
Laurent Pinchart
