Re: [PATCH 2/5] perf/arm_cspmu: Simplify attribute groups

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

On Tue, 5 Dec 2023, Robin Murphy wrote:
> The attribute group array itself is always the same, so there's no
> need to allocate it separately. Storing it directly in our instance
> data saves memory and gives us one less point of failure.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Cheers, Ilkka


> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 26 +++++++++-----------------
> drivers/perf/arm_cspmu/arm_cspmu.h |  1 +
> 2 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index a3347b1287e6..f7aa2ac5fd88 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -501,23 +501,16 @@ arm_cspmu_alloc_format_attr_group(struct arm_cspmu *cspmu)
> 	return format_group;
> }
>
> -static struct attribute_group **
> -arm_cspmu_alloc_attr_group(struct arm_cspmu *cspmu)
> +static int arm_cspmu_alloc_attr_groups(struct arm_cspmu *cspmu)
> {
> -	struct attribute_group **attr_groups = NULL;
> -	struct device *dev = cspmu->dev;
> +	const struct attribute_group **attr_groups = cspmu->attr_groups;
> 	const struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
>
> 	cspmu->identifier = impl_ops->get_identifier(cspmu);
> 	cspmu->name = impl_ops->get_name(cspmu);
>
> 	if (!cspmu->identifier || !cspmu->name)
> -		return NULL;
> -
> -	attr_groups = devm_kcalloc(dev, 5, sizeof(struct attribute_group *),
> -				   GFP_KERNEL);
> -	if (!attr_groups)
> -		return NULL;
> +		return -ENOMEM;
>
> 	attr_groups[0] = arm_cspmu_alloc_event_attr_group(cspmu);
> 	attr_groups[1] = arm_cspmu_alloc_format_attr_group(cspmu);
> @@ -525,9 +518,9 @@ arm_cspmu_alloc_attr_group(struct arm_cspmu *cspmu)
> 	attr_groups[3] = &arm_cspmu_cpumask_attr_group;
>
> 	if (!attr_groups[0] || !attr_groups[1])
> -		return NULL;
> +		return -ENOMEM;
>
> -	return attr_groups;
> +	return 0;
> }
>
> static inline void arm_cspmu_reset_counters(struct arm_cspmu *cspmu)
> @@ -1166,11 +1159,10 @@ static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
> static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
> {
> 	int ret, capabilities;
> -	struct attribute_group **attr_groups;
>
> -	attr_groups = arm_cspmu_alloc_attr_group(cspmu);
> -	if (!attr_groups)
> -		return -ENOMEM;
> +	ret = arm_cspmu_alloc_attr_groups(cspmu);
> +	if (ret)
> +		return ret;
>
> 	ret = cpuhp_state_add_instance(arm_cspmu_cpuhp_state,
> 				       &cspmu->cpuhp_node);
> @@ -1192,7 +1184,7 @@ static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
> 		.start		= arm_cspmu_start,
> 		.stop		= arm_cspmu_stop,
> 		.read		= arm_cspmu_read,
> -		.attr_groups	= (const struct attribute_group **)attr_groups,
> +		.attr_groups	= cspmu->attr_groups,
> 		.capabilities	= capabilities,
> 	};
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index 2fe723555a6b..c9163acfe810 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -157,6 +157,7 @@ struct arm_cspmu {
> 	int cycle_counter_logical_idx;
>
> 	struct arm_cspmu_hw_events hw_events;
> +	const struct attribute_group *attr_groups[5];
>
> 	struct arm_cspmu_impl impl;
> };
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>