Re: [PATCH v7 07/11] arm_mpam: propagate MSC access errors for mpam_reprogram_ris_partid()

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

Hi Jonathan,

On 8/4/26 00:28, Jonathan Cameron wrote:
> On Fri, 31 Jul 2026 19:03:20 +0200
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> Allow the mpam_reprogram_ris_partid() function check for and return
> 
> function to check
> 
>> errors, and propagate MSC read and write errors from the lower level up.
>> This also covers the callers of this function: mpam_reset_ris() and
>> apply_config().
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> One thing below to perhaps modify if you are respinning
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Thanks!

>> ---
>>   drivers/resctrl/mpam_devices.c | 136 ++++++++++++++++++++++-----------
>>   1 file changed, 93 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 38450c55e45e..32088ad1d67e 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -1601,7 +1601,7 @@ void mpam_msmon_reset_mbwu(struct mpam_component *comp, struct mon_cfg *ctx)
>>   	}
>>   }
>>   
>> -static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
>> +static int mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
>>   {
>>   	u32 num_words, msb;
>>   	u32 bm = ~0;
>> @@ -1610,15 +1610,19 @@ static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd)
>>   	lockdep_assert_held(&msc->part_sel_lock);
>>   
>>   	if (wd == 0)
>> -		return;
>> +		return 0;
>>   
>>   	/*
>>   	 * Write all ~0 to all but the last 32bit-word, which may
>>   	 * have fewer bits...
>>   	 */
>>   	num_words = DIV_ROUND_UP(wd, 32);
>> -	for (i = 0; i < num_words - 1; i++, reg += sizeof(bm))
>> -		__mpam_write_reg(msc, reg, bm);
>> +	for (i = 0; i < num_words - 1; i++, reg += sizeof(bm)) {
> 
> Not really related to what you are doing here, but if you like you could
> reduce the scope of i via
> 
> 	for (int i = 0;...
> 
> as it isn't used outside of this loop.

Yeah, true, but I'd rather keep it, otherwise I might get comments about 
unrelated changes ;-)

> 
>> +		int ret = __mpam_write_reg(msc, reg, bm);
>> +
>> +		if (ret)
>> +			return ret;
> Personally I don't like this style but feel free to ignore as this is

I know what you mean, I am always in conflict about the on-the-spot 
initialisation and the empty line requirement, tearing apart the error 
check here. Changed to what you suggested.

Thanks,
Andre.


> very much a personal taste thing. I'd do
> 		int ret;
> 
> 		ret = __mpam_write_reg(msc, reg, bm);
> 		if (ret)
> 			return ret;
> 	}
> just to keep the source of error and check for error next to each other.
> 
>> +	}
> 
>