diff mbox series

[v1,1/2] media: hi846: fix usage of pm_runtime_get_if_in_use()

Message ID 20230405092904.1129395-2-martin.kepplinger@puri.sm (mailing list archive)
State Superseded
Headers
Series media: hi846: support system suspend while streaming |

Commit Message

Martin Kepplinger April 5, 2023, 9:29 a.m. UTC 
  pm_runtime_get_if_in_use() does not only return nonzero values when
the device is in use, it can return a negative errno too.

And especially during resuming from system suspend, when runtime pm
is not yet up again, this can very well happen. And in such a case
the subsequent pm_runtime_put() call would result in a refcount underflow!

Fix it by correctly using pm_runtime_get_if_in_use().

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/media/i2c/hi846.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sakari Ailus April 5, 2023, 12:52 p.m. UTC | #1 
Hi Martin,

On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> pm_runtime_get_if_in_use() does not only return nonzero values when
> the device is in use, it can return a negative errno too.
> 
> And especially during resuming from system suspend, when runtime pm
> is not yet up again, this can very well happen. And in such a case
> the subsequent pm_runtime_put() call would result in a refcount underflow!

I think this issue should have a more generic solution, it's very difficult
to address this in drivers only with the current APIs.

pm_runtime_get_if_in_use() will also return an error if runtime PM is
disabled, so this patch will break the driver for that configuration.

> 
> Fix it by correctly using pm_runtime_get_if_in_use().
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
>  drivers/media/i2c/hi846.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 5b5ea5425e984..0b0eda2e223cd 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl)
>  					 exposure_max);
>  	}
>  
> -	if (!pm_runtime_get_if_in_use(&client->dev))
> +	if (pm_runtime_get_if_in_use(&client->dev) <= 0)
>  		return 0;
>  
>  	switch (ctrl->id) {
Laurent Pinchart April 6, 2023, 1:32 a.m. UTC | #2 
Hi Martin,

Thank you for the patch.

On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> pm_runtime_get_if_in_use() does not only return nonzero values when
> the device is in use, it can return a negative errno too.
> 
> And especially during resuming from system suspend, when runtime pm
> is not yet up again, this can very well happen. And in such a case
> the subsequent pm_runtime_put() call would result in a refcount underflow!
> 
> Fix it by correctly using pm_runtime_get_if_in_use().
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/hi846.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 5b5ea5425e984..0b0eda2e223cd 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl)
>  					 exposure_max);
>  	}
>  
> -	if (!pm_runtime_get_if_in_use(&client->dev))
> +	if (pm_runtime_get_if_in_use(&client->dev) <= 0)
>  		return 0;
>  
>  	switch (ctrl->id) {
Laurent Pinchart April 6, 2023, 1:33 a.m. UTC | #3 
Hi Sakari,

On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote:
> On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> > pm_runtime_get_if_in_use() does not only return nonzero values when
> > the device is in use, it can return a negative errno too.
> > 
> > And especially during resuming from system suspend, when runtime pm
> > is not yet up again, this can very well happen. And in such a case
> > the subsequent pm_runtime_put() call would result in a refcount underflow!
> 
> I think this issue should have a more generic solution, it's very difficult
> to address this in drivers only with the current APIs.
> 
> pm_runtime_get_if_in_use() will also return an error if runtime PM is
> disabled, so this patch will break the driver for that configuration.

I'm increasingly inclined to depend on CONFIG_PM for all camera sensor
drivers.

> > Fix it by correctly using pm_runtime_get_if_in_use().
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/media/i2c/hi846.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > index 5b5ea5425e984..0b0eda2e223cd 100644
> > --- a/drivers/media/i2c/hi846.c
> > +++ b/drivers/media/i2c/hi846.c
> > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl *ctrl)
> >  					 exposure_max);
> >  	}
> >  
> > -	if (!pm_runtime_get_if_in_use(&client->dev))
> > +	if (pm_runtime_get_if_in_use(&client->dev) <= 0)
> >  		return 0;
> >  
> >  	switch (ctrl->id) {
Sakari Ailus April 6, 2023, 1:36 p.m. UTC | #4 
Hi Laurent,

On Thu, Apr 06, 2023 at 04:33:38AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote:
> > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> > > pm_runtime_get_if_in_use() does not only return nonzero values when
> > > the device is in use, it can return a negative errno too.
> > > 
> > > And especially during resuming from system suspend, when runtime pm
> > > is not yet up again, this can very well happen. And in such a case
> > > the subsequent pm_runtime_put() call would result in a refcount underflow!
> > 
> > I think this issue should have a more generic solution, it's very difficult
> > to address this in drivers only with the current APIs.
> > 
> > pm_runtime_get_if_in_use() will also return an error if runtime PM is
> > disabled, so this patch will break the driver for that configuration.
> 
> I'm increasingly inclined to depend on CONFIG_PM for all camera sensor
> drivers.

For what reason? This is the smallest of all problems related to power
management. Also runtime PM has no-op functions for just this purpose.

(Frankly it'd be great if we could make CONFIG_PM go away. So perhaps
requiring it everywhere is one feasible approach to do that.)
Laurent Pinchart April 7, 2023, 4:32 a.m. UTC | #5 
Hi Sakari,

On Thu, Apr 06, 2023 at 04:36:25PM +0300, Sakari Ailus wrote:
> On Thu, Apr 06, 2023 at 04:33:38AM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 05, 2023 at 03:52:52PM +0300, Sakari Ailus wrote:
> > > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> > > > pm_runtime_get_if_in_use() does not only return nonzero values when
> > > > the device is in use, it can return a negative errno too.
> > > > 
> > > > And especially during resuming from system suspend, when runtime pm
> > > > is not yet up again, this can very well happen. And in such a case
> > > > the subsequent pm_runtime_put() call would result in a refcount underflow!
> > > 
> > > I think this issue should have a more generic solution, it's very difficult
> > > to address this in drivers only with the current APIs.
> > > 
> > > pm_runtime_get_if_in_use() will also return an error if runtime PM is
> > > disabled, so this patch will break the driver for that configuration.
> > 
> > I'm increasingly inclined to depend on CONFIG_PM for all camera sensor
> > drivers.
> 
> For what reason? This is the smallest of all problems related to power
> management. Also runtime PM has no-op functions for just this purpose.

Because it creates a myriad of sleep (or bigger) issues like this one,
and more seem to be popping up (or coming to my attention at least) over
time.

> (Frankly it'd be great if we could make CONFIG_PM go away. So perhaps
> requiring it everywhere is one feasible approach to do that.)

I'm all for it :-) For camera sensor drivers, are you aware of use
cases where !CONFIG_PM would be desired ?
Martin Kepplinger April 7, 2023, 1:31 p.m. UTC | #6 
Am Mittwoch, dem 05.04.2023 um 15:52 +0300 schrieb Sakari Ailus:
> Hi Martin,
> 
> On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> > pm_runtime_get_if_in_use() does not only return nonzero values when
> > the device is in use, it can return a negative errno too.
> > 
> > And especially during resuming from system suspend, when runtime pm
> > is not yet up again, this can very well happen. And in such a case
> > the subsequent pm_runtime_put() call would result in a refcount
> > underflow!
> 
> I think this issue should have a more generic solution, it's very
> difficult
> to address this in drivers only with the current APIs.
> 
> pm_runtime_get_if_in_use() will also return an error if runtime PM is
> disabled, so this patch will break the driver for that configuration.

ok but the driver is currently broken for any *other* error returned by
pm_runtime_get_if_in_use() (than the runtime-PM disabled error).

The execution-path during system-resume I'm interested in gets -EAGAIN
here. Would it be ok for you if I'd return early only for that one
error only here?


> 
> > 
> > Fix it by correctly using pm_runtime_get_if_in_use().
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/media/i2c/hi846.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > index 5b5ea5425e984..0b0eda2e223cd 100644
> > --- a/drivers/media/i2c/hi846.c
> > +++ b/drivers/media/i2c/hi846.c
> > @@ -1544,7 +1544,7 @@ static int hi846_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> >                                          exposure_max);
> >         }
> >  
> > -       if (!pm_runtime_get_if_in_use(&client->dev))
> > +       if (pm_runtime_get_if_in_use(&client->dev) <= 0)
> >                 return 0;
> >  
> >         switch (ctrl->id) {
>
Sakari Ailus April 7, 2023, 6:37 p.m. UTC | #7 
Hi Martin,

On Fri, Apr 07, 2023 at 03:31:02PM +0200, Martin Kepplinger wrote:
> Am Mittwoch, dem 05.04.2023 um 15:52 +0300 schrieb Sakari Ailus:
> > Hi Martin,
> > 
> > On Wed, Apr 05, 2023 at 11:29:03AM +0200, Martin Kepplinger wrote:
> > > pm_runtime_get_if_in_use() does not only return nonzero values when
> > > the device is in use, it can return a negative errno too.
> > > 
> > > And especially during resuming from system suspend, when runtime pm
> > > is not yet up again, this can very well happen. And in such a case
> > > the subsequent pm_runtime_put() call would result in a refcount
> > > underflow!
> > 
> > I think this issue should have a more generic solution, it's very
> > difficult
> > to address this in drivers only with the current APIs.
> > 
> > pm_runtime_get_if_in_use() will also return an error if runtime PM is
> > disabled, so this patch will break the driver for that configuration.
> 
> ok but the driver is currently broken for any *other* error returned by
> pm_runtime_get_if_in_use() (than the runtime-PM disabled error).
> 
> The execution-path during system-resume I'm interested in gets -EAGAIN
> here. Would it be ok for you if I'd return early only for that one
> error only here?

I guess... but I think to address this in a way that's reasonable to
drivers, we'll need improvements to runtime PM API. A largish number of
drivers need changes and before doing that we should figure out exactly
what should be done.

I thought you could effectively trigger this issue by calling runtime PM
resume/suspend functions before enabling runtime PM, but this seems to be a
different case.
diff mbox series

Patch

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 5b5ea5425e984..0b0eda2e223cd 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1544,7 +1544,7 @@  static int hi846_set_ctrl(struct v4l2_ctrl *ctrl)
 					 exposure_max);
 	}
 
-	if (!pm_runtime_get_if_in_use(&client->dev))
+	if (pm_runtime_get_if_in_use(&client->dev) <= 0)
 		return 0;
 
 	switch (ctrl->id) {