Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tomi,

Thank you for the patch.

On Wed, Feb 16, 2022 at 03:00:46PM +0200, 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.
> 
> However, active-state locking is complicated by the fact that currently
> the real active-state of a subdev is split into multiple parts: the new
> v4l2_subdev_state, subdev control state, and subdev's internal state.
> 
> In the future all these three states should be combined into one state
> (the v4l2_subdev_state), and then a single lock for the state should be
> sufficient.
> 
> But to solve the current split-state situation we need to share locks
> between the three states. This is accomplished by using the same lock
> management as the control handler does: we use a pointer to a mutex,
> allowing the driver to override the default mutex. Thus the driver can
> do e.g.:
> 
> sd->state_lock = sd->ctrl_handler->lock;
> 
> before calling v4l2_subdev_init_finalize(), resulting in sharing the
> same lock between the states and the controls.
> 
> The locking model for active-state is such that any subdev op that gets
> the state as a parameter expects the state to be already locked by the
> caller, and expects the caller to release the lock.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>  drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>  drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
>  drivers/staging/media/tegra-video/vi.c      |  4 +-
>  include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
>  5 files changed, 155 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index da88f968c31a..3759f4619a77 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -255,6 +255,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  {
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  	struct v4l2_subdev_state *sd_state;
> +	static struct lock_class_key key;
>  	struct v4l2_subdev_format format = {
>  		.which = which,
>  		.pad = vin->parallel.source_pad,
> @@ -267,7 +268,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	sd_state = __v4l2_subdev_state_alloc(sd);
> +	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>  	if (IS_ERR(sd_state))
>  		return PTR_ERR(sd_state);
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
> index c82b3fb7b89a..a116a3362f9e 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -613,6 +613,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>  		     const char *name, unsigned int num_pads,
>  		     const struct v4l2_subdev_ops *ops, u32 function)
>  {
> +	static struct lock_class_key key;
>  	struct v4l2_subdev *subdev;
>  	unsigned int i;
>  	int ret;
> @@ -679,7 +680,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
> +	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
> +						   "vsp1:config->lock", &key);
>  	if (IS_ERR(entity->config)) {
>  		media_entity_cleanup(&entity->subdev.entity);
>  		return PTR_ERR(entity->config);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b67bbce82612..0df9bbe1819d 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -27,8 +27,9 @@
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
>  	struct v4l2_subdev_state *state;
> +	static struct lock_class_key key;
>  
> -	state = __v4l2_subdev_state_alloc(sd);
> +	state = __v4l2_subdev_state_alloc(sd, "fh->state->lock", &key);
>  	if (IS_ERR(state))
>  		return PTR_ERR(state);
>  
> @@ -383,18 +384,15 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>  			     v4l2_subdev_get_active_state(sd);
>  }
>  
> -static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> +static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> +			    struct v4l2_subdev_state *state)
>  {
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>  	struct v4l2_fh *vfh = file->private_data;
> -	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
>  	bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
> -	struct v4l2_subdev_state *state;
>  	int rval;
>  
> -	state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
> -
>  	switch (cmd) {
>  	case VIDIOC_SUBDEV_QUERYCAP: {
>  		struct v4l2_subdev_capability *cap = arg;
> @@ -707,8 +705,24 @@ static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
>  
>  	if (lock && mutex_lock_interruptible(lock))
>  		return -ERESTARTSYS;
> -	if (video_is_registered(vdev))
> -		ret = subdev_do_ioctl(file, cmd, arg);
> +
> +	if (video_is_registered(vdev)) {
> +		struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +		struct v4l2_fh *vfh = file->private_data;
> +		struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
> +		struct v4l2_subdev_state *state;
> +
> +		state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
> +
> +		if (state)
> +			v4l2_subdev_lock_state(state);
> +
> +		ret = subdev_do_ioctl(file, cmd, arg, state);
> +
> +		if (state)
> +			v4l2_subdev_unlock_state(state);
> +	}
> +
>  	if (lock)
>  		mutex_unlock(lock);
>  	return ret;
> @@ -864,7 +878,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>  			media_entity_to_v4l2_subdev(pad->entity);
>  		struct v4l2_subdev_state *state;
>  
> -		state = v4l2_subdev_get_active_state(sd);
> +		state = v4l2_subdev_get_locked_active_state(sd);
>  
>  		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  		fmt->pad = pad->index;
> @@ -906,7 +920,9 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>  
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
> +struct v4l2_subdev_state *
> +__v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name,
> +			  struct lock_class_key *lock_key)
>  {
>  	struct v4l2_subdev_state *state;
>  	int ret;
> @@ -915,6 +931,12 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>  	if (!state)
>  		return ERR_PTR(-ENOMEM);
>  
> +	__mutex_init(&state->_lock, lock_name, lock_key);
> +	if (sd->state_lock)
> +		state->lock = sd->state_lock;
> +	else
> +		state->lock = &state->_lock;
> +
>  	if (sd->entity.num_pads) {
>  		state->pads = kvmalloc_array(sd->entity.num_pads,
>  					     sizeof(*state->pads),
> @@ -925,7 +947,14 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>  		}
>  	}
>  
> +	/*
> +	 * There can be no race at this point, but we lock the state anyway to
> +	 * satisfy lockdep checks.
> +	 */
> +	v4l2_subdev_lock_state(state);
>  	ret = v4l2_subdev_call(sd, pad, init_cfg, state);
> +	v4l2_subdev_unlock_state(state);
> +
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
>  		goto err;
>  
> @@ -946,16 +975,19 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>  	if (!state)
>  		return;
>  
> +	mutex_destroy(&state->_lock);
> +
>  	kvfree(state->pads);
>  	kfree(state);
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
>  
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> +				struct lock_class_key *key)
>  {
>  	struct v4l2_subdev_state *state;
>  
> -	state = __v4l2_subdev_state_alloc(sd);
> +	state = __v4l2_subdev_state_alloc(sd, name, key);
>  	if (IS_ERR(state))
>  		return PTR_ERR(state);
>  
> @@ -963,7 +995,7 @@ int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
> +EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>  
>  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  {
> @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>  
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> +{
> +	mutex_lock(sd->active_state->lock);
> +
> +	return sd->active_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);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index 07d368f345cd..8e184aa4c252 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  				      struct v4l2_pix_format *pix)
>  {
>  	const struct tegra_video_format *fmtinfo;
> +	static struct lock_class_key key;
>  	struct v4l2_subdev *subdev;
>  	struct v4l2_subdev_format fmt;
>  	struct v4l2_subdev_state *sd_state;
> @@ -511,7 +512,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	sd_state = __v4l2_subdev_state_alloc(subdev);
> +	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
> +					     &key);
>  	if (IS_ERR(sd_state))
>  		return PTR_ERR(sd_state);
>  	/*
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1bbe4383966c..8d089a2dbd32 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -658,6 +658,8 @@ struct v4l2_subdev_pad_config {
>  /**
>   * struct v4l2_subdev_state - Used for storing subdev state information.
>   *
> + * @_lock: default for 'lock'
> + * @lock: mutex for the state. May be replaced by the user.
>   * @pads: &struct v4l2_subdev_pad_config array
>   *
>   * This structure only needs to be passed to the pad op if the 'which' field
> @@ -665,6 +667,9 @@ struct v4l2_subdev_pad_config {
>   * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
>   */
>  struct v4l2_subdev_state {
> +	/* lock for the struct v4l2_subdev_state fields */
> +	struct mutex _lock;
> +	struct mutex *lock;
>  	struct v4l2_subdev_pad_config *pads;
>  };
>  
> @@ -888,6 +893,9 @@ struct v4l2_subdev_platform_data {
>   * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>   *		     device using v4l2_async_register_subdev_sensor().
>   * @pdata: common part of subdevice platform data
> + * @state_lock: A pointer to a lock used for all the subdev's states, set by the
> + *		driver. This is	optional. If NULL, each state instance will get
> + *		a lock of its own.
>   * @active_state: Active state for the subdev (NULL for subdevs tracking the
>   *		  state internally). Initialized by calling
>   *		  v4l2_subdev_init_finalize().
> @@ -922,6 +930,7 @@ struct v4l2_subdev {
>  	struct v4l2_async_notifier *notifier;
>  	struct v4l2_async_notifier *subdev_notifier;
>  	struct v4l2_subdev_platform_data *pdata;
> +	struct mutex *state_lock;
>  
>  	/*
>  	 * The fields below are private, and should only be accessed via
> @@ -1144,12 +1153,16 @@ int v4l2_subdev_link_validate(struct media_link *link);
>   * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
>   *
>   * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
> + * @lock_name: name of the state lock
> + * @key: lock_class_key for the lock
>   *
>   * Must call __v4l2_subdev_state_free() when state is no longer needed.
>   *
>   * Not to be called directly by the drivers.
>   */
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd,
> +						    const char *lock_name,
> +						    struct lock_class_key *key);
>  
>  /**
>   * __v4l2_subdev_state_free - free a v4l2_subdev_state
> @@ -1174,7 +1187,16 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
>   *
>   * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
>   */
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
> +#define v4l2_subdev_init_finalize(sd)                                          \
> +	({                                                                     \
> +		static struct lock_class_key __key;                            \
> +		const char *name = KBUILD_BASENAME                             \
> +			":" __stringify(__LINE__) ":sd->active_state->lock";   \
> +		__v4l2_subdev_init_finalize(sd, name, &__key);                 \
> +	})
> +
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> +				struct lock_class_key *key);
>  
>  /**
>   * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
> @@ -1191,14 +1213,71 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
>   * @sd: The subdevice
>   *
>   * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state.
> + * support active state. If the state is not NULL, calls
> + * lockdep_assert_not_held() to issue a warning if the state is locked.
> + *
> + * This function is to be used e.g. when getting the active state for the sole
> + * purpose of passing it forward, without accessing the state fields.
>   */
>  static inline struct v4l2_subdev_state *
>  v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
>  {
> +	if (sd->active_state)
> +		lockdep_assert_not_held(sd->active_state->lock);
> +	return sd->active_state;
> +}
> +
> +/**
> + * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
> + *					   is locked and returns it
> + *
> + * @sd: The subdevice
> + *
> + * Returns the active state for the subdevice, or NULL if the subdev does not
> + * support active state. If the state is not NULL, calls lockdep_assert_held()
> + * to issue a warning if the state is not locked.
> + *
> + * This function is to be used when the caller knows that the active state is
> + * already locked.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
> +{
> +	if (sd->active_state)
> +		lockdep_assert_held(sd->active_state->lock);
>  	return sd->active_state;
>  }
>  
> +/**
> + * v4l2_subdev_lock_active_state() - Locks and returns the active subdev state
> + *				     for the subdevice
> + * @sd: The subdevice
> + *
> + * Returns the locked active state for the subdevice, or NULL if the subdev
> + * does not support active state.
> + *
> + * The 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() - Locks the subdev state
> + * @state: The subdevice state
> + *
> + * Locks the given subdev state.
> + *
> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> + * @state: The subdevice state
> + *
> + * Unlocks the given subdev state.
> + */
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux