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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: Refcount underflow in s5kjn5_s_ctrl() due to mishandled pm_runtime_get_if_active() error
- [Medium] media: i2c: s5kjn5: Incorrect clock and reset sequencing
--

commit 3459a342207994da314f1844556c8af1fbac48b5
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

media: i2c: Add Samsung S5KJN5 image sensor driver

Add a V4L2 sub-device driver for the Samsung S5KJN5, a 50 MP 10-bit
RAW MIPI CSI-2 image sensor.

> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/s5kjn5.c

[ ... ]

> +static int s5kjn5_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct s5kjn5 *s5kjn5 = sd_to_s5kjn5(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(s5kjn5_supply_names),
> +				    s5kjn5->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(s5kjn5->reset_gpio, 0);
> +	usleep_range(8000, 10000);
> +
> +	ret = clk_prepare_enable(s5kjn5->inclk);

[Severity: Medium]
Does releasing the reset line before providing a stable clock put the
sensor in an indeterminate state? Many camera sensors require the clock
to be active before de-asserting reset to ensure a correct transition.

> +	if (ret) {
> +		dev_err(dev, "failed to enable clock: %d\n", ret);
> +		gpiod_set_value_cansleep(s5kjn5->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(s5kjn5_supply_names),
> +				       s5kjn5->supplies);
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	return 0;
> +}
> +
> +static int s5kjn5_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct s5kjn5 *s5kjn5 = sd_to_s5kjn5(sd);
> +
> +	clk_disable_unprepare(s5kjn5->inclk);
> +	gpiod_set_value_cansleep(s5kjn5->reset_gpio, 1);

[Severity: Medium]
Could this also leave the hardware in a bad state? The clock is being
disabled here before the sensor is safely put back into reset.

> +	regulator_bulk_disable(ARRAY_SIZE(s5kjn5_supply_names),
> +			       s5kjn5->supplies);
> +
> +	return 0;
> +}

[ ... ]

> +static int s5kjn5_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct s5kjn5 *s5kjn5 =
> +		container_of(ctrl->handler, struct s5kjn5, ctrl_handler);
> +	struct device *dev = s5kjn5->sd.dev;
> +	const struct s5kjn5_mode *mode = &s5kjn5_modes[0];
> +	u32 exposure_max;
> +	int ret;
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		exposure_max = mode->height + ctrl->val - mode->exposure_margin;
> +		ret = __v4l2_ctrl_modify_range(s5kjn5->exposure,
> +					       s5kjn5->exposure->minimum,
> +					       exposure_max,
> +					       s5kjn5->exposure->step,
> +					       s5kjn5->exposure->default_value);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!pm_runtime_get_if_active(dev))
> +		return 0;

[Severity: High]
Can this lead to a reference count underflow if the device's runtime PM
is disabled?

If pm_runtime_get_if_active() returns a negative error code (like
-EINVAL), the boolean check evaluates to false, allowing execution to
continue down to the hardware access path.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = cci_write(s5kjn5->regmap, S5KJN5_REG_EXPOSURE,
> +				ctrl->val, NULL);
> +		break;

[ ... ]

> +	case V4L2_CID_TEST_PATTERN:
> +		ret = cci_write(s5kjn5->regmap, S5KJN5_REG_TEST_PATTERN,
> +				ctrl->val, NULL);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Since the getter didn't actually increment the usage counter on a
negative error return, won't this put operation unconditionally
decrement it and trigger an underflow?

> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-sk5jn5-v6-0-517c9b34008a@oss.qualcomm.com?part=2