Re: [PATCH v2] arm_mpam: Only schedule mpam_enable work after first successful MSC probe

[Ben Horgan]

Hi Yin,

On 17/09/2026 10:19, Yin Li wrote:
> 
> 
> On 9/10/2026 5:25 PM, Ben Horgan wrote:
>> Hi Ping,
>>
>> On 08/09/2026 03:48, ping.li wrote:
>>> From: Ping Li <leeonion.muyu@xxxxxxxxx>
>>>
>>> mpam_discovery_cpu_online() sets new_device_probed unconditionally after
>>> processing each reachable MSC. Once an MSC has already been probed
>>> (msc->probed is true), later CPUs sharing it skip
>>> mpam_msc_hw_probe() but still leave err at its default value of 0.
>>> As a result, new_device_probed is still set to true, causing
>>> mpam_enable_work to be scheduled again even though no new hardware was
>>> probed.
>>
>> This patch is an improvement but, thinking again, it looks there is scope getting rid of
>> mpam_enable() altogether. Rather than walking the list after each hw probe we could increment an
>> atomic variable, similar to what is done in mpam_msc_drv_probe(), and then just schedule
>> mpam_enable_once(). What do you think?
>>
> 
> Hi Ben,
> 
> Seeing another atomic-counter based sequencing mechanism in this patch reminded me of a similar
> issue I explored while working on MPAM DT support.
> 
> At the time, I experimented with removing the fw_num_msc pre-counting logic and moving the discovery
> callback registration to a late_initcall() stage. The motivation was to avoid separate DT/ACPI
> counting paths and allow discovery to proceed based on successfully probed MSCs.
> 
> However, I eventually dropped that approach because it relied on synchronous probing and would not
> behave correctly in deferred-probe or future asynchronous-probe scenarios.
> 
> That made me curious about the motivation behind this change:
>   - Is MPAM intentionally designed around the assumption that all firmware-described MSCs must probe
> successfully before discovery can proceed?

James wrote the code but here's how I see it. The important point of synchronization is for MPAM
enabling after the h/w probe rather than before discovery. This allows the number of usable PARTID
and PMG to be calculated and allows the ris/comp/class lists to be considered read only after this
point (except if MPAM is being disabled). For discovery I expect the MSC still be considered
independently. However, the synchronization is convenient at discovery as it allows for cpu hotplug
callbacks to do the initialisation, first for all MSC that have online affine CPUs and then as those
CPUs come online.

>   - Is the count-and-compare model primarily retained to guarantee correct ordering under deferred/
> asynchronous probing?

Deferred probing for the discovery will possibly be required for enabling interrupts with GICv5.

Thanks,

Ben

> 
> I'm not suggesting changing the implementation, just interested in understanding the design rationale.
> 
> Thanks,
> Yin
> 
> 
>> Thanks,
>>
>> Ben
>>
>>>
>>> Set new_device_probed only when mpam_msc_hw_probe() is called and
>>> succeeds.
>>>
>>> Signed-off-by: Ping Li <leeonion.muyu@xxxxxxxxx>
>>> ---
>>> Changes in v2:
>>> - Drop the Fixes: tag, as the extra mpam_enable() calls cause no real
>>>    harm: schedule_work() merges the duplicate work, and mpam_enable()
>>>    is a no-op until all MSCs have been probed. This is a cleanup, not a
>>>    bug fix.
>>>
>>>   drivers/resctrl/mpam_devices.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> index 2f09f4b78bd3..fefdcf588932 100644
>>> --- a/drivers/resctrl/mpam_devices.c
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -1866,13 +1866,15 @@ static int mpam_discovery_cpu_online(unsigned int cpu)
>>>               continue;
>>>             mutex_lock(&msc->probe_lock);
>>> -        if (!msc->probed)
>>> +        if (!msc->probed) {
>>>               err = mpam_msc_hw_probe(msc);
>>> +            if (!err)
>>> +                new_device_probed = true;
>>> +        }
>>>           mutex_unlock(&msc->probe_lock);
>>>             if (err)
>>>               break;
>>> -        new_device_probed = true;
>>>       }
>>>         if (new_device_probed && !err)
>>
>