Re: [PATCH v2 07/29] arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate

[James Morse <james.morse@arm.com>]

Hi Gavin,

On 03/10/2025 04:53, Gavin Shan wrote:
> Hi James,
> 
> On 9/11/25 6:42 AM, James Morse wrote:
>> 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.

>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> new file mode 100644
>> index 000000000000..efc4738e3b4d
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_devices.c

>> +/*
>> + * 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.
>> + */
>> +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;
>> +
>> +    return err;
>> +}
>> +
> 
> Double return here and different values have been returned. I think here we
> need "return err". In this case, we needn't copy @cpu_possible_mask on error
> because the caller mpam_msc_drv_probe() will release the MSC instance.

This was the botched removal of the DT support. I'm surprised the compiler is so
forgiving. (already pointed out and already fixed)


>> +static int mpam_msc_drv_probe(struct platform_device *pdev)
>> +{
>> +    int err;
>> +    struct mpam_msc *msc;
>> +    struct resource *msc_res;
>> +    struct device *dev = &pdev->dev;
>> +    void *plat_data = pdev->dev.platform_data;
>> +
>> +    mutex_lock(&mpam_list_lock);
>> +    do {
>> +        msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL);
>> +        if (!msc) {
>> +            err = -ENOMEM;
>> +            break;
>> +        }
>> +
>> +        mutex_init(&msc->probe_lock);
>> +        mutex_init(&msc->part_sel_lock);
>> +        mutex_init(&msc->outer_mon_sel_lock);
>> +        raw_spin_lock_init(&msc->inner_mon_sel_lock);
>> +        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)
>> +            break;
>> +        if (cpumask_empty(&msc->accessibility)) {
>> +            dev_err_once(dev, "MSC is not accessible from any CPU!");
>> +            err = -EINVAL;
>> +            break;
>> +        }
>> +
> 
> This check (cpumask_empty()) would be part of update_msc_accessibility() since
> msc->accessibility is sorted out in that function where it should be validated.


Could be - but isn't. This is because with the DT support in update_msc_accessibility()
that function is more complex, and its simpler to get the caller to check things like
this.

Even if no-one ever gets DT support upstream, I don't think this matters.


>> +        if (device_property_read_u32(&pdev->dev, "pcc-channel",
>> +                         &msc->pcc_subspace_id))
>> +            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");
>> +                err = PTR_ERR(io);
>> +                break;
>> +            }
>> +            msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>> +            msc->mapped_hwpage = io;
>> +        }
>> +
>> +        list_add_rcu(&msc->all_msc_list, &mpam_all_msc);
>> +        platform_set_drvdata(pdev, msc);
>> +    } while (0);
>> +    mutex_unlock(&mpam_list_lock);
>> +
>> +    if (!err) {
>> +        /* Create RIS entries described by firmware */
>> +        err = acpi_mpam_parse_resources(msc, plat_data);
>> +    }
>> +
>> +    if (err && msc)
>> +        mpam_msc_drv_remove(pdev);
>> +
>> +    if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc)
>> +        pr_info("Discovered all MSC\n");
>> +
>> +    return err;
>> +}


Thanks,

James