Re: [PATCH v4 10/10] arm_mpam: detect and enable MPAM-Fb PCC support

Hi Srivathsa,

On 7/27/26 18:27, Srivathsa L Rao wrote:
> 
> 
> On 7/23/2026 9:24 PM, Andre Przywara wrote:
>> The Arm MPAM-Fb specification [1] describes a protocol to access MSC
>> registers through a firmware interface. This requires a shared memory
>> region to hold the message, and a mailbox to trigger the access.
>> For ACPI this is wrapped as a PCC channel, described using existing
>> ACPI abstractions.
>>
>> Add code to parse those PCC table descriptions associated with an MSC,
>> and store the parsed information in the MSC struct.
>> There can be multiple PCC channels, and each channel can serve multiple
>> MSCs, so we need to keep track of the channel usage, using a list and
>> a refcount.
>> This will be used by the MPAM-Fb access wrapper code.
>>
>> [1] https://developer.arm.com/documentation/den0144/latest
>>
>> Signed-off-by: Andre Przywara <[email protected]>
>> ---
>>   drivers/acpi/arm64/mpam.c      |   6 +-
>>   drivers/resctrl/mpam_devices.c | 127 ++++++++++++++++++++++++++++++++-
>>   2 files changed, 129 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
>> index 84963a20c3e7..ca9b8754ae5f 100644
>> --- a/drivers/acpi/arm64/mpam.c
>> +++ b/drivers/acpi/arm64/mpam.c
>> @@ -220,8 +220,8 @@ static struct platform_device * __init 
>> acpi_mpam_parse_msc(struct acpi_mpam_msc_
>>       struct platform_device *pdev __free(platform_device_put) =
>>           platform_device_alloc("mpam_msc", tbl_msc->identifier);
>>       int next_res = 0, next_prop = 0, err;
>> -    /* pcc, nrdy, affinity and a sentinel */
>> -    struct property_entry props[4] = { 0 };
>> +    /* pcc, msc-id, nrdy, affinity and a sentinel */
>> +    struct property_entry props[5] = { 0 };
>>       /* mmio, 2xirq, no sentinel. */
>>       struct resource res[3] = { 0 };
>>       struct acpi_device *companion;
>> @@ -256,6 +256,8 @@ static struct platform_device * __init 
>> acpi_mpam_parse_msc(struct acpi_mpam_msc_
>>       } else if (iface == MPAM_IFACE_PCC) {
>>           props[next_prop++] = PROPERTY_ENTRY_U32("pcc-channel",
>>                               tbl_msc->base_address);
>> +        props[next_prop++] = PROPERTY_ENTRY_U32("msc-id",
>> +                            tbl_msc->identifier);
>>       }
>>       acpi_mpam_parse_irqs(pdev, tbl_msc, res, &next_res);
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/ 
>> mpam_devices.c
>> index fea3161ffbe9..fc4b5f87fbe9 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -19,14 +19,19 @@
>>   #include <linux/irqdesc.h>
>>   #include <linux/list.h>
>>   #include <linux/lockdep.h>
>> +#include <linux/mailbox_client.h>
>>   #include <linux/mutex.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/printk.h>
>> +#include <linux/property.h>
>>   #include <linux/srcu.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/types.h>
>>   #include <linux/workqueue.h>
>> +#include <acpi/pcc.h>
>> +#include <acpi/acpi_io.h>
>> +
>>   #include "mpam_internal.h"
>>   /* Values for the T241 errata workaround */
>> @@ -49,6 +54,86 @@ static LIST_HEAD(mpam_all_msc);
>>   struct srcu_struct mpam_srcu;
>> +/* PCC channels might be serving multiple MSCs, so keep a refcounted 
>> list. */
>> +static DEFINE_MUTEX(pcc_chan_list_lock);
>> +static LIST_HEAD(pcc_chan_list);
>> +
>> +static void mpam_pcc_chan_release(struct kref *ref)
>> +{
>> +    struct mpam_pcc_chan *cur = container_of(ref, struct mpam_pcc_chan,
>> +                         refcount);
>> +
>> +    pcc_mbox_free_channel(cur->pcc_chan);
>> +    list_del(&cur->pcc_chans);
>> +    kfree(cur);
>> +}
>> +
>> +static struct mpam_pcc_chan *mpam_pcc_chan_get(struct device *dev,
>> +                           int subspace_id)
>> +{
>> +    struct mpam_pcc_chan *cur;
>> +    int ret;
>> +
>> +    guard(mutex)(&pcc_chan_list_lock);
>> +
>> +    list_for_each_entry(cur, &pcc_chan_list, pcc_chans) {
>> +        if (cur->subspace_id == subspace_id) {
>> +            kref_get(&cur->refcount);
>> +
>> +            return cur;
>> +        }
>> +    }
>> +
>> +    cur = kzalloc_obj(*cur);
>> +    if (!cur)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    cur->pcc_cl.dev = dev;
>> +    cur->pcc_cl.tx_block = true;
>> +
>> +    cur->pcc_chan = pcc_mbox_request_channel(&cur->pcc_cl, subspace_id);
>> +    if (IS_ERR(cur->pcc_chan)) {
>> +        long err = PTR_ERR(cur->pcc_chan);
>> +
>> +        kfree(cur);
>> +        return ERR_PTR(err);
>> +    }
>> +
>> +    /* Timeout based on the "nominal latency" from the PCC ACPI 
>> table. */
>> +    cur->pcc_cl.tx_tout = cur->pcc_chan->latency * 5;
>> +
>> +    ret = devm_mutex_init(dev, &cur->pcc_chan_lock);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
> 
> Here, devm_mutex_init() ties the lifetime of pcc_chan_lock to dev, which 
> is &pdev->dev of the first MSC to call mpam_pcc_chan_get(). But
> mpam_pcc_chan is shared across multiple MSCs via kref, so it can
> outlive that first device.

Ah, that's a good point, thanks for catching that. It was too tempting 
to just replace the original mutex_init() with the devm_ version, but 
indeed the lifetime is not the same.

> If a second MSC has incremented the refcount and is still active when
> the first MSC's device is removed, the devm cleanup on the first device
> would call mutex_destroy() on pcc_chan_lock while the second MSC may
> still be inside:
> 
>      guard(mutex)(&pcc_chan->pcc_chan_lock);
> 
> I am not 100% sure this is reachable in practice — it may depend on
> the order in which platform devices are unbound, and whether that can
> happen with MSCs sharing a channel. I also note that devm_mutex_init()
> is a no-op on non-debug kernels, so this would only be observable with
> CONFIG_DEBUG_MUTEXES=y. But it felt worth raising in case
> it is a real path.
> 
> If my understanding is correct, would it make sense to use a plain
> mutex_init() here instead, and move mutex_destroy() into
> mpam_pcc_chan_release() where the kref guarantees no concurrent users
> remain?

Yes, that's a good idea, since we free the data structure in the release 
function anyway, we can surely destroy the mutex there. And thanks for 
the heads up about this being a no-op mostly!

Cheers,
Andre