Re: [PATCH] driver: perf: arm_pmu: Drop some unused arguments from armv8_pmu_init()

[Anshuman Khandual]

Hi James,

On 10/9/23 14:47, James Clark wrote:
> 
> 
> On 09/10/2023 04:56, Anshuman Khandual wrote:
>> There is just a single call site remaining for armv8_pmu_init(), passing on
>> NULL pointers for all custom 'struct attribute_group'. These arguments are
>> not really getting used and hence can just be dropped off, thus simplifying
>> the code further.
>>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> This applies on v6.6-rc5.
>>
>>  drivers/perf/arm_pmuv3.c | 17 +++++------------
>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 8fcaa26f0f8a..fe4db1831662 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1187,10 +1187,7 @@ static void armv8_pmu_register_sysctl_table(void)
>>  }
>>  
>>  static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>> -			  int (*map_event)(struct perf_event *event),
>> -			  const struct attribute_group *events,
>> -			  const struct attribute_group *format,
>> -			  const struct attribute_group *caps)
>> +			  int (*map_event)(struct perf_event *event))
>>  {
>>  	int ret = armv8pmu_probe_pmu(cpu_pmu);
>>  	if (ret)
>> @@ -1212,13 +1209,9 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>>  
>>  	cpu_pmu->name			= name;
>>  	cpu_pmu->map_event		= map_event;
>> -	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = events ?
>> -			events : &armv8_pmuv3_events_attr_group;
>> -	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = format ?
>> -			format : &armv8_pmuv3_format_attr_group;
>> -	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = caps ?
>> -			caps : &armv8_pmuv3_caps_attr_group;
>> -
>> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = &armv8_pmuv3_events_attr_group;
>> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] = &armv8_pmuv3_format_attr_group;
>> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = &armv8_pmuv3_caps_attr_group;
>>  	armv8_pmu_register_sysctl_table();
>>  	return 0;
>>  }
>> @@ -1226,7 +1219,7 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>>  static int armv8_pmu_init_nogroups(struct arm_pmu *cpu_pmu, char *name,
>>  				   int (*map_event)(struct perf_event *event))
>>  {
>> -	return armv8_pmu_init(cpu_pmu, name, map_event, NULL, NULL, NULL);
>> +	return armv8_pmu_init(cpu_pmu, name, map_event);
> 
> I think the whole point of the nogroups wrapper was to add the NULLs. If
> you remove them, then you can remove the nogroups function too and just
> call armv8_pmu_init() directly instead.

Sounds reasonable. Will change all the relevant functions to just call
armv8_pmu_init() directly.

> 
> And as it wasn't clear why they were there in the first place, I went to
> look and found this (e424b17) :
> 
>   Although nobody uses non-default sysfs attributes today, there's
>   minimal impact to preserving the notion that maybe, some day, somebody
>   might, so we may as well keep up appearances.
> 
> It might be worth mentioning that the decision has now been made in the
> other way.

Sure, will update the commit message to include this.

> 
> 
>>  }
>>  
>>  #define PMUV3_INIT_SIMPLE(name)						\