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

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

Hi Gavin,

On 11/8/25 09:28, Gavin Shan wrote:
> Hi Ben,
> 
> On 11/7/25 10:34 PM, Ben Horgan 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.
>>
> 
> I'm not sure if below commit log is more clearer as I'm not a English
> native speaker:

Thanks for the detailed review of the messages and comments. I've
skipped the ones that I think don't improve the clarity. (I see Jonathan
has a detailed reply which matches my understanding of English.)

> 
> MPAM probing is convoluted. MSCs that are integrated to a set of CPUs
> may only be accessible from those CPUs, ...
> 
>> 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>
>> ---
>> Changes since v3:
>>  From Jonathan:
>> Include cleanup
>> Use devm_mutex_init()
>> Add an ERR_CAST()
>> Fenghua:
>> Return zero from update_msc_accessibility()
>> Additional:
>> Fail probe if MSC doesn't have an MMIO interface
>> ---
>>   arch/arm64/Kconfig              |   1 +
>>   drivers/Kconfig                 |   2 +
>>   drivers/Makefile                |   1 +
>>   drivers/resctrl/Kconfig         |  15 +++
>>   drivers/resctrl/Makefile        |   4 +
>>   drivers/resctrl/mpam_devices.c  | 194 ++++++++++++++++++++++++++++++++
>>   drivers/resctrl/mpam_internal.h |  49 ++++++++
>>   7 files changed, 266 insertions(+)
>>   create mode 100644 drivers/resctrl/Kconfig
>>   create mode 100644 drivers/resctrl/Makefile
>>   create mode 100644 drivers/resctrl/mpam_devices.c
>>   create mode 100644 drivers/resctrl/mpam_internal.h
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index c5e66d5d72cd..004d58cfbff8 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -2025,6 +2025,7 @@ config ARM64_TLB_RANGE
>>     config ARM64_MPAM
>>       bool "Enable support for MPAM"
>> +    select ARM64_MPAM_DRIVER if EXPERT    # does nothing yet
>>       select ACPI_MPAM if ACPI
>>       help
>>         Memory System Resource Partitioning and Monitoring (MPAM) is an
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 4915a63866b0..3054b50a2f4c 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -251,4 +251,6 @@ source "drivers/hte/Kconfig"
>>     source "drivers/cdx/Kconfig"
>>   +source "drivers/resctrl/Kconfig"
>> +
>>   endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 8e1ffa4358d5..20eb17596b89 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -194,6 +194,7 @@ obj-$(CONFIG_HTE)        += hte/
>>   obj-$(CONFIG_DRM_ACCEL)        += accel/
>>   obj-$(CONFIG_CDX_BUS)        += cdx/
>>   obj-$(CONFIG_DPLL)        += dpll/
>> +obj-y                += resctrl/
>>     obj-$(CONFIG_DIBS)        += dibs/
>>   obj-$(CONFIG_S390)        += s390/
>> diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig
>> new file mode 100644
>> index 000000000000..ef2f3adf64a9
>> --- /dev/null
>> +++ b/drivers/resctrl/Kconfig
>> @@ -0,0 +1,15 @@
>> +menuconfig ARM64_MPAM_DRIVER
>> +    bool "MPAM driver"
>> +    depends on ARM64 && ARM64_MPAM && EXPERT
>> +    help
>> +      Memory System Resource Partitioning and Monitoring (MPAM)
>> driver for
>> +      System IP, e,g. caches and memory controllers.
>> +
>> +if ARM64_MPAM_DRIVER
>> +
>> +config ARM64_MPAM_DRIVER_DEBUG
>> +    bool "Enable debug messages from the MPAM driver"
>> +    help
>> +      Say yes here to enable debug messages from the MPAM driver.
>> +
>> +endif
> 
> I am asking myself why "depends on ARM64_MPAM_DRIVER" can't be used
> here? :-)
> 
>> diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile
>> new file mode 100644
>> index 000000000000..898199dcf80d
>> --- /dev/null
>> +++ b/drivers/resctrl/Makefile
>> @@ -0,0 +1,4 @@
>> +obj-$(CONFIG_ARM64_MPAM_DRIVER)            += mpam.o
>> +mpam-y                        += mpam_devices.o
>> +
>> +ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG)    += -DDEBUG
>> 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
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2025 Arm Ltd.
>> +
>> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/arm_mpam.h>
>> +#include <linux/cacheinfo.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/gfp.h>
>> +#include <linux/list.h>
>> +#include <linux/lockdep.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/printk.h>
>> +#include <linux/srcu.h>
>> +#include <linux/types.h>
>> +
>> +#include "mpam_internal.h"
>> +
>> +/*
>> + * mpam_list_lock protects the SRCU lists when writing. Once the
>> + * mpam_enabled key is enabled these lists are read-only,
>> + * unless the error interrupt disables the driver.
>> + */
> 
> s/when writing/for writing
> s/are read-only/become read-only
> 
>> +static DEFINE_MUTEX(mpam_list_lock);
>> +static LIST_HEAD(mpam_all_msc);
>> +
>> +struct srcu_struct mpam_srcu;
>> +
>> +/*
>> + * Number of MSCs that have been probed. Once all MSC have been
>> probed MPAM
>> + * can be enabled.
>> + */
> 
> s/all MSC/all MSCs  (?)
Changed.
> 
>> +static atomic_t mpam_num_msc;
>> +
>> +/*
>> + * An MSC can control traffic from a set of CPUs, but may only be
>> accessible
>> + * from a (hopefully wider) set of CPUs. The common reason for this
>> is power
>> + * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the
>> + * corresponding cache may also be powered off. By making accesses from
>> + * one of those CPUs, we ensure this isn't the case.
>> + */
> 
> s/An MSC/A MSC (?)
> s/from a/from the
> s/isn't the case/is the case (?)
Updated this last one to be:

By making accesses from one of those CPUs, we ensure we don't access a
cache that's powered off.

> 
>> +static int update_msc_accessibility(struct mpam_msc *msc)
>> +{
>> +    u32 affinity_id;
>> +    int err;
>> +
>> +    err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
>> +                       &affinity_id);
>> +    if (err)
>> +        cpumask_copy(&msc->accessibility, cpu_possible_mask);
>> +    else
>> +        acpi_pptt_get_cpus_from_container(affinity_id,
>> +                          &msc->accessibility);
>> +    return 0;
>> +}
>> +
> 
> {} is needed for the block spanning multiple lines.

Made it one line.

> 
> I would validate msc->accessibility here instead of its caller
> (do_mpam_msc_drv_probe()).
> 
>         if (cpumask_empty(&msc->accessibility))
>             return {-EINVAL, -ENOENT};
> 
>> +static int fw_num_msc;
>> +
>> +static void mpam_msc_destroy(struct mpam_msc *msc)
>> +{
>> +    struct platform_device *pdev = msc->pdev;
>> +
>> +    lockdep_assert_held(&mpam_list_lock);
>> +
>> +    list_del_rcu(&msc->all_msc_list);
>> +    platform_set_drvdata(pdev, NULL);
>> +}
>> +
>> +static void mpam_msc_drv_remove(struct platform_device *pdev)
>> +{
>> +    struct mpam_msc *msc = platform_get_drvdata(pdev);
>> +
>> +    if (!msc)
>> +        return;
> 
> 'msc' is unlikely to be NULL here, so the check could be droped.

Dropped.

> 
>> +
>> +    mutex_lock(&mpam_list_lock);
>> +    mpam_msc_destroy(msc);
>> +    mutex_unlock(&mpam_list_lock);
>> +
>> +    synchronize_srcu(&mpam_srcu);
>> +}
>> +
>> +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);
>> +    err = devm_mutex_init(dev, &msc->part_sel_lock);
>> +    if (err)
>> +        return ERR_PTR(err);
>> +    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);
>> +    }
>> +
> 
> As suggested above, this check would be done inside
> update_msc_accessibility().

Unless you object I'll keep this as is and make void
update_msc_accessibility() a void function. I think this works better
with the naming.

> 
>> +    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);
> 
> Would be:
>         return ERR_PTR(-EINVAL);

Sure.

> 
>> +    }
>> +
>> +    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);
>> +    }
>> +
>> +    if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
>> +        pr_info("Discovered all MSC\n");
> 
> s/all MSC/all MSCs
> 
>> +
>> +    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();
>> +
>> +    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);
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/
>> mpam_internal.h
>> new file mode 100644
>> index 000000000000..540066903eca
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +// Copyright (C) 2025 Arm Ltd.
>> +
>> +#ifndef MPAM_INTERNAL_H
>> +#define MPAM_INTERNAL_H
>> +
>> +#include <linux/arm_mpam.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/io.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +struct platform_device;
>> +
>> +struct mpam_msc {
>> +    /* member of mpam_all_msc */
>> +    struct list_head    all_msc_list;
>> +
>> +    int            id;
>> +    struct platform_device    *pdev;
>> +
>> +    /* Not modified after mpam_is_enabled() becomes true */
>> +    enum mpam_msc_iface    iface;
>> +    u32            nrdy_usec;
>> +    cpumask_t        accessibility;
>> +
>> +    /*
>> +     * probe_lock is only taken during discovery. After discovery these
>> +     * properties become read-only and the lists are protected by SRCU.
>> +     */
>> +    struct mutex        probe_lock;
>> +    unsigned long        ris_idxs;
>> +    u32            ris_max;
>> +
>> +    /* mpam_msc_ris of this component */
>> +    struct list_head    ris;
>> +
>> +    /*
>> +     * part_sel_lock protects access to the MSC hardware registers
>> that are
>> +     * affected by MPAMCFG_PART_SEL. (including the ID registers that
>> vary
>> +     * by RIS).
>> +     * If needed, take msc->probe_lock first.
>> +     */
>> +    struct mutex        part_sel_lock;
>> +
>> +    void __iomem        *mapped_hwpage;
>> +    size_t            mapped_hwpage_sz;
>> +};
>> +#endif /* MPAM_INTERNAL_H */
> 
> Thanks,
> Gavin
> 

Thanks,

Ben