| Message ID | 20211007195754.678124-1-anders.roxell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: asm: vdso: gettimeofday: export common variables | expand |
On Thu, Oct 07, 2021 at 09:57:54PM +0200, Anders Roxell wrote: > When building the kernel with sparse enabled 'C=1' the following > warnings can be seen: > > arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: symbol '__kernel_clock_gettime' was not declared. Should it be static? > arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: symbol '__kernel_gettimeofday' was not declared. Should it be static? > arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: symbol '__kernel_clock_getres' was not declared. Should it be static? > > Rework so the variables are exported, since these variables are > created and used in vdso/vgettimeofday.c, also used in vdso.lds.S. Hmm, these functions are part of the vDSO and shouldn't be called from the kernel, so I don't think it makes sense to add prototypes for them to a kernel header, to be honest. Will
On Mon, Oct 11, 2021 at 12:03 PM Will Deacon <will@kernel.org> wrote: > > On Thu, Oct 07, 2021 at 09:57:54PM +0200, Anders Roxell wrote: > > When building the kernel with sparse enabled 'C=1' the following > > warnings can be seen: > > > > arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: symbol '__kernel_clock_gettime' was not declared. Should it be static? > > arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: symbol '__kernel_gettimeofday' was not declared. Should it be static? > > arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: symbol '__kernel_clock_getres' was not declared. Should it be static? > > > > Rework so the variables are exported, since these variables are > > created and used in vdso/vgettimeofday.c, also used in vdso.lds.S. > > Hmm, these functions are part of the vDSO and shouldn't be called from the > kernel, so I don't think it makes sense to add prototypes for them to a > kernel header, to be honest. It's a recurring problem, and I have recommended this method to Anders as I don't see any of the alternatives as better. The thing is that both sparse (with make C=1) and gcc/clang (with make W=1) warn about missing prototypes, and this catches a lot of real bugs. I hope that we can eventually get to the point of enabling the warning by default for all files, but that means we need a declaration for each global function and variable. It's possible to work around this by adding a declaration just before the function definition to shut up those warnings, but I think that's worse because we should not encourage having declarations in .c files where they may get out of sync with a definition in another file. It's also possible that some of the tools will start warning about extern declarations in .c files for this reason, so it would end up as double work to add them here and move them later. Arnd
On Mon, Oct 11, 2021 at 02:47:56PM +0200, Arnd Bergmann wrote: > On Mon, Oct 11, 2021 at 12:03 PM Will Deacon <will@kernel.org> wrote: > > On Thu, Oct 07, 2021 at 09:57:54PM +0200, Anders Roxell wrote: > > > When building the kernel with sparse enabled 'C=1' the following > > > warnings can be seen: > > > > > > arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: symbol '__kernel_clock_gettime' was not declared. Should it be static? > > > arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: symbol '__kernel_gettimeofday' was not declared. Should it be static? > > > arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: symbol '__kernel_clock_getres' was not declared. Should it be static? > > > > > > Rework so the variables are exported, since these variables are > > > created and used in vdso/vgettimeofday.c, also used in vdso.lds.S. > > > > Hmm, these functions are part of the vDSO and shouldn't be called from the > > kernel, so I don't think it makes sense to add prototypes for them to a > > kernel header, to be honest. > > It's a recurring problem, and I have recommended this method to Anders as > I don't see any of the alternatives as better. > > The thing is that both sparse (with make C=1) and gcc/clang (with make W=1) > warn about missing prototypes, and this catches a lot of real bugs. I hope > that we can eventually get to the point of enabling the warning by default for > all files, but that means we need a declaration for each global function and > variable. I don't think there's anything in the asm/vdso/gettimeofday.h that is required by the kernel. However, it gets dragged in via the datapage.h. If I got it right, something like this should avoid it and we can include the prototypes: diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h index 73eb622e7663..a8d26d7d042d 100644 --- a/include/vdso/datapage.h +++ b/include/vdso/datapage.h @@ -121,22 +121,6 @@ struct vdso_data { extern struct vdso_data _vdso_data[CS_BASES] __attribute__((visibility("hidden"))); extern struct vdso_data _timens_data[CS_BASES] __attribute__((visibility("hidden"))); -/* - * The generic vDSO implementation requires that gettimeofday.h - * provides: - * - __arch_get_vdso_data(): to get the vdso datapage. - * - __arch_get_hw_counter(): to get the hw counter based on the - * clock_mode. - * - gettimeofday_fallback(): fallback for gettimeofday. - * - clock_gettime_fallback(): fallback for clock_gettime. - * - clock_getres_fallback(): fallback for clock_getres. - */ -#ifdef ENABLE_COMPAT_VDSO -#include <asm/vdso/compat_gettimeofday.h> -#else -#include <asm/vdso/gettimeofday.h> -#endif /* ENABLE_COMPAT_VDSO */ - #endif /* !__ASSEMBLY__ */ #endif /* __VDSO_DATAPAGE_H */ diff --git a/include/vdso/gettimeofday.h b/include/vdso/gettimeofday.h new file mode 100644 index 000000000000..0270da504fe3 --- /dev/null +++ b/include/vdso/gettimeofday.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * The generic vDSO implementation requires that gettimeofday.h + * provides: + * - __arch_get_vdso_data(): to get the vdso datapage. + * - __arch_get_hw_counter(): to get the hw counter based on the + * clock_mode. + * - gettimeofday_fallback(): fallback for gettimeofday. + * - clock_gettime_fallback(): fallback for clock_gettime. + * - clock_getres_fallback(): fallback for clock_getres. + */ +#ifdef ENABLE_COMPAT_VDSO +#include <asm/vdso/compat_gettimeofday.h> +#else +#include <asm/vdso/gettimeofday.h> +#endif /* ENABLE_COMPAT_VDSO */ diff --git a/include/vdso/helpers.h b/include/vdso/helpers.h index 9a2af9fca45e..cb7a456323e3 100644 --- a/include/vdso/helpers.h +++ b/include/vdso/helpers.h @@ -6,6 +6,8 @@ #include <vdso/datapage.h> +#include <asm/barrier.h> + static __always_inline u32 vdso_read_begin(const struct vdso_data *vd) { u32 seq; diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c index ce2f69552003..8c1786ae59d8 100644 --- a/lib/vdso/gettimeofday.c +++ b/lib/vdso/gettimeofday.c @@ -3,6 +3,7 @@ * Generic userspace implementations of gettimeofday() and similar. */ #include <vdso/datapage.h> +#include <vdso/gettimeofday.h> #include <vdso/helpers.h> #ifndef vdso_calc_delta
Hi Catalin, >On Mon, Oct 11, 2021 at 02:47:56PM +0200, Arnd Bergmann wrote: >> On Mon, Oct 11, 2021 at 12:03 PM Will Deacon <will@kernel.org> wrote: >> > On Thu, Oct 07, 2021 at 09:57:54PM +0200, Anders Roxell wrote: >> > > When building the kernel with sparse enabled 'C=1' the following >> > > warnings can be seen: >> > > >> > > arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: symbol '__kernel_clock_gettime' was not declared. Should it be static? >> > > arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: symbol '__kernel_gettimeofday' was not declared. Should it be static? >> > > arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: symbol '__kernel_clock_getres' was not declared. Should it be static? >> > > >> > > Rework so the variables are exported, since these variables are >> > > created and used in vdso/vgettimeofday.c, also used in vdso.lds.S. >> > >> > Hmm, these functions are part of the vDSO and shouldn't be called from the >> > kernel, so I don't think it makes sense to add prototypes for them to a >> > kernel header, to be honest. >> >> It's a recurring problem, and I have recommended this method to Anders as >> I don't see any of the alternatives as better. >> >> The thing is that both sparse (with make C=1) and gcc/clang (with make W=1) >> warn about missing prototypes, and this catches a lot of real bugs. I hope >> that we can eventually get to the point of enabling the warning by default for >> all files, but that means we need a declaration for each global function and >> variable. > >I don't think there's anything in the asm/vdso/gettimeofday.h that is >required by the kernel. However, it gets dragged in via the datapage.h. >If I got it right, something like this should avoid it and we can >include the prototypes: > > I tested your patch and I still got the same sparse error. So I was thinking that we should keep Anders' arm64/include/asm/vdso/gettimeofday.h part. The following patch works for me (tested with C=1, ARCH=arm64 defconfig) diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h index 764d13e2916c..d751aa6af7bf 100644 --- a/arch/arm64/include/asm/vdso/gettimeofday.h +++ b/arch/arm64/include/asm/vdso/gettimeofday.h @@ -14,6 +14,13 @@ #define VDSO_HAS_CLOCK_GETRES 1 +extern int __kernel_clock_gettime(clockid_t clock, + struct __kernel_timespec *ts); +extern int __kernel_gettimeofday(struct __kernel_old_timeval *tv, + struct timezone *tz); +extern int __kernel_clock_getres(clockid_t clock_id, + struct __kernel_timespec *res); + static __always_inline int gettimeofday_fallback(struct __kernel_old_timeval *_tv, struct timezone *_tz) diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h index 73eb622e7663..a8d26d7d042d 100644 --- a/include/vdso/datapage.h +++ b/include/vdso/datapage.h @@ -121,22 +121,6 @@ struct vdso_data { extern struct vdso_data _vdso_data[CS_BASES] __attribute__((visibility("hidden"))); extern struct vdso_data _timens_data[CS_BASES] __attribute__((visibility("hidden"))); -/* - * The generic vDSO implementation requires that gettimeofday.h - * provides: - * - __arch_get_vdso_data(): to get the vdso datapage. - * - __arch_get_hw_counter(): to get the hw counter based on the - * clock_mode. - * - gettimeofday_fallback(): fallback for gettimeofday. - * - clock_gettime_fallback(): fallback for clock_gettime. - * - clock_getres_fallback(): fallback for clock_getres. - */ -#ifdef ENABLE_COMPAT_VDSO -#include <asm/vdso/compat_gettimeofday.h> -#else -#include <asm/vdso/gettimeofday.h> -#endif /* ENABLE_COMPAT_VDSO */ - #endif /* !__ASSEMBLY__ */ #endif /* __VDSO_DATAPAGE_H */ diff --git a/include/vdso/gettimeofday.h b/include/vdso/gettimeofday.h new file mode 100644 index 000000000000..0270da504fe3 --- /dev/null +++ b/include/vdso/gettimeofday.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * The generic vDSO implementation requires that gettimeofday.h + * provides: + * - __arch_get_vdso_data(): to get the vdso datapage. + * - __arch_get_hw_counter(): to get the hw counter based on the + * clock_mode. + * - gettimeofday_fallback(): fallback for gettimeofday. + * - clock_gettime_fallback(): fallback for clock_gettime. + * - clock_getres_fallback(): fallback for clock_getres. + */ +#ifdef ENABLE_COMPAT_VDSO +#include <asm/vdso/compat_gettimeofday.h> +#else +#include <asm/vdso/gettimeofday.h> +#endif /* ENABLE_COMPAT_VDSO */ diff --git a/include/vdso/helpers.h b/include/vdso/helpers.h index 9a2af9fca45e..cb7a456323e3 100644 --- a/include/vdso/helpers.h +++ b/include/vdso/helpers.h @@ -6,6 +6,8 @@ #include <vdso/datapage.h> +#include <asm/barrier.h> + static __always_inline u32 vdso_read_begin(const struct vdso_data *vd) { u32 seq; diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c index ce2f69552003..8c1786ae59d8 100644 --- a/lib/vdso/gettimeofday.c +++ b/lib/vdso/gettimeofday.c @@ -3,6 +3,7 @@ * Generic userspace implementations of gettimeofday() and similar. */ #include <vdso/datapage.h> +#include <vdso/gettimeofday.h> #include <vdso/helpers.h> #ifndef vdso_calc_delta thanks, Miles
diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h index 4f7a629df81f..da8dfb119c30 100644 --- a/arch/arm64/include/asm/vdso/gettimeofday.h +++ b/arch/arm64/include/asm/vdso/gettimeofday.h @@ -12,6 +12,13 @@ #define VDSO_HAS_CLOCK_GETRES 1 +extern int __kernel_clock_gettime(clockid_t clock, + struct __kernel_timespec *ts); +extern int __kernel_gettimeofday(struct __kernel_old_timeval *tv, + struct timezone *tz); +extern int __kernel_clock_getres(clockid_t clock_id, + struct __kernel_timespec *res); + static __always_inline int gettimeofday_fallback(struct __kernel_old_timeval *_tv, struct timezone *_tz) diff --git a/arch/arm64/kernel/vdso/vgettimeofday.c b/arch/arm64/kernel/vdso/vgettimeofday.c index 4236cf34d7d9..1a44d1e32746 100644 --- a/arch/arm64/kernel/vdso/vgettimeofday.c +++ b/arch/arm64/kernel/vdso/vgettimeofday.c @@ -5,6 +5,7 @@ * Copyright (C) 2018 ARM Limited * */ +#include <asm/vdso/gettimeofday.h> int __kernel_clock_gettime(clockid_t clock, struct __kernel_timespec *ts)
When building the kernel with sparse enabled 'C=1' the following warnings can be seen: arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: symbol '__kernel_clock_gettime' was not declared. Should it be static? arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: symbol '__kernel_gettimeofday' was not declared. Should it be static? arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: symbol '__kernel_clock_getres' was not declared. Should it be static? Rework so the variables are exported, since these variables are created and used in vdso/vgettimeofday.c, also used in vdso.lds.S. Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- arch/arm64/include/asm/vdso/gettimeofday.h | 7 +++++++ arch/arm64/kernel/vdso/vgettimeofday.c | 1 + 2 files changed, 8 insertions(+)