* [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings @ 2021-03-23 9:12 He Ying 2021-03-23 22:18 ` Alexandre Belloni 0 siblings, 1 reply; 9+ messages in thread From: He Ying @ 2021-03-23 9:12 UTC (permalink / raw) To: mpe, benh, paulus, a.zummo, alexandre.belloni, christophe.leroy, npiggin, msuchanek, heying24, tglx, peterz, geert+renesas, kernelfans, frederic Cc: linuxppc-dev, linux-kernel, linux-rtc 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-23 9:12 [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings He Ying @ 2021-03-23 22:18 ` Alexandre Belloni 2021-03-23 23:05 ` Alexandre Belloni ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Alexandre Belloni @ 2021-03-23 22:18 UTC (permalink / raw) To: He Ying Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-23 22:18 ` Alexandre Belloni @ 2021-03-23 23:05 ` Alexandre Belloni 2021-03-24 6:14 ` Christophe Leroy 2021-03-24 1:58 ` heying (H) 2021-03-24 8:19 ` Geert Uytterhoeven 2 siblings, 1 reply; 9+ messages in thread From: Alexandre Belloni @ 2021-03-23 23:05 UTC (permalink / raw) To: He Ying Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-23 23:05 ` Alexandre Belloni @ 2021-03-24 6:14 ` Christophe Leroy 2021-03-24 6:22 ` Christophe Leroy 0 siblings, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2021-03-24 6:14 UTC (permalink / raw) To: Alexandre Belloni, He Ying Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-24 6:14 ` Christophe Leroy @ 2021-03-24 6:22 ` Christophe Leroy 2021-03-24 8:29 ` heying (H) 0 siblings, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2021-03-24 6:22 UTC (permalink / raw) To: Alexandre Belloni, He Ying Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-24 6:22 ` Christophe Leroy @ 2021-03-24 8:29 ` heying (H) 0 siblings, 0 replies; 9+ messages in thread From: heying (H) @ 2021-03-24 8:29 UTC (permalink / raw) To: Christophe Leroy, Alexandre Belloni Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-23 22:18 ` Alexandre Belloni 2021-03-23 23:05 ` Alexandre Belloni @ 2021-03-24 1:58 ` heying (H) 2021-03-24 8:19 ` Geert Uytterhoeven 2 siblings, 0 replies; 9+ messages in thread From: heying (H) @ 2021-03-24 1:58 UTC (permalink / raw) To: Alexandre Belloni Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek, tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel, linux-rtc 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-23 22:18 ` Alexandre Belloni 2021-03-23 23:05 ` Alexandre Belloni 2021-03-24 1:58 ` heying (H) @ 2021-03-24 8:19 ` Geert Uytterhoeven 2021-03-24 9:09 ` Alexandre Belloni 2 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2021-03-24 8:19 UTC (permalink / raw) To: Alexandre Belloni Cc: He Ying, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, Alessandro Zummo, Christophe Leroy, Nicholas Piggin, Michal Suchanek, Thomas Gleixner, Peter Zijlstra, Geert Uytterhoeven, kernelfans, frederic, linuxppc-dev, Linux Kernel Mailing List, linux-rtc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings 2021-03-24 8:19 ` Geert Uytterhoeven @ 2021-03-24 9:09 ` Alexandre Belloni 0 siblings, 0 replies; 9+ messages in thread From: Alexandre Belloni @ 2021-03-24 9:09 UTC (permalink / raw) To: Geert Uytterhoeven Cc: He Ying, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, Alessandro Zummo, Christophe Leroy, Nicholas Piggin, Michal Suchanek, Thomas Gleixner, Peter Zijlstra, Geert Uytterhoeven, kernelfans, frederic, linuxppc-dev, Linux Kernel Mailing List, linux-rtc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-24 9:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-23 9:12 [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings He Ying 2021-03-23 22:18 ` Alexandre Belloni 2021-03-23 23:05 ` Alexandre Belloni 2021-03-24 6:14 ` Christophe Leroy 2021-03-24 6:22 ` Christophe Leroy 2021-03-24 8:29 ` heying (H) 2021-03-24 1:58 ` heying (H) 2021-03-24 8:19 ` Geert Uytterhoeven 2021-03-24 9:09 ` Alexandre Belloni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).