Patchwork [-next] power/domain_governor: fix a compilation error

login
register
mail settings
Submitter Qian Cai
Date April 11, 2019, 12:47 p.m.
Message ID <20190411124739.5542-1-cai@lca.pw>
Download mbox | patch
Permalink /patch/770653/
State New
Headers show

Comments

Qian Cai - April 11, 2019, 12:47 p.m.
The commit 50899f7d5078 ("PM / Domains: Add genpd governor for CPUs")
introduced a compilation error on arm64 with CONFIG_CPU_IDLE=n because
cpuidle_devices is undefined there.

drivers/base/power/domain_governor.o: In function `cpu_power_down_ok':
drivers/base/power/domain_governor.c:263: undefined reference to
'cpuidle_devices' ld: drivers/base/power/domain_governor.o: relocation
R_AARCH64_ADR_PREL_PG_HI21 against symbol 'cpuidle_devices' which may
bind externally can not be used when making a shared object; recompile
with -fPIC
drivers/base/power/domain_governor.c:263:(.text+0x638): dangerous
relocation: unsupported relocation
drivers/base/power/domain_governor.c:263: undefined reference to
'cpuidle_devices'
make: *** [Makefile:1047: vmlinux] Error 1

Signed-off-by: Qian Cai <cai@lca.pw>
---
 drivers/base/power/domain_governor.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
Ulf Hansson - April 11, 2019, 1:22 p.m.
On Thu, 11 Apr 2019 at 14:48, Qian Cai <cai@lca.pw> wrote:
>
> The commit 50899f7d5078 ("PM / Domains: Add genpd governor for CPUs")
> introduced a compilation error on arm64 with CONFIG_CPU_IDLE=n because
> cpuidle_devices is undefined there.
>
> drivers/base/power/domain_governor.o: In function `cpu_power_down_ok':
> drivers/base/power/domain_governor.c:263: undefined reference to
> 'cpuidle_devices' ld: drivers/base/power/domain_governor.o: relocation
> R_AARCH64_ADR_PREL_PG_HI21 against symbol 'cpuidle_devices' which may
> bind externally can not be used when making a shared object; recompile
> with -fPIC
> drivers/base/power/domain_governor.c:263:(.text+0x638): dangerous
> relocation: unsupported relocation
> drivers/base/power/domain_governor.c:263: undefined reference to
> 'cpuidle_devices'
> make: *** [Makefile:1047: vmlinux] Error 1
>
> Signed-off-by: Qian Cai <cai@lca.pw>

Qian, thanks for you patch!

> ---
>  drivers/base/power/domain_governor.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index 39c8a699cb53..252d88fcf760 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -270,11 +270,13 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
>          */
>         domain_wakeup = ktime_set(KTIME_SEC_MAX, 0);
>         for_each_cpu_and(cpu, genpd->cpus, cpu_online_mask) {
> -               dev = per_cpu(cpuidle_devices, cpu);
> -               if (dev) {
> -                       next_hrtimer = READ_ONCE(dev->next_hrtimer);
> -                       if (ktime_before(next_hrtimer, domain_wakeup))
> -                               domain_wakeup = next_hrtimer;
> +               if (IS_ENABLED(CONFIG_CPU_IDLE)) {
> +                       dev = per_cpu(cpuidle_devices, cpu);
> +                       if (dev) {
> +                               next_hrtimer = READ_ONCE(dev->next_hrtimer);
> +                               if (ktime_before(next_hrtimer, domain_wakeup))
> +                                       domain_wakeup = next_hrtimer;
> +                       }

This works as quick fix, but I think there is two possible options
that is probably better in long run.

1) Extend the cpuidle interface with a new function:

struct cpuidle_device *cpuidle_get_cpu_device(unsigned int cpu)

Then when CONFIG_CPU_IDLE is unset, we can just have a stub function
returning NULL.

2) Have the struct dev_power_governor pm_domain_cpu_gov to be defined
(and the corresponding functionality), but only when CONFIG_CPU_IDLE
is set and when CONFIG_PM_GENERIC_DOMAINS is set.

Kind regards
Uffe
Rafael J. Wysocki - April 11, 2019, 2:36 p.m.
On Thu, Apr 11, 2019 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 11 Apr 2019 at 14:48, Qian Cai <cai@lca.pw> wrote:
> >
> > The commit 50899f7d5078 ("PM / Domains: Add genpd governor for CPUs")
> > introduced a compilation error on arm64 with CONFIG_CPU_IDLE=n because
> > cpuidle_devices is undefined there.
> >
> > drivers/base/power/domain_governor.o: In function `cpu_power_down_ok':
> > drivers/base/power/domain_governor.c:263: undefined reference to
> > 'cpuidle_devices' ld: drivers/base/power/domain_governor.o: relocation
> > R_AARCH64_ADR_PREL_PG_HI21 against symbol 'cpuidle_devices' which may
> > bind externally can not be used when making a shared object; recompile
> > with -fPIC
> > drivers/base/power/domain_governor.c:263:(.text+0x638): dangerous
> > relocation: unsupported relocation
> > drivers/base/power/domain_governor.c:263: undefined reference to
> > 'cpuidle_devices'
> > make: *** [Makefile:1047: vmlinux] Error 1
> >
> > Signed-off-by: Qian Cai <cai@lca.pw>
>
> Qian, thanks for you patch!
>
> > ---
> >  drivers/base/power/domain_governor.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> > index 39c8a699cb53..252d88fcf760 100644
> > --- a/drivers/base/power/domain_governor.c
> > +++ b/drivers/base/power/domain_governor.c
> > @@ -270,11 +270,13 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
> >          */
> >         domain_wakeup = ktime_set(KTIME_SEC_MAX, 0);
> >         for_each_cpu_and(cpu, genpd->cpus, cpu_online_mask) {
> > -               dev = per_cpu(cpuidle_devices, cpu);
> > -               if (dev) {
> > -                       next_hrtimer = READ_ONCE(dev->next_hrtimer);
> > -                       if (ktime_before(next_hrtimer, domain_wakeup))
> > -                               domain_wakeup = next_hrtimer;
> > +               if (IS_ENABLED(CONFIG_CPU_IDLE)) {
> > +                       dev = per_cpu(cpuidle_devices, cpu);
> > +                       if (dev) {
> > +                               next_hrtimer = READ_ONCE(dev->next_hrtimer);
> > +                               if (ktime_before(next_hrtimer, domain_wakeup))
> > +                                       domain_wakeup = next_hrtimer;
> > +                       }
>
> This works as quick fix, but I think there is two possible options
> that is probably better in long run.
>
> 1) Extend the cpuidle interface with a new function:
>
> struct cpuidle_device *cpuidle_get_cpu_device(unsigned int cpu)
>
> Then when CONFIG_CPU_IDLE is unset, we can just have a stub function
> returning NULL.

So why exactly cpu_power_down_ok() would be still needed with
CONFIG_CPU_IDLE unset?

> 2) Have the struct dev_power_governor pm_domain_cpu_gov to be defined
> (and the corresponding functionality), but only when CONFIG_CPU_IDLE
> is set and when CONFIG_PM_GENERIC_DOMAINS is set.

So please do that.

Can you please send a replacement patch for the [4/4] in the series
with this fixed?
Ulf Hansson - April 11, 2019, 6 p.m.
On Thu, 11 Apr 2019 at 16:37, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 11, 2019 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 11 Apr 2019 at 14:48, Qian Cai <cai@lca.pw> wrote:
> > >
> > > The commit 50899f7d5078 ("PM / Domains: Add genpd governor for CPUs")
> > > introduced a compilation error on arm64 with CONFIG_CPU_IDLE=n because
> > > cpuidle_devices is undefined there.
> > >
> > > drivers/base/power/domain_governor.o: In function `cpu_power_down_ok':
> > > drivers/base/power/domain_governor.c:263: undefined reference to
> > > 'cpuidle_devices' ld: drivers/base/power/domain_governor.o: relocation
> > > R_AARCH64_ADR_PREL_PG_HI21 against symbol 'cpuidle_devices' which may
> > > bind externally can not be used when making a shared object; recompile
> > > with -fPIC
> > > drivers/base/power/domain_governor.c:263:(.text+0x638): dangerous
> > > relocation: unsupported relocation
> > > drivers/base/power/domain_governor.c:263: undefined reference to
> > > 'cpuidle_devices'
> > > make: *** [Makefile:1047: vmlinux] Error 1
> > >
> > > Signed-off-by: Qian Cai <cai@lca.pw>
> >
> > Qian, thanks for you patch!
> >
> > > ---
> > >  drivers/base/power/domain_governor.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> > > index 39c8a699cb53..252d88fcf760 100644
> > > --- a/drivers/base/power/domain_governor.c
> > > +++ b/drivers/base/power/domain_governor.c
> > > @@ -270,11 +270,13 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
> > >          */
> > >         domain_wakeup = ktime_set(KTIME_SEC_MAX, 0);
> > >         for_each_cpu_and(cpu, genpd->cpus, cpu_online_mask) {
> > > -               dev = per_cpu(cpuidle_devices, cpu);
> > > -               if (dev) {
> > > -                       next_hrtimer = READ_ONCE(dev->next_hrtimer);
> > > -                       if (ktime_before(next_hrtimer, domain_wakeup))
> > > -                               domain_wakeup = next_hrtimer;
> > > +               if (IS_ENABLED(CONFIG_CPU_IDLE)) {
> > > +                       dev = per_cpu(cpuidle_devices, cpu);
> > > +                       if (dev) {
> > > +                               next_hrtimer = READ_ONCE(dev->next_hrtimer);
> > > +                               if (ktime_before(next_hrtimer, domain_wakeup))
> > > +                                       domain_wakeup = next_hrtimer;
> > > +                       }
> >
> > This works as quick fix, but I think there is two possible options
> > that is probably better in long run.
> >
> > 1) Extend the cpuidle interface with a new function:
> >
> > struct cpuidle_device *cpuidle_get_cpu_device(unsigned int cpu)
> >
> > Then when CONFIG_CPU_IDLE is unset, we can just have a stub function
> > returning NULL.
>
> So why exactly cpu_power_down_ok() would be still needed with
> CONFIG_CPU_IDLE unset?
>
> > 2) Have the struct dev_power_governor pm_domain_cpu_gov to be defined
> > (and the corresponding functionality), but only when CONFIG_CPU_IDLE
> > is set and when CONFIG_PM_GENERIC_DOMAINS is set.
>
> So please do that.
>
> Can you please send a replacement patch for the [4/4] in the series
> with this fixed?

Yes, I do that!

Kind regards
Uffe

Patch

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 39c8a699cb53..252d88fcf760 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -270,11 +270,13 @@  static bool cpu_power_down_ok(struct dev_pm_domain *pd)
 	 */
 	domain_wakeup = ktime_set(KTIME_SEC_MAX, 0);
 	for_each_cpu_and(cpu, genpd->cpus, cpu_online_mask) {
-		dev = per_cpu(cpuidle_devices, cpu);
-		if (dev) {
-			next_hrtimer = READ_ONCE(dev->next_hrtimer);
-			if (ktime_before(next_hrtimer, domain_wakeup))
-				domain_wakeup = next_hrtimer;
+		if (IS_ENABLED(CONFIG_CPU_IDLE)) {
+			dev = per_cpu(cpuidle_devices, cpu);
+			if (dev) {
+				next_hrtimer = READ_ONCE(dev->next_hrtimer);
+				if (ktime_before(next_hrtimer, domain_wakeup))
+					domain_wakeup = next_hrtimer;
+			}
 		}
 	}