Re: [PATCH v2 03/15] arm_mpam: propagate MSC read errors for hw_probe functions

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

Hi Ben,

thanks for having a thorough look!

On 7/1/26 22:00, Ben Horgan wrote:
> Hi Andre,
> 
> On 7/2/26 17:22, Andre Przywara wrote:
>> Allow the functions probing for MSC hardware and features 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 | 62 ++++++++++++++++++++++++----------
>>   1 file changed, 44 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index ce8738adb6ff..011d1e3544d7 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -800,20 +800,22 @@ static bool mpam_ris_hw_probe_csu_nrdy(struct mpam_msc_ris *ris)
>>   	mpam_write_monsel_reg(msc, CFG_CSU_CTL, ctl_val);
>>   
>>   	_mpam_write_monsel_reg(msc, MSMON_CSU, MSMON___NRDY);
>> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
>> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
>> +		return false;
> 
> This early return leaves the mon_sel lock held.

Ouch, well spotted. I think this "if (WARN_ON_ONCE(...))" wrapper 
distracted me from the lock...
Changed it to use "goto out_unlock;"

> 
>>   	can_set = now & MSMON___NRDY;
> 
> As you're adding early returns to this function does it make sense to
> return early if can_set is false?

Well, this could have been done before as well, but it executes the 
remaining accesses nevertheless. I don't feel like judging whether this 
has a side effect, so just mimicked the current behaviour.
I feel like this should be a separate patch, if we need it.

Thanks,
Andre
  >>
>>   	_mpam_write_monsel_reg(msc, MSMON_CSU, 0);
>>   	/* Configuration change to try and coax hardware into setting nrdy */
>>   	mpam_write_monsel_reg(msc, CFG_CSU_FLT, 0x1);
>> -	_mpam_read_monsel_reg(msc, MSMON_CSU, &now);
>> +	if (_mpam_read_monsel_reg(msc, MSMON_CSU, &now))
>> +		return false;
>>   	can_clear = !(now & MSMON___NRDY);
>>   	mpam_mon_sel_unlock(msc);
>>   
>>   	return (!can_set || !can_clear);
>>   }
>>   
>> -static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>> +static int mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   {
>>   	int err;
>>   	struct mpam_msc *msc = ris->vmsc->msc;
>> @@ -828,7 +830,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   	if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) {
>>   		u32 ccap_features;
>>   
>> -		mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features);
>> +		err = mpam_read_partsel_reg(msc, CCAP_IDR, &ccap_features);
>> +		if (err)
>> +			return err;
>>   
>>   		props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features);
>>   		if (props->cmax_wd &&
>> @@ -853,7 +857,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   	if (FIELD_GET(MPAMF_IDR_HAS_CPOR_PART, ris->idr)) {
>>   		u32 cpor_features;
>>   
>> -		mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features);
>> +		err = mpam_read_partsel_reg(msc, CPOR_IDR, &cpor_features);
>> +		if (err)
>> +			return err;
>>   
>>   		props->cpbm_wd = FIELD_GET(MPAMF_CPOR_IDR_CPBM_WD, cpor_features);
>>   		if (props->cpbm_wd)
>> @@ -863,8 +869,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   	/* Memory bandwidth partitioning */
>>   	if (FIELD_GET(MPAMF_IDR_HAS_MBW_PART, ris->idr)) {
>>   		u32 mbw_features;
>> -
>> -		mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features);
>> +		err = mpam_read_partsel_reg(msc, MBW_IDR, &mbw_features);
>> +		if (err)
>> +			return err;
>>   
>>   		/* portion bitmap resolution */
>>   		props->mbw_pbm_bits = FIELD_GET(MPAMF_MBW_IDR_BWPBM_WD, mbw_features);
>> @@ -893,8 +900,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   	/* Priority partitioning */
>>   	if (FIELD_GET(MPAMF_IDR_HAS_PRI_PART, ris->idr)) {
>>   		u32 pri_features;
>> -
>> -		mpam_read_partsel_reg(msc, PRI_IDR, &pri_features);
>> +		err = mpam_read_partsel_reg(msc, PRI_IDR, &pri_features);
>> +		if (err)
>> +			return err;
>>   
>>   		props->intpri_wd = FIELD_GET(MPAMF_PRI_IDR_INTPRI_WD, pri_features);
>>   		if (props->intpri_wd && FIELD_GET(MPAMF_PRI_IDR_HAS_INTPRI, pri_features)) {
>> @@ -915,7 +923,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   	if (FIELD_GET(MPAMF_IDR_HAS_MSMON, ris->idr)) {
>>   		u32 msmon_features;
>>   
>> -		mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features);
>> +		err = mpam_read_partsel_reg(msc, MSMON_IDR, &msmon_features);
>> +		if (err)
>> +			return err;
>>   
>>   		/*
>>   		 * If the firmware max-nrdy-us property is missing, the
>> @@ -928,7 +938,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   		if (FIELD_GET(MPAMF_MSMON_IDR_MSMON_CSU, msmon_features)) {
>>   			u32 csumonidr;
>>   
>> -			mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr);
>> +			err = mpam_read_partsel_reg(msc, CSUMON_IDR, &csumonidr);
>> +			if (err)
>> +				return err;
>>   
>>   			props->num_csu_mon = FIELD_GET(MPAMF_CSUMON_IDR_NUM_MON, csumonidr);
>>   			if (props->num_csu_mon) {
>> @@ -954,7 +966,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   			bool has_long;
>>   			u32 mbwumon_idr;
>>   
>> -			mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr);
>> +			err = mpam_read_partsel_reg(msc, MBWUMON_IDR, &mbwumon_idr);
>> +			if (err)
>> +				return err;
>>   
>>   			props->num_mbwu_mon = FIELD_GET(MPAMF_MBWUMON_IDR_NUM_MON, mbwumon_idr);
>>   			if (props->num_mbwu_mon) {
>> @@ -987,16 +1001,22 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>>   		u16 partid_max;
>>   		u32 nrwidr;
>>   
>> -		mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr);
>> +		err = mpam_read_partsel_reg(msc, PARTID_NRW_IDR, &nrwidr);
>> +		if (err)
>> +			return err;
>> +
>>   		partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
>>   
>>   		mpam_set_feature(mpam_feat_partid_nrw, props);
>>   		msc->partid_max = min(msc->partid_max, partid_max);
>>   	}
>> +
>> +	return 0;
>>   }
>>   
>>   static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>   {
>> +	int ret;
>>   	u64 idr;
>>   	u16 partid_max;
>>   	u8 ris_idx, pmg_max;
>> @@ -1012,10 +1032,12 @@ 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);
>> -	mpam_msc_read_idr(msc, &idr);
>> -	mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
>> -
>> +	ret = mpam_msc_read_idr(msc, &idr);
>> +	if (!ret)
>> +		ret = mpam_read_partsel_reg(msc, IIDR, &msc->iidr);
>>   	mutex_unlock(&msc->part_sel_lock);
>> +	if (ret)
>> +		return ret;
>>   
>>   	mpam_enable_quirks(msc);
>>   
>> @@ -1028,8 +1050,10 @@ 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);
>> -		mpam_msc_read_idr(msc, &idr);
>> +		ret = mpam_msc_read_idr(msc, &idr);
>>   		mutex_unlock(&msc->part_sel_lock);
>> +		if (ret)
>> +			return ret;
>>   
>>   		partid_max = FIELD_GET(MPAMF_IDR_PARTID_MAX, idr);
>>   		pmg_max = FIELD_GET(MPAMF_IDR_PMG_MAX, idr);
>> @@ -1046,8 +1070,10 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
>>   
>>   		mutex_lock(&msc->part_sel_lock);
>>   		__mpam_part_sel(ris_idx, 0, msc);
>> -		mpam_ris_hw_probe(ris);
>> +		ret = mpam_ris_hw_probe(ris);
>>   		mutex_unlock(&msc->part_sel_lock);
>> +		if (ret)
>> +			return ret;
>>   	}
>>   
>>   	/* Clear any stale errors */
>