On 2020-02-21 14:48, Vincenzo Frascino wrote:

Hi Marc,

On 21/02/2020 13:34, Marc Zyngier wrote:

quoted

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.

quoted

On 2020-02-21 13:03, Vincenzo Frascino wrote:

[...]

quoted

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.

quoted

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.
-- 
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