Re: [PATCH 2/4] perf/arm-cmn: Add CMN-650 support

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

On 2022-04-20 00:05, Ilkka Koskinen wrote:
> 
> Hi Robin,
> 
> I need to go through your patches more carefully, but I do have a couple 
> of comments already:

Thanks for having a look!

> On Mon, 18 Apr 2022, Robin Murphy wrote:
>> Add the identifiers and events for CMN-650, which slots into its
>> evolutionary position between CMN-600 and the 700-series products.
>> Imagine CMN-600 made bigger, and with most of the rough edges smoothed
>> off, but that then balanced out by some bonkers PMU functionality for
>> the new HN-P enhancement in CMN-650r2.
>>
>> Most of the CXG events are actually common to newer revisions of CMN-600
>> too, so they're arguably a little late; oh well.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/perf/arm-cmn.c | 222 ++++++++++++++++++++++++++++++++---------
>> 1 file changed, 176 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index 9c1d82be7a2f..cce8516d465c 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -39,7 +39,7 @@
>> #define CMN_CHILD_NODE_ADDR        GENMASK(27, 0)
>> #define CMN_CHILD_NODE_EXTERNAL        BIT(31)
>>
>> -#define CMN_MAX_DIMENSION        8
>> +#define CMN_MAX_DIMENSION        12
> 
> I wonder if it made sense to dynamically allocate the arrays later in 
> the code instead of allocating them in stack, especially if mesh 
> topologies keeps growing fast. That would probably avoid setting max 
> dimension altogether if one could use num_xps, num_dns etc. Just for 
> future thoughts...

Note that the group validation structure *is* dynamically allocated 
since the last update, since it was already getting a bit big for the 
stack; it's just not dynamically *sized*. That's a compromise to keep 
the validation code as simple and efficient as it reasonably can be. I'm 
not entirely convinced that extra complexity and runtime overhead for 
everyone is worth it for the sake of making it slightly easier to catch 
an obvious bug if someone makes out-of-tree hacks to the driver. This is 
not the only place which needs updating (or at least checking) if the 
maximum number of possible DTMs really does increase again.

>> #define CMN_MAX_XPS            (CMN_MAX_DIMENSION * CMN_MAX_DIMENSION)
>> #define CMN_MAX_DTMS            (CMN_MAX_XPS + (CMN_MAX_DIMENSION - 1) 
>> * 4)
> 
> <snip>
> 
>> @@ -1692,8 +1802,13 @@ static int arm_cmn_discover(struct arm_cmn 
>> *cmn, unsigned int rgn_offset)
>>         cmn->num_dns += FIELD_GET(CMN_CI_CHILD_COUNT, reg);
> 
> How about checking if child count is bigger than the maximum mesh size 
> before this for loop? It would help in case someone would work on 
> enabling support for new, bigger models and would forget to update 
> CMN_MAX_DIMENSION...

Hmm, I guess we do already warn and bail out if we find a mystery node 
type that implies we're being lied to, but TBH that was always more to 
avoid compilers moaning about the switch statement lacking a default 
case than because I thought it's a necessary check in itself. Ultimately 
if someone lies to the driver and claims that a CMN product is a 
different CMN product, it's already not going to work properly due to 
the subtle differences in the hardware, so I'd argue that potential 
memory corruption due to overrunning array bounds in various places is 
really just part and parcel of "not working properly".

CMN-700 r0 and r2 seemed clear that 12x12 is the largest supported 
dimension; r1 seemed a bit ambiguous between what the TRM said and what 
I could find in the actual product deliverables, so I can double-check 
with the hardware team if you like - or if you already know better, 
please do feel free to correct my assumption :)

>>     }
>>
>> -    /* Cheeky +1 to help terminate pointer-based iteration later */
>> -    dn = devm_kcalloc(cmn->dev, cmn->num_dns + 1, sizeof(*dn), 
>> GFP_KERNEL);
>> +    /*
>> +     * Some nodes effectively have two separate types, which we'll 
>> handle
>> +     * by creating one of each internally. For a (very) safe initial 
>> upper
>> +     * bound, account for double the number of non-XP nodes.
>> +     */
>> +    dn = devm_kcalloc(cmn->dev, cmn->num_dns * 2 - cmn->num_xps,
>> +              sizeof(*dn), GFP_KERNEL);
>>     if (!dn)
>>         return -ENOMEM;
>>
> 
> <snip>
> 
>> @@ -1970,6 +2098,7 @@ static int arm_cmn_remove(struct platform_device 
>> *pdev)
>> #ifdef CONFIG_OF
>> static const struct of_device_id arm_cmn_of_match[] = {
>>     { .compatible = "arm,cmn-600", .data = (void *)CMN600 },
>> +    { .compatible = "arm,cmn-650", .data = (void *)CMN650 },
>>     { .compatible = "arm,ci-700", .data = (void *)CI700 },
>>     {}
>> };
>> @@ -1979,6 +2108,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
>> #ifdef CONFIG_ACPI
>> static const struct acpi_device_id arm_cmn_acpi_match[] = {
>>     { "ARMHC600", CMN600 },
>> +    { "ARMHC650", CMN650 },
> 
> Not the great place for this comment but there probably isn't any better.
> 
> Based on DEN0093 v1.1, CMN's DSDT entries have been changed since 
> CMN-600. ROOTNODEBASE seems to be specific for CMN-600. Moreover, if you 
> compare the address maps in TRMs' Discovery chapters, you can see the 
> difference.
> 
> I'm thinking, at the minimal the second platform_get_resource() call has 
> to be skipped and zero returned in arm_cmn600_acpi_probe(), if the model 
> is cmn650 (probably also for cmn-700)

As you've already found, things prefixed with "arm_cmn600_" vs. 
"arm_cmn_" *are* specific to CMN-600, and the CI-700 update reworked 
this area such that everything else simply probes in a firmware-agnostic 
manner. It may have been a bit subtle since CI-700 doesn't have an ACPI 
binding, but it was very much intended to cover these future ACPI 
additions as well.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel