Messages in this thread | | | Date | Mon, 27 Mar 2023 08:55:07 +0100 | Subject | Re: [PATCH RFC 1/4] driver/perf: Add identifier sysfs file for CMN | From | John Garry <> |
| |
On 27/03/2023 03:46, 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 | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index c968986..0c138ad 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1168,10 +1168,53 @@ 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)); > + if (cmn->model == CMN700) { > + return sysfs_emit(buf, "%s\n", "CMN700");
Is it possible to have a pointer to this string in struct arm_cmn, such that we don't have to do this model to identifier lookup here? If-else chains like this are not scalable.
BTW, does this HW have some HW identifier register, like iidr? I think that using that may be preferable.
> + } > + else if (cmn->model == CMN650) { > + return sysfs_emit(buf, "%s\n", "CMN650");
I'd use lowercase names
> + } > + else if (cmn->model == CMN600) { > + return sysfs_emit(buf, "%s\n", "CMN600"); > + } > + else if (cmn->model == CI700) { > + return sysfs_emit(buf, "%s\n", "CI700"); > + } > + return sysfs_emit(buf, "%s\n", "UNKNOWN");
can we have a "is_visble" attr to just no show this when unknown?
> +} > + > +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->model <= 0) > + return 0; > + 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,
nit: no need for trailing ',' on a sentinel
> +}; > + > +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 > }; >
| |