Re: [PATCH RFC 1/4] driver/perf: Add identifier sysfs file for CMN

[Robin Murphy]

On 2023-03-29 12:53, Jing Zhang wrote:
> 在 2023/3/27 下午3:55, 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@xxxxxxxxxxxxxxxxx>
> > > ---
> > >    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.
> Will do.
>
> > BTW, does this HW have some HW identifier register, like iidr? I think that using that may be preferable.
>
> I didn't find the relevant identifier register.
>
> Do Illka and Robin know that there is such a register that can identify different CMN versions? Looking forward to your suggestions.

In principle the "part number" fields from CFGM_PERIPH_ID_0/1 are 
supposed to identify the model, but for various reasons I'm suspicious 
of that being unreliable (not least that no actual values are 
documented, only "configuration-dependent"). That's why I went down the 
route of making sure we have explicit ACPI/DT identifiers for every model.

However, the model alone seems either too specific or not specific 
enough for a jevents identifier. The defined metrics are pretty trivial 
and should have no real reason not to be common to *any* CMN PMU where 
the underlying events are present. On the other hand, if we want to get 
down to the level of specific events in JSON then we'd need to consider 
the revision as well, since there are several events which only exist on 
certain revisions of a given model (but often are also common to later 
models).

This actually foreshadows a question I was planning to bring up in the 
context of another driver I'm working on - for this one I would rather 
like to try using jevents rather than have to maintain another sprawl of 
event tables in a driver, but it's still going to have the same thing of 
wanting model/revision matching along the lines of what 
arm_cmn_event_attr_is_visible() is doing for CMN events. AFAICS this 
would need jevents to grow a rather more flexible way of encoding and 
matching identifiers, since having dozens of almost-identical copies of 
event definitions for every exact identifier value is clearly 
unworkable. Does anyone happen to have any thoughts or preferences 
around how that might be approached?

> > > +    }
> > > +    else if (cmn->model == CMN650) {
> > > +        return sysfs_emit(buf, "%s\n", "CMN650");
> >
> > I'd use lowercase names
> Ok.
>
> > > +    }
> > > +    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?

No need - it will never be unknown unless someone goes out of their way 
to break the probing code and/or match_data.

> >
>
> Ok.
>
> > > +}
> > > +
> > > +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;
> > > +};

As above, "cmn->model <= 0" can never be true.

Thanks,
Robin.

> > > +
> > > +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
>
> Ok, Will do.
>
> > > +};
> > > +
> > > +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
> > >    };
> > >