Re: [PATCH] powerpc: define the variable 'uaccess_flush' as static
I think this is the case also for entry_flush. compiling with W=1 will tell you more. When I use these commands: make allmodconfig ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- make C=2 arch/powerpc/kernel/setup_64.o ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- I find warnings as followings: arch/powerpc/kernel/setup_64.c:422:6: warning: symbol 'panic_smp_self_stop' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:951:6: warning: symbol 'rfi_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:952:6: warning: symbol 'entry_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:953:6: warning: symbol 'uaccess_flush' was not declared. Should it be static? When I use the command "make W=1 arch/powerpc/kernel/setup_64.o ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu-", warning becomes this: arch/powerpc/kernel/setup_64.c:422:6: warning: no previous prototype for ‘panic_smp_self_stop’ [-Wmissing-prototypes] void panic_smp_self_stop(void) ^~~ My sparse tool is the latest one with the version "v0.6.3". So, should I fix all the warnings reported by sparse? Thanks.
Re: [PATCH] powerpc: define the variable 'uaccess_flush' as static
Hello, 在 2021/3/15 17:16, Christophe Leroy 写道: I think W=1 will only report missing function prototypes. sparse also reports missing variables prototypes so that's better. All should be fixed. OK. I'll try to fix all the warnings in the file "arch/powerpc/kernel/setup_64.c" reported by sparse and send the patch V2 soon. Thanks.
Re: [PATCH] powerpc: Fix missing prototype problems for "arch/powerpc/kernel/setup_64.c"
在 2021/3/15 20:17, Christophe Leroy 写道: You subject doesn't match the content of the patch. OK. I'll adapt that. Le 15/03/2021 à 13:04, He Ying a écrit : The variables 'uaccess_fulsh' and 'entry_flush' are not referenced outside the file. So define them as static to avoid the warnings. And add a prototype for the function 'panic_smp_self_stop' for the same purpose. Sparse also warns that 'rfi_flush' should be static. However, it's referenced outside the file. To clear that warning, you have to include asm/security_features.h, rfi_flush is declared there. Do you mean that I should include this header in arch/powerpc/kernel/setup_64.c? The warnings about the file reported by sparse are as follows: arch/powerpc/kernel/setup_64.c:422:6: warning: symbol 'panic_smp_self_stop' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:951:6: warning: symbol 'rfi_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:952:6: warning: symbol 'entry_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:953:6: warning: symbol 'uaccess_flush' was not declared. Should it be static? Reported-by: Hulk Robot Signed-off-by: He Ying --- arch/powerpc/kernel/setup_64.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c index 560ed8b975e7..603aacd8527b 100644 --- a/arch/powerpc/kernel/setup_64.c +++ b/arch/powerpc/kernel/setup_64.c @@ -71,6 +71,8 @@ #include "setup.h" +extern void panic_smp_self_stop(void); + For function prototypes 'extern' is unneeded and deprecated. And function prototypes should go in an header file. panic_smp_self_stop() is called from kernel/panic.c , it should be declared in one of the generic linux header files I think. Yes, you're right. But I have no idea which header it should be declared in. May I have your suggestions? int spinning_secondaries; u64 ppc64_pft_size; @@ -949,8 +951,8 @@ static bool no_rfi_flush; static bool no_entry_flush; static bool no_uaccess_flush; bool rfi_flush; -bool entry_flush; -bool uaccess_flush; +static bool entry_flush; +static bool uaccess_flush; DEFINE_STATIC_KEY_FALSE(uaccess_flush_key); EXPORT_SYMBOL(uaccess_flush_key); .
Re: [PATCH] powerpc: Fix missing prototype problems for "arch/powerpc/kernel/setup_64.c"
Dear Cédric Le Goater and Christophe Leroy, Thanks for all your suggestions! I'll pick them in my patch and resent it soon. Thanks again. 在 2021/3/15 21:14, Cédric Le Goater 写道: On 3/15/21 2:01 PM, Christophe Leroy wrote: Le 15/03/2021 à 13:57, Cédric Le Goater a écrit : On 3/15/21 1:48 PM, heying (H) wrote: 在 2021/3/15 20:17, Christophe Leroy 写道: You subject doesn't match the content of the patch. OK. I'll adapt that. Le 15/03/2021 à 13:04, He Ying a écrit : The variables 'uaccess_fulsh' and 'entry_flush' are not referenced outside the file. So define them as static to avoid the warnings. And add a prototype for the function 'panic_smp_self_stop' for the same purpose. Sparse also warns that 'rfi_flush' should be static. However, it's referenced outside the file. To clear that warning, you have to include asm/security_features.h, rfi_flush is declared there. Do you mean that I should include this header in arch/powerpc/kernel/setup_64.c? yes. The warnings about the file reported by sparse are as follows: arch/powerpc/kernel/setup_64.c:422:6: warning: symbol 'panic_smp_self_stop' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:951:6: warning: symbol 'rfi_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:952:6: warning: symbol 'entry_flush' was not declared. Should it be static? arch/powerpc/kernel/setup_64.c:953:6: warning: symbol 'uaccess_flush' was not declared. Should it be static? Reported-by: Hulk Robot Signed-off-by: He Ying --- arch/powerpc/kernel/setup_64.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c index 560ed8b975e7..603aacd8527b 100644 --- a/arch/powerpc/kernel/setup_64.c +++ b/arch/powerpc/kernel/setup_64.c @@ -71,6 +71,8 @@ #include "setup.h" +extern void panic_smp_self_stop(void); + For function prototypes 'extern' is unneeded and deprecated. And function prototypes should go in an header file. panic_smp_self_stop() is called from kernel/panic.c , it should be declared in one of the generic linux header files I think. Yes, you're right. But I have no idea which header it should be declared in. May I have your suggestions? arch/powerpc/include/asm/bug.h looks like a good place. Why declaring it in a powerpc header ? It's a weak function defined in core part of kernel (kernel/panic.c). I think it should go in a common header, just like for instance arch_thaw_secondary_cpus_begin() Indeed. include/linux/smp.h is a better place for a common routine. C. .
Re: [PATCH] powerpc: arch/powerpc/kernel/setup_64.c - cleanup warnings
Thank you for your reply.
在 2021/3/17 11:04, Daniel Axtens 写道:
Hi He Ying,
Thank you for this patch.
I'm not sure what the precise rules for Fixes are, but I wonder if this
should have:
Fixes: 9a32a7e78bd0 ("powerpc/64s: flush L1D after user accesses")
Fixes: f79643787e0a ("powerpc/64s: flush L1D on kernel entry")
Is that necessary for warning cleanups? I thought 'Fixes' tags are
needed only for
bugfix patches. Can someone tell me whether I am right?
Those are the commits that added the entry_flush and uaccess_flush
symbols. Perhaps one for rfi_flush too but I'm not sure what commit
introduced that.
Kind regards,
Daniel
warning: symbol 'rfi_flush' was not declared.
warning: symbol 'entry_flush' was not declared.
warning: symbol 'uaccess_flush' was not declared.
We found warnings above in arch/powerpc/kernel/setup_64.c by using
sparse tool.
Define 'entry_flush' and 'uaccess_flush' as static because they are not
referenced outside the file. Include asm/security_features.h in which
'rfi_flush' is declared.
Reported-by: Hulk Robot
Signed-off-by: He Ying
---
arch/powerpc/kernel/setup_64.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 560ed8b975e7..f92d72a7e7ce 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -68,6 +68,7 @@
#include
#include
#include
+#include
#include "setup.h"
@@ -949,8 +950,8 @@ static bool no_rfi_flush;
static bool no_entry_flush;
static bool no_uaccess_flush;
bool rfi_flush;
-bool entry_flush;
-bool uaccess_flush;
+static bool entry_flush;
+static bool uaccess_flush;
DEFINE_STATIC_KEY_FALSE(uaccess_flush_key);
EXPORT_SYMBOL(uaccess_flush_key);
--
2.17.1
.
Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
在 2021/3/17 19:16, Christophe Leroy 写道:
Le 17/03/2021 à 11:34, He Ying a écrit :
We found these warnings in arch/powerpc/kernel/time.c as follows:
warning: symbol 'decrementer_max' was not declared. Should it be static?
warning: symbol 'rtc_lock' was not declared. Should it be static?
warning: symbol 'dtl_consumer' was not declared. Should it be static?
Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And
include
proper header in which 'rtc_lock' is declared. Move 'dtl_consumer'
definition behind "include " because 'dtl_consumer' is
declared
there.
Reported-by: Hulk Robot
Signed-off-by: He Ying
---
arch/powerpc/include/asm/time.h | 1 +
arch/powerpc/kernel/time.c | 7 +++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/time.h
b/arch/powerpc/include/asm/time.h
index 8dd3cdb25338..2cd2b50bedda 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy;
extern unsigned long tb_ticks_per_usec;
extern unsigned long tb_ticks_per_sec;
extern struct clock_event_device decrementer_clockevent;
+extern u64 decrementer_max;
extern void generic_calibrate_decr(void);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b67d93a609a2..409967713ca6 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -55,6 +55,7 @@
#include
#include
#include
+#include
I don't think that's the good place. It has no link to powerpc, it is
only by chance that it has the same name.
As rtc_lock is defined in powerpc time.c, I think you should declare
it in powerpc asm/time.h
My first thought was the same as yours. I tried to add declaration in
powerpc asm/time.h, but got a compiling error:
drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of ‘rtc_lock’
follows non-static declaration
static DEFINE_SPINLOCK(rtc_lock);
In file included from ./arch/powerpc/include/asm/delay.h:7:0,
from ./arch/powerpc/include/asm/io.h:33,
from ./include/linux/io.h:13,
from drivers/rtc/rtc-vr41xx.c:11:
./arch/powerpc/include/asm/time.h:25:19: note: previous declaration of
‘rtc_lock’ was here
extern spinlock_t rtc_lock;
There's a conflict. Perhaps I can rename it in drivers/rtc/rtc-vr41xx.c.
But I find an existing declaration in linux/mc146818rtc.h and there's
only one definition for 'rtc_lock' in powerpc.
There's some includes of mc146818rtc.h in powperc. I wonder they point
to the same thing. But I'm not very sure
because the header's name looks a bit strange.
#include
#include
@@ -150,10 +151,6 @@ bool tb_invalid;
u64 __cputime_usec_factor;
EXPORT_SYMBOL(__cputime_usec_factor);
-#ifdef CONFIG_PPC_SPLPAR
-void (*dtl_consumer)(struct dtl_entry *, u64);
-#endif
-
static void calc_cputime_factors(void)
{
struct div_result res;
@@ -179,6 +176,8 @@ static inline unsigned long read_spurr(unsigned
long tb)
#include
+void (*dtl_consumer)(struct dtl_entry *, u64);
+
/*
* Scan the dispatch trace log and count up the stolen time.
* Should be called with interrupts disabled.
.
Re: [PATCH] powerpc: arch/powerpc/kernel/setup_64.c - cleanup warnings
在 2021/3/17 19:57, Michael Ellerman 写道:
Daniel Axtens writes:
"heying (H)" writes:
Thank you for your reply.
在 2021/3/17 11:04, Daniel Axtens 写道:
Hi He Ying,
Thank you for this patch.
I'm not sure what the precise rules for Fixes are, but I wonder if this
should have:
Fixes: 9a32a7e78bd0 ("powerpc/64s: flush L1D after user accesses")
Fixes: f79643787e0a ("powerpc/64s: flush L1D on kernel entry")
Is that necessary for warning cleanups? I thought 'Fixes' tags are
needed only for
bugfix patches. Can someone tell me whether I am right?
Yeah, I'm not sure either. Hopefully mpe will let us know.
It's not necessary to add a Fixes tag for a patch like this, but you can
add one if you think it's important that the fix gets backported.
I don't think the cleanups in this case are that important, so I
wouldn't bother with a Fixes tag.
Okay. That's a good explanation to me. Thanks.
Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
Dear Christophe, 在 2021/3/18 10:28, heying (H) 写道: 在 2021/3/17 19:16, Christophe Leroy 写道: Le 17/03/2021 à 11:34, He Ying a écrit : We found these warnings in arch/powerpc/kernel/time.c as follows: warning: symbol 'decrementer_max' was not declared. Should it be static? warning: symbol 'rtc_lock' was not declared. Should it be static? warning: symbol 'dtl_consumer' was not declared. Should it be static? Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And include proper header in which 'rtc_lock' is declared. Move 'dtl_consumer' definition behind "include " because 'dtl_consumer' is declared there. Reported-by: Hulk Robot Signed-off-by: He Ying --- arch/powerpc/include/asm/time.h | 1 + arch/powerpc/kernel/time.c | 7 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h index 8dd3cdb25338..2cd2b50bedda 100644 --- a/arch/powerpc/include/asm/time.h +++ b/arch/powerpc/include/asm/time.h @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy; extern unsigned long tb_ticks_per_usec; extern unsigned long tb_ticks_per_sec; extern struct clock_event_device decrementer_clockevent; +extern u64 decrementer_max; extern void generic_calibrate_decr(void); diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index b67d93a609a2..409967713ca6 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -55,6 +55,7 @@ #include #include #include +#include I don't think that's the good place. It has no link to powerpc, it is only by chance that it has the same name. As rtc_lock is defined in powerpc time.c, I think you should declare it in powerpc asm/time.h My first thought was the same as yours. I tried to add declaration in powerpc asm/time.h, but got a compiling error: drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of ‘rtc_lock’ follows non-static declaration static DEFINE_SPINLOCK(rtc_lock); In file included from ./arch/powerpc/include/asm/delay.h:7:0, from ./arch/powerpc/include/asm/io.h:33, from ./include/linux/io.h:13, from drivers/rtc/rtc-vr41xx.c:11: ./arch/powerpc/include/asm/time.h:25:19: note: previous declaration of ‘rtc_lock’ was here extern spinlock_t rtc_lock; There's a conflict. Perhaps I can rename it in drivers/rtc/rtc-vr41xx.c. But I find an existing declaration in linux/mc146818rtc.h and there's only one definition for 'rtc_lock' in powerpc. There's some includes of mc146818rtc.h in powperc. I wonder they point to the same thing. But I'm not very sure because the header's name looks a bit strange. How about including mc146818rtc.h in powperpc kernel/time.c? May I have your opinions please? Thanks.
Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
Dear Christophe, 在 2021/3/23 14:33, Christophe Leroy 写道: Le 23/03/2021 à 07:21, heying (H) a écrit : Dear Christophe, 在 2021/3/18 10:28, heying (H) 写道: 在 2021/3/17 19:16, Christophe Leroy 写道: Le 17/03/2021 à 11:34, He Ying a écrit : We found these warnings in arch/powerpc/kernel/time.c as follows: warning: symbol 'decrementer_max' was not declared. Should it be static? warning: symbol 'rtc_lock' was not declared. Should it be static? warning: symbol 'dtl_consumer' was not declared. Should it be static? Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And include proper header in which 'rtc_lock' is declared. Move 'dtl_consumer' definition behind "include " because 'dtl_consumer' is declared there. Reported-by: Hulk Robot Signed-off-by: He Ying --- arch/powerpc/include/asm/time.h | 1 + arch/powerpc/kernel/time.c | 7 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h index 8dd3cdb25338..2cd2b50bedda 100644 --- a/arch/powerpc/include/asm/time.h +++ b/arch/powerpc/include/asm/time.h @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy; extern unsigned long tb_ticks_per_usec; extern unsigned long tb_ticks_per_sec; extern struct clock_event_device decrementer_clockevent; +extern u64 decrementer_max; extern void generic_calibrate_decr(void); diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index b67d93a609a2..409967713ca6 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -55,6 +55,7 @@ #include #include #include +#include I don't think that's the good place. It has no link to powerpc, it is only by chance that it has the same name. As rtc_lock is defined in powerpc time.c, I think you should declare it in powerpc asm/time.h My first thought was the same as yours. I tried to add declaration in powerpc asm/time.h, but got a compiling error: drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of ‘rtc_lock’ follows non-static declaration static DEFINE_SPINLOCK(rtc_lock); In file included from ./arch/powerpc/include/asm/delay.h:7:0, from ./arch/powerpc/include/asm/io.h:33, from ./include/linux/io.h:13, from drivers/rtc/rtc-vr41xx.c:11: ./arch/powerpc/include/asm/time.h:25:19: note: previous declaration of ‘rtc_lock’ was here extern spinlock_t rtc_lock; There's a conflict. Perhaps I can rename it in drivers/rtc/rtc-vr41xx.c. But I find an existing declaration in linux/mc146818rtc.h and there's only one definition for 'rtc_lock' in powerpc. There's some includes of mc146818rtc.h in powperc. I wonder they point to the same thing. But I'm not very sure because the header's name looks a bit strange. How about including mc146818rtc.h in powperpc kernel/time.c? May I have your opinions please? As I said, mc146818rtc.h is not related to powerpc, and if it works that's just chance, and there is no certainty that it will still work in the future. If you can't find a clean solution, it is better to leave the warning. OK. I see. Thanks for you relpy. I'll try to find some other better way. Thanks.
Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
Dear, 在 2021/3/24 6:18, Alexandre Belloni 写道: Hello, On 23/03/2021 05:12:57-0400, He Ying wrote: We found these warnings in arch/powerpc/kernel/time.c as follows: warning: symbol 'decrementer_max' was not declared. Should it be static? warning: symbol 'rtc_lock' was not declared. Should it be static? warning: symbol 'dtl_consumer' was not declared. Should it be static? Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h. Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to avoid the conflict with the variable in powerpc asm/time.h. Move 'dtl_consumer' definition behind "include " because it is declared there. Reported-by: Hulk Robot Signed-off-by: He Ying --- v2: - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare rtc_lock in powerpc asm/time.h. V1 was actually the correct thing to do. rtc_lock is there exactly because chrp and maple are using mc146818 compatible RTCs. This is then useful because then drivers/char/nvram.c is enabled. The proper fix would be to scrap all of that and use rtc-cmos for those platforms as this drives the RTC properly and exposes the NVRAM for the mc146818. Do you mean that 'rtc_lock' declared in linux/mc146818rtc.h points to same thing as that defined in powerpc kernel/time.c? And you think V1 was correct? Oh, I should have added you to my patch V1 senders:) Or at least, if there are no users for the char/nvram driver on those two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or more likely rename the symbol as it seems to be abused by both chrp and powermac. I'm not completely against the rename in vr41xxx but the fix for the warnings can and should be contained in arch/powerpc. Yes, I agree with you. But I have no choice because there is a compiling error. Maybe there's a better way. So, what about my patch V1? Should I resend it and add you to senders? Thanks.
Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
Dear, 在 2021/3/24 14:22, Christophe Leroy 写道: Le 24/03/2021 à 07:14, Christophe Leroy a écrit : Le 24/03/2021 à 00:05, Alexandre Belloni a écrit : On 23/03/2021 23:18:17+0100, Alexandre Belloni wrote: Hello, On 23/03/2021 05:12:57-0400, He Ying wrote: We found these warnings in arch/powerpc/kernel/time.c as follows: warning: symbol 'decrementer_max' was not declared. Should it be static? warning: symbol 'rtc_lock' was not declared. Should it be static? warning: symbol 'dtl_consumer' was not declared. Should it be static? Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h. Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to avoid the conflict with the variable in powerpc asm/time.h. Move 'dtl_consumer' definition behind "include " because it is declared there. Reported-by: Hulk Robot Signed-off-by: He Ying --- v2: - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare rtc_lock in powerpc asm/time.h. V1 was actually the correct thing to do. rtc_lock is there exactly because chrp and maple are using mc146818 compatible RTCs. This is then useful because then drivers/char/nvram.c is enabled. The proper fix would be to scrap all of that and use rtc-cmos for those platforms as this drives the RTC properly and exposes the NVRAM for the mc146818. Or at least, if there are no users for the char/nvram driver on those two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or more likely rename the symbol as it seems to be abused by both chrp and powermac. Ok so rtc_lock is not even used by the char/nvram.c driver as it is completely compiled out. I guess it is fine having it move to the individual platform as looking very quickly at the Kconfig, it is not possible to select both simultaneously. Tentative patch: Looking at it once more, it looks like including linux/mc146818rtc.h is the thing to do, at least for now. Several platforms are defining the rtc_lock exactly the same way as powerpc does, and including mc146818rtc.h I think that to get it clean, this change should go in a dedicated patch and do a bit more and explain exactly what is being do and why. I'll try to draft something for it. He Y., can you make a version v3 of your patch excluding the rtc_lock change ? Finally, I think there is not enough changes to justify a separate patch. So you can send a V3 based on your V1. In addition to the changes you had in V1, please remove the declaration of rfc_lock in arch/powerpc/platforms/chrp/chrp.h So glad to hear that. I'll do that and send my V3. Thanks.
Re: [PATCH V3 -next] powerpc: kernel/time.c - cleanup warnings
Dear Alexandre, 在 2021/3/24 17:29, Alexandre Belloni 写道: On 24/03/2021 05:09:39-0400, He Ying wrote: We found these warnings in arch/powerpc/kernel/time.c as follows: warning: symbol 'decrementer_max' was not declared. Should it be static? warning: symbol 'rtc_lock' was not declared. Should it be static? warning: symbol 'dtl_consumer' was not declared. Should it be static? Declare 'decrementer_max' in powerpc asm/time.h. Include linux/mc146818rtc.h in powerpc kernel/time.c where 'rtc_lock' is declared. And remove duplicated declaration of 'rtc_lock' in powerpc platforms/chrp/time.c because it has included linux/mc146818rtc.h. Move 'dtl_consumer' definition behind "include " because it is declared there. Reported-by: Hulk Robot Signed-off-by: He Ying --- V2: - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare rtc_lock in powerpc asm/time.h. V3: - Recover to V1, that is including linux/mc146818rtc.h in powerpc kernel/time.c. And remove duplicated declaration of 'rtc_lock' in powerpc platforms/chrp/time.c because it has included linux/mc146818rtc.h. arch/powerpc/include/asm/time.h| 1 + arch/powerpc/kernel/time.c | 9 - arch/powerpc/platforms/chrp/time.c | 2 -- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h index 8dd3cdb25338..2cd2b50bedda 100644 --- a/arch/powerpc/include/asm/time.h +++ b/arch/powerpc/include/asm/time.h @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy; extern unsigned long tb_ticks_per_usec; extern unsigned long tb_ticks_per_sec; extern struct clock_event_device decrementer_clockevent; +extern u64 decrementer_max; extern void generic_calibrate_decr(void); diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index b67d93a609a2..ac81f043bf49 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -55,8 +55,9 @@ #include #include #include -#include +#include I'm fine with that but I really think my suggestion to make the rtc_lock local to the platforms was better because it is only used to synchronize between concurrent invocations of chrp_set_rtc_time or maple_set_rtc_time. The rtc core will never do that and the only case would be concurrent calls to rtc_ops.set_time and update_persistent_clock64 (which should also be removed at some point). Many thanks for your suggestion. As you suggest, rtc_lock should be local to platforms. Does it mean not only powerpc but also all other platforms should adapt this change? It might be a big change. I have no idea if that's OK. What are other maintainers' opinions? Thanks.
