| Message ID | 20200221130355.21373-1-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource: Fix arm_arch_timer clockmode when vDSO disabled | expand |
Hi, On 21.02.2020 14:03, Vincenzo Frascino wrote: > The arm_arch_timer requires that VDSO_CLOCKMODE_ARCHTIMER to be > defined to compile correctly. On arm the vDSO can be disabled and when > this is the case the compilation ends prematurely with an error: > > $ make ARCH=arm multi_v7_defconfig > $ ./scripts/config -d VDSO > $ make > > drivers/clocksource/arm_arch_timer.c:73:44: error: > ‘VDSO_CLOCKMODE_ARCHTIMER’ undeclared here (not in a function) > static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; > ^ > scripts/Makefile.build:267: recipe for target > 'drivers/clocksource/arm_arch_timer.o' failed > make[2]: *** [drivers/clocksource/arm_arch_timer.o] Error 1 > make[2]: *** Waiting for unfinished jobs.... > scripts/Makefile.build:505: recipe for target 'drivers/clocksource' failed > make[1]: *** [drivers/clocksource] Error 2 > make[1]: *** Waiting for unfinished jobs.... > Makefile:1683: recipe for target 'drivers' failed > make: *** [drivers] Error 2 > > Define VDSO_CLOCKMODE_ARCHTIMER as VDSO_CLOCKMODE_NONE when the vDSOs are > not enabled to address the issue. > > Fixes: 5e3c6a312a09 ("ARM/arm64: vdso: Use common vdso clock mode storage") > Cc: Russell King <linux@armlinux.org.uk> > Cc: Thomas Gleixner <tglx@linutronix.de> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> Thanks, this fixes the issue. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/clocksource/arm_arch_timer.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index ee2420d56f67..619839221f94 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -49,6 +49,11 @@ > #define CNTV_TVAL 0x38 > #define CNTV_CTL 0x3c > > +#ifndef CONFIG_GENERIC_GETTIMEOFDAY > +/* The define below is required because on arm the VDSOs can be disabled */ > +#define VDSO_CLOCKMODE_ARCHTIMER VDSO_CLOCKMODE_NONE > +#endif /* CONFIG_GENERIC_GETTIMEOFDAY */ > + > static unsigned arch_timers_present __initdata; > > static void __iomem *arch_counter_base; Best regards
Vincenzo, Please include Mark and myself for anything that touches the arch timers (get_maintainer.pl will tell you who you need to cc). On 2020-02-21 13:03, Vincenzo Frascino wrote: > The arm_arch_timer requires that VDSO_CLOCKMODE_ARCHTIMER to be > defined to compile correctly. On arm the vDSO can be disabled and when > this is the case the compilation ends prematurely with an error: > > $ make ARCH=arm multi_v7_defconfig > $ ./scripts/config -d VDSO > $ make > > drivers/clocksource/arm_arch_timer.c:73:44: error: > ‘VDSO_CLOCKMODE_ARCHTIMER’ undeclared here (not in a function) > static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; > ^ > scripts/Makefile.build:267: recipe for target > 'drivers/clocksource/arm_arch_timer.o' failed > make[2]: *** [drivers/clocksource/arm_arch_timer.o] Error 1 > make[2]: *** Waiting for unfinished jobs.... > scripts/Makefile.build:505: recipe for target 'drivers/clocksource' > failed > make[1]: *** [drivers/clocksource] Error 2 > make[1]: *** Waiting for unfinished jobs.... > Makefile:1683: recipe for target 'drivers' failed > make: *** [drivers] Error 2 > > Define VDSO_CLOCKMODE_ARCHTIMER as VDSO_CLOCKMODE_NONE when the vDSOs > are > not enabled to address the issue. > > Fixes: 5e3c6a312a09 ("ARM/arm64: vdso: Use common vdso clock mode > storage") > Cc: Russell King <linux@armlinux.org.uk> > Cc: Thomas Gleixner <tglx@linutronix.de> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > drivers/clocksource/arm_arch_timer.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/clocksource/arm_arch_timer.c > b/drivers/clocksource/arm_arch_timer.c > index ee2420d56f67..619839221f94 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -49,6 +49,11 @@ > #define CNTV_TVAL 0x38 > #define CNTV_CTL 0x3c > > +#ifndef CONFIG_GENERIC_GETTIMEOFDAY > +/* The define below is required because on arm the VDSOs can be > disabled */ > +#define VDSO_CLOCKMODE_ARCHTIMER VDSO_CLOCKMODE_NONE > +#endif /* CONFIG_GENERIC_GETTIMEOFDAY */ This feels pretty clunky. I'd extect VDSO_ARCH_CLOCKMODES (or some similar architecture-specific symbol) to be used for vdso_default, and that symbol to be defined as VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. Otherwise, you'll end-up replicating the same pattern in every clock-source that gets used by the VDSO. M.
Hi Marc, On 21/02/2020 13:34, Marc Zyngier wrote: > Vincenzo, > > Please include Mark and myself for anything that touches the arch timers > (get_maintainer.pl will tell you who you need to cc). > Sorry about that, I posted it too quickly without the proper Cc on the patch. > On 2020-02-21 13:03, Vincenzo Frascino wrote: [...] > > This feels pretty clunky. > > I'd extect VDSO_ARCH_CLOCKMODES (or some similar architecture-specific > symbol) to be used for vdso_default, and that symbol to be defined as > VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. > My understanding is that currently VDSO_ARCH_CLOCKMODES depending on the architecture can identify one or more clocks. In the case of arm and the arm_arch_timer the arch specific symbol is VDSO_CLOCKMODE_ARCHTIMER (used for vdso_default), which as you are correctly stating has to be defined as VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. > Otherwise, you'll end-up replicating the same pattern in every > clock-source that gets used by the VDSO. Based on my investigation this fix should be replicated for all the clocksources used by architectures supported by Unified VDSO and of which VDSOs can be disabled (otherwise the current solution works). After a quick grep on the kernel tree: $ grep -nr "config VDSO" * arch/arm/mm/Kconfig:895:config VDSO Since the only clocksource that falls into these conditions seems to be arm_arch_timer I modified its driver. Please feel free to correct me if I missed or misunderstood something. > > M.
On 2020-02-21 14:48, Vincenzo Frascino wrote: > Hi Marc, > > On 21/02/2020 13:34, Marc Zyngier wrote: >> Vincenzo, >> >> Please include Mark and myself for anything that touches the arch >> timers >> (get_maintainer.pl will tell you who you need to cc). >> > > Sorry about that, I posted it too quickly without the proper Cc on the > patch. > >> On 2020-02-21 13:03, Vincenzo Frascino wrote: > > [...] > >> >> This feels pretty clunky. >> >> I'd extect VDSO_ARCH_CLOCKMODES (or some similar architecture-specific >> symbol) to be used for vdso_default, and that symbol to be defined as >> VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. >> > > My understanding is that currently VDSO_ARCH_CLOCKMODES depending on > the > architecture can identify one or more clocks. In the case of arm and > the > arm_arch_timer the arch specific symbol is VDSO_CLOCKMODE_ARCHTIMER > (used for > vdso_default), which as you are correctly stating has to be defined as > VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. This isn't what I'm saying. What I'm suggesting here is that there is possibly a missing indirection, which defaults to ARCH_TIMER when the VDSO is selected, and NONE when it isn't. Overloading a known symbol feels like papering over the issue. Ideally, this default symbol would be provided by asm/clocksource.h, but that may not even be the right thing to do. >> Otherwise, you'll end-up replicating the same pattern in every >> clock-source that gets used by the VDSO. > > Based on my investigation this fix should be replicated for all the > clocksources > used by architectures supported by Unified VDSO and of which VDSOs can > be > disabled (otherwise the current solution works). After a quick grep on > the > kernel tree: > > $ grep -nr "config VDSO" * > > arch/arm/mm/Kconfig:895:config VDSO > > Since the only clocksource that falls into these conditions seems to be > arm_arch_timer I modified its driver. Fair enough. But don't override the symbol locally. Create a new one: diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..7eb3db75211d 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -69,7 +69,12 @@ static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI; static bool arch_timer_c3stop; static bool arch_timer_mem_use_virtual; static bool arch_counter_suspend_stop; -static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; +#ifdef CONFIG_GENERIC_GETTIMEOFDAY +#define __VDSO_DEFAULT VDSO_CLOCKMODE_ARCHTIMER +#else +#define __VDSO_DEFAULT VDSO_CLOCKMODE_NONE +#endif +static enum vdso_clock_mode vdso_default = __VDSO_DEFAULT; static cpumask_t evtstrm_available = CPU_MASK_NONE; static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM); Or even this (no, I'm not suggesting this seriously): diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..836b500d1bf1 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -69,7 +69,7 @@ static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI; static bool arch_timer_c3stop; static bool arch_timer_mem_use_virtual; static bool arch_counter_suspend_stop; -static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; +static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_MAX - 1; static cpumask_t evtstrm_available = CPU_MASK_NONE; static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM); M.
Hi Marc, On 21/02/2020 15:28, Marc Zyngier wrote: [...] > > This isn't what I'm saying. What I'm suggesting here is that there is > possibly a missing indirection, which defaults to ARCH_TIMER when the > VDSO is selected, and NONE when it isn't. > > Overloading a known symbol feels like papering over the issue. > > Ideally, this default symbol would be provided by asm/clocksource.h, but > that may not even be the right thing to do. > I must admit I really like this idea :), how about: diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 03bbfc312fe7..97864aabc2a6 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -3,6 +3,7 @@ config ARM bool default y select ARCH_32BIT_OFF_T + select ARCH_CLOCKSOURCE_DATA select ARCH_HAS_BINFMT_FLAT select ARCH_HAS_DEBUG_VIRTUAL if MMU select ARCH_HAS_DEVMEM_IS_ALLOWED diff --git a/arch/arm/include/asm/clocksource.h b/arch/arm/include/asm/clocksource.h index 73beb7f131de..e37f6d74ba49 100644 --- a/arch/arm/include/asm/clocksource.h +++ b/arch/arm/include/asm/clocksource.h @@ -1,7 +1,18 @@ #ifndef _ASM_CLOCKSOURCE_H #define _ASM_CLOCKSOURCE_H +/* + * Unused required for compilation only + */ +struct arch_clocksource_data { + bool __reserved; +}; + +#ifdef CONFIG_GENERIC_GETTIMEOFDAY #define VDSO_ARCH_CLOCKMODES \ VDSO_CLOCKMODE_ARCHTIMER +#else +#define VDSO_CLOCKMODE_ARCHTIMER VDSO_CLOCKMODE_NONE +#endif #endif [...] > > Fair enough. But don't override the symbol locally. Create a new one: > I see what you mean now, you mean to not overload the semantical meaning of the symbol. The symbol (VDSO_CLOCKMODE_ARCHTIMER) at this point is never defined when VDSO=n, but I agree with you it can cause confusion.
On 2020-02-21 15:56, Vincenzo Frascino wrote: > Hi Marc, > > On 21/02/2020 15:28, Marc Zyngier wrote: > > [...] > >> >> This isn't what I'm saying. What I'm suggesting here is that there is >> possibly a missing indirection, which defaults to ARCH_TIMER when the >> VDSO is selected, and NONE when it isn't. >> >> Overloading a known symbol feels like papering over the issue. >> >> Ideally, this default symbol would be provided by asm/clocksource.h, >> but >> that may not even be the right thing to do. >> > > I must admit I really like this idea :), how about: > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 03bbfc312fe7..97864aabc2a6 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -3,6 +3,7 @@ config ARM > bool > default y > select ARCH_32BIT_OFF_T > + select ARCH_CLOCKSOURCE_DATA > select ARCH_HAS_BINFMT_FLAT > select ARCH_HAS_DEBUG_VIRTUAL if MMU > select ARCH_HAS_DEVMEM_IS_ALLOWED > diff --git a/arch/arm/include/asm/clocksource.h > b/arch/arm/include/asm/clocksource.h > index 73beb7f131de..e37f6d74ba49 100644 > --- a/arch/arm/include/asm/clocksource.h > +++ b/arch/arm/include/asm/clocksource.h > @@ -1,7 +1,18 @@ > #ifndef _ASM_CLOCKSOURCE_H > #define _ASM_CLOCKSOURCE_H > > +/* > + * Unused required for compilation only > + */ > +struct arch_clocksource_data { > + bool __reserved; > +}; > + > +#ifdef CONFIG_GENERIC_GETTIMEOFDAY > #define VDSO_ARCH_CLOCKMODES \ > VDSO_CLOCKMODE_ARCHTIMER > +#else > +#define VDSO_CLOCKMODE_ARCHTIMER VDSO_CLOCKMODE_NONE > +#endif Which is exactly the same thing as before. It's not an indirection, it is just another overloading of an existing symbol. >> Fair enough. But don't override the symbol locally. Create a new one: >> > > I see what you mean now, you mean to not overload the semantical > meaning of the > symbol. The symbol (VDSO_CLOCKMODE_ARCHTIMER) at this point is never > defined > when VDSO=n, but I agree with you it can cause confusion. Exactly. It breaks the expectation that if VDSO_CLOCKMODE_ARCHTIMER exists, it has a unique, known value. Yes, the outcome is the same. That doesn't make it acceptable though. So building on your above example, here's what I'd like to see: diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1dcc64bd3621..202b41dae05b 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -3,6 +3,7 @@ config ARM bool default y select ARCH_32BIT_OFF_T + select ARCH_CLOCKSOURCE_DATA select ARCH_HAS_BINFMT_FLAT select ARCH_HAS_DEBUG_VIRTUAL if MMU select ARCH_HAS_DEVMEM_IS_ALLOWED diff --git a/arch/arm/include/asm/clocksource.h b/arch/arm/include/asm/clocksource.h index 73beb7f131de..bd4347865f6d 100644 --- a/arch/arm/include/asm/clocksource.h +++ b/arch/arm/include/asm/clocksource.h @@ -1,7 +1,17 @@ #ifndef _ASM_CLOCKSOURCE_H #define _ASM_CLOCKSOURCE_H +struct arch_clocksource_data { + /* Empty on purpose */ +}; + +#ifdef CONFIG_GENERIC_GETTIMEOFDAY #define VDSO_ARCH_CLOCKMODES \ VDSO_CLOCKMODE_ARCHTIMER +#define ARCH_VDSO_DEFAULT_CLOCKMODE VDSO_CLOCKMODE_ARCHTIMER +#else +#define ARCH_VDSO_DEFAULT_CLOCKMODE VDSO_CLOCKMODE_NONE +#endif + #endif diff --git a/arch/arm64/include/asm/clocksource.h b/arch/arm64/include/asm/clocksource.h index eb82e9d95c5d..de706362fa81 100644 --- a/arch/arm64/include/asm/clocksource.h +++ b/arch/arm64/include/asm/clocksource.h @@ -5,4 +5,6 @@ #define VDSO_ARCH_CLOCKMODES \ VDSO_CLOCKMODE_ARCHTIMER +#define ARCH_VDSO_DEFAULT_CLOCKMODE VDSO_CLOCKMODE_ARCHTIMER + #endif diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..8b7081583eeb 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -69,7 +69,7 @@ static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI; static bool arch_timer_c3stop; static bool arch_timer_mem_use_virtual; static bool arch_counter_suspend_stop; -static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; +static enum vdso_clock_mode vdso_default = ARCH_VDSO_DEFAULT_CLOCKMODE; static cpumask_t evtstrm_available = CPU_MASK_NONE; static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM); M.
Hi Marc, On 21/02/2020 16:24, Marc Zyngier wrote: > On 2020-02-21 15:56, Vincenzo Frascino wrote: [...] > > Which is exactly the same thing as before. It's not an indirection, > it is just another overloading of an existing symbol. > Yes, sorry, I missed the indirection here. I wanted more to agree on the concept. >>> Fair enough. But don't override the symbol locally. Create a new one: >>> >> >> I see what you mean now, you mean to not overload the semantical meaning of the >> symbol. The symbol (VDSO_CLOCKMODE_ARCHTIMER) at this point is never defined >> when VDSO=n, but I agree with you it can cause confusion. > > Exactly. It breaks the expectation that if VDSO_CLOCKMODE_ARCHTIMER exists, > it has a unique, known value. Yes, the outcome is the same. That doesn't > make it acceptable though. > > So building on your above example, here's what I'd like to see: > [...] All right, I agree, I will post the patches later on today.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..619839221f94 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -49,6 +49,11 @@ #define CNTV_TVAL 0x38 #define CNTV_CTL 0x3c +#ifndef CONFIG_GENERIC_GETTIMEOFDAY +/* The define below is required because on arm the VDSOs can be disabled */ +#define VDSO_CLOCKMODE_ARCHTIMER VDSO_CLOCKMODE_NONE +#endif /* CONFIG_GENERIC_GETTIMEOFDAY */ + static unsigned arch_timers_present __initdata; static void __iomem *arch_counter_base;
The arm_arch_timer requires that VDSO_CLOCKMODE_ARCHTIMER to be defined to compile correctly. On arm the vDSO can be disabled and when this is the case the compilation ends prematurely with an error: $ make ARCH=arm multi_v7_defconfig $ ./scripts/config -d VDSO $ make drivers/clocksource/arm_arch_timer.c:73:44: error: ‘VDSO_CLOCKMODE_ARCHTIMER’ undeclared here (not in a function) static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; ^ scripts/Makefile.build:267: recipe for target 'drivers/clocksource/arm_arch_timer.o' failed make[2]: *** [drivers/clocksource/arm_arch_timer.o] Error 1 make[2]: *** Waiting for unfinished jobs.... scripts/Makefile.build:505: recipe for target 'drivers/clocksource' failed make[1]: *** [drivers/clocksource] Error 2 make[1]: *** Waiting for unfinished jobs.... Makefile:1683: recipe for target 'drivers' failed make: *** [drivers] Error 2 Define VDSO_CLOCKMODE_ARCHTIMER as VDSO_CLOCKMODE_NONE when the vDSOs are not enabled to address the issue. Fixes: 5e3c6a312a09 ("ARM/arm64: vdso: Use common vdso clock mode storage") Cc: Russell King <linux@armlinux.org.uk> Cc: Thomas Gleixner <tglx@linutronix.de> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- drivers/clocksource/arm_arch_timer.c | 5 +++++ 1 file changed, 5 insertions(+)