Re: [PATCH v8 05/36] media: subdev: add subdev state locking

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> ---
>  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