Re: [PATCH 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] media: s5k3t2: stale mode pointer used in synchronous control updates
- [High] pm: pm_runtime_get_if_active() error bypassed causing usage count unbalance
- [Medium] media: v4l2: format not updated when layout-modifying controls change
- [Low] pm: unconditionally assigned dev_pm_ops causes dead code
--

commit 4a653d96a1e95b8288d1057ab3b4c96de4ec5282
Author: Armandas Kvietkus <armandas.kvietkus@proton.me>
Subject: media: i2c: Add Samsung S5K3T2 image sensor driver

Add a driver for the Samsung S5K3T2 20 Mpixel CMOS image sensor. This
driver supports three RAW10 modes over four D-PHY lanes. The pixel clock
remains constant, while runtime PM manages power through autosuspend.

> diff --git a/drivers/media/i2c/s5k3t2.c b/drivers/media/i2c/s5k3t2.c
> new file mode 100644
> index 0000000000000..4ca44d86d2638
> --- /dev/null
> +++ b/drivers/media/i2c/s5k3t2.c

[ ... ]

> +static int s5k3t2_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct s5k3t2 *s5k3t2 = container_of(ctrl->handler, struct s5k3t2,
> +					     ctrl_handler);
> +	const struct s5k3t2_mode *mode = s5k3t2->mode;
> +	s64 exposure_max;
> +	int ret;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +	case V4L2_CID_LINK_FREQ:
> +	case V4L2_CID_PIXEL_RATE:
> +	case V4L2_CID_HBLANK:
> +		return 0;

[Severity: Medium]
Can a user query a stale bus format code after layout-modifying controls
(like flips) are changed?

The driver correctly sets V4L2_CTRL_FLAG_MODIFY_LAYOUT in
s5k3t2_init_controls(), but this function returns 0 without updating the
active subdevice state using v4l2_subdev_get_locked_active_state().

> +	case V4L2_CID_VBLANK:
> +		exposure_max = mode->height + ctrl->val - S5K3T2_EXPOSURE_MARGIN;
> +		__v4l2_ctrl_modify_range(s5k3t2->exposure,
> +					 s5k3t2->exposure->minimum,
> +					 exposure_max,
> +					 s5k3t2->exposure->step,
> +					 s5k3t2->exposure->default_value);
> +		break;
> +	}
> +
> +	if (!pm_runtime_get_if_active(s5k3t2->dev))
> +		return 0;

[Severity: High]
Does this bypass negative error returns from pm_runtime_get_if_active()?

If pm_runtime_get_if_active() returns a negative error (for example, -EINVAL
when Runtime PM is disabled), this negation evaluates to false.

This allows execution to proceed with I2C hardware accesses and eventually
reaches an unbalanced pm_runtime_put(). Callers must check if the return
value is <= 0.

[ ... ]

> +static int s5k3t2_set_pad_format(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 struct v4l2_subdev_format *fmt)
> +{

[ ... ]

> +	exposure_max = mode->vts - S5K3T2_EXPOSURE_MARGIN;
> +	__v4l2_ctrl_modify_range(s5k3t2->exposure, S5K3T2_EXPOSURE_MIN,
> +				 exposure_max, S5K3T2_EXPOSURE_STEP,
> +				 mode->exposure);
> +	__v4l2_ctrl_s_ctrl(s5k3t2->exposure, mode->exposure);
> +
> +	if (s5k3t2->sd.ctrl_handler->error)
> +		return s5k3t2->sd.ctrl_handler->error;
> +
> +	s5k3t2->mode = mode;

[Severity: High]
Does this sequence trigger synchronous control updates via
__v4l2_ctrl_s_ctrl() before the s5k3t2->mode pointer is updated?

When VIDIOC_SUBDEV_S_FMT changes the resolution, the control updates are
dispatched while s5k3t2->mode still points to the old active mode.

In s5k3t2_set_ctrl(), this causes the handler to read the old mode->height
and apply it with the new vblank to the hardware VTS register:

	case V4L2_CID_VBLANK:
		ret = cci_write(s5k3t2->regmap, S5K3T2_REG_VTS,
				ctrl->val + mode->height, NULL);
		break;

This misconfigures the frame's vertical total size.

[ ... ]

> +static struct i2c_driver s5k3t2_i2c_driver = {
> +	.driver = {
> +		.name = "s5k3t2",
> +		.pm = &s5k3t2_pm_ops,

[Severity: Low]
Is this unconditionally assigned dev_pm_ops structure preventing dead code
elimination when CONFIG_PM is disabled?

The PM subsystem guide mandates that dev_pm_ops assignments use pm_ptr()
to ensure unused functions are discarded.

> +		.of_match_table = s5k3t2_of_match,
> +	},
> +	.probe = s5k3t2_probe,
> +	.remove = s5k3t2_remove,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-upstream-s5k3t2-v1-0-d640740f4013@proton.me?part=2