Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate

[Ben Horgan <ben.horgan@arm.com>]

Hi Jonathan,

On 11/10/25 16:58, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:27 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
> 
>> From: James Morse <james.morse@arm.com>
>>
>> Probing MPAM is convoluted. MSCs that are integrated with a CPU may
>> only be accessible from those CPUs, and they may not be online.
>> Touching the hardware early is pointless as MPAM can't be used until
>> the system-wide common values for num_partid and num_pmg have been
>> discovered.
>>
>> Start with driver probe/remove and mapping the MSC.
>>
>> CC: Carl Worth <carl@os.amperecomputing.com>
>> Tested-by: Fenghua Yu <fenghuay@nvidia.com>
>> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> Tested-by: Peter Newman <peternewman@google.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> 
> Hi Ben,
> 
> A few minor things from a fresh read.
> Nothing to prevent a tag though.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks!

> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> new file mode 100644
>> index 000000000000..6c6be133d73a
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_devices.c
> 
> 
>> +
>> +static void mpam_msc_drv_remove(struct platform_device *pdev)
>> +{
>> +	struct mpam_msc *msc = platform_get_drvdata(pdev);
>> +
>> +	if (!msc)
>> +		return;
> 
> Agree with Gavin on this. If there is a reason this might be NULL
> then a comment would avoid the question being raised again. If not
> drop the check.

Dropped.

> 
>> +
>> +	mutex_lock(&mpam_list_lock);
>> +	mpam_msc_destroy(msc);
>> +	mutex_unlock(&mpam_list_lock);
>> +
>> +	synchronize_srcu(&mpam_srcu);
> 
> Trivial but perhaps a comment on why. I assume this is because the
> devm_ cleanup isn't safe until after an RCU grace period?

This becomes clearer in the next patch where it is moved into
mpam_free_garbage() so I'll leave this bare.

> 
>> +}
>> +
>> +static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>> +{
>> +	int err;
>> +	u32 tmp;
>> +	struct mpam_msc *msc;
>> +	struct resource *msc_res;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	lockdep_assert_held(&mpam_list_lock);
>> +
>> +	msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL);
>> +	if (!msc)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	err = devm_mutex_init(dev, &msc->probe_lock);
>> +	if (err)
>> +		return ERR_PTR(err);
> 
> Trivial but I'd add a blank line here.

done

> 
>> +	err = devm_mutex_init(dev, &msc->part_sel_lock);
>> +	if (err)
>> +		return ERR_PTR(err);
> 
> Trivial but I'd add a blank line here.

done

> 
>> +	msc->id = pdev->id;
>> +	msc->pdev = pdev;
>> +	INIT_LIST_HEAD_RCU(&msc->all_msc_list);
>> +	INIT_LIST_HEAD_RCU(&msc->ris);
>> +
>> +	err = update_msc_accessibility(msc);
>> +	if (err)
>> +		return ERR_PTR(err);
>> +	if (cpumask_empty(&msc->accessibility)) {
>> +		dev_err_once(dev, "MSC is not accessible from any CPU!");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp))
>> +		msc->iface = MPAM_IFACE_MMIO;
>> +	else
>> +		msc->iface = MPAM_IFACE_PCC;
>> +
>> +	if (msc->iface == MPAM_IFACE_MMIO) {
>> +		void __iomem *io;
>> +
>> +		io = devm_platform_get_and_ioremap_resource(pdev, 0,
>> +							    &msc_res);
>> +		if (IS_ERR(io)) {
>> +			dev_err_once(dev, "Failed to map MSC base address\n");
>> +			return ERR_CAST(io);
>> +		}
>> +		msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>> +		msc->mapped_hwpage = io;
>> +	} else {
>> +		return ERR_PTR(-ENOENT);
>> +	}
>> +
>> +	list_add_rcu(&msc->all_msc_list, &mpam_all_msc);
>> +	platform_set_drvdata(pdev, msc);
>> +
>> +	return msc;
>> +}
>> +
>> +static int mpam_msc_drv_probe(struct platform_device *pdev)
>> +{
>> +	int err;
>> +	struct mpam_msc *msc = NULL;
>> +	void *plat_data = pdev->dev.platform_data;
>> +
>> +	mutex_lock(&mpam_list_lock);
>> +	msc = do_mpam_msc_drv_probe(pdev);
>> +	mutex_unlock(&mpam_list_lock);
>> +	if (!IS_ERR(msc)) {
>> +		/* Create RIS entries described by firmware */
>> +		err = acpi_mpam_parse_resources(msc, plat_data);
>> +		if (err)
>> +			mpam_msc_drv_remove(pdev);
>> +	} else {
>> +		err = PTR_ERR(msc);
>> +	}
> 
> Seems convoluted. Not obvious to me why you can't do early exits on err and
> having simpler flow. Maybe something more messy happens in patches after this
> series to justify the complex approach.
> 
> 	if (IS_ERR(msc))
> 		return PTR_ERR(msc);
> 
> 	/* Create RIS entries described by firmware */
> 	err = acpi_mpam_parse_resources(msc, plat_data);
> 	if (err) {
> 		mpam_msc_drv_remove(pdev);
> 		return err;
> 	}
> 
> 	if (atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
> 		pr_info("Discovered all MSC\n");
> 
> 	return 0;

It's still like this at the end of the current mpam snapshot branch so
I'll simplify based on your suggestion.

> 
>> +
>> +	if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
>> +		pr_info("Discovered all MSC\n");
>> +
>> +	return err;
>> +}
>> +
>> +static struct platform_driver mpam_msc_driver = {
>> +	.driver = {
>> +		.name = "mpam_msc",
>> +	},
>> +	.probe = mpam_msc_drv_probe,
>> +	.remove = mpam_msc_drv_remove,
>> +};
>> +
>> +static int __init mpam_msc_driver_init(void)
>> +{
>> +	if (!system_supports_mpam())
>> +		return -EOPNOTSUPP;
>> +
>> +	init_srcu_struct(&mpam_srcu);
>> +
>> +	fw_num_msc = acpi_mpam_count_msc();
>> +
> 
> Trivial but I'd drop this blank line to keep the call closely
> associated with the error check.

done

> 
>> +	if (fw_num_msc <= 0) {
>> +		pr_err("No MSC devices found in firmware\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return platform_driver_register(&mpam_msc_driver);
>> +}
>> +subsys_initcall(mpam_msc_driver_init);
> 
> 

Thanks,

Ben