Message-ID: <6bb7eaee-2d00-46f5-89ff-95133aea9a5d@arm.com>
Date:   Thu, 4 May 2023 13:15:25 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     John Garry <john.g.garry@...cle.com>,
        Jing Zhang <renyu.zj@...ux.alibaba.com>,
        Ian Rogers <irogers@...gle.com>, Will Deacon <will@...nel.org>,
        Shuai Xue <xueshuai@...ux.alibaba.com>
Cc:     James Clark <james.clark@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Ilkka Koskinen <ilkka@...amperecomputing.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org,
        Zhuo Song <zhuo.song@...ux.alibaba.com>
Subject: Re: [PATCH v2 1/5] driver/perf: Add identifier sysfs file for CMN

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.

Powered by blists - more mailing lists