[25/78] media: i2c: ccs-core: use pm_runtime_resume_and_get()
diff mbox series

Message ID 34da940f76da6c1d61a193409164070f47243b64.1619191723.git.mchehab+huawei@kernel.org
State New, archived
Headers show
Series
  • media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
Related show

Commit Message

Mauro Carvalho Chehab April 24, 2021, 6:44 a.m. UTC
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Sakari Ailus April 25, 2021, 6:55 p.m. UTC | #1
Hi Mauro,

Thanks for the patch.

On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors.
> 
> Use the new API, in order to cleanup the error check logic.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 9dc3f45da3dc..1441ddcc9b35 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>  	int rval;
>  
> -	rval = pm_runtime_get_sync(&client->dev);
> -	if (rval < 0) {
> -		pm_runtime_put_noidle(&client->dev);
> -
> +	rval = pm_runtime_resume_and_get(&client->dev);
> +	if (rval < 0)
>  		return rval;
> -	} else if (!rval) {
> +
> +	if (!rval) {
>  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
>  					       ctrl_handler);
>  		if (rval)
> @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
>  	bool streaming = sensor->streaming;
>  	int rval;
>  
> -	rval = pm_runtime_get_sync(dev);
> +	rval = pm_runtime_resume_and_get(dev);
>  	if (rval < 0) {
>  		pm_runtime_put_noidle(dev);

You'll need to drop pm_runtime_put_noidle() here.
Mauro Carvalho Chehab April 26, 2021, 2:01 p.m. UTC | #2
Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thanks for the patch.
> 
> On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:
> > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > added pm_runtime_resume_and_get() in order to automatically handle
> > dev->power.usage_count decrement on errors.
> > 
> > Use the new API, in order to cleanup the error check logic.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index 9dc3f45da3dc..1441ddcc9b35 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> >  	int rval;
> >  
> > -	rval = pm_runtime_get_sync(&client->dev);
> > -	if (rval < 0) {
> > -		pm_runtime_put_noidle(&client->dev);
> > -
> > +	rval = pm_runtime_resume_and_get(&client->dev);
> > +	if (rval < 0)
> >  		return rval;
> > -	} else if (!rval) {
> > +
> > +	if (!rval) {
> >  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> >  					       ctrl_handler);
> >  		if (rval)
> > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> >  	bool streaming = sensor->streaming;
> >  	int rval;
> >  
> > -	rval = pm_runtime_get_sync(dev);
> > +	rval = pm_runtime_resume_and_get(dev);
> >  	if (rval < 0) {
> >  		pm_runtime_put_noidle(dev);  
> 
> You'll need to drop pm_runtime_put_noidle() here.

OK!

---

On a non-related issue at the same code, after the change, the
suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;
  }

Not sure if "return -EAGAIN" is the right thing here. I mean,
the PM runtime core has two error conditions that are independent
on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve
those conditions.

So, I guess that the right thing to do is to change the code
to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...
  }


[1] see rpm_resume() code at drivers/base/power/runtime.c.

Thanks,
Mauro
Sakari Ailus April 26, 2021, 2:09 p.m. UTC | #3
Hi Mauro,

On Mon, Apr 26, 2021 at 04:01:51PM +0200, Mauro Carvalho Chehab wrote:
> Em Sun, 25 Apr 2021 21:55:25 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the patch.
> > 
> > On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:
> > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors.
> > > 
> > > Use the new API, in order to cleanup the error check logic.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > index 9dc3f45da3dc..1441ddcc9b35 100644
> > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > >  	int rval;
> > >  
> > > -	rval = pm_runtime_get_sync(&client->dev);
> > > -	if (rval < 0) {
> > > -		pm_runtime_put_noidle(&client->dev);
> > > -
> > > +	rval = pm_runtime_resume_and_get(&client->dev);
> > > +	if (rval < 0)
> > >  		return rval;
> > > -	} else if (!rval) {
> > > +
> > > +	if (!rval) {
> > >  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > >  					       ctrl_handler);
> > >  		if (rval)
> > > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > >  	bool streaming = sensor->streaming;
> > >  	int rval;
> > >  
> > > -	rval = pm_runtime_get_sync(dev);
> > > +	rval = pm_runtime_resume_and_get(dev);
> > >  	if (rval < 0) {
> > >  		pm_runtime_put_noidle(dev);  
> > 
> > You'll need to drop pm_runtime_put_noidle() here.
> 
> OK!
> 
> ---
> 
> On a non-related issue at the same code, after the change, the
> suspend function will be:
> 
>   static int __maybe_unused ccs_suspend(struct device *dev)
>   {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
>         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
>         bool streaming = sensor->streaming;
>         int rval;
> 
>         rval = pm_runtime_resume_and_get(dev);
>         if (rval < 0) 
>                 return -EAGAIN;
> 
>         if (sensor->streaming)
>                 ccs_stop_streaming(sensor);
> 
>         /* save state for resume */
>         sensor->streaming = streaming;
> 
>         return 0;
>   }
> 
> Not sure if "return -EAGAIN" is the right thing here. I mean,
> the PM runtime core has two error conditions that are independent
> on whatever the PM callback would be doing[1]:
> 
> 	        if (dev->power.runtime_error)
>                 retval = -EINVAL;
>         else if (dev->power.disable_depth > 0)
>                 retval = -EACCES;
> 
> It would be very unlikely that trying to suspend again would solve
> those conditions.
> 
> So, I guess that the right thing to do is to change the code
> to do, instead:
> 
>   static int __maybe_unused ccs_suspend(struct device *dev)
>   {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
>         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
>         bool streaming = sensor->streaming;
>         int rval;
> 
>         rval = pm_runtime_resume_and_get(dev);
>         if (rval < 0) 
>                 return rval;
> 	...
>   }
> 
> 
> [1] see rpm_resume() code at drivers/base/power/runtime.c.

Yeah, I agree. This code is one of the older parts the driver.

Please add:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

The same goes for the other sensor driver patches in the set you cc'd me,
i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.
Mauro Carvalho Chehab April 26, 2021, 2:16 p.m. UTC | #4
Em Mon, 26 Apr 2021 17:09:00 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> On Mon, Apr 26, 2021 at 04:01:51PM +0200, Mauro Carvalho Chehab wrote:
> > Em Sun, 25 Apr 2021 21:55:25 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:  
> > > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > > added pm_runtime_resume_and_get() in order to automatically handle
> > > > dev->power.usage_count decrement on errors.
> > > > 
> > > > Use the new API, in order to cleanup the error check logic.
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > >  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > index 9dc3f45da3dc..1441ddcc9b35 100644
> > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > > >  	int rval;
> > > >  
> > > > -	rval = pm_runtime_get_sync(&client->dev);
> > > > -	if (rval < 0) {
> > > > -		pm_runtime_put_noidle(&client->dev);
> > > > -
> > > > +	rval = pm_runtime_resume_and_get(&client->dev);
> > > > +	if (rval < 0)
> > > >  		return rval;
> > > > -	} else if (!rval) {
> > > > +
> > > > +	if (!rval) {
> > > >  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > >  					       ctrl_handler);
> > > >  		if (rval)
> > > > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > > >  	bool streaming = sensor->streaming;
> > > >  	int rval;
> > > >  
> > > > -	rval = pm_runtime_get_sync(dev);
> > > > +	rval = pm_runtime_resume_and_get(dev);
> > > >  	if (rval < 0) {
> > > >  		pm_runtime_put_noidle(dev);    
> > > 
> > > You'll need to drop pm_runtime_put_noidle() here.  
> > 
> > OK!
> > 
> > ---
> > 
> > On a non-related issue at the same code, after the change, the
> > suspend function will be:
> > 
> >   static int __maybe_unused ccs_suspend(struct device *dev)
> >   {
> >         struct i2c_client *client = to_i2c_client(dev);
> >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> >         bool streaming = sensor->streaming;
> >         int rval;
> > 
> >         rval = pm_runtime_resume_and_get(dev);
> >         if (rval < 0) 
> >                 return -EAGAIN;
> > 
> >         if (sensor->streaming)
> >                 ccs_stop_streaming(sensor);
> > 
> >         /* save state for resume */
> >         sensor->streaming = streaming;
> > 
> >         return 0;
> >   }
> > 
> > Not sure if "return -EAGAIN" is the right thing here. I mean,
> > the PM runtime core has two error conditions that are independent
> > on whatever the PM callback would be doing[1]:
> > 
> > 	        if (dev->power.runtime_error)
> >                 retval = -EINVAL;
> >         else if (dev->power.disable_depth > 0)
> >                 retval = -EACCES;
> > 
> > It would be very unlikely that trying to suspend again would solve
> > those conditions.
> > 
> > So, I guess that the right thing to do is to change the code
> > to do, instead:
> > 
> >   static int __maybe_unused ccs_suspend(struct device *dev)
> >   {
> >         struct i2c_client *client = to_i2c_client(dev);
> >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> >         bool streaming = sensor->streaming;
> >         int rval;
> > 
> >         rval = pm_runtime_resume_and_get(dev);
> >         if (rval < 0) 
> >                 return rval;
> > 	...
> >   }
> > 
> > 
> > [1] see rpm_resume() code at drivers/base/power/runtime.c.  
> 
> Yeah, I agree. This code is one of the older parts the driver.
> 
> Please add:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The same goes for the other sensor driver patches in the set you cc'd me,
> i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.

It probably makes sense to address the suspend/resume -EAGAIN
return code on a separate patch series, before this one, as:

1. this is unrelated to this change;
2. it is something that should be c/c to fixes. So, having it
   before this series makes easier to apply there.

Thanks,
Mauro
Sakari Ailus April 26, 2021, 2:29 p.m. UTC | #5
On Mon, Apr 26, 2021 at 04:16:59PM +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 26 Apr 2021 17:09:00 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Apr 26, 2021 at 04:01:51PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Sun, 25 Apr 2021 21:55:25 +0300
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >   
> > > > Hi Mauro,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:  
> > > > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > > > added pm_runtime_resume_and_get() in order to automatically handle
> > > > > dev->power.usage_count decrement on errors.
> > > > > 
> > > > > Use the new API, in order to cleanup the error check logic.
> > > > > 
> > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > ---
> > > > >  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
> > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > > index 9dc3f45da3dc..1441ddcc9b35 100644
> > > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > > > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > > > >  	int rval;
> > > > >  
> > > > > -	rval = pm_runtime_get_sync(&client->dev);
> > > > > -	if (rval < 0) {
> > > > > -		pm_runtime_put_noidle(&client->dev);
> > > > > -
> > > > > +	rval = pm_runtime_resume_and_get(&client->dev);
> > > > > +	if (rval < 0)
> > > > >  		return rval;
> > > > > -	} else if (!rval) {
> > > > > +
> > > > > +	if (!rval) {
> > > > >  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > > >  					       ctrl_handler);
> > > > >  		if (rval)
> > > > > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > > > >  	bool streaming = sensor->streaming;
> > > > >  	int rval;
> > > > >  
> > > > > -	rval = pm_runtime_get_sync(dev);
> > > > > +	rval = pm_runtime_resume_and_get(dev);
> > > > >  	if (rval < 0) {
> > > > >  		pm_runtime_put_noidle(dev);    
> > > > 
> > > > You'll need to drop pm_runtime_put_noidle() here.  
> > > 
> > > OK!
> > > 
> > > ---
> > > 
> > > On a non-related issue at the same code, after the change, the
> > > suspend function will be:
> > > 
> > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > >   {
> > >         struct i2c_client *client = to_i2c_client(dev);
> > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > >         bool streaming = sensor->streaming;
> > >         int rval;
> > > 
> > >         rval = pm_runtime_resume_and_get(dev);
> > >         if (rval < 0) 
> > >                 return -EAGAIN;
> > > 
> > >         if (sensor->streaming)
> > >                 ccs_stop_streaming(sensor);
> > > 
> > >         /* save state for resume */
> > >         sensor->streaming = streaming;
> > > 
> > >         return 0;
> > >   }
> > > 
> > > Not sure if "return -EAGAIN" is the right thing here. I mean,
> > > the PM runtime core has two error conditions that are independent
> > > on whatever the PM callback would be doing[1]:
> > > 
> > > 	        if (dev->power.runtime_error)
> > >                 retval = -EINVAL;
> > >         else if (dev->power.disable_depth > 0)
> > >                 retval = -EACCES;
> > > 
> > > It would be very unlikely that trying to suspend again would solve
> > > those conditions.
> > > 
> > > So, I guess that the right thing to do is to change the code
> > > to do, instead:
> > > 
> > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > >   {
> > >         struct i2c_client *client = to_i2c_client(dev);
> > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > >         bool streaming = sensor->streaming;
> > >         int rval;
> > > 
> > >         rval = pm_runtime_resume_and_get(dev);
> > >         if (rval < 0) 
> > >                 return rval;
> > > 	...
> > >   }
> > > 
> > > 
> > > [1] see rpm_resume() code at drivers/base/power/runtime.c.  
> > 
> > Yeah, I agree. This code is one of the older parts the driver.
> > 
> > Please add:
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > The same goes for the other sensor driver patches in the set you cc'd me,
> > i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.
> 
> It probably makes sense to address the suspend/resume -EAGAIN
> return code on a separate patch series, before this one, as:
> 
> 1. this is unrelated to this change;
> 2. it is something that should be c/c to fixes. So, having it
>    before this series makes easier to apply there.

Sounds good to me. If you can submit a patch, please add my ack. :-)
Mauro Carvalho Chehab April 26, 2021, 5:32 p.m. UTC | #6
Em Mon, 26 Apr 2021 17:29:02 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> On Mon, Apr 26, 2021 at 04:16:59PM +0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Apr 2021 17:09:00 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Mon, Apr 26, 2021 at 04:01:51PM +0200, Mauro Carvalho Chehab wrote:  
> > > > Em Sun, 25 Apr 2021 21:55:25 +0300
> > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > >     
> > > > > Hi Mauro,
> > > > > 
> > > > > Thanks for the patch.
> > > > > 
> > > > > On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote:    
> > > > > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > > > > added pm_runtime_resume_and_get() in order to automatically handle
> > > > > > dev->power.usage_count decrement on errors.
> > > > > > 
> > > > > > Use the new API, in order to cleanup the error check logic.
> > > > > > 
> > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > > ---
> > > > > >  drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
> > > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > > > > > index 9dc3f45da3dc..1441ddcc9b35 100644
> > > > > > --- a/drivers/media/i2c/ccs/ccs-core.c
> > > > > > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > > > > > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
> > > > > >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > > > > >  	int rval;
> > > > > >  
> > > > > > -	rval = pm_runtime_get_sync(&client->dev);
> > > > > > -	if (rval < 0) {
> > > > > > -		pm_runtime_put_noidle(&client->dev);
> > > > > > -
> > > > > > +	rval = pm_runtime_resume_and_get(&client->dev);
> > > > > > +	if (rval < 0)
> > > > > >  		return rval;
> > > > > > -	} else if (!rval) {
> > > > > > +
> > > > > > +	if (!rval) {
> > > > > >  		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> > > > > >  					       ctrl_handler);
> > > > > >  		if (rval)
> > > > > > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
> > > > > >  	bool streaming = sensor->streaming;
> > > > > >  	int rval;
> > > > > >  
> > > > > > -	rval = pm_runtime_get_sync(dev);
> > > > > > +	rval = pm_runtime_resume_and_get(dev);
> > > > > >  	if (rval < 0) {
> > > > > >  		pm_runtime_put_noidle(dev);      
> > > > > 
> > > > > You'll need to drop pm_runtime_put_noidle() here.    
> > > > 
> > > > OK!
> > > > 
> > > > ---
> > > > 
> > > > On a non-related issue at the same code, after the change, the
> > > > suspend function will be:
> > > > 
> > > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > > >   {
> > > >         struct i2c_client *client = to_i2c_client(dev);
> > > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > >         bool streaming = sensor->streaming;
> > > >         int rval;
> > > > 
> > > >         rval = pm_runtime_resume_and_get(dev);
> > > >         if (rval < 0) 
> > > >                 return -EAGAIN;
> > > > 
> > > >         if (sensor->streaming)
> > > >                 ccs_stop_streaming(sensor);
> > > > 
> > > >         /* save state for resume */
> > > >         sensor->streaming = streaming;
> > > > 
> > > >         return 0;
> > > >   }
> > > > 
> > > > Not sure if "return -EAGAIN" is the right thing here. I mean,
> > > > the PM runtime core has two error conditions that are independent
> > > > on whatever the PM callback would be doing[1]:
> > > > 
> > > > 	        if (dev->power.runtime_error)
> > > >                 retval = -EINVAL;
> > > >         else if (dev->power.disable_depth > 0)
> > > >                 retval = -EACCES;
> > > > 
> > > > It would be very unlikely that trying to suspend again would solve
> > > > those conditions.
> > > > 
> > > > So, I guess that the right thing to do is to change the code
> > > > to do, instead:
> > > > 
> > > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > > >   {
> > > >         struct i2c_client *client = to_i2c_client(dev);
> > > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > >         bool streaming = sensor->streaming;
> > > >         int rval;
> > > > 
> > > >         rval = pm_runtime_resume_and_get(dev);
> > > >         if (rval < 0) 
> > > >                 return rval;
> > > > 	...
> > > >   }
> > > > 
> > > > 
> > > > [1] see rpm_resume() code at drivers/base/power/runtime.c.    
> > > 
> > > Yeah, I agree. This code is one of the older parts the driver.
> > > 
> > > Please add:
> > > 
> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > 
> > > The same goes for the other sensor driver patches in the set you cc'd me,
> > > i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.  
> > 
> > It probably makes sense to address the suspend/resume -EAGAIN
> > return code on a separate patch series, before this one, as:
> > 
> > 1. this is unrelated to this change;
> > 2. it is something that should be c/c to fixes. So, having it
> >    before this series makes easier to apply there.  
> 
> Sounds good to me. If you can submit a patch, please add my ack. :-)

Sure. I'll work on such patch series.

Thanks!
Mauro
Mauro Carvalho Chehab April 27, 2021, 7:02 a.m. UTC | #7
Hi Sakari,

Em Mon, 26 Apr 2021 19:32:23 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 26 Apr 2021 17:29:02 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > On Mon, Apr 26, 2021 at 04:16:59PM +0200, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 26 Apr 2021 17:09:00 +0300
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >     
> > > > Hi Mauro,
> > > > 
> > > > On Mon, Apr 26, 2021 at 04:01:51PM +0200, Mauro Carvalho Chehab wrote:    
> > > > > Em Sun, 25 Apr 2021 21:55:25 +0300
> > > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > > >       
... 
> > > > > On a non-related issue at the same code, after the change, the
> > > > > suspend function will be:
> > > > > 
> > > > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > > > >   {
> > > > >         struct i2c_client *client = to_i2c_client(dev);
> > > > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > > >         bool streaming = sensor->streaming;
> > > > >         int rval;
> > > > > 
> > > > >         rval = pm_runtime_resume_and_get(dev);
> > > > >         if (rval < 0) 
> > > > >                 return -EAGAIN;
> > > > > 
> > > > >         if (sensor->streaming)
> > > > >                 ccs_stop_streaming(sensor);
> > > > > 
> > > > >         /* save state for resume */
> > > > >         sensor->streaming = streaming;
> > > > > 
> > > > >         return 0;
> > > > >   }
> > > > > 
> > > > > Not sure if "return -EAGAIN" is the right thing here. I mean,
> > > > > the PM runtime core has two error conditions that are independent
> > > > > on whatever the PM callback would be doing[1]:
> > > > > 
> > > > > 	        if (dev->power.runtime_error)
> > > > >                 retval = -EINVAL;
> > > > >         else if (dev->power.disable_depth > 0)
> > > > >                 retval = -EACCES;
> > > > > 
> > > > > It would be very unlikely that trying to suspend again would solve
> > > > > those conditions.
> > > > > 
> > > > > So, I guess that the right thing to do is to change the code
> > > > > to do, instead:
> > > > > 
> > > > >   static int __maybe_unused ccs_suspend(struct device *dev)
> > > > >   {
> > > > >         struct i2c_client *client = to_i2c_client(dev);
> > > > >         struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > > >         struct ccs_sensor *sensor = to_ccs_sensor(subdev);
> > > > >         bool streaming = sensor->streaming;
> > > > >         int rval;
> > > > > 
> > > > >         rval = pm_runtime_resume_and_get(dev);
> > > > >         if (rval < 0) 
> > > > >                 return rval;
> > > > > 	...
> > > > >   }
> > > > > 
> > > > > 
> > > > > [1] see rpm_resume() code at drivers/base/power/runtime.c.      
> > > > 
> > > > Yeah, I agree. This code is one of the older parts the driver.
> > > > 
> > > > Please add:
> > > > 
> > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > 
> > > > The same goes for the other sensor driver patches in the set you cc'd me,
> > > > i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.    
> > > 
> > > It probably makes sense to address the suspend/resume -EAGAIN
> > > return code on a separate patch series, before this one, as:
> > > 
> > > 1. this is unrelated to this change;
> > > 2. it is something that should be c/c to fixes. So, having it
> > >    before this series makes easier to apply there.    
> > 
> > Sounds good to me. If you can submit a patch, please add my ack. :-)  
> 
> Sure. I'll work on such patch series.

I checked the files affected by those patches:
12, 15, 26, 28,32, 40, 45, 51, 53 and 55, e.g.:


	drivers/staging/media/atomisp/pci/atomisp_fops.c
	drivers/staging/media/ipu3/ipu3.c
	drivers/media/i2c/dw9714.c
	drivers/media/i2c/dw9807-vcm.c
	drivers/media/i2c/imx258.c
	drivers/media/i2c/ov13858.c
	drivers/media/i2c/ov8865.c
	drivers/media/i2c/tvp5150.c
	drivers/media/pci/intel/ipu3/ipu3-cio2-main.c

I also double-checked the I2C drivers that use SET_SYSTEM_SLEEP_PM_OPS().
None of them are calling pm_runtime_* at suspend time, except for the
ccs-core.

So, I ended writing just a patch for ccs-core, to be applied
before this /78 series.

Thanks,
Mauro

Patch
diff mbox series

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 9dc3f45da3dc..1441ddcc9b35 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -1880,12 +1880,11 @@  static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)
@@ -3089,7 +3088,7 @@  static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 
-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);