| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2189df41-ca50-4e10-a65c-4c297f9dc63b@arm.com>
Date: Wed, 12 Nov 2025 16:05:40 +0000
From: Ben Horgan <ben.horgan@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: james.morse@....com, amitsinght@...vell.com, baisheng.gao@...soc.com,
baolin.wang@...ux.alibaba.com, bobo.shaobowang@...wei.com,
carl@...amperecomputing.com, catalin.marinas@....com, dakr@...nel.org,
dave.martin@....com, david@...hat.com, dfustini@...libre.com,
fenghuay@...dia.com, gregkh@...uxfoundation.org, gshan@...hat.com,
guohanjun@...wei.com, jeremy.linton@....com, kobak@...dia.com,
lcherian@...vell.com, lenb@...nel.org, linux-acpi@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
lpieralisi@...nel.org, peternewman@...gle.com, quic_jiles@...cinc.com,
rafael@...nel.org, robh@...nel.org, rohit.mathew@....com,
scott@...amperecomputing.com, sdonthineni@...dia.com, sudeep.holla@....com,
tan.shaopeng@...itsu.com, will@...nel.org, xhao@...ux.alibaba.com,
Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and
kbuild boiler plate
Hi Jonathan,
On 11/10/25 16:58, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:27 +0000
> Ben Horgan <ben.horgan@....com> wrote:
>
>> From: James Morse <james.morse@....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@...amperecomputing.com>
>> Tested-by: Fenghua Yu <fenghuay@...dia.com>
>> Tested-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
>> Tested-by: Peter Newman <peternewman@...gle.com>
>> Signed-off-by: James Morse <james.morse@....com>
>> Signed-off-by: Ben Horgan <ben.horgan@....com>
>
> Hi Ben,
>
> A few minor things from a fresh read.
> Nothing to prevent a tag though.
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.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
Powered by blists - more mailing lists