[PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[He Ying]

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 <asm/dtl.h>" because it
is declared there.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: He Ying <heying24@huawei.com>
---
v2:
- Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
  rtc_lock in powerpc asm/time.h.

 arch/powerpc/include/asm/time.h |  3 +++
 arch/powerpc/kernel/time.c      |  6 ++----
 drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 8dd3cdb25338..64a3ef0b4270 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -12,6 +12,7 @@
 #ifdef __KERNEL__
 #include <linux/types.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
 
 #include <asm/processor.h>
 #include <asm/cpu_has_feature.h>
@@ -22,6 +23,8 @@ 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 spinlock_t rtc_lock;
 
 
 extern void generic_calibrate_decr(void);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b67d93a609a2..60b6ac7d3685 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -150,10 +150,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 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
 
 #include <asm/dtl.h>
 
+void (*dtl_consumer)(struct dtl_entry *, u64);
+
 /*
  * Scan the dispatch trace log and count up the stolen time.
  * Should be called with interrupts disabled.
diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
index 5a9f9ad86d32..cc31db058197 100644
--- a/drivers/rtc/rtc-vr41xx.c
+++ b/drivers/rtc/rtc-vr41xx.c
@@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
 
 static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
 
-static DEFINE_SPINLOCK(rtc_lock);
+static DEFINE_SPINLOCK(vr41xx_rtc_lock);
 static char rtc_name[] = "RTC";
 static unsigned long periodic_count;
 static unsigned int alarm_enabled;
@@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
 
 static inline void write_elapsed_second(time64_t sec)
 {
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
 	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
 	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 }
 
 static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
@@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	unsigned long low, mid, high;
 	struct rtc_time *time = &wkalrm->time;
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	low = rtc1_read(ECMPLREG);
 	mid = rtc1_read(ECMPMREG);
 	high = rtc1_read(ECMPHREG);
 	wkalrm->enabled = alarm_enabled;
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
 
@@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 
 	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	if (alarm_enabled)
 		disable_irq(aie_irq);
@@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 
 	alarm_enabled = wkalrm->enabled;
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	return 0;
 }
@@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
 
 static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 	if (enabled) {
 		if (!alarm_enabled) {
 			enable_irq(aie_irq);
@@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 			alarm_enabled = 0;
 		}
 	}
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 	return 0;
 }
 
@@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
 	rtc->range_max = (1ULL << 33) - 1;
 	rtc->max_user_freq = MAX_PERIODIC_RATE;
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	rtc1_write(ECMPLREG, 0);
 	rtc1_write(ECMPMREG, 0);
@@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
 	rtc1_write(RTCL1LREG, 0);
 	rtc1_write(RTCL1HREG, 0);
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	aie_irq = platform_get_irq(pdev, 0);
 	if (aie_irq <= 0) {
-- 
2.17.1

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[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 <asm/dtl.h>" because it
> is declared there.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: He Ying <heying24@huawei.com>
> ---
> 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.

I'm not completely against the rename in vr41xxx but the fix for the
warnings can and should be contained in arch/powerpc.

>  arch/powerpc/include/asm/time.h |  3 +++
>  arch/powerpc/kernel/time.c      |  6 ++----
>  drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 8dd3cdb25338..64a3ef0b4270 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -12,6 +12,7 @@
>  #ifdef __KERNEL__
>  #include <linux/types.h>
>  #include <linux/percpu.h>
> +#include <linux/spinlock.h>
>  
>  #include <asm/processor.h>
>  #include <asm/cpu_has_feature.h>
> @@ -22,6 +23,8 @@ 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 spinlock_t rtc_lock;
>  
>  
>  extern void generic_calibrate_decr(void);
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index b67d93a609a2..60b6ac7d3685 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -150,10 +150,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 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
>  
>  #include <asm/dtl.h>
>  
> +void (*dtl_consumer)(struct dtl_entry *, u64);
> +
>  /*
>   * Scan the dispatch trace log and count up the stolen time.
>   * Should be called with interrupts disabled.
> diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
> index 5a9f9ad86d32..cc31db058197 100644
> --- a/drivers/rtc/rtc-vr41xx.c
> +++ b/drivers/rtc/rtc-vr41xx.c
> @@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
>  
>  static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
>  
> -static DEFINE_SPINLOCK(rtc_lock);
> +static DEFINE_SPINLOCK(vr41xx_rtc_lock);
>  static char rtc_name[] = "RTC";
>  static unsigned long periodic_count;
>  static unsigned int alarm_enabled;
> @@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
>  
>  static inline void write_elapsed_second(time64_t sec)
>  {
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
>  	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
>  	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  }
>  
>  static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
> @@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  	unsigned long low, mid, high;
>  	struct rtc_time *time = &wkalrm->time;
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	low = rtc1_read(ECMPLREG);
>  	mid = rtc1_read(ECMPMREG);
>  	high = rtc1_read(ECMPHREG);
>  	wkalrm->enabled = alarm_enabled;
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
>  
> @@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  
>  	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	if (alarm_enabled)
>  		disable_irq(aie_irq);
> @@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  
>  	alarm_enabled = wkalrm->enabled;
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	return 0;
>  }
> @@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
>  
>  static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  {
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  	if (enabled) {
>  		if (!alarm_enabled) {
>  			enable_irq(aie_irq);
> @@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  			alarm_enabled = 0;
>  		}
>  	}
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  	return 0;
>  }
>  
> @@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
>  	rtc->range_max = (1ULL << 33) - 1;
>  	rtc->max_user_freq = MAX_PERIODIC_RATE;
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	rtc1_write(ECMPLREG, 0);
>  	rtc1_write(ECMPMREG, 0);
> @@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
>  	rtc1_write(RTCL1LREG, 0);
>  	rtc1_write(RTCL1HREG, 0);
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	aie_irq = platform_get_irq(pdev, 0);
>  	if (aie_irq <= 0) {
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[Alexandre Belloni]

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 <asm/dtl.h>" because it
> > is declared there.
> > 
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: He Ying <heying24@huawei.com>
> > ---
> > 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:

8<-----
From dfa59b6f44fdfdefafffa7666aec89e62bbd5c80 Mon Sep 17 00:00:00 2001
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: Wed, 24 Mar 2021 00:00:03 +0100
Subject: [PATCH] powerpc: move rtc_lock to specific platforms

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/powerpc/kernel/time.c          | 3 ---
 arch/powerpc/platforms/chrp/time.c  | 2 +-
 arch/powerpc/platforms/maple/time.c | 2 ++
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 67feb3524460..d3bb189ea7f4 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -123,9 +123,6 @@ EXPORT_SYMBOL(tb_ticks_per_usec);
 unsigned long tb_ticks_per_sec;
 EXPORT_SYMBOL(tb_ticks_per_sec);	/* for cputime_t conversions */
 
-DEFINE_SPINLOCK(rtc_lock);
-EXPORT_SYMBOL_GPL(rtc_lock);
-
 static u64 tb_to_ns_scale __read_mostly;
 static unsigned tb_to_ns_shift __read_mostly;
 static u64 boot_tb __read_mostly;
diff --git a/arch/powerpc/platforms/chrp/time.c b/arch/powerpc/platforms/chrp/time.c
index acde7bbe0716..ea90c15f5edd 100644
--- a/arch/powerpc/platforms/chrp/time.c
+++ b/arch/powerpc/platforms/chrp/time.c
@@ -30,7 +30,7 @@
 
 #include <platforms/chrp/chrp.h>
 
-extern spinlock_t rtc_lock;
+DEFINE_SPINLOCK(rtc_lock);
 
 #define NVRAM_AS0  0x74
 #define NVRAM_AS1  0x75
diff --git a/arch/powerpc/platforms/maple/time.c b/arch/powerpc/platforms/maple/time.c
index 78209bb7629c..ddda02010d86 100644
--- a/arch/powerpc/platforms/maple/time.c
+++ b/arch/powerpc/platforms/maple/time.c
@@ -34,6 +34,8 @@
 #define DBG(x...)
 #endif
 
+DEFINE_SPINLOCK(rtc_lock);
+
 static int maple_rtc_addr;
 
 static int maple_clock_read(int addr)
-- 
2.25.1


> I'm not completely against the rename in vr41xxx but the fix for the
> warnings can and should be contained in arch/powerpc.
> 
> >  arch/powerpc/include/asm/time.h |  3 +++
> >  arch/powerpc/kernel/time.c      |  6 ++----
> >  drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
> >  3 files changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> > index 8dd3cdb25338..64a3ef0b4270 100644
> > --- a/arch/powerpc/include/asm/time.h
> > +++ b/arch/powerpc/include/asm/time.h
> > @@ -12,6 +12,7 @@
> >  #ifdef __KERNEL__
> >  #include <linux/types.h>
> >  #include <linux/percpu.h>
> > +#include <linux/spinlock.h>
> >  
> >  #include <asm/processor.h>
> >  #include <asm/cpu_has_feature.h>
> > @@ -22,6 +23,8 @@ 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 spinlock_t rtc_lock;
> >  
> >  
> >  extern void generic_calibrate_decr(void);
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index b67d93a609a2..60b6ac7d3685 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -150,10 +150,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 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
> >  
> >  #include <asm/dtl.h>
> >  
> > +void (*dtl_consumer)(struct dtl_entry *, u64);
> > +
> >  /*
> >   * Scan the dispatch trace log and count up the stolen time.
> >   * Should be called with interrupts disabled.
> > diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
> > index 5a9f9ad86d32..cc31db058197 100644
> > --- a/drivers/rtc/rtc-vr41xx.c
> > +++ b/drivers/rtc/rtc-vr41xx.c
> > @@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
> >  
> >  static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
> >  
> > -static DEFINE_SPINLOCK(rtc_lock);
> > +static DEFINE_SPINLOCK(vr41xx_rtc_lock);
> >  static char rtc_name[] = "RTC";
> >  static unsigned long periodic_count;
> >  static unsigned int alarm_enabled;
> > @@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
> >  
> >  static inline void write_elapsed_second(time64_t sec)
> >  {
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
> >  	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
> >  	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  }
> >  
> >  static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
> > @@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  	unsigned long low, mid, high;
> >  	struct rtc_time *time = &wkalrm->time;
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	low = rtc1_read(ECMPLREG);
> >  	mid = rtc1_read(ECMPMREG);
> >  	high = rtc1_read(ECMPHREG);
> >  	wkalrm->enabled = alarm_enabled;
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
> >  
> > @@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  
> >  	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	if (alarm_enabled)
> >  		disable_irq(aie_irq);
> > @@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  
> >  	alarm_enabled = wkalrm->enabled;
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	return 0;
> >  }
> > @@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
> >  
> >  static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> >  {
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  	if (enabled) {
> >  		if (!alarm_enabled) {
> >  			enable_irq(aie_irq);
> > @@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> >  			alarm_enabled = 0;
> >  		}
> >  	}
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  	return 0;
> >  }
> >  
> > @@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
> >  	rtc->range_max = (1ULL << 33) - 1;
> >  	rtc->max_user_freq = MAX_PERIODIC_RATE;
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc1_write(ECMPLREG, 0);
> >  	rtc1_write(ECMPMREG, 0);
> > @@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
> >  	rtc1_write(RTCL1LREG, 0);
> >  	rtc1_write(RTCL1HREG, 0);
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	aie_irq = platform_get_irq(pdev, 0);
> >  	if (aie_irq <= 0) {
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[heying (H)]

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 <asm/dtl.h>" because it
>> is declared there.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: He Ying <heying24@huawei.com>
>> ---
>> 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

[Christophe Leroy]

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 <asm/dtl.h>" because it
>>> is declared there.
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Signed-off-by: He Ying <heying24@huawei.com>
>>> ---
>>> 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 ?

Christophe

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[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 <asm/dtl.h>" because it
>>>> is declared there.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>> ---
>>>> 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

Christophe

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[Geert Uytterhoeven]

Hi Alexandre,

On Tue, Mar 23, 2021 at 11:18 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> 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 <asm/dtl.h>" because it
> > is declared there.
> >
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: He Ying <heying24@huawei.com>
> > ---
> > 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.

IIRC, on CHRP LongTrail, NVRAM was inherited from CHRP's Mac ancestry,
not from CHRP's PC ancestry, and thus NVRAM is not the one in the
mc146818-compatible RTC.

http://users.telenet.be/geertu/Linux/PPC/DeviceTree.html confirms that,
showing that nvram is a different device node than rtc.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings

[heying (H)]

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 <asm/dtl.h>" 
>>>>> because it
>>>>> is declared there.
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>>> ---
>>>>> 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 v2 -next] powerpc: kernel/time.c - cleanup warnings

[Alexandre Belloni]

On 24/03/2021 09:19:58+0100, Geert Uytterhoeven wrote:
> Hi Alexandre,
> 
> On Tue, Mar 23, 2021 at 11:18 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > 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 <asm/dtl.h>" because it
> > > is declared there.
> > >
> > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > Signed-off-by: He Ying <heying24@huawei.com>
> > > ---
> > > 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.
> 
> IIRC, on CHRP LongTrail, NVRAM was inherited from CHRP's Mac ancestry,
> not from CHRP's PC ancestry, and thus NVRAM is not the one in the
> mc146818-compatible RTC.
> 
> http://users.telenet.be/geertu/Linux/PPC/DeviceTree.html confirms that,
> showing that nvram is a different device node than rtc.
> 

Yes, what I missed was the ifdefery in drivers/char/nvram.c that makes
it a completely different driver on both platforms. I tend to forget
about that as reading this driver is not a pleasant experience. I would
really like to get rid of the x86 part which would in turn allow to
remove the global rtc_lock spinlock on all architectures.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com