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;
+ 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;
+ }
+}
+
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);
if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
--
1.8.3.1On 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
> + 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
> +}
> +
> 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);
>
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
On 04/05/2023 10:47, Robin Murphy wrote:
>> nit: generally if (!val) is preferred
Hi Robin,
>
> 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.
If there was some broken code for setting this identifier, then we would
just not show the identifier file, rather than show it containing "NULL"
- that seems better. However, there may be other implications from that
same broken code, so you maintainers and contributors please decide.
>
>>> + 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?
ok, I see, that's what I was asking about in the cmn-700 JSON review;
and from what you say, it is not the case that we always have the same
events for every revision. So we need a more fine-grained identifier.
For DT support, I suppose per-revision compat strings could be added,
but I would not be sure what to do about ACPI.
BTW, My comment was more about coding style of case a, case b, case c,
... case z, does not scale well.
Thanks,
John
On 2023-05-04 12:02, John Garry wrote:
> On 04/05/2023 10:47, Robin Murphy wrote:
>>> nit: generally if (!val) is preferred
>
> Hi Robin,
>
>>
>> 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.
>
> If there was some broken code for setting this identifier, then we would
> just not show the identifier file, rather than show it containing "NULL"
> - that seems better. However, there may be other implications from that
> same broken code, so you maintainers and contributors please decide.
Yeah, from the usage point of view, if there should be an identifier at
all then there should always be one, so it makes little sense to pretend
to accommodate a case where there wouldn't be. And it would be trivially
obvious to see in review if someone adds a new model enum without any
necessary identifier updates at the same time (far more so than spotting
whether all the subtle functional differences of the new model have been
accounted for).
>>>> + 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?
>
> ok, I see, that's what I was asking about in the cmn-700 JSON review;
> and from what you say, it is not the case that we always have the same
> events for every revision. So we need a more fine-grained identifier.
Yes, it's mostly just a case of new events getting added as the
microarchitecture evolves over the product's lifetime, but there has
been at least one event ID which had a meaning in very early versions of
CMN-600, was subsequently removed, and then got reused for a *different*
event a couple of revisions after that. Thankfully I believe those
earliest versions only ever existed on FPGA internally, and the TRMs
were never made public, so upstream doesn't care about that specific case.
> For DT support, I suppose per-revision compat strings could be added,
> but I would not be sure what to do about ACPI.
We know the version from the ID registers, that's no problem - it's
already used to manage visibility of the sysfs event aliases. In
principle we *should* have a model code in CFGM_PERIPH_ID_0 as well, and
be able to compose an identifier exactly the same way as
smmu_pmu_get_iidr() does in the SMMUv3 PMCG driver, but as I mentioned
before I'm not entirely confident in the implementation: I do happen to
know what codes have been nominally assigned for each product, but the
TRMs claim otherwise :(
> BTW, My comment was more about coding style of case a, case b, case c,
> ... case z, does not scale well.
Indeed, it's probably the nature of the switch statement that leads to
the perceived need for a not-practically-meaningful "default" case in
the first place.
Thanks,
Robin.
On 04/05/2023 13:15, Robin Murphy wrote: >> >> ok, I see, that's what I was asking about in the cmn-700 JSON review; >> and from what you say, it is not the case that we always have the same >> events for every revision. So we need a more fine-grained identifier. > > Yes, it's mostly just a case of new events getting added as the > microarchitecture evolves over the product's lifetime, I might have mentioned this before: Currently userspace matches both the PMU sysfs name and this HW identifier for creating these event aliases. For fixed events - like those non-IMPDEF ones on the PMCG - userspace could just assume those standard ones are implemented and not need to check the HW identifier. Maybe this will help with scalability of JSONs for CMN PMU implementations. Or even HW identifier wildcard matching. However, as you might anticipate, we may need to add some sort of userspace support for times when HW implementations get messed up... > but there has > been at least one event ID which had a meaning in very early versions of > CMN-600, was subsequently removed, and then got reused for a *different* > event a couple of revisions after that. > Thankfully I believe those > earliest versions only ever existed on FPGA internally, and the TRMs > were never made public, so upstream doesn't care about that specific case. ok, good > >> For DT support, I suppose per-revision compat strings could be added, >> but I would not be sure what to do about ACPI. > > We know the version from the ID registers, that's no problem - it's > already used to manage visibility of the sysfs event aliases. In > principle we *should* have a model code in CFGM_PERIPH_ID_0 as well, and > be able to compose an identifier exactly the same way as > smmu_pmu_get_iidr() does in the SMMUv3 PMCG driver, but as I mentioned > before I'm not entirely confident in the implementation: I do happen to > know what codes have been nominally assigned for each product, but the > TRMs claim otherwise 🙁 Thanks, John
在 2023/5/5 下午8:24, John Garry 写道: > On 04/05/2023 13:15, Robin Murphy wrote: >>> >>> ok, I see, that's what I was asking about in the cmn-700 JSON review; and from what you say, it is not the case that we always have the same events for every revision. So we need a more fine-grained identifier. >> >> Yes, it's mostly just a case of new events getting added as the microarchitecture evolves over the product's lifetime, > > I might have mentioned this before: Currently userspace matches both the PMU sysfs name and this HW identifier for creating these event aliases. For fixed events - like those non-IMPDEF ones on the PMCG - userspace could just assume those standard ones are implemented and not need to check the HW identifier. Maybe this will help with scalability of JSONs for CMN PMU implementations. Or even HW identifier wildcard matching. However, as you might anticipate, we may need to add some sort of userspace support for times when HW implementations get messed up... > >> but there has been at least one event ID which had a meaning in very early versions of CMN-600, was subsequently removed, and then got reused for a *different* event a couple of revisions after that. >> Thankfully I believe those earliest versions only ever existed on FPGA internally, and the TRMs were never made public, so upstream doesn't care about that specific case. > > ok, good > Thanks John and Robin, perhaps using CMN's model and revision to form an identifier is currently the most feasible solution? For example: CMN600_R1P0, CMN600_R1P1. The revision of CMN can be obtained from the revision register. >> >>> For DT support, I suppose per-revision compat strings could be added, but I would not be sure what to do about ACPI. >> Yes, I also have this doubt. On DT, char* can be passed in through the data field because it is of void* type, but the driver_data field of ACPI is ulong. In addition, the revision is read out through the register, and I cannot get the revision in arm_cmn_acpi/of_match[]. We still need to compose the identifier in the probe function. So, I wonder if it is better to use an additional function to form the identifier instead of passing the identifier through arm_cmn_acpi/of_match[]? >> We know the version from the ID registers, that's no problem - it's already used to manage visibility of the sysfs event aliases. In principle we *should* have a model code in CFGM_PERIPH_ID_0 as well, and be able to compose an identifier exactly the same way as smmu_pmu_get_iidr() does in the SMMUv3 PMCG driver, but as I mentioned before I'm not entirely confident in the implementation: I do happen to know what codes have been nominally assigned for each product, but the TRMs claim otherwise 🙁 > > Thanks, > John Thanks, Jing
© 2016 - 2026 Red Hat, Inc.