The static key used to deal with the errata workaround that plague a
significant number of arm64 systems (who thought that building a timer
was that hard?) has proved to be a disaster when dealing with
lockdep. We try to activate it in contexts that were never expected,
and things break pretty loudly.
This series takes the easy way out and removes the static key
altogether. It always looked like premature optimisation anyway, and
some of the hooks can be implemented in saner ways. To get there, some
unrelated bits have to be fixed first: the 32bit vdso as well as some
of the arm64 stuff.
Marc Zyngier (7):
ARM: vdso: Remove dependency with the arch_timer driver internals
watchdog/sbsa: Use arch_timer_read_counter instead of
arch_counter_get_cntvct
arm64: Use arch_timer_read_counter instead of arch_counter_get_cntvct
clocksource/arm_arch_timer: Direcly assign set_next_event workaround
clocksource/arm_arch_timer: Drop use of static key in
arch_timer_reg_read_stable
clocksource/arm_arch_timer: Remove use of workaround static key
clocksource/arm_arch_timer: Use arch_timer_read_counter to access
stable counters
arch/arm/include/asm/arch_timer.h | 18 ++++-
arch/arm/include/asm/cp15.h | 2 +
arch/arm/vdso/vgettimeofday.c | 5 +-
arch/arm64/include/asm/arch_timer.h | 78 +++++++++++++-----
arch/arm64/kernel/traps.c | 4 +-
drivers/clocksource/arm_arch_timer.c | 115 +++++++++++++--------------
drivers/watchdog/sbsa_gwdt.c | 2 +-
7 files changed, 139 insertions(+), 85 deletions(-)
--
2.20.1
Let's start with the removal of the arch_timer_read_ool_enabled
static key in arch_timer_reg_read_stable. IT is not a fast path,
and we can simplify things a bit.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/arch_timer.h | 42 +++++++++++++++++++----------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index c3762ffcc933..4a06d46def7e 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -77,23 +77,37 @@ struct arch_timer_erratum_workaround {
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
timer_unstable_counter_workaround);
+/* inline sysreg accessors that make erratum_handler() work */
+static inline notrace u32 arch_timer_read_cntp_tval_el0(void)
+{
+ return read_sysreg(cntp_tval_el0);
+}
+
+static inline notrace u32 arch_timer_read_cntv_tval_el0(void)
+{
+ return read_sysreg(cntv_tval_el0);
+}
+
+static inline notrace u64 arch_timer_read_cntpct_el0(void)
+{
+ return read_sysreg(cntpct_el0);
+}
+
+static inline notrace u64 arch_timer_read_cntvct_el0(void)
+{
+ return read_sysreg(cntvct_el0);
+}
+
#define arch_timer_reg_read_stable(reg) \
-({ \
- u64 _val; \
- if (needs_unstable_timer_counter_workaround()) { \
- const struct arch_timer_erratum_workaround *wa; \
+ ({ \
+ u64 _val; \
+ \
preempt_disable_notrace(); \
- wa = __this_cpu_read(timer_unstable_counter_workaround); \
- if (wa && wa->read_##reg) \
- _val = wa->read_##reg(); \
- else \
- _val = read_sysreg(reg); \
+ _val = erratum_handler(read_ ## reg)(); \
preempt_enable_notrace(); \
- } else { \
- _val = read_sysreg(reg); \
- } \
- _val; \
-})
+ \
+ _val; \
+ })
/*
* These register accessors are marked inline so the compiler can
--
2.20.1
Only arch_timer_read_counter will guarantee that workarounds are
applied. So let's use this one instead of arch_counter_get_cntvct.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/watchdog/sbsa_gwdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index e8bd9887c566..e221e47396ab 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -161,7 +161,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
- arch_counter_get_cntvct();
+ arch_timer_read_counter();
do_div(timeleft, gwdt->clk);
--
2.20.1
Only arch_timer_read_counter will guarantee that workarounds are
applied. So let's use this one instead of arch_counter_get_cntvct.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/traps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8ad119c3f665..6190a60388cf 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -493,7 +493,7 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
{
int rt = ESR_ELx_SYS64_ISS_RT(esr);
- pt_regs_write_reg(regs, rt, arch_counter_get_cntvct());
+ pt_regs_write_reg(regs, rt, arch_timer_read_counter());
arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
}
@@ -665,7 +665,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
{
int rt = (esr & ESR_ELx_CP15_64_ISS_RT_MASK) >> ESR_ELx_CP15_64_ISS_RT_SHIFT;
int rt2 = (esr & ESR_ELx_CP15_64_ISS_RT2_MASK) >> ESR_ELx_CP15_64_ISS_RT2_SHIFT;
- u64 val = arch_counter_get_cntvct();
+ u64 val = arch_timer_read_counter();
pt_regs_write_reg(regs, rt, lower_32_bits(val));
pt_regs_write_reg(regs, rt2, upper_32_bits(val));
--
2.20.1
On Mon, Apr 08, 2019 at 04:49:01PM +0100, Marc Zyngier wrote:
> THe VDSO code uses the kernel helper that was originally designed
Nit: s/THe/The/
> to abstract the access between 32 and 64bit systems. It worked so
> far because this function is declared as 'inline'.
>
> As we're about to revamp that part of the code, the VDSO would
> break. Let's fix it by doing what should have been done from
> the start, a proper system register access.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> arch/arm/include/asm/cp15.h | 2 ++
> arch/arm/vdso/vgettimeofday.c | 5 +++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
> index 07e27f212dc7..d2453e2d3f1f 100644
> --- a/arch/arm/include/asm/cp15.h
> +++ b/arch/arm/include/asm/cp15.h
> @@ -68,6 +68,8 @@
> #define BPIALL __ACCESS_CP15(c7, 0, c5, 6)
> #define ICIALLU __ACCESS_CP15(c7, 0, c5, 0)
>
> +#define CNTVCT __ACCESS_CP15_64(1, c14)
This encoding looks right to me per ARM DDI 0406C.d section B4.1.34. The
rest also looks sound, so with that typo fixed:
Reviewed-by: Mark Rutland <[email protected]>
Mark.
> +
> extern unsigned long cr_alignment; /* defined in entry-armv.S */
>
> static inline unsigned long get_cr(void)
> diff --git a/arch/arm/vdso/vgettimeofday.c b/arch/arm/vdso/vgettimeofday.c
> index a9dd619c6c29..7bdbf5d5c47d 100644
> --- a/arch/arm/vdso/vgettimeofday.c
> +++ b/arch/arm/vdso/vgettimeofday.c
> @@ -18,9 +18,9 @@
> #include <linux/compiler.h>
> #include <linux/hrtimer.h>
> #include <linux/time.h>
> -#include <asm/arch_timer.h>
> #include <asm/barrier.h>
> #include <asm/bug.h>
> +#include <asm/cp15.h>
> #include <asm/page.h>
> #include <asm/unistd.h>
> #include <asm/vdso_datapage.h>
> @@ -123,7 +123,8 @@ static notrace u64 get_ns(struct vdso_data *vdata)
> u64 cycle_now;
> u64 nsec;
>
> - cycle_now = arch_counter_get_cntvct();
> + isb();
> + cycle_now = read_sysreg(CNTVCT);
>
> cycle_delta = (cycle_now - vdata->cs_cycle_last) & vdata->cs_mask;
>
> --
> 2.20.1
>
On Mon, Apr 08, 2019 at 04:49:06PM +0100, Marc Zyngier wrote:
> The use of a static key in a hotplug path has proved to be a real
> nightmare, and makes it impossible to have scream-free lockdep
> kernel.
>
> Let's remove the static key altogether, and focus on something saner.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/include/asm/arch_timer.h | 4 ----
> drivers/clocksource/arm_arch_timer.c | 25 +++++++------------------
> 2 files changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 4a06d46def7e..5502ea049b63 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -45,13 +45,9 @@
> (__wa && __wa->h) ? __wa->h : arch_timer_##h; \
> })
>
> -extern struct static_key_false arch_timer_read_ool_enabled;
> -#define needs_unstable_timer_counter_workaround() \
> - static_branch_unlikely(&arch_timer_read_ool_enabled)
> #else
> #define has_erratum_handler(h) false
> #define erratum_handler(h) (arch_timer_##h)
> -#define needs_unstable_timer_counter_workaround() false
> #endif
>
> enum arch_timer_erratum_match_type {
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index c7f5b66d893c..da487fbfada3 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -372,8 +372,6 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>
> -DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
> -EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
>
> static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
> struct clock_event_device *clk)
> @@ -552,12 +550,6 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
> per_cpu(timer_unstable_counter_workaround, i) = wa;
> }
>
> - /*
> - * Use the locked version, as we're called from the CPU
> - * hotplug framework. Otherwise, we end-up in deadlock-land.
> - */
> - static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
> -
> /*
> * Don't use the vdso fastpath if errata require using the
> * out-of-line counter accessor. We may change our mind pretty
> @@ -573,7 +565,7 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
> static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type,
> void *arg)
> {
> - const struct arch_timer_erratum_workaround *wa;
> + const struct arch_timer_erratum_workaround *wa, *__wa;
> ate_match_fn_t match_fn = NULL;
> bool local = false;
>
> @@ -597,16 +589,13 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
> if (!wa)
> return;
>
> - if (needs_unstable_timer_counter_workaround()) {
> - const struct arch_timer_erratum_workaround *__wa;
> - __wa = __this_cpu_read(timer_unstable_counter_workaround);
> - if (__wa && wa != __wa)
> - pr_warn("Can't enable workaround for %s (clashes with %s\n)",
> - wa->desc, __wa->desc);
> + __wa = __this_cpu_read(timer_unstable_counter_workaround);
> + if (__wa && wa != __wa)
> + pr_warn("Can't enable workaround for %s (clashes with %s\n)",
> + wa->desc, __wa->desc);
>
> - if (__wa)
> - return;
> - }
> + if (__wa)
> + return;
>
> arch_timer_enable_workaround(wa, local);
> pr_info("Enabling %s workaround for %s\n",
> --
> 2.20.1
>
Instead of always going via arch_counter_get_cntvct_stable to
access the counter workaround, let's have arch_timer_read_counter
to point to the right method.
For that, we need to track whether any CPU in the system has a
workaround for the counter. This is done by having an atomic
variable tracking this.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/arch_timer.h | 14 ++++++--
arch/arm64/include/asm/arch_timer.h | 16 ++++++++--
drivers/clocksource/arm_arch_timer.c | 48 +++++++++++++++++++++++++---
3 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 3f0a0191f763..4b66ecd6be99 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -83,7 +83,7 @@ static inline u32 arch_timer_get_cntfrq(void)
return val;
}
-static inline u64 arch_counter_get_cntpct(void)
+static inline u64 __arch_counter_get_cntpct(void)
{
u64 cval;
@@ -92,7 +92,12 @@ static inline u64 arch_counter_get_cntpct(void)
return cval;
}
-static inline u64 arch_counter_get_cntvct(void)
+static inline u64 __arch_counter_get_cntpct_stable(void)
+{
+ return __arch_counter_get_cntpct();
+}
+
+static inline u64 __arch_counter_get_cntvct(void)
{
u64 cval;
@@ -101,6 +106,11 @@ static inline u64 arch_counter_get_cntvct(void)
return cval;
}
+static inline u64 __arch_counter_get_cntvct_stable(void)
+{
+ return __arch_counter_get_cntvct();
+}
+
static inline u32 arch_timer_get_cntkctl(void)
{
u32 cntkctl;
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 5502ea049b63..48b2100f4aaa 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -174,18 +174,30 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
isb();
}
-static inline u64 arch_counter_get_cntpct(void)
+static inline u64 __arch_counter_get_cntpct_stable(void)
{
isb();
return arch_timer_reg_read_stable(cntpct_el0);
}
-static inline u64 arch_counter_get_cntvct(void)
+static inline u64 __arch_counter_get_cntpct(void)
+{
+ isb();
+ return read_sysreg(cntpct_el0);
+}
+
+static inline u64 __arch_counter_get_cntvct_stable(void)
{
isb();
return arch_timer_reg_read_stable(cntvct_el0);
}
+static inline u64 __arch_counter_get_cntvct(void)
+{
+ isb();
+ return read_sysreg(cntvct_el0);
+}
+
static inline int arch_timer_arch_init(void)
{
return 0;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index da487fbfada3..5fcccc467868 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -152,6 +152,26 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
return val;
}
+static u64 arch_counter_get_cntpct_stable(void)
+{
+ return __arch_counter_get_cntpct_stable();
+}
+
+static u64 arch_counter_get_cntpct(void)
+{
+ return __arch_counter_get_cntpct();
+}
+
+static u64 arch_counter_get_cntvct_stable(void)
+{
+ return __arch_counter_get_cntvct_stable();
+}
+
+static u64 arch_counter_get_cntvct(void)
+{
+ return __arch_counter_get_cntvct();
+}
+
/*
* Default to cp15 based access because arm64 uses this function for
* sched_clock() before DT is probed and the cp15 method is guaranteed
@@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
+static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
struct clock_event_device *clk)
@@ -550,6 +571,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
per_cpu(timer_unstable_counter_workaround, i) = wa;
}
+ if (wa->read_cntvct_el0 || wa->read_cntpct_el0)
+ atomic_set(&timer_unstable_counter_workaround_in_use, 1);
+
/*
* Don't use the vdso fastpath if errata require using the
* out-of-line counter accessor. We may change our mind pretty
@@ -606,9 +630,15 @@ static bool arch_timer_this_cpu_has_cntvct_wa(void)
{
return has_erratum_handler(read_cntvct_el0);
}
+
+static bool arch_timer_counter_has_wa(void)
+{
+ return atomic_read(&timer_unstable_counter_workaround_in_use);
+}
#else
#define arch_timer_check_ool_workaround(t,a) do { } while(0)
#define arch_timer_this_cpu_has_cntvct_wa() ({false;})
+#define arch_timer_counter_has_wa() ({false;})
#endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
static __always_inline irqreturn_t timer_handler(const int access,
@@ -957,12 +987,22 @@ static void __init arch_counter_register(unsigned type)
/* Register the CP15 based counter if we have one */
if (type & ARCH_TIMER_TYPE_CP15) {
+ u64 (*rd)(void);
+
if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
- arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
- arch_timer_read_counter = arch_counter_get_cntvct;
- else
- arch_timer_read_counter = arch_counter_get_cntpct;
+ arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
+ if (arch_timer_counter_has_wa())
+ rd = arch_counter_get_cntvct_stable;
+ else
+ rd = arch_counter_get_cntvct;
+ } else {
+ if (arch_timer_counter_has_wa())
+ rd = arch_counter_get_cntpct_stable;
+ else
+ rd = arch_counter_get_cntpct;
+ }
+ arch_timer_read_counter = rd;
clocksource_counter.archdata.vdso_direct = vdso_default;
} else {
arch_timer_read_counter = arch_counter_get_cntvct_mem;
--
2.20.1
On Mon, Apr 08, 2019 at 04:49:04PM +0100, Marc Zyngier wrote:
> When a given timer is affected by an erratum and requires an
> alternative implementation of set_next_event, we do a rather
> complicated dance to detect and call the workaround on each
> set_next_event call.
>
> This is clearly idiotic, as we can perfectly detect whether
> this CPU requires a workaround while setting up the clock event
> device.
>
> This only requires the CPU-specific detection to be done a bit
> earlier, and we can then safely override the set_next_event pointer
> if we have a workaround associated to that CPU.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm/include/asm/arch_timer.h | 4 +++
> arch/arm64/include/asm/arch_timer.h | 16 ++++++++++
> drivers/clocksource/arm_arch_timer.c | 46 +++++-----------------------
> 3 files changed, 28 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0a8d7bba2cb0..3f0a0191f763 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -11,6 +11,10 @@
> #include <clocksource/arm_arch_timer.h>
>
> #ifdef CONFIG_ARM_ARCH_TIMER
> +/* 32bit ARM doesn't know anything about timer errata... */
> +#define has_erratum_handler(h) (false)
> +#define erratum_handler(h) (arch_timer_##h)
> +
> int arch_timer_arch_init(void);
>
> /*
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index f2a234d6516c..c3762ffcc933 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -31,10 +31,26 @@
> #include <clocksource/arm_arch_timer.h>
>
> #if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
> +#define has_erratum_handler(h) \
> + ({ \
> + const struct arch_timer_erratum_workaround *__wa; \
> + __wa = __this_cpu_read(timer_unstable_counter_workaround); \
> + (__wa && __wa->h); \
> + })
> +
> +#define erratum_handler(h) \
> + ({ \
> + const struct arch_timer_erratum_workaround *__wa; \
> + __wa = __this_cpu_read(timer_unstable_counter_workaround); \
> + (__wa && __wa->h) ? __wa->h : arch_timer_##h; \
> + })
> +
> extern struct static_key_false arch_timer_read_ool_enabled;
> #define needs_unstable_timer_counter_workaround() \
> static_branch_unlikely(&arch_timer_read_ool_enabled)
> #else
> +#define has_erratum_handler(h) false
> +#define erratum_handler(h) (arch_timer_##h)
> #define needs_unstable_timer_counter_workaround() false
> #endif
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index aa4ec53281ce..c7f5b66d893c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -613,36 +613,12 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
> local ? "local" : "global", wa->desc);
> }
>
> -#define erratum_handler(fn, r, ...) \
> -({ \
> - bool __val; \
> - if (needs_unstable_timer_counter_workaround()) { \
> - const struct arch_timer_erratum_workaround *__wa; \
> - __wa = __this_cpu_read(timer_unstable_counter_workaround); \
> - if (__wa && __wa->fn) { \
> - r = __wa->fn(__VA_ARGS__); \
> - __val = true; \
> - } else { \
> - __val = false; \
> - } \
> - } else { \
> - __val = false; \
> - } \
> - __val; \
> -})
> -
> static bool arch_timer_this_cpu_has_cntvct_wa(void)
> {
> - const struct arch_timer_erratum_workaround *wa;
> -
> - wa = __this_cpu_read(timer_unstable_counter_workaround);
> - return wa && wa->read_cntvct_el0;
> + return has_erratum_handler(read_cntvct_el0);
> }
> #else
> #define arch_timer_check_ool_workaround(t,a) do { } while(0)
> -#define erratum_set_next_event_tval_virt(...) ({BUG(); 0;})
> -#define erratum_set_next_event_tval_phys(...) ({BUG(); 0;})
> -#define erratum_handler(fn, r, ...) ({false;})
> #define arch_timer_this_cpu_has_cntvct_wa() ({false;})
> #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>
> @@ -736,11 +712,6 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
> static int arch_timer_set_next_event_virt(unsigned long evt,
> struct clock_event_device *clk)
> {
> - int ret;
> -
> - if (erratum_handler(set_next_event_virt, ret, evt, clk))
> - return ret;
> -
> set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> return 0;
> }
> @@ -748,11 +719,6 @@ static int arch_timer_set_next_event_virt(unsigned long evt,
> static int arch_timer_set_next_event_phys(unsigned long evt,
> struct clock_event_device *clk)
> {
> - int ret;
> -
> - if (erratum_handler(set_next_event_phys, ret, evt, clk))
> - return ret;
> -
> set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> return 0;
> }
> @@ -777,6 +743,10 @@ static void __arch_timer_setup(unsigned type,
> clk->features = CLOCK_EVT_FEAT_ONESHOT;
>
> if (type == ARCH_TIMER_TYPE_CP15) {
> + typeof(clk->set_next_event) sne;
> +
> + arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
> +
> if (arch_timer_c3stop)
> clk->features |= CLOCK_EVT_FEAT_C3STOP;
> clk->name = "arch_sys_timer";
> @@ -787,20 +757,20 @@ static void __arch_timer_setup(unsigned type,
> case ARCH_TIMER_VIRT_PPI:
> clk->set_state_shutdown = arch_timer_shutdown_virt;
> clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
> - clk->set_next_event = arch_timer_set_next_event_virt;
> + sne = erratum_handler(set_next_event_virt);
> break;
> case ARCH_TIMER_PHYS_SECURE_PPI:
> case ARCH_TIMER_PHYS_NONSECURE_PPI:
> case ARCH_TIMER_HYP_PPI:
> clk->set_state_shutdown = arch_timer_shutdown_phys;
> clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
> - clk->set_next_event = arch_timer_set_next_event_phys;
> + sne = erratum_handler(set_next_event_phys);
> break;
> default:
> BUG();
> }
>
> - arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
> + clk->set_next_event = sne;
> } else {
> clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
> clk->name = "arch_mem_timer";
> --
> 2.20.1
>
On Mon, Apr 08, 2019 at 04:49:05PM +0100, Marc Zyngier wrote:
> Let's start with the removal of the arch_timer_read_ool_enabled
> static key in arch_timer_reg_read_stable. IT is not a fast path,
Nit: s/IT/It/
> and we can simplify things a bit.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/include/asm/arch_timer.h | 42 +++++++++++++++++++----------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index c3762ffcc933..4a06d46def7e 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -77,23 +77,37 @@ struct arch_timer_erratum_workaround {
> DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
> timer_unstable_counter_workaround);
>
> +/* inline sysreg accessors that make erratum_handler() work */
> +static inline notrace u32 arch_timer_read_cntp_tval_el0(void)
> +{
> + return read_sysreg(cntp_tval_el0);
> +}
> +
> +static inline notrace u32 arch_timer_read_cntv_tval_el0(void)
> +{
> + return read_sysreg(cntv_tval_el0);
> +}
> +
> +static inline notrace u64 arch_timer_read_cntpct_el0(void)
> +{
> + return read_sysreg(cntpct_el0);
> +}
> +
> +static inline notrace u64 arch_timer_read_cntvct_el0(void)
> +{
> + return read_sysreg(cntvct_el0);
> +}
> +
> #define arch_timer_reg_read_stable(reg) \
> -({ \
> - u64 _val; \
> - if (needs_unstable_timer_counter_workaround()) { \
> - const struct arch_timer_erratum_workaround *wa; \
> + ({ \
> + u64 _val; \
> + \
> preempt_disable_notrace(); \
> - wa = __this_cpu_read(timer_unstable_counter_workaround); \
> - if (wa && wa->read_##reg) \
> - _val = wa->read_##reg(); \
> - else \
> - _val = read_sysreg(reg); \
> + _val = erratum_handler(read_ ## reg)(); \
> preempt_enable_notrace(); \
> - } else { \
> - _val = read_sysreg(reg); \
> - } \
> - _val; \
> -})
> + \
> + _val; \
> + })
>
> /*
> * These register accessors are marked inline so the compiler can
> --
> 2.20.1
>
THe VDSO code uses the kernel helper that was originally designed
to abstract the access between 32 and 64bit systems. It worked so
far because this function is declared as 'inline'.
As we're about to revamp that part of the code, the VDSO would
break. Let's fix it by doing what should have been done from
the start, a proper system register access.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/cp15.h | 2 ++
arch/arm/vdso/vgettimeofday.c | 5 +++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
index 07e27f212dc7..d2453e2d3f1f 100644
--- a/arch/arm/include/asm/cp15.h
+++ b/arch/arm/include/asm/cp15.h
@@ -68,6 +68,8 @@
#define BPIALL __ACCESS_CP15(c7, 0, c5, 6)
#define ICIALLU __ACCESS_CP15(c7, 0, c5, 0)
+#define CNTVCT __ACCESS_CP15_64(1, c14)
+
extern unsigned long cr_alignment; /* defined in entry-armv.S */
static inline unsigned long get_cr(void)
diff --git a/arch/arm/vdso/vgettimeofday.c b/arch/arm/vdso/vgettimeofday.c
index a9dd619c6c29..7bdbf5d5c47d 100644
--- a/arch/arm/vdso/vgettimeofday.c
+++ b/arch/arm/vdso/vgettimeofday.c
@@ -18,9 +18,9 @@
#include <linux/compiler.h>
#include <linux/hrtimer.h>
#include <linux/time.h>
-#include <asm/arch_timer.h>
#include <asm/barrier.h>
#include <asm/bug.h>
+#include <asm/cp15.h>
#include <asm/page.h>
#include <asm/unistd.h>
#include <asm/vdso_datapage.h>
@@ -123,7 +123,8 @@ static notrace u64 get_ns(struct vdso_data *vdata)
u64 cycle_now;
u64 nsec;
- cycle_now = arch_counter_get_cntvct();
+ isb();
+ cycle_now = read_sysreg(CNTVCT);
cycle_delta = (cycle_now - vdata->cs_cycle_last) & vdata->cs_mask;
--
2.20.1
When a given timer is affected by an erratum and requires an
alternative implementation of set_next_event, we do a rather
complicated dance to detect and call the workaround on each
set_next_event call.
This is clearly idiotic, as we can perfectly detect whether
this CPU requires a workaround while setting up the clock event
device.
This only requires the CPU-specific detection to be done a bit
earlier, and we can then safely override the set_next_event pointer
if we have a workaround associated to that CPU.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/arch_timer.h | 4 +++
arch/arm64/include/asm/arch_timer.h | 16 ++++++++++
drivers/clocksource/arm_arch_timer.c | 46 +++++-----------------------
3 files changed, 28 insertions(+), 38 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 0a8d7bba2cb0..3f0a0191f763 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -11,6 +11,10 @@
#include <clocksource/arm_arch_timer.h>
#ifdef CONFIG_ARM_ARCH_TIMER
+/* 32bit ARM doesn't know anything about timer errata... */
+#define has_erratum_handler(h) (false)
+#define erratum_handler(h) (arch_timer_##h)
+
int arch_timer_arch_init(void);
/*
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index f2a234d6516c..c3762ffcc933 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -31,10 +31,26 @@
#include <clocksource/arm_arch_timer.h>
#if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
+#define has_erratum_handler(h) \
+ ({ \
+ const struct arch_timer_erratum_workaround *__wa; \
+ __wa = __this_cpu_read(timer_unstable_counter_workaround); \
+ (__wa && __wa->h); \
+ })
+
+#define erratum_handler(h) \
+ ({ \
+ const struct arch_timer_erratum_workaround *__wa; \
+ __wa = __this_cpu_read(timer_unstable_counter_workaround); \
+ (__wa && __wa->h) ? __wa->h : arch_timer_##h; \
+ })
+
extern struct static_key_false arch_timer_read_ool_enabled;
#define needs_unstable_timer_counter_workaround() \
static_branch_unlikely(&arch_timer_read_ool_enabled)
#else
+#define has_erratum_handler(h) false
+#define erratum_handler(h) (arch_timer_##h)
#define needs_unstable_timer_counter_workaround() false
#endif
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index aa4ec53281ce..c7f5b66d893c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -613,36 +613,12 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
local ? "local" : "global", wa->desc);
}
-#define erratum_handler(fn, r, ...) \
-({ \
- bool __val; \
- if (needs_unstable_timer_counter_workaround()) { \
- const struct arch_timer_erratum_workaround *__wa; \
- __wa = __this_cpu_read(timer_unstable_counter_workaround); \
- if (__wa && __wa->fn) { \
- r = __wa->fn(__VA_ARGS__); \
- __val = true; \
- } else { \
- __val = false; \
- } \
- } else { \
- __val = false; \
- } \
- __val; \
-})
-
static bool arch_timer_this_cpu_has_cntvct_wa(void)
{
- const struct arch_timer_erratum_workaround *wa;
-
- wa = __this_cpu_read(timer_unstable_counter_workaround);
- return wa && wa->read_cntvct_el0;
+ return has_erratum_handler(read_cntvct_el0);
}
#else
#define arch_timer_check_ool_workaround(t,a) do { } while(0)
-#define erratum_set_next_event_tval_virt(...) ({BUG(); 0;})
-#define erratum_set_next_event_tval_phys(...) ({BUG(); 0;})
-#define erratum_handler(fn, r, ...) ({false;})
#define arch_timer_this_cpu_has_cntvct_wa() ({false;})
#endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
@@ -736,11 +712,6 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
static int arch_timer_set_next_event_virt(unsigned long evt,
struct clock_event_device *clk)
{
- int ret;
-
- if (erratum_handler(set_next_event_virt, ret, evt, clk))
- return ret;
-
set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
return 0;
}
@@ -748,11 +719,6 @@ static int arch_timer_set_next_event_virt(unsigned long evt,
static int arch_timer_set_next_event_phys(unsigned long evt,
struct clock_event_device *clk)
{
- int ret;
-
- if (erratum_handler(set_next_event_phys, ret, evt, clk))
- return ret;
-
set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
return 0;
}
@@ -777,6 +743,10 @@ static void __arch_timer_setup(unsigned type,
clk->features = CLOCK_EVT_FEAT_ONESHOT;
if (type == ARCH_TIMER_TYPE_CP15) {
+ typeof(clk->set_next_event) sne;
+
+ arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
+
if (arch_timer_c3stop)
clk->features |= CLOCK_EVT_FEAT_C3STOP;
clk->name = "arch_sys_timer";
@@ -787,20 +757,20 @@ static void __arch_timer_setup(unsigned type,
case ARCH_TIMER_VIRT_PPI:
clk->set_state_shutdown = arch_timer_shutdown_virt;
clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
- clk->set_next_event = arch_timer_set_next_event_virt;
+ sne = erratum_handler(set_next_event_virt);
break;
case ARCH_TIMER_PHYS_SECURE_PPI:
case ARCH_TIMER_PHYS_NONSECURE_PPI:
case ARCH_TIMER_HYP_PPI:
clk->set_state_shutdown = arch_timer_shutdown_phys;
clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
- clk->set_next_event = arch_timer_set_next_event_phys;
+ sne = erratum_handler(set_next_event_phys);
break;
default:
BUG();
}
- arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
+ clk->set_next_event = sne;
} else {
clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
clk->name = "arch_mem_timer";
--
2.20.1
The use of a static key in a hotplug path has proved to be a real
nightmare, and makes it impossible to have scream-free lockdep
kernel.
Let's remove the static key altogether, and focus on something saner.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/arch_timer.h | 4 ----
drivers/clocksource/arm_arch_timer.c | 25 +++++++------------------
2 files changed, 7 insertions(+), 22 deletions(-)
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 4a06d46def7e..5502ea049b63 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -45,13 +45,9 @@
(__wa && __wa->h) ? __wa->h : arch_timer_##h; \
})
-extern struct static_key_false arch_timer_read_ool_enabled;
-#define needs_unstable_timer_counter_workaround() \
- static_branch_unlikely(&arch_timer_read_ool_enabled)
#else
#define has_erratum_handler(h) false
#define erratum_handler(h) (arch_timer_##h)
-#define needs_unstable_timer_counter_workaround() false
#endif
enum arch_timer_erratum_match_type {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index c7f5b66d893c..da487fbfada3 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -372,8 +372,6 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
-DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
-EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
struct clock_event_device *clk)
@@ -552,12 +550,6 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
per_cpu(timer_unstable_counter_workaround, i) = wa;
}
- /*
- * Use the locked version, as we're called from the CPU
- * hotplug framework. Otherwise, we end-up in deadlock-land.
- */
- static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
-
/*
* Don't use the vdso fastpath if errata require using the
* out-of-line counter accessor. We may change our mind pretty
@@ -573,7 +565,7 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type,
void *arg)
{
- const struct arch_timer_erratum_workaround *wa;
+ const struct arch_timer_erratum_workaround *wa, *__wa;
ate_match_fn_t match_fn = NULL;
bool local = false;
@@ -597,16 +589,13 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
if (!wa)
return;
- if (needs_unstable_timer_counter_workaround()) {
- const struct arch_timer_erratum_workaround *__wa;
- __wa = __this_cpu_read(timer_unstable_counter_workaround);
- if (__wa && wa != __wa)
- pr_warn("Can't enable workaround for %s (clashes with %s\n)",
- wa->desc, __wa->desc);
+ __wa = __this_cpu_read(timer_unstable_counter_workaround);
+ if (__wa && wa != __wa)
+ pr_warn("Can't enable workaround for %s (clashes with %s\n)",
+ wa->desc, __wa->desc);
- if (__wa)
- return;
- }
+ if (__wa)
+ return;
arch_timer_enable_workaround(wa, local);
pr_info("Enabling %s workaround for %s\n",
--
2.20.1
On Mon, Apr 08, 2019 at 04:49:02PM +0100, Marc Zyngier wrote:
> Only arch_timer_read_counter will guarantee that workarounds are
> applied. So let's use this one instead of arch_counter_get_cntvct.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> drivers/watchdog/sbsa_gwdt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index e8bd9887c566..e221e47396ab 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -161,7 +161,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
> timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
>
> timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
> - arch_counter_get_cntvct();
> + arch_timer_read_counter();
>
> do_div(timeleft, gwdt->clk);
>
> --
> 2.20.1
>
On Mon, Apr 08, 2019 at 04:49:03PM +0100, Marc Zyngier wrote:
> Only arch_timer_read_counter will guarantee that workarounds are
> applied. So let's use this one instead of arch_counter_get_cntvct.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/kernel/traps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 8ad119c3f665..6190a60388cf 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -493,7 +493,7 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
> {
> int rt = ESR_ELx_SYS64_ISS_RT(esr);
>
> - pt_regs_write_reg(regs, rt, arch_counter_get_cntvct());
> + pt_regs_write_reg(regs, rt, arch_timer_read_counter());
> arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
> }
>
> @@ -665,7 +665,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
> {
> int rt = (esr & ESR_ELx_CP15_64_ISS_RT_MASK) >> ESR_ELx_CP15_64_ISS_RT_SHIFT;
> int rt2 = (esr & ESR_ELx_CP15_64_ISS_RT2_MASK) >> ESR_ELx_CP15_64_ISS_RT2_SHIFT;
> - u64 val = arch_counter_get_cntvct();
> + u64 val = arch_timer_read_counter();
>
> pt_regs_write_reg(regs, rt, lower_32_bits(val));
> pt_regs_write_reg(regs, rt2, upper_32_bits(val));
> --
> 2.20.1
>
On Mon, Apr 08, 2019 at 04:49:07PM +0100, Marc Zyngier wrote:
> Instead of always going via arch_counter_get_cntvct_stable to
> access the counter workaround, let's have arch_timer_read_counter
> to point to the right method.
Nit: s/to point/point/
> For that, we need to track whether any CPU in the system has a
> workaround for the counter. This is done by having an atomic
> variable tracking this.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm/include/asm/arch_timer.h | 14 ++++++--
> arch/arm64/include/asm/arch_timer.h | 16 ++++++++--
> drivers/clocksource/arm_arch_timer.c | 48 +++++++++++++++++++++++++---
> 3 files changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 3f0a0191f763..4b66ecd6be99 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -83,7 +83,7 @@ static inline u32 arch_timer_get_cntfrq(void)
> return val;
> }
>
> -static inline u64 arch_counter_get_cntpct(void)
> +static inline u64 __arch_counter_get_cntpct(void)
> {
> u64 cval;
>
> @@ -92,7 +92,12 @@ static inline u64 arch_counter_get_cntpct(void)
> return cval;
> }
>
> -static inline u64 arch_counter_get_cntvct(void)
> +static inline u64 __arch_counter_get_cntpct_stable(void)
> +{
> + return __arch_counter_get_cntpct();
> +}
> +
> +static inline u64 __arch_counter_get_cntvct(void)
> {
> u64 cval;
>
> @@ -101,6 +106,11 @@ static inline u64 arch_counter_get_cntvct(void)
> return cval;
> }
>
> +static inline u64 __arch_counter_get_cntvct_stable(void)
> +{
> + return __arch_counter_get_cntvct();
> +}
> +
> static inline u32 arch_timer_get_cntkctl(void)
> {
> u32 cntkctl;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 5502ea049b63..48b2100f4aaa 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -174,18 +174,30 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
> isb();
> }
>
> -static inline u64 arch_counter_get_cntpct(void)
> +static inline u64 __arch_counter_get_cntpct_stable(void)
> {
> isb();
> return arch_timer_reg_read_stable(cntpct_el0);
> }
>
> -static inline u64 arch_counter_get_cntvct(void)
> +static inline u64 __arch_counter_get_cntpct(void)
> +{
> + isb();
> + return read_sysreg(cntpct_el0);
> +}
> +
> +static inline u64 __arch_counter_get_cntvct_stable(void)
> {
> isb();
> return arch_timer_reg_read_stable(cntvct_el0);
> }
>
> +static inline u64 __arch_counter_get_cntvct(void)
> +{
> + isb();
> + return read_sysreg(cntvct_el0);
> +}
> +
> static inline int arch_timer_arch_init(void)
> {
> return 0;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index da487fbfada3..5fcccc467868 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -152,6 +152,26 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> return val;
> }
>
> +static u64 arch_counter_get_cntpct_stable(void)
> +{
> + return __arch_counter_get_cntpct_stable();
> +}
> +
> +static u64 arch_counter_get_cntpct(void)
> +{
> + return __arch_counter_get_cntpct();
> +}
> +
> +static u64 arch_counter_get_cntvct_stable(void)
> +{
> + return __arch_counter_get_cntvct_stable();
> +}
> +
> +static u64 arch_counter_get_cntvct(void)
> +{
> + return __arch_counter_get_cntvct();
> +}
> +
> /*
> * Default to cp15 based access because arm64 uses this function for
> * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>
> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>
> static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
> struct clock_event_device *clk)
> @@ -550,6 +571,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
> per_cpu(timer_unstable_counter_workaround, i) = wa;
> }
>
> + if (wa->read_cntvct_el0 || wa->read_cntpct_el0)
> + atomic_set(&timer_unstable_counter_workaround_in_use, 1);
> +
> /*
> * Don't use the vdso fastpath if errata require using the
> * out-of-line counter accessor. We may change our mind pretty
> @@ -606,9 +630,15 @@ static bool arch_timer_this_cpu_has_cntvct_wa(void)
> {
> return has_erratum_handler(read_cntvct_el0);
> }
> +
> +static bool arch_timer_counter_has_wa(void)
> +{
> + return atomic_read(&timer_unstable_counter_workaround_in_use);
> +}
> #else
> #define arch_timer_check_ool_workaround(t,a) do { } while(0)
> #define arch_timer_this_cpu_has_cntvct_wa() ({false;})
> +#define arch_timer_counter_has_wa() ({false;})
> #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>
> static __always_inline irqreturn_t timer_handler(const int access,
> @@ -957,12 +987,22 @@ static void __init arch_counter_register(unsigned type)
>
> /* Register the CP15 based counter if we have one */
> if (type & ARCH_TIMER_TYPE_CP15) {
> + u64 (*rd)(void);
> +
> if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
> - arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI)
> - arch_timer_read_counter = arch_counter_get_cntvct;
> - else
> - arch_timer_read_counter = arch_counter_get_cntpct;
> + arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
> + if (arch_timer_counter_has_wa())
> + rd = arch_counter_get_cntvct_stable;
> + else
> + rd = arch_counter_get_cntvct;
> + } else {
> + if (arch_timer_counter_has_wa())
> + rd = arch_counter_get_cntpct_stable;
> + else
> + rd = arch_counter_get_cntpct;
> + }
>
> + arch_timer_read_counter = rd;
> clocksource_counter.archdata.vdso_direct = vdso_default;
> } else {
> arch_timer_read_counter = arch_counter_get_cntvct_mem;
> --
> 2.20.1
>
On Mon, Apr 08, 2019 at 04:49:02PM +0100, Marc Zyngier wrote:
> Only arch_timer_read_counter will guarantee that workarounds are
> applied. So let's use this one instead of arch_counter_get_cntvct.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
... assuming/hoping that those counters are actually the same.
Guenter
> ---
> drivers/watchdog/sbsa_gwdt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index e8bd9887c566..e221e47396ab 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -161,7 +161,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
> timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
>
> timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
> - arch_counter_get_cntvct();
> + arch_timer_read_counter();
>
> do_div(timeleft, gwdt->clk);
>
> --
> 2.20.1
>
On 08/04/2019 19:07, Guenter Roeck wrote:
> On Mon, Apr 08, 2019 at 04:49:02PM +0100, Marc Zyngier wrote:
>> Only arch_timer_read_counter will guarantee that workarounds are
>> applied. So let's use this one instead of arch_counter_get_cntvct.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>
> Reviewed-by: Guenter Roeck <[email protected]>
>
> ... assuming/hoping that those counters are actually the same.
There is only a single counter. It is just that in a number of cases,
the HW will return nonsensical values, which is a bit annoying if you
end-up feeding this garbage to a watchdog.
arch_timer_read_counter() guarantees that *if* there is a workaround for
the timer, it gets applied. arch_counter_get_cntvct() only returns the
raw value, with potential side effects.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 08/04/2019 17:49, Marc Zyngier wrote:
> When a given timer is affected by an erratum and requires an
> alternative implementation of set_next_event, we do a rather
> complicated dance to detect and call the workaround on each
> set_next_event call.
>
> This is clearly idiotic, as we can perfectly detect whether
> this CPU requires a workaround while setting up the clock event
> device.
>
> This only requires the CPU-specific detection to be done a bit
> earlier, and we can then safely override the set_next_event pointer
> if we have a workaround associated to that CPU.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Do you want me to take the patch ?
Otherwise:
Acked-by; Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 11/04/2019 18:17, Daniel Lezcano wrote:
> On 08/04/2019 17:49, Marc Zyngier wrote:
>> When a given timer is affected by an erratum and requires an
>> alternative implementation of set_next_event, we do a rather
>> complicated dance to detect and call the workaround on each
>> set_next_event call.
>>
>> This is clearly idiotic, as we can perfectly detect whether
>> this CPU requires a workaround while setting up the clock event
>> device.
>>
>> This only requires the CPU-specific detection to be done a bit
>> earlier, and we can then safely override the set_next_event pointer
>> if we have a workaround associated to that CPU.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>
> Do you want me to take the patch ?
>
> Otherwise:
>
> Acked-by; Daniel Lezcano <[email protected]>
I'd like to keep most of the series together (just so that I don't have
to track extra stuff).
Thanks for the Ack though.
M.
--
Jazz is not dead. It just smells funny...
On 08/04/2019 16:49, Marc Zyngier wrote:
> THe VDSO code uses the kernel helper that was originally designed
> to abstract the access between 32 and 64bit systems. It worked so
> far because this function is declared as 'inline'.
>
> As we're about to revamp that part of the code, the VDSO would
> break. Let's fix it by doing what should have been done from
> the start, a proper system register access.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> arch/arm/include/asm/cp15.h | 2 ++
> arch/arm/vdso/vgettimeofday.c | 5 +++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
> index 07e27f212dc7..d2453e2d3f1f 100644
> --- a/arch/arm/include/asm/cp15.h
> +++ b/arch/arm/include/asm/cp15.h
> @@ -68,6 +68,8 @@
> #define BPIALL __ACCESS_CP15(c7, 0, c5, 6)
> #define ICIALLU __ACCESS_CP15(c7, 0, c5, 0)
>
> +#define CNTVCT __ACCESS_CP15_64(1, c14)
> +
> extern unsigned long cr_alignment; /* defined in entry-armv.S */
>
> static inline unsigned long get_cr(void)
> diff --git a/arch/arm/vdso/vgettimeofday.c b/arch/arm/vdso/vgettimeofday.c
> index a9dd619c6c29..7bdbf5d5c47d 100644
> --- a/arch/arm/vdso/vgettimeofday.c
> +++ b/arch/arm/vdso/vgettimeofday.c
> @@ -18,9 +18,9 @@
> #include <linux/compiler.h>
> #include <linux/hrtimer.h>
> #include <linux/time.h>
> -#include <asm/arch_timer.h>
> #include <asm/barrier.h>
> #include <asm/bug.h>
> +#include <asm/cp15.h>
> #include <asm/page.h>
> #include <asm/unistd.h>
> #include <asm/vdso_datapage.h>
> @@ -123,7 +123,8 @@ static notrace u64 get_ns(struct vdso_data *vdata)
> u64 cycle_now;
> u64 nsec;
>
> - cycle_now = arch_counter_get_cntvct();
> + isb();
> + cycle_now = read_sysreg(CNTVCT);
>
> cycle_delta = (cycle_now - vdata->cs_cycle_last) & vdata->cs_mask;
>
>
Russell, are you OK with this being carried via the arm64 tree? Or would
you prefer me to stick it in your patch system?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 08/04/2019 17:49, Marc Zyngier wrote:
> Let's start with the removal of the arch_timer_read_ool_enabled
> static key in arch_timer_reg_read_stable. IT is not a fast path,
> and we can simplify things a bit.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 08/04/2019 17:49, Marc Zyngier wrote:
> The use of a static key in a hotplug path has proved to be a real
> nightmare, and makes it impossible to have scream-free lockdep
> kernel.
>
> Let's remove the static key altogether, and focus on something saner.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 08/04/2019 17:49, Marc Zyngier wrote:
> Instead of always going via arch_counter_get_cntvct_stable to
> access the counter workaround, let's have arch_timer_read_counter
> to point to the right method.
>
> For that, we need to track whether any CPU in the system has a
> workaround for the counter. This is done by having an atomic
> variable tracking this.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
[ ... ]
> +
> /*
> * Default to cp15 based access because arm64 uses this function for
> * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>
> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?
> static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
> struct clock_event_device *clk)
> @@ -550,6 +571,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
> per_cpu(timer_unstable_counter_workaround, i) = wa;
> }
>
> + if (wa->read_cntvct_el0 || wa->read_cntpct_el0)
> + atomic_set(&timer_unstable_counter_workaround_in_use, 1);
> +
[ ... ]
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hi Marc,
On Mon, Apr 08, 2019 at 04:49:01PM +0100, Marc Zyngier wrote:
> THe VDSO code uses the kernel helper that was originally designed
> to abstract the access between 32 and 64bit systems. It worked so
> far because this function is declared as 'inline'.
>
> As we're about to revamp that part of the code, the VDSO would
> break. Let's fix it by doing what should have been done from
> the start, a proper system register access.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> arch/arm/include/asm/cp15.h | 2 ++
> arch/arm/vdso/vgettimeofday.c | 5 +++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
This looks ok to me and I'd like to take the series via arm64, but I
could do with an Ack from Russell on these 32-bit ARM parts first.
Will
> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
> index 07e27f212dc7..d2453e2d3f1f 100644
> --- a/arch/arm/include/asm/cp15.h
> +++ b/arch/arm/include/asm/cp15.h
> @@ -68,6 +68,8 @@
> #define BPIALL __ACCESS_CP15(c7, 0, c5, 6)
> #define ICIALLU __ACCESS_CP15(c7, 0, c5, 0)
>
> +#define CNTVCT __ACCESS_CP15_64(1, c14)
> +
> extern unsigned long cr_alignment; /* defined in entry-armv.S */
>
> static inline unsigned long get_cr(void)
> diff --git a/arch/arm/vdso/vgettimeofday.c b/arch/arm/vdso/vgettimeofday.c
> index a9dd619c6c29..7bdbf5d5c47d 100644
> --- a/arch/arm/vdso/vgettimeofday.c
> +++ b/arch/arm/vdso/vgettimeofday.c
> @@ -18,9 +18,9 @@
> #include <linux/compiler.h>
> #include <linux/hrtimer.h>
> #include <linux/time.h>
> -#include <asm/arch_timer.h>
> #include <asm/barrier.h>
> #include <asm/bug.h>
> +#include <asm/cp15.h>
> #include <asm/page.h>
> #include <asm/unistd.h>
> #include <asm/vdso_datapage.h>
> @@ -123,7 +123,8 @@ static notrace u64 get_ns(struct vdso_data *vdata)
> u64 cycle_now;
> u64 nsec;
>
> - cycle_now = arch_counter_get_cntvct();
> + isb();
> + cycle_now = read_sysreg(CNTVCT);
>
> cycle_delta = (cycle_now - vdata->cs_cycle_last) & vdata->cs_mask;
>
> --
> 2.20.1
>
On 15/04/2019 13:16, Daniel Lezcano wrote:
> On 08/04/2019 17:49, Marc Zyngier wrote:
>> Instead of always going via arch_counter_get_cntvct_stable to
>> access the counter workaround, let's have arch_timer_read_counter
>> to point to the right method.
>>
>> For that, we need to track whether any CPU in the system has a
>> workaround for the counter. This is done by having an atomic
>> variable tracking this.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>
> [ ... ]
>
>> +
>> /*
>> * Default to cp15 based access because arm64 uses this function for
>> * sched_clock() before DT is probed and the cp15 method is guaranteed
>> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
>> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
>> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>>
>> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>
> Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?
I don't think *_ONCE says anything about the atomicity of the access. It
only instruct the compiler that this should only be accessed once, and
not reloaded/rewritten. In this case, WRITE_ONCE() would work just as
well, but I feel that setting the expectations do matter.
I also had a vague idea to use this as a refcount to drop the
workarounds as CPUs get hotplugged off, in which case we'd need the RMW
operations to be atomic.
Anyway, I'm not hell bent on this. If you fundamentally disagree with me
I'll change it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Hi,
On 30/04/2019 16:27, Marc Zyngier wrote:
[...]
>>> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
>>> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
>>> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>>>
>>> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>>
>> Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?
>
> I don't think *_ONCE says anything about the atomicity of the access. It
> only instruct the compiler that this should only be accessed once, and
> not reloaded/rewritten.
FWIW 7bd3e239d6c6 ("locking: Remove atomicy checks from {READ,WRITE}_ONCE")
points this out.
[...]
Hi Valentin,
On 30/04/2019 17:39, Valentin Schneider wrote:
> Hi,
>
> On 30/04/2019 16:27, Marc Zyngier wrote:
> [...]
>>>> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
>>>> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
>>>> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>>>>
>>>> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>>>
>>> Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?
>>
>> I don't think *_ONCE says anything about the atomicity of the access. It
>> only instruct the compiler that this should only be accessed once, and
>> not reloaded/rewritten.
>
> FWIW 7bd3e239d6c6 ("locking: Remove atomicy checks from {READ,WRITE}_ONCE")
> points this out.
Interesting, thanks for the pointer.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hi Marc,
On 30/04/2019 17:27, Marc Zyngier wrote:
> On 15/04/2019 13:16, Daniel Lezcano wrote:
>> On 08/04/2019 17:49, Marc Zyngier wrote:
>>> Instead of always going via arch_counter_get_cntvct_stable to
>>> access the counter workaround, let's have arch_timer_read_counter
>>> to point to the right method.
>>>
>>> For that, we need to track whether any CPU in the system has a
>>> workaround for the counter. This is done by having an atomic
>>> variable tracking this.
>>>
>>> Signed-off-by: Marc Zyngier <[email protected]>
>>> ---
>>
>> [ ... ]
>>
>>> +
>>> /*
>>> * Default to cp15 based access because arm64 uses this function for
>>> * sched_clock() before DT is probed and the cp15 method is guaranteed
>>> @@ -372,6 +392,7 @@ static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
>>> DEFINE_PER_CPU(const struct arch_timer_erratum_workaround *, timer_unstable_counter_workaround);
>>> EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>>>
>>> +static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>>
>> Wouldn't make sense to READ_ONCE / WRITE_ONCE instead of using an atomic?
>
> I don't think *_ONCE says anything about the atomicity of the access. It
> only instruct the compiler that this should only be accessed once, and
> not reloaded/rewritten. In this case, WRITE_ONCE() would work just as
> well, but I feel that setting the expectations do matter.
>
> I also had a vague idea to use this as a refcount to drop the
> workarounds as CPUs get hotplugged off, in which case we'd need the RMW
> operations to be atomic.
>
> Anyway, I'm not hell bent on this. If you fundamentally disagree with me
> I'll change it.
As you are planning to extend its usage for refcounting in the hotplug
path, I think it is fine.
Thanks
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog