Re: [PATCH 2/3] media: i2c: Add driver for OmniVision OV32C4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect use of pm_sleep_ptr() for the dev_pm_ops structure pointer disables runtime PM when CONFIG_PM_SLEEP is disabled.
- [Medium] Memory leak of V4L2 control handler resources on the probe error path if ov32c4_init_controls() fails.
--

commit 86444baf0012dbc98352979b151e14a3e84e5a51
Author: Robert Bozik <robertbozik@gmail.com>
Subject: media: i2c: Add driver for OmniVision OV32C4

This commit adds a new V4L2 I2C sensor driver for the OmniVision OV32C4
camera. It configures the sensor for a 3264x1840 at 30 fps mode and includes
controls for exposure, gain, blanking, along with runtime PM support.

> diff --git a/drivers/media/i2c/ov32c4.c b/drivers/media/i2c/ov32c4.c
> new file mode 100644
> index 0000000000000..4039aa85cf5e8
> --- /dev/null
> +++ b/drivers/media/i2c/ov32c4.c
[ ... ]
> +static int ov32c4_init_controls(struct ov32c4 *ov32c4)
> +{
> +	struct v4l2_ctrl_handler *ctrl_hdlr = &ov32c4->ctrl_handler;
[ ... ]
> +	ret = v4l2_fwnode_device_parse(ov32c4->dev, &props);
> +	if (ret)
> +		return ret;
> +
> +	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov32c4_ctrl_ops, &props);
> +
> +	if (ctrl_hdlr->error)
> +		return ctrl_hdlr->error;
> +
> +	ov32c4->sd.ctrl_handler = ctrl_hdlr;
> +
> +	return 0;
> +}
[ ... ]
> +static int ov32c4_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = ov32c4_init_controls(ov32c4);
> +	if (ret) {
> +		dev_err_probe(ov32c4->dev, ret, "failed to init controls\n");
> +		goto probe_error_v4l2_ctrl_handler_free;
> +	}
[ ... ]
> +probe_error_v4l2_ctrl_handler_free:
> +	v4l2_ctrl_handler_free(ov32c4->sd.ctrl_handler);

[Severity: Medium]
If ov32c4_init_controls() fails (e.g. because v4l2_fwnode_device_parse()
fails or ctrl_hdlr->error is set), it returns early without assigning
ov32c4->sd.ctrl_handler = ctrl_hdlr.

Since the pointer remains NULL, does the probe cleanup path leak the
allocated V4L2 controls when it calls v4l2_ctrl_handler_free(NULL)? Should
this explicitly free the embedded &ov32c4->ctrl_handler instead?

[ ... ]
> +static DEFINE_RUNTIME_DEV_PM_OPS(ov32c4_pm_ops, ov32c4_power_off,
> +				 ov32c4_power_on, NULL);
[ ... ]
> +static struct i2c_driver ov32c4_i2c_driver = {
> +	.driver = {
> +		.name = "ov32c4",
> +		.pm = pm_sleep_ptr(&ov32c4_pm_ops),

[Severity: Medium]
Is pm_sleep_ptr() the correct wrapper for &ov32c4_pm_ops here?

The ov32c4_pm_ops structure is defined using DEFINE_RUNTIME_DEV_PM_OPS()
which relies on CONFIG_PM. By wrapping it with pm_sleep_ptr(), the entire
dev_pm_ops structure pointer evaluates to NULL when CONFIG_PM_SLEEP is
disabled, even if CONFIG_PM is enabled.

Does this unintentionally disable runtime PM callbacks in configurations
without sleep support? Should this use pm_ptr() instead?

> +		.acpi_match_table = ACPI_PTR(ov32c4_acpi_ids),
> +		.of_match_table = ov32c4_of_match,
> +	},
> +	.probe = ov32c4_probe,
> +	.remove = ov32c4_remove,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826072002.14357-1-robertbozik@gmail.com?part=2