Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
@ 2020-05-20 13:29 Dinghao Liu
  2020-05-20 15:42 ` Sverdlin, Alexander (Nokia - DE/Ulm)
  0 siblings, 1 reply; 7+ messages in thread
From: Dinghao Liu @ 2020-05-20 13:29 UTC (permalink / raw)
  To: dinghao.liu, kjlu
  Cc: Matt Mackall, Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
	Ben Dooks, Richard Fontana, Alexander Sverdlin, Allison Randal,
	YueHaibing, Thomas Gleixner, linux-crypto, linux-kernel

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 <dinghao.liu@zju.edu.cn>
---
 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-20 13:29 [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error Dinghao Liu
@ 2020-05-20 15:42 ` Sverdlin, Alexander (Nokia - DE/Ulm)
  2020-05-20 16:45   ` stern
  2020-05-21  6:21   ` dinghao.liu
  0 siblings, 2 replies; 7+ messages in thread
From: Sverdlin, Alexander (Nokia - DE/Ulm) @ 2020-05-20 15:42 UTC (permalink / raw)
  To: stern, dinghao.liu, kjlu
  Cc: mpm, gregkh, herbert, ben.dooks, linux-kernel, arnd, allison,
	yuehaibing, rfontana, linux-crypto, tglx

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 <dinghao.liu@zju.edu.cn>
> ---
>  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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-20 15:42 ` Sverdlin, Alexander (Nokia - DE/Ulm)
@ 2020-05-20 16:45   ` stern
  2020-05-28  6:55     ` Herbert Xu
  2020-05-21  6:21   ` dinghao.liu
  1 sibling, 1 reply; 7+ messages in thread
From: stern @ 2020-05-20 16:45 UTC (permalink / raw)
  To: Sverdlin, Alexander (Nokia - DE/Ulm)
  Cc: dinghao.liu, kjlu, mpm, gregkh, herbert, ben.dooks, linux-kernel,
	arnd, allison, yuehaibing, rfontana, linux-crypto, tglx

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-20 15:42 ` Sverdlin, Alexander (Nokia - DE/Ulm)
  2020-05-20 16:45   ` stern
@ 2020-05-21  6:21   ` dinghao.liu
  1 sibling, 0 replies; 7+ messages in thread
From: dinghao.liu @ 2020-05-21  6:21 UTC (permalink / raw)
  To: Sverdlin, Alexander (Nokia - DE/Ulm)
  Cc: stern, kjlu, mpm, gregkh, herbert, ben.dooks, linux-kernel, arnd,
	allison, yuehaibing, rfontana, linux-crypto, tglx

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 <dinghao.liu@zju.edu.cn>
> > ---
> >  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.
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-20 16:45   ` stern
@ 2020-05-28  6:55     ` Herbert Xu
  2020-05-28  7:13       ` dinghao.liu
  2020-05-28 15:39       ` stern
  0 siblings, 2 replies; 7+ messages in thread
From: Herbert Xu @ 2020-05-28  6:55 UTC (permalink / raw)
  To: stern
  Cc: Sverdlin, Alexander (Nokia - DE/Ulm),
	dinghao.liu, kjlu, mpm, gregkh, ben.dooks, linux-kernel, arnd,
	allison, yuehaibing, rfontana, linux-crypto, tglx,
	Rafael J. Wysocki

On Wed, May 20, 2020 at 12:45:56PM -0400, stern@rowland.harvard.edu 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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-28  6:55     ` Herbert Xu
@ 2020-05-28  7:13       ` dinghao.liu
  2020-05-28 15:39       ` stern
  1 sibling, 0 replies; 7+ messages in thread
From: dinghao.liu @ 2020-05-28  7:13 UTC (permalink / raw)
  To: Herbert Xu
  Cc: stern, Sverdlin, Alexander (Nokia - DE/Ulm),
	kjlu, mpm, gregkh, ben.dooks, linux-kernel, arnd, allison,
	yuehaibing, rfontana, linux-crypto, tglx, Rafael J. Wysocki

> On Wed, May 20, 2020 at 12:45:56PM -0400, stern@rowland.harvard.edu 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error
  2020-05-28  6:55     ` Herbert Xu
  2020-05-28  7:13       ` dinghao.liu
@ 2020-05-28 15:39       ` stern
  1 sibling, 0 replies; 7+ messages in thread
From: stern @ 2020-05-28 15:39 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Sverdlin, Alexander (Nokia - DE/Ulm),
	dinghao.liu, kjlu, mpm, gregkh, ben.dooks, linux-kernel, arnd,
	allison, yuehaibing, rfontana, linux-crypto, tglx,
	Rafael J. Wysocki

On Thu, May 28, 2020 at 04:55:19PM +1000, Herbert Xu wrote:
> On Wed, May 20, 2020 at 12:45:56PM -0400, stern@rowland.harvard.edu 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).

It doesn't matter which function gets called; in these circumstances 
(device still suspended or in an error state because a resume attempt 
failed) they will do the same thing.

> This API does seem to be in a bit of a mess.

Suggestions and patches are welcome.

Alan Stern

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 13:29 [PATCH] hwrng: ks-sa - fix runtime pm imbalance on error Dinghao Liu
2020-05-20 15:42 ` Sverdlin, Alexander (Nokia - DE/Ulm)
2020-05-20 16:45   ` stern
2020-05-28  6:55     ` Herbert Xu
2020-05-28  7:13       ` dinghao.liu
2020-05-28 15:39       ` stern
2020-05-21  6:21   ` dinghao.liu

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git