Re: [PATCH v3 1/3] arm64: wrap and generalise counter read functions

[Mark Rutland]

Hi Ionela,

On Thu, Nov 05, 2020 at 12:27:00PM +0000, Ionela Voinescu wrote:
> -#ifdef CONFIG_ARM64_AMU_EXTN
> +#define COUNTER_READ_STORE(NAME, VAL) \
> +static inline u64 read_##NAME(void) \
> +{ \
> +	return VAL; \
> +} \
> +static inline void store_##NAME(void *val) \
> +{ \
> +	*(u64 *)val = read_##NAME(); \
> +}
>  
> -#undef pr_fmt
> -#define pr_fmt(fmt) "AMU: " fmt
> +#ifdef CONFIG_ARM64_AMU_EXTN
> +COUNTER_READ_STORE(corecnt, read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0));
> +COUNTER_READ_STORE(constcnt, read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0));
> +#else
> +COUNTER_READ_STORE(corecnt, 0);
> +COUNTER_READ_STORE(constcnt, 0);
> +#endif

I think this level of indirection is confusing (e.g. since you can't
grep for `read_constcnt` to find the implementation). I also don't think
we need to store_* functions here -- they're only needed by patch 3, and
they can be wrapped in helper functions there (which should also solve
the warning you mention in the cover letter).

I think if we need these wrappers, it'd be better to have:

#ifdef CONFIG_ARM64_AMU_EXTN
#define read_corecnt()	read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0)
#define read_constcnt()	read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0)
#else
#define read_corecnt()	(0UL)
#define read_constcnt()	(0UL)
#endif

Thanks,
Mark.