Re: [PATCH 6/8] media: i2c: Add Samsung S5KGD1 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] media: i2c: s5kgd1: unbalanced power off in s5kgd1_probe() on
  hw check error
- [Medium] media: i2c: s5kgd1: control handler leak in s5kgd1_probe() on
  ctrl init error
- [Low] media: i2c: s5kgd1: missing link-frequencies check in
  s5kgd1_check_hw()
- [Low] media: i2c: s5kgd1: mismatch between V4L2_SEL_TGT_CROP and mode
  registers
- [Low] MAINTAINERS: SAMSUNG S5KGD1 CAMERA DRIVER entry is out of
  alphabetical order
--

commit ef74f5fd81a29e539440b215310a76d207e486a4
Author: Gaurav Suman <gauravsuman007@gmail.com>

media: i2c: Add Samsung S5KGD1 image sensor driver

This commit adds a V4L2 sub-device driver for the Samsung S5KGD1 32MP
quad-Bayer image sensor supporting the 2x2-binned 3264x2448 RAW10 mode.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index fb12d4c0c22e9..b13cf3ad43a18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22017,6 +22017,14 @@ F:	Documentation/devicetree/bindings/media/i2c/samsung,s5k3m3.yaml
>  F:	drivers/media/i2c/s5k3m3-regs.h
>  F:	drivers/media/i2c/s5k3m3.c
>  
> +SAMSUNG S5KGD1 CAMERA DRIVER
> +M:	Gaurav Suman <gauravsuman007@gmail.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/i2c/samsung,s5kgd1.yaml
> +F:	drivers/media/i2c/s5kgd1-regs.h
> +F:	drivers/media/i2c/s5kgd1.c
> +
>  TASK DELAY MONITORING TOOLS
>  M:	Andrew Morton <akpm@linux-foundation.org>
>  M:	Wang Yaxin <wang.yaxin@zte.com.cn>

[Severity: Low]
This entry is placed in the P section (where TASK DELAY MONITORING TOOLS
was previously misplaced) rather than in alphabetical order between
SAMSUNG S5K5BAF CAMERA DRIVER and SAMSUNG S5KJN1 CAMERA DRIVER.

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

[ ... ]

> +static int s5kgd1_get_selection(struct v4l2_subdev *sd,
> +				const struct v4l2_subdev_client_info *ci,
> +				struct v4l2_subdev_state *st,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:		/* analogue crop of the mode (0x0344..0x034a) */
> +		sel->r.left = 24;
> +		sel->r.top = 16;
> +		sel->r.width = 6528;
> +		sel->r.height = 4896;
> +		return 0;

[Severity: Low]
Does V4L2_SEL_TGT_CROP match the crop registers programmed in
s5kgd1_mode_regs[]?

drivers/media/i2c/s5kgd1-regs.h:s5kgd1_mode_regs[] {
    ...
	{ 0x0344, 0x0018, 0 },
	{ 0x0348, 0x1997, 0 },
	{ 0x0346, 0x0010, 0 },
	{ 0x034a, 0x133f, 0 },
	{ 0x034c, 0x0cc0, 0 },
	{ 0x034e, 0x0990, 0 },
	{ 0x0352, 0x0004, 0 },
    ...
}

Registers 0x0346..0x034a select rows 16..4927 (4912 native rows), and
register 0x0352 applies a 4-row binned (8-row native) Y digital crop
offset before outputting 2448 binned rows (4896 native rows). Reporting
top = 16 with height = 4896 mixes the pre-digital-crop start row with the
post-digital-crop height instead of reporting top = 24 (or height = 4912
for the raw analogue window).

[ ... ]

> +static int s5kgd1_check_hw(struct s5kgd1 *s)
> +{
> +	struct fwnode_handle *ep;
> +	struct v4l2_fwnode_endpoint bus = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> +	u64 id = 0;
> +	int ret;
> +
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(s->dev), NULL);
> +	if (!ep)
> +		return -EPROBE_DEFER;
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return ret;
> +	if (bus.bus.mipi_csi2.num_data_lanes != 4)
> +		ret = dev_err_probe(s->dev, -EINVAL, "need 4 data lanes\n");
> +	v4l2_fwnode_endpoint_free(&bus);

[Severity: Low]
Should s5kgd1_check_hw() also validate bus.link_frequencies against
s5kgd1_link_freqs[] (e.g. via v4l2_link_freq_to_bitmap()) before freeing
bus?

v4l2_fwnode_endpoint_alloc_parse() parses the endpoint's link-frequencies
property, but it is discarded without checking that the firmware specifies
S5KGD1_LINK_FREQ (380.8 MHz).

[ ... ]

> +	ret = s5kgd1_check_hw(s);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			s5kgd1_power_off(dev);
> +		return ret;
> +	}

[Severity: Medium]
Can this call s5kgd1_power_off() when s->mclk and s->supplies are not
enabled?

If s5kgd1_check_hw() fails before s5kgd1_power_on() (for example when
num_data_lanes != 4 returns -EINVAL) or inside s5kgd1_power_on() itself,
s5kgd1_probe() still calls s5kgd1_power_off(dev), which unconditionally
disables and unprepares s->mclk and disables s->supplies, triggering clock
and regulator enable-count underflow warnings. Powering off inside
s5kgd1_check_hw() when the chip ID check fails avoids this.

> +
> +	ret = s5kgd1_init_ctrls(s);
> +	if (ret)
> +		goto err_power;
> +	s->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	s->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	s->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&s->sd.entity, 1, &s->pad);
> +	if (ret)
> +		goto err_ctrls;
> +	s->sd.state_lock = s->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&s->sd);
> +	if (ret)
> +		goto err_entity;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	ret = v4l2_async_register_subdev_sensor(&s->sd);
> +	if (ret)
> +		goto err_pm;
> +	pm_runtime_idle(dev);
> +	return 0;
> +
> +err_pm:
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	v4l2_subdev_cleanup(&s->sd);
> +err_entity:
> +	media_entity_cleanup(&s->sd.entity);
> +err_ctrls:
> +	v4l2_ctrl_handler_free(&s->ctrls);
> +err_power:
> +	s5kgd1_power_off(dev);
> +	return ret;
> +}

[Severity: Medium]
Does this leak s->ctrls when s5kgd1_init_ctrls() fails?

s5kgd1_init_ctrls() calls v4l2_ctrl_handler_init(h, 10) and allocates
controls before checking h->error, and returning h->error without calling
v4l2_ctrl_handler_free(h) while jumping to err_power skips err_ctrls and
leaks the bucket array and any controls allocated before the failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-sm8150-camera-v1-0-3d2e7b407516@gmail.com?part=6