| Message ID | 20210706100519.13851-1-peng.fan@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM: domains: not update genpd status when power on fail | expand |
On Tue, 6 Jul 2021 at 11:31, <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > When _genpd_power_on fail, the generic power domain status > should not be changed to GENPD_STATE_ON. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/base/power/domain.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index ab0b740cc0f1..754a5d384479 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1101,6 +1101,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, > unsigned int depth) > { > struct gpd_link *link; > + int ret; > > if (genpd_status_on(genpd)) > return; > @@ -1117,8 +1118,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, > genpd_unlock(link->parent); > } > > - _genpd_power_on(genpd, false); > - genpd->status = GENPD_STATE_ON; > + ret = _genpd_power_on(genpd, false); > + if (!ret) To support this error path, a lot more code needs to be added. Just have a look at the callers of genpd_sync_power_on() and compare it with genpd_power_on(). In my opinion, supporting this error path isn't really worth it, at least until someone comes with a valid use case. Do you have one - or did you send the $subject patch based solely on code inspection? > + genpd->status = GENPD_STATE_ON; > } > > /** > -- > 2.30.0 > Kind regards Uffe
Hi Ulf, > Subject: Re: [PATCH] PM: domains: not update genpd status when power on > fail > > On Tue, 6 Jul 2021 at 11:31, <peng.fan@oss.nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > When _genpd_power_on fail, the generic power domain status should not > > be changed to GENPD_STATE_ON. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/base/power/domain.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index ab0b740cc0f1..754a5d384479 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -1101,6 +1101,7 @@ static void genpd_sync_power_on(struct > generic_pm_domain *genpd, bool use_lock, > > unsigned int depth) { > > struct gpd_link *link; > > + int ret; > > > > if (genpd_status_on(genpd)) > > return; > > @@ -1117,8 +1118,9 @@ static void genpd_sync_power_on(struct > generic_pm_domain *genpd, bool use_lock, > > genpd_unlock(link->parent); > > } > > > > - _genpd_power_on(genpd, false); > > - genpd->status = GENPD_STATE_ON; > > + ret = _genpd_power_on(genpd, false); > > + if (!ret) > > To support this error path, a lot more code needs to be added. Just > have a look at the callers of genpd_sync_power_on() and compare it > with genpd_power_on(). > > In my opinion, supporting this error path isn't really worth it, at > least until someone comes with a valid use case. Do you have one - or > did you send the $subject patch based solely on code inspection? This is two patchset: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210604111005.6804-1-peng.fan@oss.nxp.com/ https://patchwork.kernel.org/project/linux-arm-kernel/cover/20210629072941.7980-1-peng.fan@oss.nxp.com/ I collected in github with a few fixes: https://github.com/MrVan/linux/tree/linux-next-master-628-blk-ctl-test In my test, when suspend/resume, I see pgc_vpu_h1/g1/g2 has status on, but actually they are not on in hardware. Because vpumix_blk_ctl bus domain is not on, pgc_vpu_h1/g1/g2 runtime power off will fail. Thanks, Peng. > > > + genpd->status = GENPD_STATE_ON; > > } > > > > /** > > -- > > 2.30.0 > > > > Kind regards > Uffe
On Tue, 6 Jul 2021 at 11:52, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > Hi Ulf, > > > Subject: Re: [PATCH] PM: domains: not update genpd status when power on > > fail > > > > On Tue, 6 Jul 2021 at 11:31, <peng.fan@oss.nxp.com> wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > When _genpd_power_on fail, the generic power domain status should not > > > be changed to GENPD_STATE_ON. > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > drivers/base/power/domain.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > > index ab0b740cc0f1..754a5d384479 100644 > > > --- a/drivers/base/power/domain.c > > > +++ b/drivers/base/power/domain.c > > > @@ -1101,6 +1101,7 @@ static void genpd_sync_power_on(struct > > generic_pm_domain *genpd, bool use_lock, > > > unsigned int depth) { > > > struct gpd_link *link; > > > + int ret; > > > > > > if (genpd_status_on(genpd)) > > > return; > > > @@ -1117,8 +1118,9 @@ static void genpd_sync_power_on(struct > > generic_pm_domain *genpd, bool use_lock, > > > genpd_unlock(link->parent); > > > } > > > > > > - _genpd_power_on(genpd, false); > > > - genpd->status = GENPD_STATE_ON; > > > + ret = _genpd_power_on(genpd, false); > > > + if (!ret) > > > > To support this error path, a lot more code needs to be added. Just > > have a look at the callers of genpd_sync_power_on() and compare it > > with genpd_power_on(). > > > > In my opinion, supporting this error path isn't really worth it, at > > least until someone comes with a valid use case. Do you have one - or > > did you send the $subject patch based solely on code inspection? > > This is two patchset: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210604111005.6804-1-peng.fan@oss.nxp.com/ > https://patchwork.kernel.org/project/linux-arm-kernel/cover/20210629072941.7980-1-peng.fan@oss.nxp.com/ > > I collected in github with a few fixes: > https://github.com/MrVan/linux/tree/linux-next-master-628-blk-ctl-test I looked at some of the changes there - and I especially don't quite understand why you enable runtime PM for the genpd->dev. That looks wrong to me. I am referring to: "soc: imx: blk-ctl: fix to v8 patchset". > > > In my test, when suspend/resume, I see > pgc_vpu_h1/g1/g2 has status on, but actually they are not on in hardware. That sounds like a bug somewhere. Did genpd fail to power off the PM domain or what happened, more exactly? > Because vpumix_blk_ctl bus domain is not on, pgc_vpu_h1/g1/g2 runtime > power off will fail. What is pgc_vpu_h1/g1/g2? I am happy to help/guide you forward, but it sounds a bit that more in-depth debugging is needed at your side. At least I need some stack trace or kernel logs with debug messages enabled. [...] Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index ab0b740cc0f1..754a5d384479 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1101,6 +1101,7 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, unsigned int depth) { struct gpd_link *link; + int ret; if (genpd_status_on(genpd)) return; @@ -1117,8 +1118,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, genpd_unlock(link->parent); } - _genpd_power_on(genpd, false); - genpd->status = GENPD_STATE_ON; + ret = _genpd_power_on(genpd, false); + if (!ret) + genpd->status = GENPD_STATE_ON; } /**