linux-kernel - Re: [PATCH 01/13] media: i2c: ov5647: Parse and register properties

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <qoxea2nxh24cprpt7mcnuv5ksx6ylxvpno7tlwgsinxkeu35sq@xf3uyciyn3df>
Date: Sun, 2 Nov 2025 11:20:38 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Dave Stevenson <dave.stevenson@...pberrypi.com>, Jacopo Mondi <jacopo@...ndi.org>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, Kieran Bingham <kieran.bingham@...asonboard.com>, 
	David Plowman <david.plowman@...pberrypi.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Peter Robinson <pbrobinson@...il.com>, Stefan Wahren <wahrenst@....net>, 
	"Ivan T. Ivanov" <iivanov@...e.de>
Subject: Re: [PATCH 01/13] media: i2c: ov5647: Parse and register properties

Hi Jai

On Tue, Oct 28, 2025 at 12:57:12PM +0530, Jai Luthra wrote:
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> Parse device properties and register controls for them using the V4L2
> fwnode properties helpers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fcedf4661564c032cd7dbd80a9fd30a6..985a8e81529d2f88cb38ccb8c94f8605026a28a9 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1284,10 +1284,11 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
>  	.s_ctrl = ov5647_s_ctrl,
>  };
>
> -static int ov5647_init_controls(struct ov5647 *sensor)
> +static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	int hblank, exposure_max, exposure_def;
> +	struct v4l2_fwnode_device_properties props;

Since I have other comments, let me annoying and suggest moving this
up to maintain line length ordering in variables declaration.

>
>  	v4l2_ctrl_handler_init(&sensor->ctrls, 9);

Should we make this 11 ? With this change we can end up registering 2
additional controls.

>
> @@ -1338,6 +1339,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
>  				     0, 0, ov5647_test_pattern_menu);
>
> +	v4l2_fwnode_device_parse(dev, &props);
> +
> +	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> +					&props);
> +
>  	if (sensor->ctrls.error)
>  		goto handler_free;
>
> @@ -1420,7 +1426,7 @@ static int ov5647_probe(struct i2c_client *client)
>
>  	sensor->mode = OV5647_DEFAULT_MODE;
>
> -	ret = ov5647_init_controls(sensor);
> +	ret = ov5647_init_controls(sensor, dev);

You know, I think we have a bug that went unnoticed..

ov5647_init_controls() retrieves the i2c client with

	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);

but v4l2_set_subdevdata() is called by v4l2_i2c_subdev_init() which is
called after  ov5647_init_controls(). We don't hit a segfault because
client so far as only be used in the error path.

If you move init_controls() after v4l2_i2c_subdev_init() you can
access dev from the i2c_client instead of passing it to the function ?

Thanks
  j

>  	if (ret)
>  		goto mutex_destroy;
>
>
> --
> 2.51.0
>