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

Hi,

On 7/31/26 15:33, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/30/26 16:25, Andre Przywara 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.
>> The change 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.
>> Also forbid per-CPU interrupts (PPIs) for MPAM-Fb, as we cannot use a
>> threaded IRQ here.
>>
>> Signed-off-by: Andre Przywara <[email protected]>
>> ---
>>   drivers/resctrl/mpam_devices.c | 64 ++++++++++++++++++++++++++--------
>>   1 file changed, 49 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index daaf3cf48475..606ae4af2be9 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -2660,24 +2660,38 @@ static int mpam_disable_msc_ecr(void *_msc)
>>   	return 0;
>>   }
>>   
>> +/*
>> + * This will run as the threaded IRQ handler part when using MPAM-Fb, but
>> + * as the sole hard-IRQ handler for MMIO based accesses.
>> + */
>>   static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>>   {
>>   	u64 reg;
>> +	int ret;
>>   	u16 partid;
>>   	u8 errcode, pmg, ris;
>>   
>> -	if (WARN_ON_ONCE(!msc) ||
>> +	if (WARN_ON_ONCE(!msc))
>> +		return IRQ_NONE;
>> +
>> +	if (msc->iface == MPAM_IFACE_MMIO &&
>>   	    WARN_ON_ONCE(!cpumask_test_cpu(smp_processor_id(),
>>   					   &msc->accessibility)))
>>   		return IRQ_NONE;
>>   
>> -	mpam_msc_read_esr(msc, &reg);
>> +	ret = mpam_msc_read_esr(msc, &reg);
>> +	if (ret) {
>> +		pr_err_ratelimited("unknown error irq from msc:%u\n", msc->id);
>> +
>> +		/* Try out best here ... */
>> +		goto out_disable;
>> +	}
>>   
>>   	errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
>>   	if (!errcode)
>>   		return IRQ_NONE;
>>   
>> -	/* Clear level triggered irq */
>> +	/* Clear level triggered irq. Ignore errors, we need to proceed. */
>>   	mpam_msc_clear_esr(msc);
>>   
>>   	partid = FIELD_GET(MPAMF_ESR_PARTID_MON, reg);
>> @@ -2688,19 +2702,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);
> 
> If we fail to disable the interrupt then I don't think we can claim we handled it.
> Shouldn't we, after scheduling broken work, return IRQ_NONE?

That's tricky, I'd say. IRQ_NONE means the handler is not recognising 
the interrupt, so it probably becomes spurious, which doesn't sound right.
So I think IRQ_HANDLED is the least worst of the two. We are somewhat 
doomed, but eventually mpam_disable() will unregister the IRQ, so it 
won't fire again. And we protect again repeated calls to mpam_disable(), 
should the IRQ handler be called again.

Cheers,
Andre

>>   
>> -	/* 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);
>> +	}
>>   
>>   	return IRQ_HANDLED;
>>   }
>> @@ -2736,6 +2750,11 @@ static int mpam_register_irqs(void)
>>   		/* The MPAM spec says the interrupt can be SPI, PPI or LPI */
>>   		/* We anticipate sharing the interrupt with other MSCs */
>>   		if (irq_is_percpu(irq)) {
>> +			if (msc->iface != MPAM_IFACE_MMIO) {
>> +				dev_err(&msc->pdev->dev,
>> +					"Only MMIO MSCs can use per-CPU interrupts\n");
>> +				return -EINVAL;
>> +			}
>>   			err = request_percpu_irq(irq, &mpam_ppi_handler,
>>   						 "mpam:msc:error",
>>   						 msc->error_dev_id);
>> @@ -2747,9 +2766,24 @@ static int mpam_register_irqs(void)
>>   					       &_enable_percpu_irq, &irq,
>>   					       true);
>>   		} else {
>> -			err = devm_request_irq(&msc->pdev->dev, irq,
>> -					       &mpam_spi_handler, IRQF_SHARED,
>> -					       "mpam:msc:error", msc);
>> +			irq_handler_t hard, threaded;
>> +
>> +			/*
>> +			 * MPAM-Fb needs a sleepable context for the MSC
>> +			 * accesses, whereas MMIO requires at least a
>> +			 * non-migrateable context for the processor ID check.
>> +			 */
>> +			if (msc->iface == MPAM_IFACE_MMIO) {
>> +				hard = &mpam_spi_handler;
>> +				threaded = NULL;
>> +			} else {
>> +				hard = NULL;
>> +				threaded = &mpam_spi_handler;
>> +			}
>> +			err = devm_request_threaded_irq(&msc->pdev->dev, irq,
>> +							hard, threaded,
>> +							IRQF_SHARED | IRQF_ONESHOT,
>> +							"mpam:msc:error", msc);
>>   			if (err)
>>   				return err;
>>   		}
>