Re: [PATCH v1 02/11] arm_mpam: Restore the error interrupt enable from mpam_cpu_online()

[Gavin Shan]

Hi Ben,

On 7/16/26 6:59 PM, Ben Horgan wrote:
> On 7/16/26 04:59, Gavin Shan wrote:
> > On 7/10/26 9:55 PM, Ben Horgan wrote:
> > > When all CPUs affine to an MSC are offline the MSC may lose register state
> > > which the driver then restores when an affine CPU comes back online. The
> > > error interrupt enable, MPAMF_ECR.INTEN, is missed.
> > >
> > > Restore MPAMF_ECR at CPU online.
> > >
> > > Fixes: 49aa621c4dca ("arm_mpam: Register and enable IRQs")
> > > Signed-off-by: Ben Horgan <ben.horgan@xxxxxxx>
> > > ---
> > >    drivers/resctrl/mpam_devices.c | 8 +++++++-
> > >    1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> > > index 82966ca2a631..acfa9a4dc2fc 100644
> > > --- a/drivers/resctrl/mpam_devices.c
> > > +++ b/drivers/resctrl/mpam_devices.c
> > > @@ -1852,8 +1852,14 @@ static int mpam_cpu_online(unsigned int cpu)
> > >            if (msc->reenable_error_ppi)
> > >                _enable_percpu_irq(&msc->reenable_error_ppi);
> > >    -        if (atomic_fetch_inc(&msc->online_refs) == 0)
> > > +        if (atomic_fetch_inc(&msc->online_refs) == 0) {
> > > +            mutex_lock(&msc->error_irq_lock);
> > > +            if (msc->error_irq_hw_enabled)
> > > +                mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
> > > +            mutex_unlock(&msc->error_irq_lock);
> > > +
> > >                mpam_reprogram_msc(msc);
> > > +        }
> > >        }
> > >    
> >
> > I don't understand how this happened that MPAMF_ECR gets lost, but msc->error_irq_hw_enabled
> > is kept as true. mpam_disable_msc_ecr() is triggered in __mpam_irq_handler() or mpam_unregister_irqs().
> > Both are related to mpam_broken_work, which is invoked to disable the driver completely.
> > I don't see how MPAMF_ECR is cleared at CPU offline time.
>
> The reasoning is the same as to why mpam_reprogram_msc() is already run here. When all the CPUs
> affine to an MSC at a cache are offline then the cache instance and so the MSC might be switched off
> and lose register state. Hence, any registers we rely on need to set when the MSC comes back online.
> For PSCI_CPU_SUSPEND this needs to be handled by the firmware and for PSCI_CPU_OFF the driver
> handles it. Does that make any more sense?

Ok. You mean PSCI_CPU_OFF on the last associated CPU enforces a power-off on the MSC
either? In this case, the commit messages need to be clearer with something below.

  When all the associated CPUs to an MSC are offline, possibly caused by
  PSCI_CPU_OFF, the power supply to the MSC can be cut off and all registers,
  including MPAMF_ECR.INTEN, are lost. Afterwards, we need to explicitly set
  MPAMF_ECR.INTEN when the first associated CPU becomes online and the power
  supply to the MSC is recovered.

  Restore MPAMF_ECR.INTEN in mpam_cpu_online() when the first associated CPU
  of the MSC becomes online

> Thanks,
>
> Ben
>
> > >        if (mpam_resctrl_enabled)

Thanks,
Gavin