Re: [PATCH v7 10/11] arm_mpam: change MPAM-Fb error IRQ to use a threaded IRQ handler

Hi Jonathan,

On 8/4/26 00:46, Jonathan Cameron wrote:
> On Fri, 31 Jul 2026 19:03:23 +0200
> Andre Przywara <[email protected]> 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 <[email protected]>
> 
> 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 <[email protected]>
> 
>> ---
>>   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;
>>   }