[v1] media: i2c: ov5647: fix power cleanup on remove

[PATCH] media: i2c: ov5647: fix power cleanup on remove

Posted by Guangshuo Li 1 week, 2 days ago

ov5647_remove() disables runtime PM without powering off the sensor if
it is still runtime active or updating the runtime PM state to
suspended.

pm_runtime_disable() prevents further runtime PM callbacks and waits for
pending operations, but it does not force the runtime suspend callback
to run. If the sensor is active when the driver is removed, the external
clock and regulators can remain enabled and the power-down GPIO can
remain deasserted.

After disabling runtime PM, call ov5647_power_off() if the device is not
already runtime suspended, and then mark the runtime PM state as
suspended. Checking the runtime status avoids disabling the hardware
resources a second time when runtime PM has already powered off the
sensor.

This issue was found by manual code inspection.

Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/media/i2c/ov5647.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 3facf92b3841..d42d009772ac 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(&sensor->ctrls);
 	v4l2_device_unregister_subdev(sd);
 	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		ov5647_power_off(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
 }
 
 static const struct dev_pm_ops ov5647_pm_ops = {
-- 
2.43.0

Re: [PATCH] media: i2c: ov5647: fix power cleanup on remove

Posted by Dave Stevenson 1 week ago

Hi Guangshuo

On Tue, 15 Sept 2026 at 10:05, Guangshuo Li <lgs201920130244@gmail.com> wrote:
>
> ov5647_remove() disables runtime PM without powering off the sensor if
> it is still runtime active or updating the runtime PM state to
> suspended.
>
> pm_runtime_disable() prevents further runtime PM callbacks and waits for
> pending operations, but it does not force the runtime suspend callback
> to run. If the sensor is active when the driver is removed, the external
> clock and regulators can remain enabled and the power-down GPIO can
> remain deasserted.
>
> After disabling runtime PM, call ov5647_power_off() if the device is not
> already runtime suspended, and then mark the runtime PM state as
> suspended. Checking the runtime status avoids disabling the hardware
> resources a second time when runtime PM has already powered off the
> sensor.
>
> This issue was found by manual code inspection.
>
> Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/media/i2c/ov5647.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 3facf92b3841..d42d009772ac 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
>         v4l2_ctrl_handler_free(&sensor->ctrls);
>         v4l2_device_unregister_subdev(sd);
>         pm_runtime_disable(&client->dev);
> +       if (!pm_runtime_status_suspended(&client->dev))
> +               ov5647_power_off(&client->dev);
> +       pm_runtime_set_suspended(&client->dev);

I was unsure whether pm_runtime_set_suspended can be unconditional or
should be in the if clause. There are numerous drivers doing each.

As it happens, Sakari's just answered that in [1] that it should be
conditional, so I'd take that as gospel. It looks like he's given a
similar answer on your ov2740 patch.

  Dave

[1] https://lore.kernel.org/linux-media/aqvb_n-aPcXLnEa7@kekkonen.localdomain/T/#u

>  }
>
>  static const struct dev_pm_ops ov5647_pm_ops = {
> --
> 2.43.0
>

Re: [PATCH] media: i2c: ov5647: fix power cleanup on remove

Posted by Sakari Ailus 6 days, 22 hours ago

Hi Dave,

On Thu, Sep 17, 2026 at 02:27:30PM +0100, Dave Stevenson wrote:
> Hi Guangshuo
> 
> On Tue, 15 Sept 2026 at 10:05, Guangshuo Li <lgs201920130244@gmail.com> wrote:
> >
> > ov5647_remove() disables runtime PM without powering off the sensor if
> > it is still runtime active or updating the runtime PM state to
> > suspended.
> >
> > pm_runtime_disable() prevents further runtime PM callbacks and waits for
> > pending operations, but it does not force the runtime suspend callback
> > to run. If the sensor is active when the driver is removed, the external
> > clock and regulators can remain enabled and the power-down GPIO can
> > remain deasserted.
> >
> > After disabling runtime PM, call ov5647_power_off() if the device is not
> > already runtime suspended, and then mark the runtime PM state as
> > suspended. Checking the runtime status avoids disabling the hardware
> > resources a second time when runtime PM has already powered off the
> > sensor.
> >
> > This issue was found by manual code inspection.
> >
> > Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 3facf92b3841..d42d009772ac 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
> >         v4l2_ctrl_handler_free(&sensor->ctrls);
> >         v4l2_device_unregister_subdev(sd);
> >         pm_runtime_disable(&client->dev);
> > +       if (!pm_runtime_status_suspended(&client->dev))
> > +               ov5647_power_off(&client->dev);
> > +       pm_runtime_set_suspended(&client->dev);
> 
> I was unsure whether pm_runtime_set_suspended can be unconditional or
> should be in the if clause. There are numerous drivers doing each.
> 
> As it happens, Sakari's just answered that in [1] that it should be
> conditional, so I'd take that as gospel. It looks like he's given a
> similar answer on your ov2740 patch.

Runtime PM is a bit mystical in some places. pm_runtime_set_suspended()
appears to be setting the device's Runtime PM state disabled as it name
implies, but it may also e.g. increment dev->power.disable_depth. I'm not
fully certain if this is by design or not.

-- 
Sakari Ailus

Re: [PATCH] media: i2c: ov5647: fix power cleanup on remove

Posted by Guangshuo Li 2 days, 23 hours ago

Hi Dave, Sakari,

Thanks for the clarification.

On Fri, 18 Sept 2026 at 16:45, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Dave,
>
> On Thu, Sep 17, 2026 at 02:27:30PM +0100, Dave Stevenson wrote:
> > Hi Guangshuo
> >
> > On Tue, 15 Sept 2026 at 10:05, Guangshuo Li <lgs201920130244@gmail.com> wrote:
> > >
> > > ov5647_remove() disables runtime PM without powering off the sensor if
> > > it is still runtime active or updating the runtime PM state to
> > > suspended.
> > >
> > > pm_runtime_disable() prevents further runtime PM callbacks and waits for
> > > pending operations, but it does not force the runtime suspend callback
> > > to run. If the sensor is active when the driver is removed, the external
> > > clock and regulators can remain enabled and the power-down GPIO can
> > > remain deasserted.
> > >
> > > After disabling runtime PM, call ov5647_power_off() if the device is not
> > > already runtime suspended, and then mark the runtime PM state as
> > > suspended. Checking the runtime status avoids disabling the hardware
> > > resources a second time when runtime PM has already powered off the
> > > sensor.
> > >
> > > This issue was found by manual code inspection.
> > >
> > > Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > > ---
> > >  drivers/media/i2c/ov5647.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index 3facf92b3841..d42d009772ac 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
> > >         v4l2_ctrl_handler_free(&sensor->ctrls);
> > >         v4l2_device_unregister_subdev(sd);
> > >         pm_runtime_disable(&client->dev);
> > > +       if (!pm_runtime_status_suspended(&client->dev))
> > > +               ov5647_power_off(&client->dev);
> > > +       pm_runtime_set_suspended(&client->dev);
> >
> > I was unsure whether pm_runtime_set_suspended can be unconditional or
> > should be in the if clause. There are numerous drivers doing each.
> >
> > As it happens, Sakari's just answered that in [1] that it should be
> > conditional, so I'd take that as gospel. It looks like he's given a
> > similar answer on your ov2740 patch.
>
> Runtime PM is a bit mystical in some places. pm_runtime_set_suspended()
> appears to be setting the device's Runtime PM state disabled as it name
> implies, but it may also e.g. increment dev->power.disable_depth. I'm not
> fully certain if this is by design or not.
>
> --
> Sakari Ailus

Would this be the right way to change it?

-       if (!pm_runtime_status_suspended(&client->dev))
+       if (!pm_runtime_status_suspended(&client->dev)) {
                ov5647_power_off(&client->dev);
-       pm_runtime_set_suspended(&client->dev);
+               pm_runtime_set_suspended(&client->dev);
+       }

If so, I can send a v2 with this change.

Thanks,
Guangshuo

Re: [PATCH] media: i2c: ov5647: fix power cleanup on remove

Posted by Sakari Ailus 2 days, 23 hours ago

Hi,

On Tue, Sep 22, 2026 at 03:45:13PM +0800, Guangshuo Li wrote:
> Hi Dave, Sakari,
> 
> Thanks for the clarification.
> 
> On Fri, 18 Sept 2026 at 16:45, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Dave,
> >
> > On Thu, Sep 17, 2026 at 02:27:30PM +0100, Dave Stevenson wrote:
> > > Hi Guangshuo
> > >
> > > On Tue, 15 Sept 2026 at 10:05, Guangshuo Li <lgs201920130244@gmail.com> wrote:
> > > >
> > > > ov5647_remove() disables runtime PM without powering off the sensor if
> > > > it is still runtime active or updating the runtime PM state to
> > > > suspended.
> > > >
> > > > pm_runtime_disable() prevents further runtime PM callbacks and waits for
> > > > pending operations, but it does not force the runtime suspend callback
> > > > to run. If the sensor is active when the driver is removed, the external
> > > > clock and regulators can remain enabled and the power-down GPIO can
> > > > remain deasserted.
> > > >
> > > > After disabling runtime PM, call ov5647_power_off() if the device is not
> > > > already runtime suspended, and then mark the runtime PM state as
> > > > suspended. Checking the runtime status avoids disabling the hardware
> > > > resources a second time when runtime PM has already powered off the
> > > > sensor.
> > > >
> > > > This issue was found by manual code inspection.
> > > >
> > > > Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > > > ---
> > > >  drivers/media/i2c/ov5647.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index 3facf92b3841..d42d009772ac 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
> > > >         v4l2_ctrl_handler_free(&sensor->ctrls);
> > > >         v4l2_device_unregister_subdev(sd);
> > > >         pm_runtime_disable(&client->dev);
> > > > +       if (!pm_runtime_status_suspended(&client->dev))
> > > > +               ov5647_power_off(&client->dev);
> > > > +       pm_runtime_set_suspended(&client->dev);
> > >
> > > I was unsure whether pm_runtime_set_suspended can be unconditional or
> > > should be in the if clause. There are numerous drivers doing each.
> > >
> > > As it happens, Sakari's just answered that in [1] that it should be
> > > conditional, so I'd take that as gospel. It looks like he's given a
> > > similar answer on your ov2740 patch.
> >
> > Runtime PM is a bit mystical in some places. pm_runtime_set_suspended()
> > appears to be setting the device's Runtime PM state disabled as it name
> > implies, but it may also e.g. increment dev->power.disable_depth. I'm not
> > fully certain if this is by design or not.
> >
> > --
> > Sakari Ailus
> 
> Would this be the right way to change it?
> 
> -       if (!pm_runtime_status_suspended(&client->dev))
> +       if (!pm_runtime_status_suspended(&client->dev)) {
>                 ov5647_power_off(&client->dev);
> -       pm_runtime_set_suspended(&client->dev);
> +               pm_runtime_set_suspended(&client->dev);
> +       }

Correct.

> 
> If so, I can send a v2 with this change.

Please!

-- 
Regards,

Sakari Ailus