[PATCH v3 2/2] media: i2c: vd55g1: Use the control handler lock for the subdev state

[Fernando Rimoli]

vd55g1_subdev_init() sets sd.state_lock to ctrl_handler.lock before the
control handler is initialised, while the pointer is still NULL, so the
subdev state gets a mutex of its own. vd55g1_s_ctrl() under
VIDIOC_S_CTRL, and __v4l2_ctrl_modify_range() and
__v4l2_ctrl_handler_setup() under set_fmt and enable_streams, then run
without the lock they assert, and format and control changes can race.

Initialise the controls first and finalise last. The blanking ranges
depend on the format, so set them, and the default frame length, once
the active state exists. vd55g1_grab_ctrls() now runs with the lock
held, so use __v4l2_ctrl_grab().

With the control handler set, v4l2_subdev_init_finalize() also enables
control events, which v4l2-compliance reported missing.

Fixes: e56616d7b23c ("media: i2c: Add driver for ST VD55G1 camera sensor")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Fernando Rimoli <fernandorimoli11@xxxxxxxxx>
---
 drivers/media/i2c/vd55g1.c | 61 +++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 69ffb3a..9f737ea 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1113,10 +1113,10 @@ static int vd55g1_ro_ctrls_setup(struct vd55g1 *sensor, struct v4l2_rect *crop)
 static void vd55g1_grab_ctrls(struct vd55g1 *sensor, bool enable)
 {
 	/* These settings cannot change during stream */
-	v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
-	v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
-	v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
-	v4l2_ctrl_grab(sensor->hdr_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->hdr_ctrl, enable);
 }
 
 static int vd55g1_enable_streams(struct v4l2_subdev *sd,
@@ -1580,14 +1580,6 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_fwnode_device_properties fwnode_props;
-	struct vd55g1_vblank_limits vblank;
-	unsigned int hblank;
-	struct v4l2_subdev_state *state =
-		v4l2_subdev_lock_and_get_active_state(&sensor->sd);
-	struct v4l2_rect *crop =
-		v4l2_subdev_state_get_crop(state, 0);
-	struct v4l2_mbus_framefmt *format =
-		v4l2_subdev_state_get_format(state, 0);
 	int ret;
 
 	v4l2_ctrl_handler_init(hdl, 16);
@@ -1645,15 +1637,13 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 					     V4L2_CID_HDR_SENSOR_MODE,
 					     ARRAY_SIZE(vd55g1_hdr_menu) - 1, 0,
 					     VD55G1_NO_HDR, vd55g1_hdr_menu);
-	hblank = vd55g1_get_hblank_min(sensor, format, crop);
+	/* Placeholder ranges, set from the format in vd55g1_subdev_init() */
 	sensor->hblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
-						hblank, hblank, 1, hblank);
+						1, 1, 1, 1);
 	if (sensor->hblank_ctrl)
 		sensor->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	vd55g1_get_vblank_limits(sensor, crop, &vblank);
 	sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
-						vblank.min, vblank.max,
-						1, vblank.def);
+						1, 1, 1, 1);
 
 	/* Additional controls based on device tree properties */
 	if (sensor->ext_leds_mask) {
@@ -1673,12 +1663,11 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 		goto free_ctrls;
 
 	sensor->sd.ctrl_handler = hdl;
-	goto unlock_state;
+
+	return 0;
 
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
-unlock_state:
-	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1915,6 +1904,9 @@ static int vd55g1_parse_dt(struct vd55g1 *sensor)
 
 static int vd55g1_subdev_init(struct vd55g1 *sensor)
 {
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
 	int ret;
 
 	/* Init sub device */
@@ -1930,6 +1922,14 @@ static int vd55g1_subdev_init(struct vd55g1 *sensor)
 		return ret;
 	}
 
+	ret = vd55g1_init_ctrls(sensor);
+	if (ret) {
+		dev_err(sensor->dev, "Controls initialization failed %d\n",
+			ret);
+		goto err_media;
+	}
+
+	/* The state shares the control handler lock, so finalize last */
 	sensor->sd.state_lock = sensor->ctrl_handler.lock;
 	ret = v4l2_subdev_init_finalize(&sensor->sd);
 	if (ret) {
@@ -1937,19 +1937,24 @@ static int vd55g1_subdev_init(struct vd55g1 *sensor)
 		goto err_ctrls;
 	}
 
-	/*
-	 * Initialize controls after v4l2_subdev_init_finalize() to make sure
-	 * active state is set
-	 */
-	ret = vd55g1_init_ctrls(sensor);
+	/* Set the format dependent ranges and the default frame length */
+	state = v4l2_subdev_lock_and_get_active_state(&sensor->sd);
+	format = v4l2_subdev_state_get_format(state, 0);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	ret = vd55g1_new_format_change_controls(sensor, format, crop);
+	if (!ret)
+		ret = __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl,
+					 sensor->vblank_ctrl->default_value);
+	v4l2_subdev_unlock_state(state);
 	if (ret) {
-		dev_err(sensor->dev, "Controls initialization failed %d\n",
-			ret);
-		goto err_media;
+		dev_err(sensor->dev, "Controls update failed %d\n", ret);
+		goto err_state;
 	}
 
 	return 0;
 
+err_state:
+	v4l2_subdev_cleanup(&sensor->sd);
 err_ctrls:
 	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
 
-- 
2.43.0