Re: [PATCH] clocksource: Fix arm_arch_timer clockmode when vDSO disabled
From: Marc Zyngier <maz@kernel.org>
Date: 2020-02-21 16:24:59
Also in:
linux-arch, lkml
Subsystem:
arm port, the rest · Maintainers:
Russell King, Linus Torvalds
On 2020-02-21 15:56, Vincenzo Frascino wrote:
quoted hunk ↗ jump to hunk
Hi Marc, On 21/02/2020 15:28, Marc Zyngier wrote: [...]quoted
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_ALLOWEDdiff --git a/arch/arm/include/asm/clocksource.hb/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.
quoted
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.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel