| Message ID | 20200520132957.18776-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | hwrng: ks-sa - fix runtime pm imbalance on error | expand |
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; > }
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 <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. >
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,
> 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
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
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; }
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(+)