Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Miles Chen <miles.chen@mediatek.com>
To:     <catalin.marinas@arm.com>
CC:     <anders.roxell@linaro.org>, <arnd@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <will@kernel.org>
Subject: Re: [PATCH] arm64: asm: vdso: gettimeofday: export common variables
Date:   Mon, 9 Jan 2023 15:48:46 +0800
Message-ID: <20230109074846.23138-1-miles.chen@mediatek.com>
In-Reply-To: <YWQ9FTJUMu9PMPpa@arm.com>
References: <YWQ9FTJUMu9PMPpa@arm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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