Re: [PATCH v9 2/3] media: i2c: Add Sony IMX681 sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: i2c: imx681: double imx681_power_off() on imx681_probe() error path
--

commit c9e6568c02771cf63bede017166c475cf41cb773
Author: Andre Gilerson <andre.gilerson@gmail.com>

media: i2c: Add Sony IMX681 sensor driver

This commit adds a V4L2 sub-device driver for the Sony IMX681 10-bit Bayer
sensor supporting 3844x2640 at 30 fps over a two-lane CSI-2 bus, with controls
for exposure, gains, vertical blanking, flips, and test patterns.

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

[ ... ]

> +static int imx681_probe(struct i2c_client *client)
> +{
> +	struct imx681 *imx681;
> +	unsigned long xclk_freq;
> +	int ret;

[ ... ]

> +	/* Power on and verify chip ID */
> +	ret = imx681_power_on(imx681->dev);
> +	if (ret)
> +		return dev_err_probe(imx681->dev, ret, "power on failed\n");
> +
> +	ret = imx681_identify_module(imx681);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Enable runtime PM */
> +	pm_runtime_set_active(imx681->dev);
> +	pm_runtime_enable(imx681->dev);
> +	pm_runtime_set_autosuspend_delay(imx681->dev, 1000);
> +	pm_runtime_use_autosuspend(imx681->dev);
> +
> +	/* Init V4L2 controls */
> +	ret = imx681_init_controls(imx681);
> +	if (ret)
> +		goto error_pm;
> +
> +	/* Setup subdev */
> +	imx681->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx681->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	imx681->sd.internal_ops = &imx681_internal_ops;
> +
> +	/* Init media entity */
> +	imx681->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&imx681->sd.entity, 1, &imx681->pad);
> +	if (ret) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "media entity init failed\n");
> +		goto error_handler_free;
> +	}
> +
> +	imx681->sd.state_lock = imx681->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&imx681->sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "subdev init finalize failed\n");
> +		goto error_media_entity;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor(&imx681->sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "async register subdev failed\n");
> +		goto error_subdev_cleanup;
> +	}
> +
> +	pm_runtime_idle(imx681->dev);
> +
> +	return 0;
> +
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&imx681->sd);
> +error_media_entity:
> +	media_entity_cleanup(&imx681->sd.entity);
> +error_handler_free:
> +	v4l2_ctrl_handler_free(imx681->sd.ctrl_handler);
> +error_pm:
> +	pm_runtime_dont_use_autosuspend(imx681->dev);
> +	pm_runtime_disable(imx681->dev);
> +	pm_runtime_set_suspended(imx681->dev);
> +error_power_off:
> +	imx681_power_off(imx681->dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Can imx681_probe() call imx681_power_off() twice on the error path?

When pm_runtime_set_autosuspend_delay() is called after pm_runtime_enable()
with a usage_count of 0 (and use_autosuspend not yet set),
update_autosuspend() invokes:

  pm_runtime_set_autosuspend_delay()
    -> update_autosuspend()
      -> rpm_idle(dev, RPM_AUTO)
        -> rpm_suspend(dev, RPM_AUTO)
          -> imx681_power_off()

This synchronously suspends the device and disables xclk and the supplies
during probe. Similarly, at the error_pm label, calling
pm_runtime_dont_use_autosuspend() before pm_runtime_disable() also runs
update_autosuspend() -> rpm_idle() -> rpm_suspend() -> imx681_power_off() if
the device is still runtime-active.

If imx681_init_controls(), media_entity_pads_init(),
v4l2_subdev_init_finalize(), or v4l2_async_register_subdev_sensor() fails,
execution falls through error_pm to error_power_off and calls
imx681_power_off(imx681->dev) a second time, underflowing the enable counts
on imx681->xclk and imx681->supplies.

Would holding a runtime PM reference across probe (via pm_runtime_get_noresume()
before pm_runtime_enable(), paired with pm_runtime_put_noidle() on error or
pm_runtime_put_autosuspend() at the end of probe) or checking
!pm_runtime_status_suspended() after pm_runtime_disable() as done in
imx681_remove() avoid the double power-off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106041116.78937-1-lsa.uz@pm.me?part=2