[38/78] media: i2c: mt9m001: use pm_runtime_resume_and_get()
diff mbox series

Message ID beddb7295807f43a190f2add6c1665b7475cb154.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/mt9m001.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi April 24, 2021, 8:24 a.m. UTC | #1
Hi Mauro,

On Sat, Apr 24, 2021 at 08:44:48AM +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>

Thanks
Acked-by: Jacopo Mondi <jacopo@jmondi.org>

I should re-work the error handling sequence there on top of this
patch as right now it's not the best, that 'done' label bothers me...
anyway, for later.

> ---
>  drivers/media/i2c/mt9m001.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index 3b0ba8ed5233..57e15a291ebd 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
>  		goto done;
>
>  	if (enable) {
> -		ret = pm_runtime_get_sync(&client->dev);
> +		ret = pm_runtime_resume_and_get(&client->dev);
>  		if (ret < 0)
> -			goto put_unlock;
> +			goto unlock;
>
>  		ret = mt9m001_apply_selection(sd);
>  		if (ret)
> @@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
>
>  put_unlock:
>  	pm_runtime_put(&client->dev);
> +unlock:
>  	mutex_unlock(&mt9m001->mutex);
>
>  	return ret;
> @@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
>  {
>  	struct mt9m001 *mt9m001 = to_mt9m001(client);
>
> -	pm_runtime_get_sync(&client->dev);
> +	pm_runtime_resume_and_get(&client->dev);
>
>  	v4l2_async_unregister_subdev(&mt9m001->subdev);
>  	media_entity_cleanup(&mt9m001->subdev.entity);
> --
> 2.30.2
>
Johan Hovold April 24, 2021, 10 a.m. UTC | #2
On Sat, Apr 24, 2021 at 10:24:54AM +0200, Jacopo Mondi wrote:
> Hi Mauro,
> 
> On Sat, Apr 24, 2021 at 08:44:48AM +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>
> 
> Thanks
> Acked-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> I should re-work the error handling sequence there on top of this
> patch as right now it's not the best, that 'done' label bothers me...
> anyway, for later.
> 
> > ---
> >  drivers/media/i2c/mt9m001.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index 3b0ba8ed5233..57e15a291ebd 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
> >  		goto done;
> >
> >  	if (enable) {
> > -		ret = pm_runtime_get_sync(&client->dev);
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> >  		if (ret < 0)
> > -			goto put_unlock;
> > +			goto unlock;
> >
> >  		ret = mt9m001_apply_selection(sd);
> >  		if (ret)
> > @@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
> >
> >  put_unlock:
> >  	pm_runtime_put(&client->dev);
> > +unlock:
> >  	mutex_unlock(&mt9m001->mutex);
> >
> >  	return ret;
> > @@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
> >  {
> >  	struct mt9m001 *mt9m001 = to_mt9m001(client);
> >
> > -	pm_runtime_get_sync(&client->dev);
> > +	pm_runtime_resume_and_get(&client->dev);
> >
> >  	v4l2_async_unregister_subdev(&mt9m001->subdev);
> >  	media_entity_cleanup(&mt9m001->subdev.entity);

I couldn't help looking at one more now that you got feedback on this
one.

Here you have the same problem as the one I reported earlier, in that
the usage count could end up negative on resume failure due to the later
put_noidle() call in remove().

Also note that you're adding more lines than you're removing.

I'd say this kind of mass-conversion is of questionable worth as
pm_runtime_resume_and_get() isn't necessarily an improvement (even if it
may have its use in some places).

Johan
Mauro Carvalho Chehab April 26, 2021, 2:38 p.m. UTC | #3
Em Sat, 24 Apr 2021 12:00:46 +0200
Johan Hovold <johan@kernel.org> escreveu:

> On Sat, Apr 24, 2021 at 10:24:54AM +0200, Jacopo Mondi wrote:
> > Hi Mauro,
> > 
> > On Sat, Apr 24, 2021 at 08:44:48AM +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>  
> > 
> > Thanks
> > Acked-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > I should re-work the error handling sequence there on top of this
> > patch as right now it's not the best, that 'done' label bothers me...
> > anyway, for later.
> >   
> > > ---
> > >  drivers/media/i2c/mt9m001.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > > index 3b0ba8ed5233..57e15a291ebd 100644
> > > --- a/drivers/media/i2c/mt9m001.c
> > > +++ b/drivers/media/i2c/mt9m001.c
> > > @@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
> > >  		goto done;
> > >
> > >  	if (enable) {
> > > -		ret = pm_runtime_get_sync(&client->dev);
> > > +		ret = pm_runtime_resume_and_get(&client->dev);
> > >  		if (ret < 0)
> > > -			goto put_unlock;
> > > +			goto unlock;
> > >
> > >  		ret = mt9m001_apply_selection(sd);
> > >  		if (ret)
> > > @@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
> > >
> > >  put_unlock:
> > >  	pm_runtime_put(&client->dev);
> > > +unlock:
> > >  	mutex_unlock(&mt9m001->mutex);
> > >
> > >  	return ret;
> > > @@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
> > >  {
> > >  	struct mt9m001 *mt9m001 = to_mt9m001(client);
> > >
> > > -	pm_runtime_get_sync(&client->dev);
> > > +	pm_runtime_resume_and_get(&client->dev);
> > >
> > >  	v4l2_async_unregister_subdev(&mt9m001->subdev);
> > >  	media_entity_cleanup(&mt9m001->subdev.entity);  
> 
> I couldn't help looking at one more now that you got feedback on this
> one.
> 
> Here you have the same problem as the one I reported earlier, in that
> the usage count could end up negative on resume failure due to the later
> put_noidle() call in remove().

I'll double-check this at the entire series. Different sensor
drivers are handling this on different ways, which sounds
bad, as they are meant to be independent on the media bridge
driver.

> Also note that you're adding more lines than you're removing.

Ok, but the end goal is not really reducing the number of lines,
but to have the code following the same pattern, and to avoid
cut-and-paste errors when new drivers are written.

The mt9m001 is one of the oldest sensor drivers, written a long time
before the PM runtime core, written for the soc_camera driver, back
in 2008. The port to use PM runtime isn't old:

  commit 8fcfc491c6ca5887bb341b3a622cca3ed8e3c9f0
  Author: Akinobu Mita <akinobu.mita@gmail.com>
  Date:   Tue Jan 8 12:51:44 2019 -0200

    media: mt9m001: switch s_power callback to runtime PM

It was part of an attempt to recover the soc_camera sensor drives
from staging. 

Yet, the logic on this driver seems to be different than the
one used on more modern I2C sensors. So, better to re-check
everything.

> I'd say this kind of mass-conversion is of questionable worth as
> pm_runtime_resume_and_get() isn't necessarily an improvement (even if it
> may have its use in some places).

The main problem is that other parts of the driver's core APIs
assume that get object methods will only increment the usage
counter if no errors. The pm_runtime_get_sync() is an exception.

Its name doesn't help at all: A function like that should, IMHO,
be called, instead:

	pm_runtime_inc_usage_count_and_try_to_resume().

Or something similar, in order to make clearer that it always
increment the usage count, no matter what. If possible, all drivers
should get rid of it too (or alternatively add comments warning
people that keeping the usage_count incremented is desired on the
very specific places where it is really needed), as it is risky
to use something that has a different usage_count increement behavior
than other more usual *_get() functions.

With regards to mass-fixing it, I've seen several patches seen
to media fixing bugs due to the bad usage_count decrement logic.
So, the best is to solve them all at once, and stop using
pm_runtime_get_sync() inside the subsystem.

Thanks,
Mauro
Johan Hovold April 27, 2021, 12:23 p.m. UTC | #4
On Mon, Apr 26, 2021 at 04:38:40PM +0200, Mauro Carvalho Chehab wrote:
> Em Sat, 24 Apr 2021 12:00:46 +0200
> Johan Hovold <johan@kernel.org> escreveu:
> 
> > On Sat, Apr 24, 2021 at 10:24:54AM +0200, Jacopo Mondi wrote:
> > > Hi Mauro,
> > > 
> > > On Sat, Apr 24, 2021 at 08:44:48AM +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>  

> > I'd say this kind of mass-conversion is of questionable worth as
> > pm_runtime_resume_and_get() isn't necessarily an improvement (even if it
> > may have its use in some places).
> 
> The main problem is that other parts of the driver's core APIs
> assume that get object methods will only increment the usage
> counter if no errors. The pm_runtime_get_sync() is an exception.
> 
> Its name doesn't help at all: A function like that should, IMHO,
> be called, instead:
> 
> 	pm_runtime_inc_usage_count_and_try_to_resume().
> 
> Or something similar, in order to make clearer that it always
> increment the usage count, no matter what. If possible, all drivers
> should get rid of it too (or alternatively add comments warning
> people that keeping the usage_count incremented is desired on the
> very specific places where it is really needed), as it is risky
> to use something that has a different usage_count increement behavior
> than other more usual *_get() functions.

pm_runtime_get_sync() has worked this way since it was merged 12 years
ago, and for someone who's used to this interface this is not such a big
deal as you seem to think. Sure, you need to remember to put the usage
counter on errors, but that's it (and the other side of that is that you
don't need to worry about error handling where it doesn't matter).

Also note all the pm_runtime_get functions *always* increment the usage
count even if an async resume may later fail so there is consistency
here.

And regarding naming, the new pm_resume_and_get() looks completely out
of place to me since it uses a different naming scheme than the other
helpers (including the ones that are used to balance the new call).

> With regards to mass-fixing it, I've seen several patches seen
> to media fixing bugs due to the bad usage_count decrement logic.
> So, the best is to solve them all at once, and stop using
> pm_runtime_get_sync() inside the subsystem.

Sure, having the script kiddies patch drivers without understanding what
they're are really doing is bound to introduce bugs unless it can be
caught in review.

You're call, but converting functioning drivers where the authors knew
what they were doing just because you're not used to the API and risk
introducing new bugs in the process isn't necessarily a good idea.
Especially since the pm_runtime_get_sync() will continue to be used
elsewhere, and possibly even in media in cases where you don't need to
check for errors (e.g. remove()).

Johan
Mauro Carvalho Chehab April 28, 2021, 8:31 a.m. UTC | #5
Em Tue, 27 Apr 2021 14:23:20 +0200
Johan Hovold <johan@kernel.org> escreveu:

> On Mon, Apr 26, 2021 at 04:38:40PM +0200, Mauro Carvalho Chehab wrote:
> > Em Sat, 24 Apr 2021 12:00:46 +0200
> > Johan Hovold <johan@kernel.org> escreveu:
> >   
> > > On Sat, Apr 24, 2021 at 10:24:54AM +0200, Jacopo Mondi wrote:  
> > > > Hi Mauro,
> > > > 
> > > > On Sat, Apr 24, 2021 at 08:44:48AM +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>    
> 
> > > I'd say this kind of mass-conversion is of questionable worth as
> > > pm_runtime_resume_and_get() isn't necessarily an improvement (even if it
> > > may have its use in some places).  
> > 
> > The main problem is that other parts of the driver's core APIs
> > assume that get object methods will only increment the usage
> > counter if no errors. The pm_runtime_get_sync() is an exception.
> > 
> > Its name doesn't help at all: A function like that should, IMHO,
> > be called, instead:
> > 
> > 	pm_runtime_inc_usage_count_and_try_to_resume().
> > 
> > Or something similar, in order to make clearer that it always
> > increment the usage count, no matter what. If possible, all drivers
> > should get rid of it too (or alternatively add comments warning
> > people that keeping the usage_count incremented is desired on the
> > very specific places where it is really needed), as it is risky
> > to use something that has a different usage_count increement behavior
> > than other more usual *_get() functions.  
> 
> pm_runtime_get_sync() has worked this way since it was merged 12 years
> ago, and for someone who's used to this interface this is not such a big
> deal as you seem to think. Sure, you need to remember to put the usage
> counter on errors, but that's it (and the other side of that is that you
> don't need to worry about error handling where it doesn't matter).

Before we have those at PM subsystem, the media had its own way to
set/disable power for their sub-devices. The PCI and USB drivers 
still use it, instead of pm_runtime, mostly due to historic reasons.

So, basically, its usage at the media subsystem is restricted to
drivers for embedded systems. The vast majority of drivers supporting
PM runtime are the I2C camera drivers. The camera drivers can be used 
interchangeable. So, in practice, the same bridge driver can work 
with a lot of different camera models, depending on the hardware
vendors' personal preferences and the desired max resolution.

So, in thesis, all such drivers should behave exactly the same 
with regards to PM.

However, on most existing drivers, the pm_runtime was added a
couple of years ago, and by people that are not too familiar
with the PM subsystem.

That probably explains why there were/are several places that 
do things like this[1]:

	ret = pm_runtime_get_sync(dev);
	if (ret < 0)
		return ret;

without taking care of calling a pm_runtime_put*() function.

[1] besides the 13 patches made by UCN addressing it on
    existing code, I discovered the same pattern on a 
    couple of other drivers with current upstream code.

That shows a pattern: several media developers are not familiar
with the usage_count behavior for pm_runtime_get functions.

So, doing this work is not only helping to make the PM support
more uniform, but it is also helping to solve existing issues.

> Also note all the pm_runtime_get functions *always* increment the usage
> count even if an async resume may later fail so there is consistency
> here.
> 
> And regarding naming, the new pm_resume_and_get() looks completely out
> of place to me since it uses a different naming scheme than the other
> helpers (including the ones that are used to balance the new call).
> 
> > With regards to mass-fixing it, I've seen several patches seen
> > to media fixing bugs due to the bad usage_count decrement logic.
> > So, the best is to solve them all at once, and stop using
> > pm_runtime_get_sync() inside the subsystem.  
> 
> Sure, having the script kiddies patch drivers without understanding what
> they're are really doing is bound to introduce bugs unless it can be
> caught in review.

Yes, but as I pointed, the current code has bugs already.

> You're call, but converting functioning drivers where the authors knew
> what they were doing just because you're not used to the API and risk
> introducing new bugs in the process isn't necessarily a good idea.

The problem is that the above assumption is not necessarily true:
based on the number of drivers that pm_runtime_get_sync() weren't
decrementing usage_count on errors, several driver authors were not 
familiar enough with the PM runtime behavior by the time the drivers
were written or converted to use the PM runtime, instead of the
media .s_power()/.s_stream() callbacks.

> Especially since the pm_runtime_get_sync() will continue to be used
> elsewhere, and possibly even in media in cases where you don't need to
> check for errors (e.g. remove()).

Talking about the remove(), I'm not sure if just ignoring errors
there would do the right thing. I mean, if pm_runtime_get_sync()
fails, probably any attempts to disable clocks and other things
that depend on PM runtime will also (silently) fail.

This may put the device on an unknown PM and keep clock lines enabled
after its removal.

Thanks,
Mauro
Johan Hovold April 28, 2021, 10:05 a.m. UTC | #6
On Wed, Apr 28, 2021 at 10:31:48AM +0200, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Apr 2021 14:23:20 +0200
> Johan Hovold <johan@kernel.org> escreveu:

> > pm_runtime_get_sync() has worked this way since it was merged 12 years
> > ago, and for someone who's used to this interface this is not such a big
> > deal as you seem to think. Sure, you need to remember to put the usage
> > counter on errors, but that's it (and the other side of that is that you
> > don't need to worry about error handling where it doesn't matter).
> 
> Before we have those at PM subsystem, the media had its own way to
> set/disable power for their sub-devices. The PCI and USB drivers 
> still use it, instead of pm_runtime, mostly due to historic reasons.
> 
> So, basically, its usage at the media subsystem is restricted to
> drivers for embedded systems. The vast majority of drivers supporting
> PM runtime are the I2C camera drivers. The camera drivers can be used 
> interchangeable. So, in practice, the same bridge driver can work 
> with a lot of different camera models, depending on the hardware
> vendors' personal preferences and the desired max resolution.
> 
> So, in thesis, all such drivers should behave exactly the same 
> with regards to PM.
> 
> However, on most existing drivers, the pm_runtime was added a
> couple of years ago, and by people that are not too familiar
> with the PM subsystem.
> 
> That probably explains why there were/are several places that 
> do things like this[1]:
> 
> 	ret = pm_runtime_get_sync(dev);
> 	if (ret < 0)
> 		return ret;
> 
> without taking care of calling a pm_runtime_put*() function.
> 
> [1] besides the 13 patches made by UCN addressing it on
>     existing code, I discovered the same pattern on a 
>     couple of other drivers with current upstream code.
> 
> That shows a pattern: several media developers are not familiar
> with the usage_count behavior for pm_runtime_get functions.
> 
> So, doing this work is not only helping to make the PM support
> more uniform, but it is also helping to solve existing issues.

Sure, I don't doubt that there are issues with the current code too.

> > You're call, but converting functioning drivers where the authors knew
> > what they were doing just because you're not used to the API and risk
> > introducing new bugs in the process isn't necessarily a good idea.
> 
> The problem is that the above assumption is not necessarily true:
> based on the number of drivers that pm_runtime_get_sync() weren't
> decrementing usage_count on errors, several driver authors were not 
> familiar enough with the PM runtime behavior by the time the drivers
> were written or converted to use the PM runtime, instead of the
> media .s_power()/.s_stream() callbacks.

That may very well be the case. My point is just that this work needs to
be done carefully and by people familiar with the code (and runtime pm)
or you risk introducing new issues.

I really don't want the bot-warning-suppression crew to start with this
for example.

> > Especially since the pm_runtime_get_sync() will continue to be used
> > elsewhere, and possibly even in media in cases where you don't need to
> > check for errors (e.g. remove()).
> 
> Talking about the remove(), I'm not sure if just ignoring errors
> there would do the right thing. I mean, if pm_runtime_get_sync()
> fails, probably any attempts to disable clocks and other things
> that depend on PM runtime will also (silently) fail.
> 
> This may put the device on an unknown PM and keep clock lines enabled
> after its removal.

Right, a resume failure is a pretty big issue and it's not really clear
how to to even handle that generally. But at remove() time you don't
have much choice but to go on and release resource anyway. 

So unless actually implementing some error handling too, using
pm_runtime_sync_get() without checking for errors is still preferred
over pm_runtime_resume_and_get(). That is 

	pm_runtime_get_sync();
	/* cleanup */
	pm_runtime_disable()
	pm_runtime_put_noidle();

is better than:

	ret = pm_runtime_resume_and_get();
	/* cleanup */
	pm_runtime_disable();
	if (ret == 0)
		pm_runtime_put_noidle();

unless you also start doing something ret.

Johan
Mauro Carvalho Chehab April 28, 2021, 11:28 a.m. UTC | #7
Em Wed, 28 Apr 2021 12:05:26 +0200
Johan Hovold <johan@kernel.org> escreveu:

> On Wed, Apr 28, 2021 at 10:31:48AM +0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 27 Apr 2021 14:23:20 +0200
> > Johan Hovold <johan@kernel.org> escreveu:  
> 

> > > You're call, but converting functioning drivers where the authors knew
> > > what they were doing just because you're not used to the API and risk
> > > introducing new bugs in the process isn't necessarily a good idea.  
> > 
> > The problem is that the above assumption is not necessarily true:
> > based on the number of drivers that pm_runtime_get_sync() weren't
> > decrementing usage_count on errors, several driver authors were not 
> > familiar enough with the PM runtime behavior by the time the drivers
> > were written or converted to use the PM runtime, instead of the
> > media .s_power()/.s_stream() callbacks.  
> 
> That may very well be the case. My point is just that this work needs to
> be done carefully and by people familiar with the code (and runtime pm)
> or you risk introducing new issues.

Yeah, that's for sure.

> I really don't want the bot-warning-suppression crew to start with this
> for example.
> 
> > > Especially since the pm_runtime_get_sync() will continue to be used
> > > elsewhere, and possibly even in media in cases where you don't need to
> > > check for errors (e.g. remove()).  
> > 
> > Talking about the remove(), I'm not sure if just ignoring errors
> > there would do the right thing. I mean, if pm_runtime_get_sync()
> > fails, probably any attempts to disable clocks and other things
> > that depend on PM runtime will also (silently) fail.
> > 
> > This may put the device on an unknown PM and keep clock lines enabled
> > after its removal.  
> 
> Right, a resume failure is a pretty big issue and it's not really clear
> how to to even handle that generally. But at remove() time you don't
> have much choice but to go on and release resource anyway. 
> 
> So unless actually implementing some error handling too, using
> pm_runtime_sync_get() without checking for errors is still preferred
> over pm_runtime_resume_and_get(). That is 
> 
> 	pm_runtime_get_sync();
> 	/* cleanup */
> 	pm_runtime_disable()
> 	pm_runtime_put_noidle();
> 
> is better than:
> 
> 	ret = pm_runtime_resume_and_get();
> 	/* cleanup */
> 	pm_runtime_disable();
> 	if (ret == 0)
> 		pm_runtime_put_noidle();
> 
> unless you also start doing something ret.

Perhaps the best would be to use, instead:

	pm_runtime_get_noresume();
 	/* cleanup */
 	pm_runtime_disable()
 	pm_runtime_put_noidle();
	pm_runtime_set_suspended();

I mean, at least for my eyes, it doesn't make sense to do a PM
resume during driver's removal/unbind time.

Regards,
Mauro


> 
> Johan



Thanks,
Mauro
Johan Hovold April 28, 2021, 11:56 a.m. UTC | #8
On Wed, Apr 28, 2021 at 01:28:53PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 12:05:26 +0200
> Johan Hovold <johan@kernel.org> escreveu:

> > Right, a resume failure is a pretty big issue and it's not really clear
> > how to to even handle that generally. But at remove() time you don't
> > have much choice but to go on and release resource anyway. 
> > 
> > So unless actually implementing some error handling too, using
> > pm_runtime_sync_get() without checking for errors is still preferred
> > over pm_runtime_resume_and_get(). That is 
> > 
> > 	pm_runtime_get_sync();
> > 	/* cleanup */
> > 	pm_runtime_disable()
> > 	pm_runtime_put_noidle();
> > 
> > is better than:
> > 
> > 	ret = pm_runtime_resume_and_get();
> > 	/* cleanup */
> > 	pm_runtime_disable();
> > 	if (ret == 0)
> > 		pm_runtime_put_noidle();
> > 
> > unless you also start doing something ret.
> 
> Perhaps the best would be to use, instead:
> 
> 	pm_runtime_get_noresume();
>  	/* cleanup */
>  	pm_runtime_disable()
>  	pm_runtime_put_noidle();
> 	pm_runtime_set_suspended();
> 
> I mean, at least for my eyes, it doesn't make sense to do a PM
> resume during driver's removal/unbind time.

The cleanup bit above would depend on the device being resumed so using
pm_runtime_get_noresume() makes little sense.

Some drivers disable clocks etc explicitly at remove instead of relying
on pm runtime for that and then they'd use the above scheme (plus
explicit pm_runtime_set_suspended()).

Johan

Patch
diff mbox series

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index 3b0ba8ed5233..57e15a291ebd 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -217,9 +217,9 @@  static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 		goto done;
 
 	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 		if (ret < 0)
-			goto put_unlock;
+			goto unlock;
 
 		ret = mt9m001_apply_selection(sd);
 		if (ret)
@@ -247,6 +247,7 @@  static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 
 put_unlock:
 	pm_runtime_put(&client->dev);
+unlock:
 	mutex_unlock(&mt9m001->mutex);
 
 	return ret;
@@ -834,7 +835,7 @@  static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 
-	pm_runtime_get_sync(&client->dev);
+	pm_runtime_resume_and_get(&client->dev);
 
 	v4l2_async_unregister_subdev(&mt9m001->subdev);
 	media_entity_cleanup(&mt9m001->subdev.entity);