From d8d7d31b54743452800693fb7155f72428f999e5 Mon Sep 17 00:00:00 2001 From: Leon Silcott Date: Fri, 13 Nov 2026 07:44:06 +0000 Subject: [PATCH 1/4] media: i2c: retain ownership on failed stops and quiesce on remove ov13858 discarded the standby-write error in its stop path: the sensor could be powered down mid-stream while the caller was told the stop succeeded. Capture the error, keep the runtime-PM reference on failure, and opt into V4L2_SUBDEV_FL_STOP_RETRY so the framework preserves the enabled state and retries. vd55g1 returned -EINVAL from the failed-start path, discarding the real transport error; return it instead. Its remove path unregistered the subdev and cut power without stopping a live stream: quiesce a still- streaming sensor to software standby first (saving the exposure cluster for AE coldstart), mirroring disable_streams without the PM put. --- drivers/media/i2c/ov13858.c | 16 ++++++++++++++-- drivers/media/i2c/vd55g1.c | 28 +++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c index bb81aca8d79338..887b63ce52de7e 100644 --- a/drivers/media/i2c/ov13858.c +++ b/drivers/media/i2c/ov13858.c @@ -1504,7 +1504,18 @@ static int ov13858_set_stream(struct v4l2_subdev *sd, int enable) if (ret) goto err_rpm_put; } else { - ov13858_stop_streaming(ov13858); + ret = ov13858_stop_streaming(ov13858); + if (ret) { + /* + * The standby write failed: the sensor may still be + * streaming. Keep the PM reference so the sensor is + * not powered down mid-stream, and report the failure + * so the framework retains the enabled state and can + * retry the stop (V4L2_SUBDEV_FL_STOP_RETRY). + */ + mutex_unlock(&ov13858->mutex); + return ret; + } pm_runtime_put(ov13858->dev); } @@ -1916,7 +1927,8 @@ static int ov13858_probe(struct i2c_client *client) /* Initialize subdev */ ov13858->sd.internal_ops = &ov13858_internal_ops; ov13858->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | - V4L2_SUBDEV_FL_HAS_EVENTS; + V4L2_SUBDEV_FL_HAS_EVENTS | + V4L2_SUBDEV_FL_STOP_RETRY; ov13858->sd.entity.ops = &ov13858_subdev_entity_ops; ov13858->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c index 340da6d64811d7..42a9168834a20c 100644 --- a/drivers/media/i2c/vd55g1.c +++ b/drivers/media/i2c/vd55g1.c @@ -2192,7 +2192,7 @@ static int vd55g1_enable_streams(struct v4l2_subdev *sd, err_rpm_put: pm_runtime_put(sensor->dev); - return -EINVAL; + return ret; } static int vd55g1_disable_streams(struct v4l2_subdev *sd, @@ -3163,6 +3163,32 @@ static void vd55g1_remove(struct i2c_client *client) struct v4l2_subdev *sd = i2c_get_clientdata(client); struct vd55g1 *sensor = to_vd55g1(sd); + /* + * Unregistering the subdev does not stop a live stream: quiesce a + * still-streaming sensor to software standby before cutting power, + * mirroring vd55g1_disable_streams() without the PM put, so the + * exposure cluster is saved and the sensor is not powered down + * mid-stream. + */ + if (v4l2_subdev_is_streaming(sd)) { + int stop_ret = 0; + int ret; + + ret = vd55g1_read_expo_cluster(sensor); + if (ret) + dev_warn(sensor->dev, + "Failed to save exposure: %d\n", ret); + + vd55g1_write(sensor, REG_STREAMING, + VD55G1_STREAMING_STOP_STREAM, &stop_ret); + vd55g1_poll_reg(sensor, REG_STREAMING, 0, &stop_ret); + vd55g1_wait_state(sensor, VD55G1_SYSTEM_FSM_SW_STBY, &stop_ret); + if (stop_ret) + dev_warn(sensor->dev, + "Failed to stop stream on remove: %d\n", + stop_ret); + } + vd55g1_subdev_cleanup(sensor); pm_runtime_disable(&client->dev); From 0f3c7545b19f3294bcdbbcae456dabfd03e6afd2 Mon Sep 17 00:00:00 2001 From: Leon Silcott Date: Fri, 13 Nov 2026 08:06:50 +0000 Subject: [PATCH 2/4] media: i2c: ov13858: convert to the streams API V4L2_SUBDEV_FL_STOP_RETRY is only honored by call_s_stream() when .s_stream is v4l2_subdev_s_stream_helper. With the legacy .s_stream callback the flag is dead: a failed stop is suppressed and the driver keeps its PM runtime reference, leaving the usage counter unbalanced. Convert the driver to the streams API so the helper is used and the failed-stop error, together with the retained PM reference, is propagated: - replace .open() with an .init_state() internal op that initializes the pad-0 format to the highest-resolution default mode - add .enable_streams()/.disable_streams() pad ops carrying over the power-fault check, PM get/put, start/stop logic and the failed-stop error return with retained PM reference - set .s_stream to v4l2_subdev_s_stream_helper - call v4l2_subdev_init_finalize() in probe and v4l2_subdev_cleanup() in the probe error paths and remove() No behavior change on the success paths. --- drivers/media/i2c/ov13858.c | 116 ++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 46 deletions(-) diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c index 887b63ce52de7e..8a55fc57f1d4c3 100644 --- a/drivers/media/i2c/ov13858.c +++ b/drivers/media/i2c/ov13858.c @@ -1168,23 +1168,18 @@ static int ov13858_write_reg_list(struct ov13858 *ov13858, return ov13858_write_regs(ov13858, r_list->regs, r_list->num_of_regs); } -/* Open sub-device */ -static int ov13858_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +/* Initialize pad format */ +static int ov13858_init_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state) { struct ov13858 *ov13858 = to_ov13858(sd); - struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_state_get_format(fh->state, - 0); + struct v4l2_mbus_framefmt *format = + v4l2_subdev_state_get_format(sd_state, 0); - mutex_lock(&ov13858->mutex); - - /* Initialize try_fmt */ - try_fmt->width = ov13858->cur_mode->width; - try_fmt->height = ov13858->cur_mode->height; - try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10; - try_fmt->field = V4L2_FIELD_NONE; - - /* No crop or compose */ - mutex_unlock(&ov13858->mutex); + format->width = ov13858->cur_mode->width; + format->height = ov13858->cur_mode->height; + format->code = MEDIA_BUS_FMT_SGRBG10_1X10; + format->field = V4L2_FIELD_NONE; return 0; } @@ -1479,49 +1474,35 @@ static int ov13858_stop_streaming(struct ov13858 *ov13858) OV13858_REG_VALUE_08BIT, OV13858_MODE_STANDBY); } -static int ov13858_set_stream(struct v4l2_subdev *sd, int enable) +static int ov13858_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) { struct ov13858 *ov13858 = to_ov13858(sd); - int ret = 0; + int ret; mutex_lock(&ov13858->mutex); - if (enable && READ_ONCE(ov13858->power_fault)) { + if (READ_ONCE(ov13858->power_fault)) { ret = -EIO; goto err_unlock; } - if (enable) { - ret = pm_runtime_resume_and_get(ov13858->dev); - if (ret < 0) - goto err_unlock; + ret = pm_runtime_resume_and_get(ov13858->dev); + if (ret < 0) + goto err_unlock; - /* - * Apply default & customized values - * and then start streaming. - */ - ret = ov13858_start_streaming(ov13858); - if (ret) - goto err_rpm_put; - } else { - ret = ov13858_stop_streaming(ov13858); - if (ret) { - /* - * The standby write failed: the sensor may still be - * streaming. Keep the PM reference so the sensor is - * not powered down mid-stream, and report the failure - * so the framework retains the enabled state and can - * retry the stop (V4L2_SUBDEV_FL_STOP_RETRY). - */ - mutex_unlock(&ov13858->mutex); - return ret; - } - pm_runtime_put(ov13858->dev); - } + /* + * Apply default & customized values + * and then start streaming. + */ + ret = ov13858_start_streaming(ov13858); + if (ret) + goto err_rpm_put; mutex_unlock(&ov13858->mutex); - return ret; + return 0; err_rpm_put: pm_runtime_put(ov13858->dev); @@ -1531,6 +1512,35 @@ static int ov13858_set_stream(struct v4l2_subdev *sd, int enable) return ret; } +static int ov13858_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct ov13858 *ov13858 = to_ov13858(sd); + int ret; + + mutex_lock(&ov13858->mutex); + + ret = ov13858_stop_streaming(ov13858); + if (ret) { + /* + * The standby write failed: the sensor may still be + * streaming. Keep the PM reference so the sensor is + * not powered down mid-stream, and report the failure + * so the framework retains the enabled state and can + * retry the stop (V4L2_SUBDEV_FL_STOP_RETRY). + */ + mutex_unlock(&ov13858->mutex); + return ret; + } + + pm_runtime_put(ov13858->dev); + + mutex_unlock(&ov13858->mutex); + + return 0; +} + /* Verify chip ID */ static int ov13858_identify_module(struct ov13858 *ov13858) { @@ -1558,7 +1568,7 @@ static const struct v4l2_subdev_core_ops ov13858_core_ops = { }; static const struct v4l2_subdev_video_ops ov13858_video_ops = { - .s_stream = ov13858_set_stream, + .s_stream = v4l2_subdev_s_stream_helper, }; static const struct v4l2_subdev_pad_ops ov13858_pad_ops = { @@ -1566,6 +1576,8 @@ static const struct v4l2_subdev_pad_ops ov13858_pad_ops = { .get_fmt = ov13858_get_pad_format, .set_fmt = ov13858_set_pad_format, .enum_frame_size = ov13858_enum_frame_size, + .enable_streams = ov13858_enable_streams, + .disable_streams = ov13858_disable_streams, }; static const struct v4l2_subdev_sensor_ops ov13858_sensor_ops = { @@ -1584,7 +1596,7 @@ static const struct media_entity_operations ov13858_subdev_entity_ops = { }; static const struct v4l2_subdev_internal_ops ov13858_internal_ops = { - .open = ov13858_open, + .init_state = ov13858_init_state, }; /* Initialize control handlers */ @@ -1931,6 +1943,7 @@ static int ov13858_probe(struct i2c_client *client) V4L2_SUBDEV_FL_STOP_RETRY; ov13858->sd.entity.ops = &ov13858_subdev_entity_ops; ov13858->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + ov13858->sd.state_lock = &ov13858->mutex; /* Initialize source pad */ ov13858->pad.flags = MEDIA_PAD_FL_SOURCE; @@ -1940,6 +1953,13 @@ static int ov13858_probe(struct i2c_client *client) goto error_handler_free; } + ret = v4l2_subdev_init_finalize(&ov13858->sd); + if (ret < 0) { + dev_err(ov13858->dev, "subdev initialization failed: %d\n", + ret); + goto error_media_entity; + } + /* * The driver powers DT devices; i2c-core powers ACPI devices. Hold the * initial reference until async registration has finished, since binding @@ -1963,6 +1983,9 @@ static int ov13858_probe(struct i2c_client *client) pm_runtime_disable(ov13858->dev); pm_runtime_put_noidle(ov13858->dev); +error_subdev_cleanup: + v4l2_subdev_cleanup(&ov13858->sd); + error_media_entity: media_entity_cleanup(&ov13858->sd.entity); @@ -1986,6 +2009,7 @@ static void ov13858_remove(struct i2c_client *client) int ret = 0; v4l2_async_unregister_subdev(sd); + v4l2_subdev_cleanup(sd); media_entity_cleanup(&sd->entity); ov13858_free_controls(ov13858); From 3aab7022c2c2481e7e485cd26314c5bc4c63be90 Mon Sep 17 00:00:00 2001 From: Leon Silcott Date: Fri, 13 Nov 2026 08:07:13 +0000 Subject: [PATCH 3/4] media: i2c: vd55g1: hold state lock over quiesce in remove v4l2_subdev_is_streaming() must be called with the active-state lock held, and the quiesce sequence it guards can race with a concurrent stream or control operation. Take the active-state lock before the check and release it only after the sensor has been quiesced, before vd55g1_subdev_cleanup() frees the state. No behavior change on the success paths. --- drivers/media/i2c/vd55g1.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c index 42a9168834a20c..f859d2501b796b 100644 --- a/drivers/media/i2c/vd55g1.c +++ b/drivers/media/i2c/vd55g1.c @@ -3162,6 +3162,7 @@ static void vd55g1_remove(struct i2c_client *client) { struct v4l2_subdev *sd = i2c_get_clientdata(client); struct vd55g1 *sensor = to_vd55g1(sd); + struct v4l2_subdev_state *state; /* * Unregistering the subdev does not stop a live stream: quiesce a @@ -3169,7 +3170,14 @@ static void vd55g1_remove(struct i2c_client *client) * mirroring vd55g1_disable_streams() without the PM put, so the * exposure cluster is saved and the sensor is not powered down * mid-stream. + * + * v4l2_subdev_is_streaming() requires the active-state lock to be + * held; keep it across the quiesce so the check cannot race with a + * concurrent stream or control operation. The active state is still + * allocated here: it is freed by vd55g1_subdev_cleanup() below. */ + state = v4l2_subdev_lock_and_get_active_state(sd); + if (v4l2_subdev_is_streaming(sd)) { int stop_ret = 0; int ret; @@ -3189,6 +3197,8 @@ static void vd55g1_remove(struct i2c_client *client) stop_ret); } + v4l2_subdev_unlock_state(state); + vd55g1_subdev_cleanup(sensor); pm_runtime_disable(&client->dev); From 6c51390bcd80b81c36acb42f277cb710bdeec8df Mon Sep 17 00:00:00 2001 From: Leon Silcott Date: Fri, 13 Nov 2026 08:18:51 +0000 Subject: [PATCH 4/4] media: i2c: ov13858: drop redundant locking from state-locked callbacks With the conversion to the streams API the subdev core holds sd->state_lock (ov13858->mutex) while calling the pad ops, stream ops and init_state. Taking the mutex again in get_fmt, set_fmt and the enable/disable_streams callbacks self-deadlocks; remove the redundant locking. Also unwind a failed pm_runtime_set_active() through error_subdev_cleanup so v4l2_subdev_cleanup() releases the allocated active state. --- drivers/media/i2c/ov13858.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c index 8a55fc57f1d4c3..1cff5be69aa2d8 100644 --- a/drivers/media/i2c/ov13858.c +++ b/drivers/media/i2c/ov13858.c @@ -1355,9 +1355,8 @@ static int ov13858_get_pad_format(struct v4l2_subdev *sd, struct ov13858 *ov13858 = to_ov13858(sd); int ret; - mutex_lock(&ov13858->mutex); + /* The caller holds sd->state_lock, which is ov13858->mutex. */ ret = ov13858_do_get_pad_format(ov13858, sd_state, fmt); - mutex_unlock(&ov13858->mutex); return ret; } @@ -1376,7 +1375,7 @@ ov13858_set_pad_format(struct v4l2_subdev *sd, s64 pixel_rate; s64 link_freq; - mutex_lock(&ov13858->mutex); + /* The caller holds sd->state_lock, which is ov13858->mutex. */ /* Only one raw bayer(GRBG) order is supported */ if (fmt->format.code != MEDIA_BUS_FMT_SGRBG10_1X10) @@ -1413,8 +1412,6 @@ ov13858_set_pad_format(struct v4l2_subdev *sd, h_blank, 1, h_blank); } - mutex_unlock(&ov13858->mutex); - return 0; } @@ -1481,7 +1478,7 @@ static int ov13858_enable_streams(struct v4l2_subdev *sd, struct ov13858 *ov13858 = to_ov13858(sd); int ret; - mutex_lock(&ov13858->mutex); + /* The caller holds sd->state_lock, which is ov13858->mutex. */ if (READ_ONCE(ov13858->power_fault)) { ret = -EIO; @@ -1500,15 +1497,11 @@ static int ov13858_enable_streams(struct v4l2_subdev *sd, if (ret) goto err_rpm_put; - mutex_unlock(&ov13858->mutex); - return 0; err_rpm_put: pm_runtime_put(ov13858->dev); err_unlock: - mutex_unlock(&ov13858->mutex); - return ret; } @@ -1519,7 +1512,7 @@ static int ov13858_disable_streams(struct v4l2_subdev *sd, struct ov13858 *ov13858 = to_ov13858(sd); int ret; - mutex_lock(&ov13858->mutex); + /* The caller holds sd->state_lock, which is ov13858->mutex. */ ret = ov13858_stop_streaming(ov13858); if (ret) { @@ -1530,14 +1523,11 @@ static int ov13858_disable_streams(struct v4l2_subdev *sd, * so the framework retains the enabled state and can * retry the stop (V4L2_SUBDEV_FL_STOP_RETRY). */ - mutex_unlock(&ov13858->mutex); return ret; } pm_runtime_put(ov13858->dev); - mutex_unlock(&ov13858->mutex); - return 0; } @@ -1967,7 +1957,7 @@ static int ov13858_probe(struct i2c_client *client) */ ret = pm_runtime_set_active(ov13858->dev); if (ret) - goto error_media_entity; + goto error_subdev_cleanup; pm_runtime_get_noresume(ov13858->dev); pm_runtime_enable(ov13858->dev);