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

Ben Horgan
2025-11-12T16:05:46Z

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