pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu <[email protected]>
---
drivers/char/hw_random/ks-sa-rng.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/hw_random/ks-sa-rng.c b/drivers/char/hw_random/ks-sa-rng.c
index e2330e757f1f..85c81da4a8af 100644
--- a/drivers/char/hw_random/ks-sa-rng.c
+++ b/drivers/char/hw_random/ks-sa-rng.c
@@ -244,6 +244,7 @@ static int ks_sa_rng_probe(struct platform_device *pdev)
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
dev_err(dev, "Failed to enable SA power-domain\n");
+ pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
return ret;
}
--
2.17.1
Hello Dinghao,
On Wed, 2020-05-20 at 21:29 +0800, Dinghao Liu wrote:
> pm_runtime_get_sync() increments the runtime PM usage counter even
> the call returns an error code. Thus a pairing decrement is needed
> on the error handling path to keep the counter balanced.
I believe, this is the wrong place for such kind of fix.
pm_runtime_get_sync() has obviously a broken semantics with regards to
your observation but no other driver does what you propose.
I think the proper fix belong into PM subsystem, please take a look
onto commit 15bcb91d7e60 "PM / Runtime: Implement autosuspend support".
> Signed-off-by: Dinghao Liu <[email protected]>
> ---
> drivers/char/hw_random/ks-sa-rng.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/hw_random/ks-sa-rng.c b/drivers/char/hw_random/ks-sa-rng.c
> index e2330e757f1f..85c81da4a8af 100644
> --- a/drivers/char/hw_random/ks-sa-rng.c
> +++ b/drivers/char/hw_random/ks-sa-rng.c
> @@ -244,6 +244,7 @@ static int ks_sa_rng_probe(struct platform_device *pdev)
> ret = pm_runtime_get_sync(dev);
> if (ret < 0) {
> dev_err(dev, "Failed to enable SA power-domain\n");
> + pm_runtime_put_sync(dev);
> pm_runtime_disable(dev);
> return ret;
> }
--
Alexander Sverdlin.
On Wed, May 20, 2020 at 03:42:17PM +0000, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hello Dinghao,
>
> On Wed, 2020-05-20 at 21:29 +0800, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
>
> I believe, this is the wrong place for such kind of fix.
> pm_runtime_get_sync() has obviously a broken semantics with regards to
> your observation but no other driver does what you propose.
Look again. For example, see what usb_autoresume_device() in
drivers/usb/core/driver.c does.
You really shouldn't make generalizations such as "no other driver does
..." unless you have read the code for every driver in the kernel.
Alan Stern
Hi Alexander,
There are large amounts of cases that assume pm_runtime_get_sync()
will modify runtime PM usage counter on error. Fixing this in PM
subsystem will influence all callers of pm_runtime_get_sync() and
introduce new bugs. Therefore I think the better solution is to fix
misused cases individually.
Dinghao
> Hello Dinghao,
>
> On Wed, 2020-05-20 at 21:29 +0800, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
>
> I believe, this is the wrong place for such kind of fix.
> pm_runtime_get_sync() has obviously a broken semantics with regards to
> your observation but no other driver does what you propose.
> I think the proper fix belong into PM subsystem, please take a look
> onto commit 15bcb91d7e60 "PM / Runtime: Implement autosuspend support".
>
> > Signed-off-by: Dinghao Liu <[email protected]>
> > ---
> > drivers/char/hw_random/ks-sa-rng.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/char/hw_random/ks-sa-rng.c b/drivers/char/hw_random/ks-sa-rng.c
> > index e2330e757f1f..85c81da4a8af 100644
> > --- a/drivers/char/hw_random/ks-sa-rng.c
> > +++ b/drivers/char/hw_random/ks-sa-rng.c
> > @@ -244,6 +244,7 @@ static int ks_sa_rng_probe(struct platform_device *pdev)
> > ret = pm_runtime_get_sync(dev);
> > if (ret < 0) {
> > dev_err(dev, "Failed to enable SA power-domain\n");
> > + pm_runtime_put_sync(dev);
> > pm_runtime_disable(dev);
> > return ret;
> > }
> --
> Alexander Sverdlin.
>
On Wed, May 20, 2020 at 12:45:56PM -0400, [email protected] wrote:
> On Wed, May 20, 2020 at 03:42:17PM +0000, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> > Hello Dinghao,
> >
> > On Wed, 2020-05-20 at 21:29 +0800, Dinghao Liu wrote:
> > > pm_runtime_get_sync() increments the runtime PM usage counter even
> > > the call returns an error code. Thus a pairing decrement is needed
> > > on the error handling path to keep the counter balanced.
> >
> > I believe, this is the wrong place for such kind of fix.
> > pm_runtime_get_sync() has obviously a broken semantics with regards to
> > your observation but no other driver does what you propose.
>
> Look again. For example, see what usb_autoresume_device() in
> drivers/usb/core/driver.c does.
However, there seems to be some disagreement as to what to do
when pm_runtime_get_sync fails. Your driver chooses to call
put_sync while others prefer pm_runtime_put_noidle (e.g., see
drivers/base/power/runtime.c).
This API does seem to be in a bit of a mess.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> On Wed, May 20, 2020 at 12:45:56PM -0400, [email protected] wrote:
> > On Wed, May 20, 2020 at 03:42:17PM +0000, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> > > Hello Dinghao,
> > >
> > > On Wed, 2020-05-20 at 21:29 +0800, Dinghao Liu wrote:
> > > > pm_runtime_get_sync() increments the runtime PM usage counter even
> > > > the call returns an error code. Thus a pairing decrement is needed
> > > > on the error handling path to keep the counter balanced.
> > >
> > > I believe, this is the wrong place for such kind of fix.
> > > pm_runtime_get_sync() has obviously a broken semantics with regards to
> > > your observation but no other driver does what you propose.
> >
> > Look again. For example, see what usb_autoresume_device() in
> > drivers/usb/core/driver.c does.
>
> However, there seems to be some disagreement as to what to do
> when pm_runtime_get_sync fails. Your driver chooses to call
> put_sync while others prefer pm_runtime_put_noidle (e.g., see
> drivers/base/power/runtime.c).
>
> This API does seem to be in a bit of a mess.
>
Here I think _put_noidle() is better. It's enough for this bug
and has no side effect (e.g., _put_sync may suspend the driver).
I will send a new patch for this bug.
Regards,
Dinghao