Re: [PATCH v2 1/5] driver/perf: Add identifier sysfs file for CMN

[Robin Murphy <robin.murphy@arm.com>]

On 2023-05-01 13:38, John Garry wrote:
> On 24/04/2023 10:44, Jing Zhang wrote:
>> To allow userspace to identify the specific implementation of the device,
>> add an "identifier" sysfs file.
>>
>> The perf tool can match the arm CMN metric through the identifier.
>>
>> Signed-off-by: Jing Zhang<renyu.zj@linux.alibaba.com>
>> ---
>>   drivers/perf/arm-cmn.c | 49 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index c968986..f425610 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -334,6 +334,7 @@ struct arm_cmn {
>>       struct pmu pmu;
>>       struct dentry *debug;
>> +    const char *identifier;
>>   };
>>   #define to_cmn(p)    container_of(p, struct arm_cmn, pmu)
>> @@ -1168,10 +1169,41 @@ static ssize_t arm_cmn_cpumask_show(struct 
>> device *dev,
>>       .attrs = arm_cmn_cpumask_attrs,
>>   };
>> +static ssize_t arm_cmn_identifier_show(struct device *dev,
>> +        struct device_attribute *attr, char *buf)
>> +{
>> +    struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
>> +
>> +    return sysfs_emit(buf, "%s\n", cmn->identifier);
>> +}
>> +
>> +static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj,
>> +        struct attribute *attr, int n)
>> +{
>> +    struct device *dev = kobj_to_dev(kobj);
>> +    struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
>> +
>> +    if (cmn->identifier == NULL)
>> +        return 0;
> 
> nit: generally if (!val) is preferred

Although either way it can only be NULL in cases of memory corruption or 
developers making broken changes to the driver, neither of which are 
worth pretending to defend against.

>> +    return attr->mode;
>> +};
>> +
>> +static struct device_attribute arm_cmn_identifier_attr =
>> +__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
>> +
>> +static struct attribute *arm_cmn_identifier_attrs[] = {
>> +    &arm_cmn_identifier_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group arm_cmn_identifier_attr_group = {
>> +    .attrs = arm_cmn_identifier_attrs,
>> +    .is_visible = arm_cmn_identifier_attr_visible
>> +};
>> +
>>   static const struct attribute_group *arm_cmn_attr_groups[] = {
>>       &arm_cmn_event_attrs_group,
>>       &arm_cmn_format_attrs_group,
>>       &arm_cmn_cpumask_attr_group,
>> +    &arm_cmn_identifier_attr_group,
>>       NULL
>>   };
>> @@ -2241,6 +2273,22 @@ static int arm_cmn600_of_probe(struct 
>> device_node *np)
>>       return of_property_read_u32(np, "arm,root-node", &rootnode) ?: 
>> rootnode;
>>   }
>> +const char *arm_cmn_identifier(unsigned long model)
>> +{
>> +    switch (model) {
>> +    case CMN600:
>> +        return "cmn600";
>> +    case CMN650:
>> +        return "cmn650";
>> +    case CMN700:
>> +        return "cmn700";
>> +    case CI700:
>> +        return "ci700";
>> +    default:
>> +        return NULL;
>> +    }
> 
> nit: I think that it would be nicer to have this per-model string stored 
> statically in arm_cmn_acpi_match[].driver_data and 
> arm_cmn_of_match[].data, so we have a straight lookup

Again, I'm not really convinced how useful this coarse per-model scheme 
is - for instance, in terms of many events, CMN-600 r3 is closer to 
CMN-650 than it is to CMN-600 r1, so what exactly would "CMN-600" mean 
to the user?

Thanks,
Robin.

>> +}
>> +
>>   static int arm_cmn_probe(struct platform_device *pdev)
>>   {
>>       struct arm_cmn *cmn;
>> @@ -2254,6 +2302,7 @@ static int arm_cmn_probe(struct platform_device 
>> *pdev)
>>       cmn->dev = &pdev->dev;
>>       cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>> +    cmn->identifier = arm_cmn_identifier(cmn->model);
>>       platform_set_drvdata(pdev, cmn);
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel