Re: [PATCH v10 08/14] arm_mpam: propagate MSC access errors for interrupt control

[Andre Przywara <andre.przywara@arm.com>]

Hi Ben,

On 9/11/26 16:26, Ben Horgan wrote:
> Hi Andre,
> 
> On 11/09/2026 12:28, Andre Przywara wrote:
>> Allow the functions dealing with interrupt registration and enablement
>> to check for and return errors, and propagate MSC read and write errors
>> from the lower level up.
>> This does not cover the IRQ handler yet, as this needs some more
>> attention.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> Reviewed-by: Srivathsa L Rao <srivathsa.rao@oss.qualcomm.com>
> 
> Commenting here as this is the last error propagation patch. I think we need a clean boundary for
> the error propagation. As you said a while back I think we can propagate error for all non-static
> functions from mpam_devices.c and the callers can discard the errors if appropriate. For instance,
> mpam_reset_class_locked() should propagate any error.

So I checked all non-static functions in mpam_devices.c: most either 
don't deal with MSCs at all, or already propagate errors.
The two outliers are mpam_disable(), which must be "void", due to it 
being called via DECLARE_WORK, and mpam_reset_class_locked(), as you 
write above. The latter is a bit sad, because the caller is void, so any 
errors would be discarded there anyway, but for the sake of completeness 
we should indeed propagate here.

And I guess it doesn't make sense to continue the vMSC or RIS list 
iterations after detecting the first error, but we just bail out 
immediately? Because any error would trigger mpam_disable() anyway?

Cheers,
Andre


> 
> Thanks,
> 
> Ben
> 
>> ---
>>   drivers/resctrl/mpam_devices.c | 31 +++++++++++++++----------------
>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 558efab75f911..eb26c25b9f970 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1105,7 +1105,9 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>   	}
>>   
>>   	/* Clear any stale errors */
>> -	mpam_msc_clear_esr(msc);
>> +	ret = mpam_msc_clear_esr(msc);
>> +	if (ret)
>> +		return ret;
>>   
>>   	spin_lock(&partid_max_lock);
>>   	mpam_partid_max = min(mpam_partid_max, msc->partid_max);
>> @@ -2668,9 +2670,7 @@ static int mpam_enable_msc_ecr(void *_msc)
>>   {
>>   	struct mpam_msc *msc = _msc;
>>   
>> -	__mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
>> -
>> -	return 0;
>> +	return __mpam_write_reg(msc, MPAMF_ECR, MPAMF_ECR_INTEN);
>>   }
>>   
>>   /* This can run in mpam_disable(), and the interrupt handler on the same CPU */
>> @@ -2678,9 +2678,7 @@ static int mpam_disable_msc_ecr(void *_msc)
>>   {
>>   	struct mpam_msc *msc = _msc;
>>   
>> -	__mpam_write_reg(msc, MPAMF_ECR, 0);
>> -
>> -	return 0;
>> +	return __mpam_write_reg(msc, MPAMF_ECR, 0);
>>   }
>>   
>>   static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>> @@ -2777,11 +2775,13 @@ static int mpam_register_irqs(void)
>>   				return err;
>>   		}
>>   
>> -		mutex_lock(&msc->error_irq_lock);
>> -		msc->error_irq_req = true;
>> -		mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>> -		msc->error_irq_hw_enabled = true;
>> -		mutex_unlock(&msc->error_irq_lock);
>> +		scoped_guard(mutex, &msc->error_irq_lock) {
>> +			msc->error_irq_req = true;
>> +			err = mpam_touch_msc(msc, mpam_enable_msc_ecr, msc);
>> +			if (err)
>> +				return err;
>> +			msc->error_irq_hw_enabled = true;
>> +		}
>>   	}
>>   
>>   	return 0;
>> @@ -2800,10 +2800,10 @@ static void mpam_unregister_irqs(void)
>>   		if (irq <= 0)
>>   			continue;
>>   
>> -		mutex_lock(&msc->error_irq_lock);
>> +		guard(mutex)(&msc->error_irq_lock);
>>   		if (msc->error_irq_hw_enabled) {
>> -			mpam_touch_msc(msc, mpam_disable_msc_ecr, msc);
>> -			msc->error_irq_hw_enabled = false;
>> +			if (!mpam_touch_msc(msc, mpam_disable_msc_ecr, msc))
>> +				msc->error_irq_hw_enabled = false;
>>   		}
>>   
>>   		if (msc->error_irq_req) {
>> @@ -2815,7 +2815,6 @@ static void mpam_unregister_irqs(void)
>>   			}
>>   			msc->error_irq_req = false;
>>   		}
>> -		mutex_unlock(&msc->error_irq_lock);
>>   	}
>>   }
>>   
>