Skip to content

media: i2c: retain ownership on failed stops and quiesce on remove #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ooaklee merged 4 commits into from
Nov 13, 2026
Merged

media: i2c: retain ownership on failed stops and quiesce on remove #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8d7d31
media: i2c: retain ownership on failed stops and quiesce on remove
ooaklee Nov 13, 2026
0f3c754
media: i2c: ov13858: convert to the streams API
ooaklee Nov 13, 2026
3aab702
media: i2c: vd55g1: hold state lock over quiesce in remove
ooaklee Nov 13, 2026
6c51390
media: i2c: ov13858: drop redundant locking from state-locked callbacks
ooaklee Nov 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 73 additions & 47 deletions drivers/media/i2c/ov13858.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -1360,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;
}
Expand All @@ -1381,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)
Expand Down Expand Up @@ -1418,8 +1412,6 @@ ov13858_set_pad_format(struct v4l2_subdev *sd,
h_blank, 1, h_blank);
}

mutex_unlock(&ov13858->mutex);

return 0;
}

Expand Down Expand Up @@ -1479,47 +1471,66 @@ 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);
/* The caller holds sd->state_lock, which is 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;

/*
* Apply default & customized values
* and then start streaming.
*/
ret = ov13858_start_streaming(ov13858);
if (ret)
goto err_rpm_put;
} else {
ov13858_stop_streaming(ov13858);
pm_runtime_put(ov13858->dev);
}
ret = pm_runtime_resume_and_get(ov13858->dev);
if (ret < 0)
goto err_unlock;

mutex_unlock(&ov13858->mutex);
/*
* Apply default & customized values
* and then start streaming.
*/
ret = ov13858_start_streaming(ov13858);
if (ret)
goto err_rpm_put;

return ret;
return 0;

err_rpm_put:
pm_runtime_put(ov13858->dev);
err_unlock:
mutex_unlock(&ov13858->mutex);

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;

/* The caller holds sd->state_lock, which is 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).
*/
return ret;
}

pm_runtime_put(ov13858->dev);

return 0;
}

/* Verify chip ID */
static int ov13858_identify_module(struct ov13858 *ov13858)
{
Expand Down Expand Up @@ -1547,14 +1558,16 @@ 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 = {
.enum_mbus_code = ov13858_enum_mbus_code,
.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 = {
Expand All @@ -1573,7 +1586,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 */
Expand Down Expand Up @@ -1916,9 +1929,11 @@ 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;
ov13858->sd.state_lock = &ov13858->mutex;

/* Initialize source pad */
ov13858->pad.flags = MEDIA_PAD_FL_SOURCE;
Expand All @@ -1928,14 +1943,21 @@ 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
* a bridge can expose sub-device operations immediately.
*/
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);

Expand All @@ -1951,6 +1973,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);

Expand All @@ -1974,6 +1999,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);

Expand Down
38 changes: 37 additions & 1 deletion drivers/media/i2c/vd55g1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -3162,6 +3162,42 @@ 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
* 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.
*
* 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;

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);
}

v4l2_subdev_unlock_state(state);

vd55g1_subdev_cleanup(sensor);

Expand Down