Re: [PATCH v9 3/3] media: ipu-bridge: Add Sony IMX681

[sashiko-bot@kernel.org]

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

commit 948d595796e70ce27a350282b1ce33a466c43a6f
Author: Sergey Lebedev <lsa.uz@pm.me>

media: ipu-bridge: Add Sony IMX681

This commit adds the SONY0681 ACPI sensor configuration to ipu_supported_sensors
so ipu_bridge_connect_sensors() connects the Sony IMX681 sensor endpoint on the
Microsoft Surface Pro 11 for Business.

> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 3739c4a2a8c43..2759c776cfcae 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -124,6 +124,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>  	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
>  	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */
>  	IPU_SENSOR_CONFIG("TBE20A0", 1, 200000000),
> +	/* Sony IMX681 */
> +	IPU_SENSOR_CONFIG("SONY0681", 1, 969600000),

[Severity: Medium]
With SONY0681 added to ipu_supported_sensors[], ipu_bridge_connect_sensors()
populates the sensor endpoint so imx681_probe() in drivers/media/i2c/imx681.c
proceeds past imx681_parse_endpoint(). Can this trigger an unbalanced
imx681_power_off() call if a later step in imx681_probe() fails when
CONFIG_PM is enabled?

In drivers/media/i2c/imx681.c, imx681_probe() powers on the sensor and enables
runtime PM without taking a usage reference via pm_runtime_get_noresume():

drivers/media/i2c/imx681.c:imx681_probe() {
	...
	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);
	...
}

Because dev->power.usage_count is 0 and use_autosuspend is not yet set when
pm_runtime_set_autosuspend_delay() runs, the call chain:

  pm_runtime_set_autosuspend_delay()
    -> update_autosuspend()
      -> rpm_idle()
        -> rpm_suspend()
          -> imx681_power_off()

synchronously suspends the device and disables imx681->xclk and
imx681->supplies.

If imx681_init_controls(), media_entity_pads_init(),
v4l2_subdev_init_finalize(), or v4l2_async_register_subdev_sensor() subsequently
fails, the error path jumps to error_pm and falls through to error_power_off:

drivers/media/i2c/imx681.c:imx681_probe() {
	...
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;
}

Calling imx681_power_off() unconditionally a second time will call
clk_disable_unprepare() and regulator_bulk_disable() on already-disabled
resources. Should imx681_probe() hold a runtime PM reference across probe (via
pm_runtime_get_noresume() before pm_runtime_enable(), paired with
pm_runtime_put_autosuspend() on success and pm_runtime_put_noidle() in
error_pm), or guard imx681_power_off() with
!pm_runtime_status_suspended(imx681->dev) as in imx681_remove()?

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