[PATCH v1 0/2] media: hi846: support system suspend while streaming

[PATCH v1 0/2] media: hi846: support system suspend while streaming

[Martin Kepplinger]

hi all,

the hi846 sensor driver was broken w.r.t. system suspend **while streaming**.
This is something that I did not test before initial submission.

These 2 small patches adjust the pm logic to support just that - stopping
streaming before suspending and resuming it again after system-resume.

I'm happy for any feedback,

thanks

                                martin


Martin Kepplinger (2):
  media: hi846: fix usage of pm_runtime_get_if_in_use()
  media: hi846: preserve the streaming state during system suspend

 drivers/media/i2c/hi846.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.30.2

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

[Martin Kepplinger]

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(-)

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) {
-- 
2.30.2

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

[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.

> 
> 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) {

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

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) {

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

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.)

-- 
Regards,

Sakari Ailus

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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

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

[Martin Kepplinger]

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) {
> 

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

[Sakari Ailus]

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.

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

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) {

-- 
Regards,

Laurent Pinchart

[PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Martin Kepplinger]

The hi846 driver changed the "streaming" state inside of "start/stop_streaming".
The problem is that inside of the (system) suspend callback, it calls
"stop_streaming" unconditionally. So streaming would always be stopped
when suspending.

That makes sense with runtime pm for example, after s_stream(..., 0) but
does not preserve the "streaming" state during system suspend when
currently streaming.

Fix this by simply setting the streaming state outside of "start/stop_streaming"
which is s_stream().

While at it, improve things a bit by not assigning "1", but the "enable"
value we later compare against, and fix one error handling path in resume().

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

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 0b0eda2e223cd..1ca6e9407d618 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846 *hi846)
 		return ret;
 	}
 
-	hi846->streaming = 1;
-
 	dev_dbg(&client->dev, "%s: started streaming successfully\n", __func__);
 
 	return ret;
@@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846 *hi846)
 
 	if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT, HI846_MODE_STANDBY))
 		dev_err(&client->dev, "failed to stop stream");
-
-	hi846->streaming = 0;
 }
 
 static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
@@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
 		}
 
 		ret = hi846_start_streaming(hi846);
+		hi846->streaming = enable;
 	}
 
 	if (!enable || ret) {
 		hi846_stop_streaming(hi846);
+		hi846->streaming = 0;
 		pm_runtime_put(&client->dev);
 	}
 
@@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct device *dev)
 		if (ret) {
 			dev_err(dev, "%s: start streaming failed: %d\n",
 				__func__, ret);
+			hi846_stop_streaming(hi846);
+			hi846->streaming = 0;
 			goto error;
 		}
 	}
-- 
2.30.2

Re: [PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Laurent Pinchart]

Hi Martin,

Thank you for the patch.

On Wed, Apr 05, 2023 at 11:29:04AM +0200, Martin Kepplinger wrote:
> The hi846 driver changed the "streaming" state inside of "start/stop_streaming".
> The problem is that inside of the (system) suspend callback, it calls
> "stop_streaming" unconditionally. So streaming would always be stopped
> when suspending.
> 
> That makes sense with runtime pm for example, after s_stream(..., 0) but
> does not preserve the "streaming" state during system suspend when
> currently streaming.

The driver shouldn't need to stop streaming at system suspend time. It
should have had its .s_stream(0) operation called and shouldn't be
streaming anymore. If that's not the case, there's an issue somewhere
else, which should be fixed. The code that stops streaming at system
suspend and restarts it at system resume should then be dropped from
this driver.

> Fix this by simply setting the streaming state outside of "start/stop_streaming"
> which is s_stream().
> 
> While at it, improve things a bit by not assigning "1", but the "enable"
> value we later compare against, and fix one error handling path in resume().
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
>  drivers/media/i2c/hi846.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 0b0eda2e223cd..1ca6e9407d618 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846 *hi846)
>  		return ret;
>  	}
>  
> -	hi846->streaming = 1;
> -
>  	dev_dbg(&client->dev, "%s: started streaming successfully\n", __func__);
>  
>  	return ret;
> @@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846 *hi846)
>  
>  	if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT, HI846_MODE_STANDBY))
>  		dev_err(&client->dev, "failed to stop stream");
> -
> -	hi846->streaming = 0;
>  }
>  
>  static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> @@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
>  		}
>  
>  		ret = hi846_start_streaming(hi846);
> +		hi846->streaming = enable;
>  	}
>  
>  	if (!enable || ret) {
>  		hi846_stop_streaming(hi846);
> +		hi846->streaming = 0;
>  		pm_runtime_put(&client->dev);
>  	}
>  
> @@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct device *dev)
>  		if (ret) {
>  			dev_err(dev, "%s: start streaming failed: %d\n",
>  				__func__, ret);
> +			hi846_stop_streaming(hi846);
> +			hi846->streaming = 0;
>  			goto error;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Martin Kepplinger]

Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> Thank you for the patch.
> 
> On Wed, Apr 05, 2023 at 11:29:04AM +0200, Martin Kepplinger wrote:
> > The hi846 driver changed the "streaming" state inside of
> > "start/stop_streaming".
> > The problem is that inside of the (system) suspend callback, it
> > calls
> > "stop_streaming" unconditionally. So streaming would always be
> > stopped
> > when suspending.
> > 
> > That makes sense with runtime pm for example, after s_stream(...,
> > 0) but
> > does not preserve the "streaming" state during system suspend when
> > currently streaming.
> 
> The driver shouldn't need to stop streaming at system suspend time.
> It
> should have had its .s_stream(0) operation called and shouldn't be
> streaming anymore. If that's not the case, there's an issue somewhere
> else, which should be fixed. The code that stops streaming at system
> suspend and restarts it at system resume should then be dropped from
> this driver.
> 
> > Fix this by simply setting the streaming state outside of
> > "start/stop_streaming"
> > which is s_stream().
> > 
> > While at it, improve things a bit by not assigning "1", but the
> > "enable"
> > value we later compare against, and fix one error handling path in
> > resume().
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> >  drivers/media/i2c/hi846.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > index 0b0eda2e223cd..1ca6e9407d618 100644
> > --- a/drivers/media/i2c/hi846.c
> > +++ b/drivers/media/i2c/hi846.c
> > @@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846
> > *hi846)
> >                 return ret;
> >         }
> >  
> > -       hi846->streaming = 1;
> > -
> >         dev_dbg(&client->dev, "%s: started streaming
> > successfully\n", __func__);
> >  
> >         return ret;
> > @@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846
> > *hi846)
> >  
> >         if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT,
> > HI846_MODE_STANDBY))
> >                 dev_err(&client->dev, "failed to stop stream");
> > -
> > -       hi846->streaming = 0;
> >  }
> >  
> >  static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > @@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct
> > v4l2_subdev *sd, int enable)
> >                 }
> >  
> >                 ret = hi846_start_streaming(hi846);
> > +               hi846->streaming = enable;
> >         }
> >  
> >         if (!enable || ret) {
> >                 hi846_stop_streaming(hi846);
> > +               hi846->streaming = 0;
> >                 pm_runtime_put(&client->dev);
> >         }
> >  
> > @@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct
> > device *dev)
> >                 if (ret) {
> >                         dev_err(dev, "%s: start streaming failed:
> > %d\n",
> >                                 __func__, ret);
> > +                       hi846_stop_streaming(hi846);
> > +                       hi846->streaming = 0;
> >                         goto error;
> >                 }
> >         }
> 

hi Laurent,

ok I see. My first test without any streaming-state handling in
suspend/resume doesn't succeed. But now I know that's the goal and I'll
put this on my list to do.

Since this driver *already* tracks "streaming", would you be ok with
this fix in the meantime?

thanks,
                               martin

Re: [PATCH v1 2/2] media: hi846: preserve the streaming state during system suspend

[Laurent Pinchart]

Hi Martin,

On Tue, Apr 11, 2023 at 11:39:32AM +0200, Martin Kepplinger wrote:
> Am Donnerstag, dem 06.04.2023 um 04:35 +0300 schrieb Laurent Pinchart:
> > On Wed, Apr 05, 2023 at 11:29:04AM +0200, Martin Kepplinger wrote:
> > > The hi846 driver changed the "streaming" state inside of "start/stop_streaming".
> > > The problem is that inside of the (system) suspend callback, it calls
> > > "stop_streaming" unconditionally. So streaming would always be stopped
> > > when suspending.
> > > 
> > > That makes sense with runtime pm for example, after s_stream(..., 0) but
> > > does not preserve the "streaming" state during system suspend when
> > > currently streaming.
> > 
> > The driver shouldn't need to stop streaming at system suspend time. It
> > should have had its .s_stream(0) operation called and shouldn't be
> > streaming anymore. If that's not the case, there's an issue somewhere
> > else, which should be fixed. The code that stops streaming at system
> > suspend and restarts it at system resume should then be dropped from
> > this driver.
> > 
> > > Fix this by simply setting the streaming state outside of "start/stop_streaming"
> > > which is s_stream().
> > > 
> > > While at it, improve things a bit by not assigning "1", but the "enable"
> > > value we later compare against, and fix one error handling path in
> > > resume().
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > ---
> > >  drivers/media/i2c/hi846.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > > index 0b0eda2e223cd..1ca6e9407d618 100644
> > > --- a/drivers/media/i2c/hi846.c
> > > +++ b/drivers/media/i2c/hi846.c
> > > @@ -1780,8 +1780,6 @@ static int hi846_start_streaming(struct hi846 *hi846)
> > >                 return ret;
> > >         }
> > >  
> > > -       hi846->streaming = 1;
> > > -
> > >         dev_dbg(&client->dev, "%s: started streaming successfully\n", __func__);
> > >  
> > >         return ret;
> > > @@ -1793,8 +1791,6 @@ static void hi846_stop_streaming(struct hi846 *hi846)
> > >  
> > >         if (hi846_write_reg(hi846, HI846_REG_MODE_SELECT, HI846_MODE_STANDBY))
> > >                 dev_err(&client->dev, "failed to stop stream");
> > > -
> > > -       hi846->streaming = 0;
> > >  }
> > >  
> > >  static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > > @@ -1816,10 +1812,12 @@ static int hi846_set_stream(struct v4l2_subdev *sd, int enable)
> > >                 }
> > >  
> > >                 ret = hi846_start_streaming(hi846);
> > > +               hi846->streaming = enable;
> > >         }
> > >  
> > >         if (!enable || ret) {
> > >                 hi846_stop_streaming(hi846);
> > > +               hi846->streaming = 0;
> > >                 pm_runtime_put(&client->dev);
> > >         }
> > >  
> > > @@ -1898,6 +1896,8 @@ static int __maybe_unused hi846_resume(struct device *dev)
> > >                 if (ret) {
> > >                         dev_err(dev, "%s: start streaming failed: %d\n",
> > >                                 __func__, ret);
> > > +                       hi846_stop_streaming(hi846);
> > > +                       hi846->streaming = 0;
> > >                         goto error;
> > >                 }
> > >         }
> 
> hi Laurent,
> 
> ok I see. My first test without any streaming-state handling in
> suspend/resume doesn't succeed. But now I know that's the goal and I'll
> put this on my list to do.
> 
> Since this driver *already* tracks "streaming", would you be ok with
> this fix in the meantime?

I'd rather get a patch that drops streaming handling :-) It's fine
carrying a local patch in the Librem5 kernel to work around the problem
until a proper fix is available, but do we have a need to get the
workaround in mainline too ?

When it comes to a proper fix, it may be as simple as manually calling
device_link_add() in consumer (e.g. the CSI-2 receiver) drivers to
create links to suppliers(e.g. the camera sensor). The PM core should
then guarantee that the consumer gets suspended before the producer.
Would you be able to test that ?

-- 
Regards,

Laurent Pinchart