[PATCH] psci: add CPU_IDLE dependency

[PATCH] psci: add CPU_IDLE dependency

[Arnd Bergmann]

I ran into a build error for the psci_checker:

drivers/firmware/psci_checker.o: In function `psci_checker':
psci_checker.c:(.init.text+0x528): undefined reference to `cpuidle_devices'

As far as I can tell, this is simply a very rare combination of options,
but the problem has existed since the code was initially added.
Adding a Kconfig dependency makes it build properly.

Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/firmware/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e4ed5a9c6fd..1983e6e0106f 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -10,7 +10,7 @@ config ARM_PSCI_FW
 
 config ARM_PSCI_CHECKER
 	bool "ARM PSCI checker"
-	depends on ARM_PSCI_FW && HOTPLUG_CPU && !TORTURE_TEST
+	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
 	help
 	  Run the PSCI checker during startup. This checks that hotplug and
 	  suspend operations work correctly when using PSCI.
-- 
2.9.0

Re: [PATCH] psci: add CPU_IDLE dependency

[Kevin Brodsky]

[Apologies for the resending, damn Thunderbird sending HTML...]

On 31/07/17 09:55, Arnd Bergmann wrote:
> I ran into a build error for the psci_checker:
>
> drivers/firmware/psci_checker.o: In function `psci_checker':
> psci_checker.c:(.init.text+0x528): undefined reference to `cpuidle_devices'
>
> As far as I can tell, this is simply a very rare combination of options,
> but the problem has existed since the code was initially added.
> Adding a Kconfig dependency makes it build properly.

Good catch! For some reason I missed this config option when figuring out the 
dependencies... I wonder though, shouldn't cpuidle.h declare cpuidle_devices 
conditionally on CONFIG_CPU_IDLE?

> Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Kevin Brodsky <kevin.brodsky@arm.com>

Cheers,
Kevin

> ---
>   drivers/firmware/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 6e4ed5a9c6fd..1983e6e0106f 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -10,7 +10,7 @@ config ARM_PSCI_FW
>   
>   config ARM_PSCI_CHECKER
>   	bool "ARM PSCI checker"
> -	depends on ARM_PSCI_FW && HOTPLUG_CPU && !TORTURE_TEST
> +	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
>   	help
>   	  Run the PSCI checker during startup. This checks that hotplug and
>   	  suspend operations work correctly when using PSCI.

Re: [PATCH] psci: add CPU_IDLE dependency

[Jean Delvare]

Hi Kevin,

On Mon, 31 Jul 2017 11:19:39 +0100, Kevin Brodsky wrote:
> On 31/07/17 09:55, Arnd Bergmann wrote:
> > I ran into a build error for the psci_checker:
> >
> > drivers/firmware/psci_checker.o: In function `psci_checker':
> > psci_checker.c:(.init.text+0x528): undefined reference to `cpuidle_devices'
> >
> > As far as I can tell, this is simply a very rare combination of options,
> > but the problem has existed since the code was initially added.
> > Adding a Kconfig dependency makes it build properly.  
> 
> Good catch! For some reason I missed this config option when figuring out the 
> dependencies... I wonder though, shouldn't cpuidle.h declare cpuidle_devices 
> conditionally on CONFIG_CPU_IDLE?

Such conditional declarations only make sense if there is a legitimate
use of the disabled case and if they make the disabled case fully
transparent to the users. This is typically done by replacing function
declarations by inline stubs doing nothing in the right way when the
feature is disabled. It avoids having to put the condition checks on the
side of all users.

In this case however, you can't stub out cpuidle_devices alone. If you
omit the declaration when CONFIG_CPU_IDLE isn't set, all you'll get is a
failure at compilation time, instead of at linkage time. This barely
helps. For it to be useful, you would additionally have to provide
wrappers around
	this_cpu_read(cpuidle_devices)
and
	per_cpu(cpuidle_devices, cpu)
and stub out these wrappers when CONFIG_CPU_IDLE is disabled (so you
don't refer to cpuidle_devices at all when it isn't available.)

But then again this would only make sense if the psci_checker still
serves a purpose when CONFIG_CPU_IDLE isn't set. Not my area, but after
a quick look at the code I strongly suspect this is not the case.

> > Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> 
> Acked-by: Kevin Brodsky <kevin.brodsky@arm.com>

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] psci: add CPU_IDLE dependency

[Kevin Brodsky]

On 31/07/17 15:14, Jean Delvare wrote:
> Hi Kevin,
>
> On Mon, 31 Jul 2017 11:19:39 +0100, Kevin Brodsky wrote:
>> On 31/07/17 09:55, Arnd Bergmann wrote:
>>> I ran into a build error for the psci_checker:
>>>
>>> drivers/firmware/psci_checker.o: In function `psci_checker':
>>> psci_checker.c:(.init.text+0x528): undefined reference to `cpuidle_devices'
>>>
>>> As far as I can tell, this is simply a very rare combination of options,
>>> but the problem has existed since the code was initially added.
>>> Adding a Kconfig dependency makes it build properly.
>> Good catch! For some reason I missed this config option when figuring out the
>> dependencies... I wonder though, shouldn't cpuidle.h declare cpuidle_devices
>> conditionally on CONFIG_CPU_IDLE?
> Such conditional declarations only make sense if there is a legitimate
> use of the disabled case and if they make the disabled case fully
> transparent to the users. This is typically done by replacing function
> declarations by inline stubs doing nothing in the right way when the
> feature is disabled. It avoids having to put the condition checks on the
> side of all users.
>
> In this case however, you can't stub out cpuidle_devices alone. If you
> omit the declaration when CONFIG_CPU_IDLE isn't set, all you'll get is a
> failure at compilation time, instead of at linkage time. This barely
> helps. For it to be useful, you would additionally have to provide
> wrappers around
> 	this_cpu_read(cpuidle_devices)
> and
> 	per_cpu(cpuidle_devices, cpu)
> and stub out these wrappers when CONFIG_CPU_IDLE is disabled (so you
> don't refer to cpuidle_devices at all when it isn't available.)
>
> But then again this would only make sense if the psci_checker still
> serves a purpose when CONFIG_CPU_IDLE isn't set. Not my area, but after
> a quick look at the code I strongly suspect this is not the case.

I see your point: unlike functions, you can't provide empty stubs for variables, and 
therefore you can't make their absence transparent for the users. That wasn't my 
intention. My point was simply that a variable that's not defined should not be 
declared either in the header (if possible), and I do think that a compilation error 
is preferable to a linkage error. As far as I can tell, other headers also apply this 
principle to per-cpu variables: bpf_prog_active is conditional on CONFIG_BPF_SYSCALL 
in linux/bpf.h, vm_event_states is conditional on CONFIG_VM_EVENT_COUNTERS in 
linux/vmstat.h, etc.

Kevin

>
>>> Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Acked-by: Kevin Brodsky <kevin.brodsky@arm.com>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

Re: [PATCH] psci: add CPU_IDLE dependency

[Arnd Bergmann]

On Mon, Jul 31, 2017 at 4:14 PM, Jean Delvare <jdelvare@suse.de> wrote:
>
> On Mon, 31 Jul 2017 11:19:39 +0100, Kevin Brodsky wrote:
>> On 31/07/17 09:55, Arnd Bergmann wrote:
>> > I ran into a build error for the psci_checker:
>> >
>> > Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> Acked-by: Kevin Brodsky <kevin.brodsky@arm.com>
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

I just came across this old bugfix of mine while going through stuff that
had never been applied by anyone. It turns out that I'm the one who
should have applied it back then, so instead of resending it, I added
it to arm-soc/next/drivers now.

      Arnd

Re: [PATCH] psci: add CPU_IDLE dependency

[Nishanth Menon]

On 07/31/2017 03:55 AM, Arnd Bergmann wrote:
> I ran into a build error for the psci_checker:
> 
> drivers/firmware/psci_checker.o: In function `psci_checker':
> psci_checker.c:(.init.text+0x528): undefined reference to `cpuidle_devices'
> 
> As far as I can tell, this is simply a very rare combination of options,
> but the problem has existed since the code was initially added.
> Adding a Kconfig dependency makes it build properly.
> 
> Fixes: ea8b1c4a6019 ("drivers: psci: PSCI checker module")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/firmware/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 6e4ed5a9c6fd..1983e6e0106f 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -10,7 +10,7 @@ config ARM_PSCI_FW
>   
>   config ARM_PSCI_CHECKER
>   	bool "ARM PSCI checker"
> -	depends on ARM_PSCI_FW && HOTPLUG_CPU && !TORTURE_TEST
> +	depends on ARM_PSCI_FW && HOTPLUG_CPU && CPU_IDLE && !TORTURE_TEST
>   	help
>   	  Run the PSCI checker during startup. This checks that hotplug and
>   	  suspend operations work correctly when using PSCI.
> 
Ccying linux-arm and linux-pm

Reviewed-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon