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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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