Re: [PATCH] psci: add CPU_IDLE dependency

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

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>
>