Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 8 Apr 2019 17:02:51 +0100
From:   Mark Rutland <mark.rutland@arm.com>
To:     Marc Zyngier <marc.zyngier@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Russell King <linux@arm.linux.org.uk>,
        Will Deacon <will.deacon@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Valentin Schneider <valentin.schneider@arm.com>
Subject: Re: [PATCH 4/7] clocksource/arm_arch_timer: Direcly assign
 set_next_event workaround
Message-ID: <20190408160251.GP6139@lakrids.cambridge.arm.com>
References: <20190408154907.223536-1-marc.zyngier@arm.com>
 <20190408154907.223536-5-marc.zyngier@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190408154907.223536-5-marc.zyngier@arm.com>
User-Agent: Mutt/1.11.1+11 (2f07cb52) (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

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
>