To make a decision about whether to expose an mpam class as
a resctrl resource we need to know its overall supported
features and properties.

Once we've probed all the resources, we can walk the tree
and produce overall values by merging the bitmaps. This
eliminates features that are only supported by some MSC
that make up a component or class.

If bitmap properties are mismatched within a component we
cannot support the mismatched feature.

Care has to be taken as vMSC may hold mismatched RIS.

Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Ben Horgan <ben.horgan@arm.com>
---
 drivers/resctrl/mpam_devices.c  | 215 ++++++++++++++++++++++++++++++++
 drivers/resctrl/mpam_internal.h |   8 ++
 2 files changed, 223 insertions(+)

diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index ba8e8839cdc4..cd8e95fa5fd6 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -961,8 +961,223 @@ static struct platform_driver mpam_msc_driver = {
 	.remove = mpam_msc_drv_remove,
 };
 
+/* Any of these features mean the BWA_WD field is valid. */
+static bool mpam_has_bwa_wd_feature(struct mpam_props *props)
+{
+	if (mpam_has_feature(mpam_feat_mbw_min, props))
+		return true;
+	if (mpam_has_feature(mpam_feat_mbw_max, props))
+		return true;
+	if (mpam_has_feature(mpam_feat_mbw_prop, props))
+		return true;
+	return false;
+}
+
+#define MISMATCHED_HELPER(parent, child, helper, field, alias)		\
+	helper(parent) &&						\
+	((helper(child) && (parent)->field != (child)->field) ||	\
+	 (!helper(child) && !(alias)))
+
+#define MISMATCHED_FEAT(parent, child, feat, field, alias)		     \
+	mpam_has_feature((feat), (parent)) &&				     \
+	((mpam_has_feature((feat), (child)) && (parent)->field != (child)->field) || \
+	 (!mpam_has_feature((feat), (child)) && !(alias)))
+
+#define CAN_MERGE_FEAT(parent, child, feat, alias)			\
+	(alias) && !mpam_has_feature((feat), (parent)) &&		\
+	mpam_has_feature((feat), (child))
+
+/*
+ * Combine two props fields.
+ * If this is for controls that alias the same resource, it is safe to just
+ * copy the values over. If two aliasing controls implement the same scheme
+ * a safe value must be picked.
+ * For non-aliasing controls, these control different resources, and the
+ * resulting safe value must be compatible with both. When merging values in
+ * the tree, all the aliasing resources must be handled first.
+ * On mismatch, parent is modified.
+ */
+static void __props_mismatch(struct mpam_props *parent,
+			     struct mpam_props *child, bool alias)
+{
+	if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) {
+		parent->cpbm_wd = child->cpbm_wd;
+	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part,
+				   cpbm_wd, alias)) {
+		pr_debug("%s cleared cpor_part\n", __func__);
+		mpam_clear_feature(mpam_feat_cpor_part, &parent->features);
+		parent->cpbm_wd = 0;
+	}
+
+	if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) {
+		parent->mbw_pbm_bits = child->mbw_pbm_bits;
+	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part,
+				   mbw_pbm_bits, alias)) {
+		pr_debug("%s cleared mbw_part\n", __func__);
+		mpam_clear_feature(mpam_feat_mbw_part, &parent->features);
+		parent->mbw_pbm_bits = 0;
+	}
+
+	/* bwa_wd is a count of bits, fewer bits means less precision */
+	if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) {
+		parent->bwa_wd = child->bwa_wd;
+	} else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature,
+				     bwa_wd, alias)) {
+		pr_debug("%s took the min bwa_wd\n", __func__);
+		parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
+	}
+
+	/* For num properties, take the minimum */
+	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
+		parent->num_csu_mon = child->num_csu_mon;
+	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu,
+				   num_csu_mon, alias)) {
+		pr_debug("%s took the min num_csu_mon\n", __func__);
+		parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon);
+	}
+
+	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) {
+		parent->num_mbwu_mon = child->num_mbwu_mon;
+	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu,
+				   num_mbwu_mon, alias)) {
+		pr_debug("%s took the min num_mbwu_mon\n", __func__);
+		parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
+	}
+
+	if (alias) {
+		/* Merge features for aliased resources */
+		parent->features |= child->features;
+	} else {
+		/* Clear missing features for non aliasing */
+		parent->features &= child->features;
+	}
+}
+
+/*
+ * If a vmsc doesn't match class feature/configuration, do the right thing(tm).
+ * For 'num' properties we can just take the minimum.
+ * For properties where the mismatched unused bits would make a difference, we
+ * nobble the class feature, as we can't configure all the resources.
+ * e.g. The L3 cache is composed of two resources with 13 and 17 portion
+ * bitmaps respectively.
+ */
+static void
+__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc)
+{
+	struct mpam_props *cprops = &class->props;
+	struct mpam_props *vprops = &vmsc->props;
+
+	lockdep_assert_held(&mpam_list_lock); /* we modify class */
+
+	pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n",
+		 dev_name(&vmsc->msc->pdev->dev),
+		 (long)cprops->features, (long)vprops->features);
+
+	/* Take the safe value for any common features */
+	__props_mismatch(cprops, vprops, false);
+}
+
+static void
+__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris)
+{
+	struct mpam_props *rprops = &ris->props;
+	struct mpam_props *vprops = &vmsc->props;
+
+	lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */
+
+	pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n",
+		 dev_name(&vmsc->msc->pdev->dev),
+		 (long)vprops->features, (long)rprops->features);
+
+	/*
+	 * Merge mismatched features - Copy any features that aren't common,
+	 * but take the safe value for any common features.
+	 */
+	__props_mismatch(vprops, rprops, true);
+}
+
+/*
+ * Copy the first component's first vMSC's properties and features to the
+ * class. __class_props_mismatch() will remove conflicts.
+ * It is not possible to have a class with no components, or a component with
+ * no resources. The vMSC properties have already been built.
+ */
+static void mpam_enable_init_class_features(struct mpam_class *class)
+{
+	struct mpam_vmsc *vmsc;
+	struct mpam_component *comp;
+
+	comp = list_first_entry_or_null(&class->components,
+					struct mpam_component, class_list);
+	if (WARN_ON(!comp))
+		return;
+
+	vmsc = list_first_entry_or_null(&comp->vmsc,
+					struct mpam_vmsc, comp_list);
+	if (WARN_ON(!vmsc))
+		return;
+
+	class->props = vmsc->props;
+}
+
+static void mpam_enable_merge_vmsc_features(struct mpam_component *comp)
+{
+	struct mpam_vmsc *vmsc;
+	struct mpam_msc_ris *ris;
+	struct mpam_class *class = comp->class;
+
+	list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
+		list_for_each_entry(ris, &vmsc->ris, vmsc_list) {
+			__vmsc_props_mismatch(vmsc, ris);
+			class->nrdy_usec = max(class->nrdy_usec,
+					       vmsc->msc->nrdy_usec);
+		}
+	}
+}
+
+static void mpam_enable_merge_class_features(struct mpam_component *comp)
+{
+	struct mpam_vmsc *vmsc;
+	struct mpam_class *class = comp->class;
+
+	list_for_each_entry(vmsc, &comp->vmsc, comp_list)
+		__class_props_mismatch(class, vmsc);
+}
+
+/*
+ * Merge all the common resource features into class.
+ * vmsc features are bitwise-or'd together, this must be done first.
+ * Next the class features are the bitwise-and of all the vmsc features.
+ * Other features are the min/max as appropriate.
+ *
+ * To avoid walking the whole tree twice, the class->nrdy_usec property is
+ * updated when working with the vmsc as it is a max(), and doesn't need
+ * initialising first.
+ */
+static void mpam_enable_merge_features(struct list_head *all_classes_list)
+{
+	struct mpam_class *class;
+	struct mpam_component *comp;
+
+	lockdep_assert_held(&mpam_list_lock);
+
+	list_for_each_entry(class, all_classes_list, classes_list) {
+		list_for_each_entry(comp, &class->components, class_list)
+			mpam_enable_merge_vmsc_features(comp);
+
+		mpam_enable_init_class_features(class);
+
+		list_for_each_entry(comp, &class->components, class_list)
+			mpam_enable_merge_class_features(comp);
+	}
+}
+
 static void mpam_enable_once(void)
 {
+	mutex_lock(&mpam_list_lock);
+	mpam_enable_merge_features(&mpam_classes);
+	mutex_unlock(&mpam_list_lock);
+
 	/*
 	 * Once the cpuhp callbacks have been changed, mpam_partid_max can no
 	 * longer change.
diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
index 5ae5d4eee8ec..eace5ba871f3 100644
--- a/drivers/resctrl/mpam_internal.h
+++ b/drivers/resctrl/mpam_internal.h
@@ -161,12 +161,20 @@ static inline void mpam_set_feature(enum mpam_device_features feat,
 	props->features |= (1 << feat);
 }
 
+static inline void mpam_clear_feature(enum mpam_device_features feat,
+				      mpam_features_t *supported)
+{
+	*supported &= ~(1 << feat);
+}
+
 struct mpam_class {
 	/* mpam_components in this class */
 	struct list_head	components;
 
 	cpumask_t		affinity;
 
+	struct mpam_props	props;
+	u32			nrdy_usec;
 	u8			level;
 	enum mpam_class_types	type;
 
-- 
2.39.5

On Wed, 10 Sep 2025 20:42:54 +0000
James Morse <james.morse@arm.com> wrote:

> To make a decision about whether to expose an mpam class as
> a resctrl resource we need to know its overall supported
> features and properties.
> 
> Once we've probed all the resources, we can walk the tree
> and produce overall values by merging the bitmaps. This
> eliminates features that are only supported by some MSC
> that make up a component or class.
> 
> If bitmap properties are mismatched within a component we
> cannot support the mismatched feature.
> 
> Care has to be taken as vMSC may hold mismatched RIS.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Ben Horgan <ben.horgan@arm.com>

A trivial things inline.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> +/*
> + * Combine two props fields.
> + * If this is for controls that alias the same resource, it is safe to just
> + * copy the values over. If two aliasing controls implement the same scheme
> + * a safe value must be picked.
> + * For non-aliasing controls, these control different resources, and the
> + * resulting safe value must be compatible with both. When merging values in
> + * the tree, all the aliasing resources must be handled first.
> + * On mismatch, parent is modified.
> + */
> +static void __props_mismatch(struct mpam_props *parent,
> +			     struct mpam_props *child, bool alias)
> +{
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) {
> +		parent->cpbm_wd = child->cpbm_wd;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part,
> +				   cpbm_wd, alias)) {
> +		pr_debug("%s cleared cpor_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_cpor_part, &parent->features);
> +		parent->cpbm_wd = 0;
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) {
> +		parent->mbw_pbm_bits = child->mbw_pbm_bits;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part,
> +				   mbw_pbm_bits, alias)) {
> +		pr_debug("%s cleared mbw_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_mbw_part, &parent->features);
> +		parent->mbw_pbm_bits = 0;
> +	}
> +
> +	/* bwa_wd is a count of bits, fewer bits means less precision */
> +	if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) {

Seems like an overly long line given other local wrapping.

> +		parent->bwa_wd = child->bwa_wd;
> +	} else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature,
> +				     bwa_wd, alias)) {
> +		pr_debug("%s took the min bwa_wd\n", __func__);
> +		parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
> +	}
> +
> +	/* For num properties, take the minimum */
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
> +		parent->num_csu_mon = child->num_csu_mon;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu,
> +				   num_csu_mon, alias)) {
> +		pr_debug("%s took the min num_csu_mon\n", __func__);
> +		parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon);
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) {
> +		parent->num_mbwu_mon = child->num_mbwu_mon;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu,
> +				   num_mbwu_mon, alias)) {
> +		pr_debug("%s took the min num_mbwu_mon\n", __func__);
> +		parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
> +	}

> +
> +/*
> + * If a vmsc doesn't match class feature/configuration, do the right thing(tm).
> + * For 'num' properties we can just take the minimum.
> + * For properties where the mismatched unused bits would make a difference, we
> + * nobble the class feature, as we can't configure all the resources.
> + * e.g. The L3 cache is composed of two resources with 13 and 17 portion
> + * bitmaps respectively.
> + */
> +static void
> +__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc)

I'm not really sure what the __ prefix denotes here.

> +{
> +	struct mpam_props *cprops = &class->props;
> +	struct mpam_props *vprops = &vmsc->props;
> +
> +	lockdep_assert_held(&mpam_list_lock); /* we modify class */
> +
> +	pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n",
> +		 dev_name(&vmsc->msc->pdev->dev),
> +		 (long)cprops->features, (long)vprops->features);

According to https://docs.kernel.org/core-api/printk-formats.html
should be fine using %x for u16 values. So why dance through a cast to long?
> +
> +	/* Take the safe value for any common features */
> +	__props_mismatch(cprops, vprops, false);
> +}
> +
> +static void
> +__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris)
> +{
> +	struct mpam_props *rprops = &ris->props;
> +	struct mpam_props *vprops = &vmsc->props;
> +
> +	lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */
> +
> +	pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n",
> +		 dev_name(&vmsc->msc->pdev->dev),
> +		 (long)vprops->features, (long)rprops->features);

Same as above comment on casts being unnecessary.

> +
> +	/*
> +	 * Merge mismatched features - Copy any features that aren't common,
> +	 * but take the safe value for any common features.
> +	 */
> +	__props_mismatch(vprops, rprops, true);
> +}
> +
> +/*
> + * Copy the first component's first vMSC's properties and features to the
> + * class. __class_props_mismatch() will remove conflicts.
> + * It is not possible to have a class with no components, or a component with
> + * no resources. The vMSC properties have already been built.

If it's not possible do we need the defensive _or_null and error checks?

> + */
> +static void mpam_enable_init_class_features(struct mpam_class *class)
> +{
> +	struct mpam_vmsc *vmsc;
> +	struct mpam_component *comp;
> +
> +	comp = list_first_entry_or_null(&class->components,
> +					struct mpam_component, class_list);
> +	if (WARN_ON(!comp))
> +		return;
> +
> +	vmsc = list_first_entry_or_null(&comp->vmsc,
> +					struct mpam_vmsc, comp_list);
> +	if (WARN_ON(!vmsc))
> +		return;
> +
> +	class->props = vmsc->props;
> +}

> +/*
> + * Merge all the common resource features into class.
> + * vmsc features are bitwise-or'd together, this must be done first.

I'm not sure what 'this' is here - I think it's a missing plural that has
me confused.  Perhaps 'these must be done first.'

> + * Next the class features are the bitwise-and of all the vmsc features.
> + * Other features are the min/max as appropriate.
> + *
> + * To avoid walking the whole tree twice, the class->nrdy_usec property is
> + * updated when working with the vmsc as it is a max(), and doesn't need
> + * initialising first.

Perhaps state that this comment is about what happens in each call of
mpam_enable_merge_vmsc_features()  Or move the comment to that function.

> + */
> +static void mpam_enable_merge_features(struct list_head *all_classes_list)
> +{
> +	struct mpam_class *class;
> +	struct mpam_component *comp;
> +
> +	lockdep_assert_held(&mpam_list_lock);
> +
> +	list_for_each_entry(class, all_classes_list, classes_list) {
> +		list_for_each_entry(comp, &class->components, class_list)
> +			mpam_enable_merge_vmsc_features(comp);
> +
> +		mpam_enable_init_class_features(class);
> +
> +		list_for_each_entry(comp, &class->components, class_list)
> +			mpam_enable_merge_class_features(comp);
> +	}
> +}

Hi Jonathan,

On 12/09/2025 12:49, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:54 +0000
> James Morse <james.morse@arm.com> wrote:
> 
>> To make a decision about whether to expose an mpam class as
>> a resctrl resource we need to know its overall supported
>> features and properties.
>>
>> Once we've probed all the resources, we can walk the tree
>> and produce overall values by merging the bitmaps. This
>> eliminates features that are only supported by some MSC
>> that make up a component or class.
>>
>> If bitmap properties are mismatched within a component we
>> cannot support the mismatched feature.
>>
>> Care has to be taken as vMSC may hold mismatched RIS.

> A trivial things inline.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks!


>> +/*
>> + * Combine two props fields.
>> + * If this is for controls that alias the same resource, it is safe to just
>> + * copy the values over. If two aliasing controls implement the same scheme
>> + * a safe value must be picked.
>> + * For non-aliasing controls, these control different resources, and the
>> + * resulting safe value must be compatible with both. When merging values in
>> + * the tree, all the aliasing resources must be handled first.
>> + * On mismatch, parent is modified.
>> + */
>> +static void __props_mismatch(struct mpam_props *parent,
>> +			     struct mpam_props *child, bool alias)
>> +{
>> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) {
>> +		parent->cpbm_wd = child->cpbm_wd;
>> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part,
>> +				   cpbm_wd, alias)) {
>> +		pr_debug("%s cleared cpor_part\n", __func__);
>> +		mpam_clear_feature(mpam_feat_cpor_part, &parent->features);
>> +		parent->cpbm_wd = 0;
>> +	}
>> +
>> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) {
>> +		parent->mbw_pbm_bits = child->mbw_pbm_bits;
>> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part,
>> +				   mbw_pbm_bits, alias)) {
>> +		pr_debug("%s cleared mbw_part\n", __func__);
>> +		mpam_clear_feature(mpam_feat_mbw_part, &parent->features);
>> +		parent->mbw_pbm_bits = 0;
>> +	}
>> +
>> +	/* bwa_wd is a count of bits, fewer bits means less precision */
>> +	if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) {
> 
> Seems like an overly long line given other local wrapping.

Fixed.


>> +		parent->bwa_wd = child->bwa_wd;
>> +	} else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature,
>> +				     bwa_wd, alias)) {

>> +		pr_debug("%s took the min bwa_wd\n", __func__);

These __func__ arguments need to go as pr_fmt has this covered.


>> +		parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
>> +	}
>> +
>> +	/* For num properties, take the minimum */
>> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
>> +		parent->num_csu_mon = child->num_csu_mon;
>> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu,
>> +				   num_csu_mon, alias)) {
>> +		pr_debug("%s took the min num_csu_mon\n", __func__);
>> +		parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon);
>> +	}
>> +
>> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) {
>> +		parent->num_mbwu_mon = child->num_mbwu_mon;
>> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu,
>> +				   num_mbwu_mon, alias)) {
>> +		pr_debug("%s took the min num_mbwu_mon\n", __func__);
>> +		parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
>> +	}


>> +
>> +/*
>> + * If a vmsc doesn't match class feature/configuration, do the right thing(tm).
>> + * For 'num' properties we can just take the minimum.
>> + * For properties where the mismatched unused bits would make a difference, we
>> + * nobble the class feature, as we can't configure all the resources.
>> + * e.g. The L3 cache is composed of two resources with 13 and 17 portion
>> + * bitmaps respectively.
>> + */
>> +static void
>> +__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc)
> 
> I'm not really sure what the __ prefix denotes here.

Just that its the innards of something and needs comprehending as part of its caller.


>> +{
>> +	struct mpam_props *cprops = &class->props;
>> +	struct mpam_props *vprops = &vmsc->props;
>> +
>> +	lockdep_assert_held(&mpam_list_lock); /* we modify class */
>> +
>> +	pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n",
>> +		 dev_name(&vmsc->msc->pdev->dev),
>> +		 (long)cprops->features, (long)vprops->features);

> According to https://docs.kernel.org/core-api/printk-formats.html
> should be fine using %x for u16 values. So why dance through a cast to long?

To isolate it from a subsequent change that makes that field a u32, the existence of which
means one day it'll be a u64. If it ever gets bigger than unsigned-long, it'll need to be
a bitmap array, which would need this code to change. Until then doing, it like this makes
changes to the size less churny.


>> +
>> +	/* Take the safe value for any common features */
>> +	__props_mismatch(cprops, vprops, false);
>> +}
>> +
>> +static void
>> +__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris)
>> +{
>> +	struct mpam_props *rprops = &ris->props;
>> +	struct mpam_props *vprops = &vmsc->props;
>> +
>> +	lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */
>> +
>> +	pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n",
>> +		 dev_name(&vmsc->msc->pdev->dev),
>> +		 (long)vprops->features, (long)rprops->features);
> 
> Same as above comment on casts being unnecessary.

I expect to have to change this in the future.

As these two debug messages have a dev to hand, they should probably use dev_debug()
instead of manually printing the dev_name().


>> +
>> +	/*
>> +	 * Merge mismatched features - Copy any features that aren't common,
>> +	 * but take the safe value for any common features.
>> +	 */
>> +	__props_mismatch(vprops, rprops, true);
>> +}
>> +
>> +/*
>> + * Copy the first component's first vMSC's properties and features to the
>> + * class. __class_props_mismatch() will remove conflicts.
>> + * It is not possible to have a class with no components, or a component with
>> + * no resources. The vMSC properties have already been built.
> 
> If it's not possible do we need the defensive _or_null and error checks?

This is just paranoia. I've removed it.


>> + */
>> +static void mpam_enable_init_class_features(struct mpam_class *class)
>> +{
>> +	struct mpam_vmsc *vmsc;
>> +	struct mpam_component *comp;
>> +
>> +	comp = list_first_entry_or_null(&class->components,
>> +					struct mpam_component, class_list);
>> +	if (WARN_ON(!comp))
>> +		return;
>> +
>> +	vmsc = list_first_entry_or_null(&comp->vmsc,
>> +					struct mpam_vmsc, comp_list);
>> +	if (WARN_ON(!vmsc))
>> +		return;
>> +
>> +	class->props = vmsc->props;
>> +}
> 
>> +/*
>> + * Merge all the common resource features into class.
>> + * vmsc features are bitwise-or'd together, this must be done first.
> 
> I'm not sure what 'this' is here - I think it's a missing plural that has
> me confused.  Perhaps 'these must be done first.'

The bitwise-or. It's singular because its done for each class/component at a time.

The reason is so that mpam_enable_init_class_features() see's all the features in the
first vmsc, not a subset of them. It's because vmsc hold non-overlapping features, which
is all to support a platform that has two RIS with different types of control that do the
same thing. (made up example:) memory min/max on ingress and memory portions on egress.
Both are memory bandwidth for the same hardware block, but for whatever structural reason,
it gets exposed as separate RIS.

Rephrased as:
| * vmsc features are bitwise-or'd together as the first step so that
| * mpam_enable_init_class_features() can initialise the class with a
| * representive set of features.



>> + * Next the class features are the bitwise-and of all the vmsc features.
>> + * Other features are the min/max as appropriate.
>> + *
>> + * To avoid walking the whole tree twice, the class->nrdy_usec property is
>> + * updated when working with the vmsc as it is a max(), and doesn't need
>> + * initialising first.
> 
> Perhaps state that this comment is about what happens in each call of
> mpam_enable_merge_vmsc_features()  Or move the comment to that function.

Sure. The comment is to try and stop people 'fixing' it to only loop over
class->components once. That'll work on 99% of platforms, but discard all the
controls on a few strange ones.


>> + */
>> +static void mpam_enable_merge_features(struct list_head *all_classes_list)
>> +{
>> +	struct mpam_class *class;
>> +	struct mpam_component *comp;
>> +
>> +	lockdep_assert_held(&mpam_list_lock);
>> +
>> +	list_for_each_entry(class, all_classes_list, classes_list) {
>> +		list_for_each_entry(comp, &class->components, class_list)
>> +			mpam_enable_merge_vmsc_features(comp);
>> +
>> +		mpam_enable_init_class_features(class);
>> +
>> +		list_for_each_entry(comp, &class->components, class_list)
>> +			mpam_enable_merge_class_features(comp);
>> +	}
>> +}
> 
> 


Thanks,

James