Re: [PATCH v2 02/15] arm_mpam: propagate MSC read errors for wrapper functions

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

Hi,

On 7/1/26 21:56, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/2/26 17:22, Andre Przywara wrote:
>> Allow the wrapper functions for IDR and ESR accesses to return an
>> error, and propagate read errors from the lower level up.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   drivers/resctrl/mpam_devices.c | 51 +++++++++++++++++++++++-----------
>>   1 file changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index df14b4513382..ce8738adb6ff 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -247,27 +247,34 @@ static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>>   	return true;
>>   }
>>   
>> -static u64 mpam_msc_read_idr(struct mpam_msc *msc)
>> +static int mpam_msc_read_idr(struct mpam_msc *msc, u64 *res)
>>   {
>>   	u32 idr_high = 0, idr_low;
>> +	int ret;
>>   
>>   	lockdep_assert_held(&msc->part_sel_lock);
>>   
>> -	mpam_read_partsel_reg(msc, IDR, &idr_low);
>> -	if (FIELD_GET(MPAMF_IDR_EXT, idr_low))
>> -		mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
>> +	ret = mpam_read_partsel_reg(msc, IDR, &idr_low);
>> +	if (!ret && FIELD_GET(MPAMF_IDR_EXT, idr_low))
>> +		ret = mpam_read_partsel_reg(msc, IDR + 4, &idr_high);
>>   
>> -	return ((u64)idr_high << 32) | idr_low;
>> +	if (!ret)
>> +		*res = ((u64)idr_high << 32) | idr_low;
>> +
>> +	return ret;
>>   }
> 
> Why does the pattern here, use of !ret rather than early return, differ
> from the one you've used in mpam_msc_read_esr()?

No particular reason, I think I switched patterns, for instance 
depending on whether a lock was held or not, or when there are some side 
effect of that function, so probably just repeated the last exercise.

I changed it to return early here.

Cheers,
Andre

> 
>>   
>> -static void mpam_msc_clear_esr(struct mpam_msc *msc)
>> +static int mpam_msc_clear_esr(struct mpam_msc *msc)
>>   {
>>   	u32 esr_low;
>> +	int ret;
>>   
>> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
>> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
>> +	if (ret)
>> +		return ret;
>>   
>>   	if (!esr_low)
>> -		return;
>> +		return 0;
>>   
>>   	/*
>>   	 * Clearing the high/low bits of MPAMF_ESR can not be atomic.
>> @@ -277,18 +284,30 @@ static void mpam_msc_clear_esr(struct mpam_msc *msc)
>>   	 */
>>   	if (msc->has_extd_esr)
>>   		__mpam_write_reg(msc, MPAMF_ESR + 4, 0);
>> +
>>   	__mpam_write_reg(msc, MPAMF_ESR, 0);
>> +
>> +	return 0;
>>   }
>>   
>> -static u64 mpam_msc_read_esr(struct mpam_msc *msc)
>> +static int mpam_msc_read_esr(struct mpam_msc *msc, u64 *res)
>>   {
>>   	u32 esr_high = 0, esr_low;
>> +	int ret;
>>   
>> -	__mpam_read_reg(msc, MPAMF_ESR, &esr_low);
>> -	if (msc->has_extd_esr)
>> -		__mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
>> +	ret = __mpam_read_reg(msc, MPAMF_ESR, &esr_low);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (msc->has_extd_esr) {
>> +		ret = __mpam_read_reg(msc, MPAMF_ESR + 4, &esr_high);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>> -	return ((u64)esr_high << 32) | esr_low;
>> +	*res = ((u64)esr_high << 32) | esr_low;
>> +
>> +	return 0;
>>   }
>>   
>>   static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
>> @@ -993,7 +1012,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>   
>>   	/* Grab an IDR value to find out how many RIS there are */
>>   	mutex_lock(&msc->part_sel_lock);
>> -	idr = mpam_msc_read_idr(msc);
>> +	mpam_msc_read_idr(msc, &idr);
>>   	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
>>   
>>   	mutex_unlock(&msc->part_sel_lock);
>> @@ -1009,7 +1028,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>   	for (ris_idx = 0; ris_idx <= msc->ris_max; ris_idx++) {
>>   		mutex_lock(&msc->part_sel_lock);
>>   		__mpam_part_sel(ris_idx, 0, msc);
>> -		idr = mpam_msc_read_idr(msc);
>> +		mpam_msc_read_idr(msc, &idr);
>>   		mutex_unlock(&msc->part_sel_lock);
>>   
>>   		partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
>> @@ -2492,7 +2511,7 @@ static irqreturn_t __mpam_irq_handler(int irq, struct mpam_msc *msc)
>>   					   &msc->accessibility)))
>>   		return IRQ_NONE;
>>   
>> -	reg = mpam_msc_read_esr(msc);
>> +	mpam_msc_read_esr(msc, &reg);
>>   
>>   	errcode = FIELD_GET(MPAMF_ESR_ERRCODE, reg);
>>   	if (!errcode)
>