Re: [PATCH v7 10/11] arm_mpam: change MPAM-Fb error IRQ to use a threaded IRQ handler
From: Andre Przywara
Date: Tue Aug 04 2026 - 04:21:30 EST
Hi Jonathan,
On 8/4/26 00:46, Jonathan Cameron wrote:
On Fri, 31 Jul 2026 19:03:23 +0200
Andre Przywara <andre.przywara@xxxxxxx> wrote:
When an MPAM MSC gets into an error condition, it can trigger an error
IRQ. We cannot really do much about those errors, but we at least query
and log the error, then disable MPAM functionality.
This error report relies on reading the MSC's error status register
(ESR) in the current hard-IRQ handler, which is not possible for MPAM-Fb
based MSC accesses, since they involve mailbox routines that might sleep.
The same is true for clearing the interrupt at the source, which requires
an MSC access as well.
When an MSC is using MPAM-Fb, change the error IRQ to use a threaded IRQ
handler, with an empty hard IRQ routine, and doing all the MSC accesses
(to access the status and disable the IRQ line) in the threaded part.
Also forbid per-CPU interrupts (PPIs) for MPAM-Fb, as we cannot use a
threaded IRQ here.
The change in the actual IRQ handler is minimal, we just check for the
first MSC access error and bail out early. MMIO based MSCs keep using a
hard-IRQ handler, since they must be at least non-migrate-able when doing
MSC accesses, for the CPU affinity check to work.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
A small thing inline about a refactor that I can't spot a reason for.
May it's one of those cases where the code evolved into something that
would benefit from another look.
Either way I'm fine with this
Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
---
drivers/resctrl/mpam_devices.c | 52 +++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
index ea08c12f717a..b238feb559d4 100644
--- a/drivers/resctrl/mpam_devices.c
+++ b/drivers/resctrl/mpam_devices.c
@@ -2741,19 +2755,19 @@ static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
msc->id, mpam_errcode_names[errcode], partid, pmg,
ris);
- /* Disable this interrupt. */
+out_disable:
+ /* Disable this interrupt. Ignore errors, we need to proceed anyway. */
mpam_disable_msc_ecr(msc);
- /* Are we racing with the thread disabling MPAM? */
- if (!mpam_is_enabled())
- return IRQ_HANDLED;
-
/*
- * Schedule the teardown work. Don't use a threaded IRQ as we can't
- * unregister the interrupt from the threaded part of the handler.
+ * Schedule the teardown work. We have to defer it as we can't
+ * unregister the interrupt from the threaded part of a handler.
+ * Check whether we are racing with the thread disabling MPAM.
*/
- mpam_disable_reason = "hardware error interrupt";
- schedule_work(&mpam_broken_work);
+ if (mpam_is_enabled()) {
+ mpam_disable_reason = "hardware error interrupt";
+ schedule_work(&mpam_broken_work);
+ }
Why the logic flip here? (!mpam_is_enabled() and early return vs
if (mpam_is_enabled())
It seems to be doing exactly the same.
Is it to couple the comment more tightly with the code?
I don't mind it done this way but it is a reasonable amount of churn
for no strong benefit that I can immediately spot.
Yes, this went through several reworks. I think in this case it's not really an error, so doesn't really warrant the early-error-return pattern. And it makes further extensions harder. If there is ever something to be added later, we would probably need to change this flow first.
Also I prefer this simpler logic of: if (mpam_enabled()) disable_it(). I think that's less twisted than:
if (!mpam_enabled())
return;
Do_what_you_wanted_to_do_if_mpam_was_enabled().
But of course that's probably just a matter of preference. I changed it to what you suggested, mostly just to keep the current flow.
Cheers,
Andre
return IRQ_HANDLED;
}