| 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: <f2c77f14-c10a-4074-9cdb-5d725f140983@arm.com>
Date: Fri, 17 Oct 2025 19:51:14 +0100
From: James Morse <james.morse@....com>
To: Gavin Shan <gshan@...hat.com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org
Cc: D Scott Phillips OS <scott@...amperecomputing.com>,
carl@...amperecomputing.com, lcherian@...vell.com,
bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
dfustini@...libre.com, amitsinght@...vell.com,
David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
fenghuay@...dia.com, baisheng.gao@...soc.com,
Jonathan Cameron <jonathan.cameron@...wei.com>, Rob Herring
<robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
<guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH v2 07/29] arm_mpam: Add probe/remove for mpam msc driver
and kbuild boiler plate
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
Powered by blists - more mailing lists