| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YFp0Qc2P61V+3bm0@piout.net>
Date: Wed, 24 Mar 2021 00:05:37 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: He Ying <heying24@...wei.com>
Cc: mpe@...erman.id.au, benh@...nel.crashing.org, paulus@...ba.org,
a.zummo@...ertech.it, christophe.leroy@...roup.eu,
npiggin@...il.com, msuchanek@...e.de, tglx@...utronix.de,
peterz@...radead.org, geert+renesas@...der.be,
kernelfans@...il.com, frederic@...nel.org,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
linux-rtc@...r.kernel.org
Subject: Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
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@...wei.com>
> > Signed-off-by: He Ying <heying24@...wei.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@...tlin.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@...tlin.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
Powered by blists - more mailing lists