Linus Walleij <linus.wall...@linaro.org> writes: > On Thu, Dec 21, 2023 at 1:04 PM Michael Ellerman <m...@ellerman.id.au> wrote: > >> The VDSO needs PAGE_SHIFT/SIZE defined, so it includes asm/page.h. >> >> However when COMPAT=y the VDSO is built 32-bit, even though the kernel >> is 64-bit. That can lead to odd warnings because some kernel constants >> are 64-bit, but unsigned long is 32-bit, for example: >> >> VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o >> In file included from <built-in>:4: >> In file included from /home/michael/linux/lib/vdso/gettimeofday.c:5: >> In file included from ../include/vdso/datapage.h:137: >> In file included from ../arch/powerpc/include/asm/vdso/gettimeofday.h:7: >> ../arch/powerpc/include/asm/page.h:230:9: warning: result of comparison of >> constant 13835058055282163712 with expression of type 'unsigned long' is >> always true >> 230 | return __pa(kaddr) >> PAGE_SHIFT; >> | ^~~~~~~~~~~ >> >> Fix it by moving the PAGE_SHIFT/SIZE constants into a separate header, >> which can be included by the VDSO, and also by the existing kernel >> headers. That avoids exposing the rest of the header to the non-standard >> build environment of the compat VDSO. >> >> The particular warning above was introduced by commit 58b6fed89ab0 >> ("powerpc: Make virt_to_pfn() a static inline"), though it is not at >> fault, it just exposed the fact that the VDSO was including parts of >> page.h that weren't needed or appropriate for the VDSO. >> >> Don't copy the comment about page sizes, it just risks becoming >> outdated, that information is better available in the Kconfig >> dependencies and help text. >> >> Fixes: 58b6fed89ab0 ("powerpc: Make virt_to_pfn() a static inline") >> Reported-by: kernel test robot <l...@intel.com> >> Closes: >> https://lore.kernel.org/oe-kbuild-all/202311061940.4pbrm44u-...@intel.com/ >> Signed-off-by: Michael Ellerman <m...@ellerman.id.au> > > Clearly a working solution! > Acked-by: Linus Walleij <linus.wall...@linaro.org>
Thanks. > Just a note: > >> diff --git a/arch/powerpc/include/asm/page.h >> b/arch/powerpc/include/asm/page.h > (...) >> +#include <asm/vdso/page.h> > > (...) >> +++ b/arch/powerpc/include/asm/vdso/page.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +#ifndef _ASM_POWERPC_VDSO_PAGE_H >> +#define _ASM_POWERPC_VDSO_PAGE_H >> + >> +#include <vdso/const.h> > > Now the whole kernel includes <vdso/const.h>, is this necessary? It's already included by approximately the whole kernel via: include/linux/kernel.h - include/uapi/linux/kernel.h - include/linux/const.h - include/vdso/const.h And arch/powerpc/include/asm/page.h already includes linux/kernel.h, so includers of page.h should not see any new headers other than asm/vdso/page.h. cheers