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

[Yin Li]

On 9/21/2026 8:39 PM, Ben Horgan wrote:
> 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.

Hi Ben,

Thanks for the explanation.

Agreed that MSC enabling is handled independently.I understand that the 
"(atomic_add_return(1, &mpam_num_msc) == fw_num_msc)" condition as an 
ordering guarantee: it ensures mpam_all_msc is complete before the
cpuhp callback is registered, so no MSC is missed when the callback
walks the MSC list.

CPU onlining and MSC probing are two independent sequences. The cpuhp
mechanism handles the case where a CPU comes online after an MSC is
probed. But it cannot handle the reverse: an MSC that completes probing
after cpuhp has already executed will be missed.


> >    - 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.

Deferred probing for GICv5 would indeed require the count-and-compare
model to ensure all MSCs have probed before the cpuhp callback is
registered. Thanks for the clarification.

Thanks,
Yin

> 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)

--
Thx and BRs,
Yin