Re: [PATCH v5 09/16] media: sony-starvis2: Use pm_runtime_autosuspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: sony-starvis2: double starvis2_power_off() on probe error
--

commit 7f5363175aa2ea147ca66ea63a6e15a5e06c0920
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: sony-starvis2: Use pm_runtime_autosuspend

This commit switches the sony-starvis2 driver to use runtime PM autosuspend
with a 1000ms delay so the sensor is not immediately powered down when
stopping and restarting streaming.

> diff --git a/drivers/media/i2c/sony-starvis2.c b/drivers/media/i2c/sony-starvis2.c
> index 2388105b6a304..bf14768dbf981 100644
> --- a/drivers/media/i2c/sony-starvis2.c
> +++ b/drivers/media/i2c/sony-starvis2.c

[ ... ]

> @@ -1448,6 +1448,8 @@ static int starvis2_probe(struct i2c_client *client)
>  
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
>  
>  	ret = starvis2_init_controls(starvis2);
>  	if (ret)
> @@ -1495,6 +1497,7 @@ static int starvis2_probe(struct i2c_client *client)
>  error_pm_runtime:
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_dont_use_autosuspend(&client->dev);
>  
>  error_power_off:
>  	starvis2_power_off(&client->dev);

[Severity: Medium]
Can this cause a double call to starvis2_power_off() if a probe error occurs
after enabling runtime PM?

In starvis2_probe(), pm_runtime_enable(dev) is called while dev->power.usage_count
is 0 (since pm_runtime_get_noresume() was not called). Calling
pm_runtime_set_autosuspend_delay(dev, 1000) and pm_runtime_use_autosuspend(dev)
after pm_runtime_enable(dev) invokes update_autosuspend() in
drivers/base/power/runtime.c:

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

Because usage_count is 0, runtime_status is RPM_ACTIVE, and the autosuspend
expiration is 0, rpm_suspend() synchronously runs starvis2_power_off(dev) and
sets the status to RPM_SUSPENDED right here during probe.

If a subsequent step in starvis2_probe() fails (such as
starvis2_init_controls(), media_entity_pads_init(),
v4l2_subdev_init_finalize(), or v4l2_async_register_subdev_sensor()), the
error path jumps to error_pm_runtime and falls through to error_power_off:

    error_pm_runtime:
        pm_runtime_disable(&client->dev);
        pm_runtime_set_suspended(&client->dev);
        pm_runtime_dont_use_autosuspend(&client->dev);

    error_power_off:
        starvis2_power_off(&client->dev);

This calls starvis2_power_off() a second time on the already-suspended device,
resulting in unbalanced clk_disable_unprepare(starvis2->xclk) and
regulator_bulk_disable(ARRAY_SIZE(starvis2_supply_name), starvis2->supplies)
calls.

Additionally, on the normal probe path, the device is already suspended before
v4l2_async_register_subdev_sensor(), rather than staying active until the end
of probe and entering autosuspend. Should starvis2_probe() take a usage
reference with pm_runtime_get_noresume(dev) before pm_runtime_enable(dev),
replace pm_runtime_idle(dev) at the end of probe with
pm_runtime_put_autosuspend(dev), and call pm_runtime_put_noidle(&client->dev)
in error_pm_runtime?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-media-starvis2-v5-0-eef935347ce9@raspberrypi.com?part=9