Re: [PATCH v2 2/3] media: i2c: Add Samsung S5K3L6 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] media: i2c: s5k3l6: unlocked mode read in
  s5k3l6_get_mbus_config()
- [Low] media: i2c: s5k3l6: wrap dev_pm_ops pointer with pm_ptr()
--

commit e21dc2bfba16578c02da687c308377fcc959fee4
Author: Vincent Cloutier <vincent@cloutier.co>

media: i2c: Add Samsung S5K3L6 image sensor driver

Add a V4L2 sub-device driver for the Samsung S5K3L6 raw Bayer image sensor,
supporting RAW8 and RAW10 modes over two MIPI CSI-2 data lanes.

Link: https://source.puri.sm/Librem5/linux/-/blob/ad184fca880e02e7b6973f117b60dddfa163ad/drivers/media/i2c/s5k3l6xx.c
Link: https://source.puri.sm/Librem5/linux/-/commit/9fa726ea0a12a1fb72e3b6e0ab09aadff8708078

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

[ ... ]

> +static int s5k3l6_update_controls(struct s5k3l6 *s5k3l6,
> +				  const struct s5k3l6_mode *mode)
> +{
> +	const struct s5k3l6_mode *old_mode = s5k3l6->mode;
> +	struct s5k3l6_ctrl_state old_ctrls[4];
> +	u32 hblank = mode->hts - mode->width;
> +	u32 vblank = mode->vts - mode->height;
> +	u32 exposure_max = mode->vts - S5K3L6_EXPOSURE_MARGIN;
> +	u32 exposure_default = min_t(u32, 0x03de, exposure_max);
> +	unsigned int i;
> +	int restore_ret;
> +	int ret;
> +
> +	s5k3l6_save_ctrl(&old_ctrls[0], s5k3l6->link_freq);
> +	s5k3l6_save_ctrl(&old_ctrls[1], s5k3l6->hblank);
> +	s5k3l6_save_ctrl(&old_ctrls[2], s5k3l6->vblank);
> +	s5k3l6_save_ctrl(&old_ctrls[3], s5k3l6->exposure);
> +	s5k3l6->mode = mode;
> +
> +	ret = __v4l2_ctrl_s_ctrl(s5k3l6->link_freq, mode->link_freq_index);
> +	if (ret)
> +		goto restore_ctrls;
> +
> +	ret = __v4l2_ctrl_modify_range(s5k3l6->hblank, hblank, hblank, 1,
> +				       hblank);
> +	if (ret)
> +		goto restore_ctrls;
> +
> +	ret = __v4l2_ctrl_modify_range(s5k3l6->vblank, vblank,
> +				       0xffff - mode->height, 1, vblank);
> +	if (ret)
> +		goto restore_ctrls;
> +
> +	ret = __v4l2_ctrl_s_ctrl(s5k3l6->vblank, vblank);
> +	if (ret)
> +		goto restore_ctrls;
> +
> +	ret = __v4l2_ctrl_modify_range(s5k3l6->exposure, S5K3L6_EXPOSURE_MIN,
> +				       exposure_max, 1, exposure_default);
> +	if (ret)
> +		goto restore_ctrls;
> +
> +	ret = __v4l2_ctrl_s_ctrl(s5k3l6->exposure, exposure_default);
> +	if (!ret)
> +		return 0;
> +
> +restore_ctrls:
> +	/* A failed mode change may have partially updated the idle sensor. */
> +	gpiod_set_value_cansleep(s5k3l6->reset_gpio, 1);
> +	s5k3l6->needs_reset = true;
> +	s5k3l6->mode = old_mode;

[ ... ]

> +static int s5k3l6_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_config *config)
> +{
> +	struct s5k3l6 *s5k3l6 = to_s5k3l6(sd);
> +	const struct s5k3l6_mode *mode = s5k3l6->mode;
> +
> +	if (pad)
> +		return -EINVAL;
> +
> +	*config = (struct v4l2_mbus_config) {
> +		.type = V4L2_MBUS_CSI2_DPHY,
> +		.link_freq = s5k3l6_link_freq_menu[mode->link_freq_index],
> +	};
> +	config->bus.mipi_csi2.num_data_lanes = S5K3L6_DATA_LANES;
> +
> +	return 0;
> +}

[Severity: Medium]
Can s5k3l6_get_mbus_config() read a transient, uncommitted s5k3l6->mode
pointer without holding the subdev active state lock?

call_get_mbus_config() invokes .get_mbus_config without locking the subdev
active state, and v4l2_subdev_is_streaming(sd) remains false until
v4l2_subdev_enable_streams() runs on the sensor.  If userspace issues
VIDIOC_SUBDEV_S_FMT while the CSI-2 receiver queries v4l2_get_link_freq()
during stream startup, s5k3l6_get_mbus_config() can observe the new mode
before s5k3l6_update_controls() rolls s5k3l6->mode back to old_mode on a
control I2C failure:

CPU0                                  CPU1
s5k3l6_set_pad_format()
  s5k3l6_update_controls()
    s5k3l6->mode = mode;
    __v4l2_ctrl_s_ctrl(vblank)
      s5k3l6_set_ctrl()
        cci_write()  [fails]
                                      v4l2_get_link_freq()
                                        s5k3l6_get_mbus_config()
                                          mode = s5k3l6->mode;
                                          <-- reads uncommitted mode
    s5k3l6->mode = old_mode;

The receiver then configures its D-PHY timing for a link frequency that does
not match the active sensor mode.  Locking the active subdev state with
v4l2_subdev_lock_and_get_active_state() in s5k3l6_get_mbus_config() (or
reading the active format from the locked state) would serialize the access.

[ ... ]

> +static const struct dev_pm_ops s5k3l6_pm_ops = {
> +	SET_RUNTIME_PM_OPS(s5k3l6_power_off, s5k3l6_power_on, NULL)
> +};
> +
> +static const struct of_device_id s5k3l6_of_match[] = {
> +	{ .compatible = "samsung,s5k3l6" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, s5k3l6_of_match);
> +
> +static struct i2c_driver s5k3l6_i2c_driver = {
> +	.driver = {
> +		.name = "s5k3l6",
> +		.pm = &s5k3l6_pm_ops,
> +		.of_match_table = s5k3l6_of_match,
> +	},

[Severity: Low]
Should .pm use pm_ptr(&s5k3l6_pm_ops) (along with RUNTIME_PM_OPS() or
DEFINE_RUNTIME_DEV_PM_OPS()) so the compiler can drop the PM ops structure
and callbacks when CONFIG_PM is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112004636.739749-1-vincent.cloutier@icloud.com?part=2