+ Mark
On 30/08/17 06:06, Leo Yan wrote:
> commit fa8d815fac96e7c9247783d5a1f8fa4685b3c543 upstream.
>
> Cortex-A73 (all versions) counter read can return a wrong value
> when the counter crosses a 32bit boundary.
>
> The workaround involves performing the read twice, and to return
> one or the other depending on whether a transition has taken place.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
> arch/arm64/Kconfig | 12 ++++++++++++
> arch/arm64/include/asm/arch_timer.h | 11 +++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 14cdc6d..68e7c98 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -374,6 +374,18 @@ config ARM64_ERRATUM_843419
>
> If unsure, say Y.
>
> +config ARM64_ERRATUM_858921
> + bool "Cortex-A73: 858921: arch timer counter read can return a wrong value"
> + default y
> + depends on ARM_ARCH_TIMER && ARM64
> + help
> + This option enables a workaround applicable to Cortex-A73
> + (all versions), whose counter may return incorrect values.
> + The workaround will be dynamically enabled when an affected
> + core is detected.
> +
> + If unsure, say Y.
> +
> config CAVIUM_ERRATUM_22375
> bool "Cavium erratum 22375, 24313"
> default y
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index fbe0ca3..9b2b0f5 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -114,6 +114,16 @@ static inline u64 arch_counter_get_cntpct(void)
> return 0;
> }
>
> +#ifdef CONFIG_ARM64_ERRATUM_858921
> +static inline u64 arch_counter_get_cntvct(void)
> +{
> + u64 old, new;
> +
> + asm volatile("mrs %0, cntvct_el0" : "=r" (old));
> + asm volatile("mrs %0, cntvct_el0" : "=r" (new));
> + return (((old ^ new) >> 32) & 1) ? old : new;
> +}
> +#else
> static inline u64 arch_counter_get_cntvct(void)
> {
> u64 cval;
> @@ -123,6 +133,7 @@ static inline u64 arch_counter_get_cntvct(void)
>
> return cval;
> }
> +#endif
>
> static inline int arch_timer_arch_init(void)
> {
>
This is completely busted:
- You are only addressing the kernel side, and ignore userspace (which
is just as broken as the kernel).
- You lie in the description of the option (this is in no way dynamic,
since you didn't backport the whole workaround infrastructure).
So I'm afraid I'm NAKing this. Please refrain from blindly backporting
random patches. fa8d815fac96e7c9 only makes sense in the context of the
whole series, and on its own gives you a very false sense of having
properly addressed it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...