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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect check of pm_runtime_get_if_active() return value leads to usage count underflow and unsafe hardware access.
--

commit 7a020d143de8df2df23fab327b8ef8884815a0d1
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

media: i2c: Add Samsung S5KJN5 image sensor driver

This commit adds a V4L2 sub-device driver for the Samsung S5KJN5, a 50 MP
10-bit RAW MIPI CSI-2 image sensor. It supports the 4096x3072 mode and exposes
the standard controls (exposure, gain, vblank/hblank, test pattern).

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

[ ... ]

> +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) {
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = mode->height + ctrl->val - mode->exposure_margin;
> +		__v4l2_ctrl_modify_range(s5kjn5->exposure,
> +					 s5kjn5->exposure->minimum,
> +					 exposure_max,
> +					 s5kjn5->exposure->step,
> +					 s5kjn5->exposure->default_value);
> +	}
> +
> +	/* V4L2 controls are applied when the sensor is powered up for streaming. */
> +	if (!pm_runtime_get_if_active(dev))
> +		return 0;

[Severity: High]
Does this code handle negative error codes properly?

pm_runtime_get_if_active() can return a negative error code (like -EINVAL)
when runtime PM is disabled. In that case, !pm_runtime_get_if_active(dev)
evaluates to false, causing the function to proceed with hardware access
even though the device might be powered off.

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

[ ... ]

> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Could this result in a usage count underflow?

Since pm_runtime_get_if_active() only increments the usage count when it
returns 1, an unconditional pm_runtime_put_autosuspend() here will cause an
underflow if the get function returned a negative error code.

> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-sk5jn5-v3-0-17e728917bd4@oss.qualcomm.com?part=2