Re: [PATCH 05/13] drivers/perf: apple_m1: Use macros for per-implementation init functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drivers/perf: apple_m1: unused macro m2_pmu_init_pmceid aliases removed symbol
- [Low] perf: apple_m1: redundant macro aliasing and duplication in APPLE_PMU_INIT()
--

commit b798f559facbfb19db0eef028f7f7e42ebc0042c
Author: Myria Sarvay <myrialsarvay@gmail.com>

drivers/perf: apple_m1: Use macros for per-implementation init functions

This commit replaces the per-core-family PMU initialization functions in
drivers/perf/apple_m1_cpu_pmu.c with an APPLE_PMU_INIT() macro and adds
compatibility macro aliases for M2 callbacks and attribute groups.

> diff --git a/drivers/perf/apple_m1_cpu_pmu.c b/drivers/perf/apple_m1_cpu_pmu.c
> index a14864263bf1f..e7e9b51ffadcc 100644
> --- a/drivers/perf/apple_m1_cpu_pmu.c
> +++ b/drivers/perf/apple_m1_cpu_pmu.c
> @@ -653,71 +653,40 @@ static int apple_pmu_init(struct arm_pmu *cpu_pmu)
>  }
>  
>  /* Device driver gunk */
> -static int m1_pmu_ice_init(struct arm_pmu *cpu_pmu)
> -{
> -	cpu_pmu->name = "apple_icestorm_pmu";
> -	cpu_pmu->enable		  = m1_pmu_enable_event;
> -	cpu_pmu->get_event_idx	  = m1_pmu_get_event_idx;
> -	cpu_pmu->map_event	  = m1_pmu_map_event;
> -	cpu_pmu->set_event_filter = m1_pmu_set_event_filter;
> -
> -	cpu_pmu->map_pmuv3_event  = m1_pmu_map_pmuv3_event;
> -
> -	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = &m1_pmu_events_attr_group;
> -	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = &m1_pmu_format_attr_group;
> -	return apple_pmu_init(cpu_pmu);
> -}

[ ... ]

> +#define APPLE_PMU_INIT(gen, family)								\
> +static int gen ## _pmu_ ## family ## _init(struct arm_pmu *cpu_pmu)				\
> +{												\
> +	cpu_pmu->name			= "apple_" #family "_pmu";				\
> +	cpu_pmu->enable			= gen ## _pmu_enable_event;				\
> +	cpu_pmu->get_event_idx		= gen ## _pmu_get_event_idx;				\
> +	cpu_pmu->map_event		= gen ## _pmu_map_event;				\
> +	cpu_pmu->set_event_filter	= gen ## _pmu_set_event_filter;				\
> +												\
> +	cpu_pmu->map_pmuv3_event	= gen ## _pmu_map_pmuv3_event;				\
> +												\
> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = &gen ## _pmu_events_attr_group;	\
> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = &gen ## _pmu_format_attr_group;	\
> +	return apple_pmu_init(cpu_pmu);								\
> +}

[Severity: Low]
This isn't a bug, but APPLE_PMU_INIT(gen, family) expands all seven callback
and attr_groups assignments into every per-family init function (such as
m1_pmu_icestorm_init() and m1_pmu_firestorm_init()), even though core
families within the same generation only differ in cpu_pmu->name.

Because APPLE_PMU_INIT() unconditionally token-pastes gen ## _pmu_* for
every field, any generation that shares callbacks or attribute groups with
an earlier generation also requires a block of #define aliases to satisfy
token pasting (including in subsequent patches for m4 and a18).

Would it be cleaner to follow the pattern in drivers/perf/arm_pmuv3.c
(PMUV3_INIT_SIMPLE calling armv8_pmu_init), or have per-generation init
helpers called by a small per-family wrapper that only sets cpu_pmu->name,
to avoid both the #define alias blocks and the repeated per-family
assignments?

> +
> +#define m2_pmu_enable_event		m1_pmu_enable_event
> +#define m2_pmu_get_event_idx		m1_pmu_get_event_idx
> +#define m2_pmu_set_event_filter		m1_pmu_set_event_filter
> +#define m2_pmu_map_pmuv3_event		m1_pmu_map_pmuv3_event
> +#define m2_pmu_init_pmceid		m1_pmu_init_pmceid

[Severity: Low]
Is m2_pmu_init_pmceid needed here?

Earlier in the series ("drivers/perf: apple_m1: Support per-implementation
event tables"), m1_pmu_init_pmceid() was replaced by apple_pmu_init_pmceid(),
which is called directly from apple_pmu_init():

static int apple_pmu_init(struct arm_pmu *cpu_pmu)
{
	...
	apple_pmu_init_pmceid(cpu_pmu);

	bitmap_set(cpu_pmu->cntr_mask, 0, M1_PMU_NR_COUNTERS);
	return 0;
}

APPLE_PMU_INIT() does not reference gen ## _pmu_init_pmceid, so
m2_pmu_init_pmceid (and a18_pmu_init_pmceid in "drivers/perf: apple_m1: Add
Apple A18 Support") is unused and aliases a non-existent symbol. Can this
macro definition be dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-m3-plus-pmu-events-v1-0-a8eb6c1139fe@gmail.com?part=5