Re: [PATCH 4/7] clocksource/arm_arch_timer: Direcly assign set_next_event workaround
From: Mark Rutland
Date: Mon Apr 08 2019 - 12:02:58 EST
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 <marc.zyngier@xxxxxxx>
Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
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
>