[PATCH v2 08/29] arm_mpam: Add the class and component structures for firmware described ris

[James Morse]

Hi Gavin,

On 07/10/2025 00:13, Gavin Shan wrote:
> On 9/11/25 6:42 AM, James Morse wrote:
>> An MSC is a container of resources, each identified by their RIS index.
>> Some RIS are described by firmware to provide their position in the system.
>> Others are discovered when the driver probes the hardware.
>>
>> To configure a resource it needs to be found by its class, e.g. 'L2'.
>> There are two kinds of grouping, a class is a set of components, which
>> are visible to user-space as there are likely to be multiple instances
>> of the L2 cache. (e.g. one per cluster or package)
>>
>> Add support for creating and destroying structures to allow a hierarchy
>> of resources to be created.

>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index efc4738e3b4d..c7f4981b3545 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c

>> +static struct mpam_class *
>> +mpam_class_get(u8 level_idx, enum mpam_class_types type)
>> +{
>> +    bool found = false;
>> +    struct mpam_class *class;
>> +
>> +    lockdep_assert_held(&mpam_list_lock);
>> +
>> +    list_for_each_entry(class, &mpam_classes, classes_list) {
>> +        if (class->type == type && class->level == level_idx) {
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (found)
>> +        return class;
>> +
>> +    return mpam_class_alloc(level_idx, type);
>> +}
>> +
> 
> The variable @found can be avoided if the found class can be returned immediately.
> 
>     list_for_each_entry(class, &mpam_classes, classes_list) {
>         if (class->type == type && class->level == level_idx)
>             return class;
>     }
> 
>     return mpam_class_alloc(level_idx, type);

Yes, feedback like this already came from Jonathan.


>> +static int mpam_ris_get_affinity(struct mpam_msc *msc, cpumask_t *affinity,
>> +                 enum mpam_class_types type,
>> +                 struct mpam_class *class,
>> +                 struct mpam_component *comp)
>> +{
>> +    int err;
>> +
>> +    switch (type) {
>> +    case MPAM_CLASS_CACHE:
>> +        err = mpam_get_cpumask_from_cache_id(comp->comp_id, class->level,
>> +                             affinity);
>> +        if (err)
>> +            return err;
>> +
>> +        if (cpumask_empty(affinity))
>> +            pr_warn_once("%s no CPUs associated with cache node",
>> +                     dev_name(&msc->pdev->dev));
>> +
>> +        break;
> 
> "\n" missed in the error message and dev_warn_once() can be used:
> 
>         if (cpumask_empty(affinity))
>             dev_warn_once(&msc->pdev->dev, "No CPUs associated with cache node\n");

Yup, I'd mopped up most of these but missed this one.


>> +    case MPAM_CLASS_MEMORY:
>> +        get_cpumask_from_node_id(comp->comp_id, affinity);
>> +        /* affinity may be empty for CPU-less memory nodes */
>> +        break;
>> +    case MPAM_CLASS_UNKNOWN:
>> +        return 0;
>> +    }
>> +
>> +    cpumask_and(affinity, affinity, &msc->accessibility);
>> +
>> +    return 0;
>> +}


Thanks,

James