Re: [PATCH v3 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] NULL pointer dereference in `s5kjn5_set_fmt` when `state` is NULL.
- [High] Incorrect hardware power sequencing between the reset GPIO and the input clock.
--

--- Patch [2]: [PATCH v3 2/2] media: i2c: Add Samsung S5KJN5 image sensor driver ---
commit 0c20120b2e36399c4fc0b31ce99ece2819948b3a
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 standard V4L2 controls.

> 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_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(S5KJN5_NUM_SUPPLIES, 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: High]
Will this hardware sequence fail on some devices? Standard hardware
sequencing for MIPI CSI-2 image sensors usually requires the external clock
to be running before the chip is brought out of reset, allowing its internal
logic and state machines to safely initialize.

[ ... ]
> +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: High]
Does the clock need to remain active until after reset is asserted here?
Disabling the clock before putting the sensor back into reset might leave
the hardware in an invalid state.

[ ... ]
> +static int s5kjn5_set_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *state,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	const struct s5kjn5_mode *mode = &s5kjn5_modes[0];
> +	struct v4l2_mbus_framefmt *format;
> +
> +	s5kjn5_fill_format(mode, &fmt->format);
> +
> +	format = v4l2_subdev_state_get_format(state, 0);
> +	*format = fmt->format;

[Severity: High]
Can this cause a null pointer dereference when called by a legacy bridge
driver?

When the kernel is compiled without CONFIG_MEDIA_CONTROLLER, V4L2 core
wrappers do not fall back to the active state if the provided state is null.

If a driver calls v4l2_subdev_call(sd, pad, set_fmt, NULL, &fmt),
the state parameter passed to s5kjn5_set_fmt() will be null. The lookup
function v4l2_subdev_state_get_format() detects the null state, triggers a
warning, and returns null, leading to a crash on the immediate assignment
to *format.

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