Re: [PATCH v2 04/11] media: adv7180: Use v4l2-ctrls core to handle s_ctrl locking

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 28/08/2025 18:06, Niklas Söderlund wrote:
> Instead of handling the state lock ourself in .s_ctrl use the v4l2-ctrls
> core to handle it for us. This will allow us later to use the unlocked
> __v4l2_ctrl_handler_setup() in initialization code where the state lock
> is already held.
> 
> Add a lockdep assert to demonstrate the mutex must be held when setting
> controls.
> 
> There is no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/adv7180.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 9dbd33c4a30c..7b0387151c3a 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -601,11 +601,11 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct v4l2_subdev *sd = to_adv7180_sd(ctrl);
>  	struct adv7180_state *state = to_state(sd);
> -	int ret = mutex_lock_interruptible(&state->mutex);
> +	int ret = 0;
>  	int val;
>  
> -	if (ret)
> -		return ret;
> +	lockdep_assert_held(&state->mutex);
> +
>  	val = ctrl->val;
>  	switch (ctrl->id) {
>  	case V4L2_CID_BRIGHTNESS:
> @@ -647,7 +647,6 @@ static int adv7180_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = -EINVAL;
>  	}
>  
> -	mutex_unlock(&state->mutex);
>  	return ret;
>  }
>  
> @@ -668,6 +667,7 @@ static const struct v4l2_ctrl_config adv7180_ctrl_fast_switch = {
>  static int adv7180_init_controls(struct adv7180_state *state)
>  {
>  	v4l2_ctrl_handler_init(&state->ctrl_hdl, 4);
> +	state->ctrl_hdl.lock = &state->mutex;

While perfectly legal, I would really like to avoid drivers messing with internal
fields of the v4l2_ctrl_handler structure. I wondered why I hadn't noticed this
construct before, and it is primarily used in sensor drivers, which I typically
don't review.

What I would prefer to see is a new function: v4l2_ctrl_handler_init_with_mutex()
where the mutex pointer is passed as an extra argument.

And a static inline for the old function like this:

static inline int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
					 unsigned nr_of_controls_hint)
{
	mutex_init(&hdl->_lock);
	return v4l2_ctrl_handler_init_with_mutex(hdl, nr_of_controls_hint, &hdl->_lock);
}

(it's actually a bit more work due to LOCKDEP class handling)

If a driver uses v4l2_ctrl_handler_init_with_mutex then hdl->_lock is never inited
(and will typically be all zeroes), so any use of that lock will cause errors.

v4l2_ctrl_handler_init_with_mutex() could actually check if the mutex is != _lock
and zero _lock explicitly, clearly marking it as unused.

If you prefer to do this as a follow-up series (also updating existing drivers
that use this), then that would be fine.

Regards,

	Hans

>  
>  	v4l2_ctrl_new_std(&state->ctrl_hdl, &adv7180_ctrl_ops,
>  			  V4L2_CID_BRIGHTNESS, ADV7180_BRI_MIN,