linux-kernel - Re: [PATCH] psci: add CPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c64a27d9-a645-f5d1-a7bf-9eaa2368c120@arm.com>
Date:   Mon, 31 Jul 2017 17:22:03 +0100
From:   Kevin Brodsky <kevin.brodsky@....com>
To:     Jean Delvare <jdelvare@...e.de>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Olof Johansson <olof@...om.net>, Tero Kristo <t-kristo@...com>,
        Thierry Reding <treding@...dia.com>,
        Carlo Caione <carlo@...lessm.com>, Nishanth Menon <nm@...com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH] psci: add CPU_IDLE dependency

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@...db.de>
>> Acked-by: Kevin Brodsky <kevin.brodsky@....com>
> Reviewed-by: Jean Delvare <jdelvare@...e.de>
>