Re: [PATCH v3 01/16] arm_mpam: let low level MSC read accessors return an error

[Jonathan Cameron]

On Fri, 10 Jul 2026 16:45:05 +0200
Andre Przywara <andre.przywara@xxxxxxx> wrote:

> The upcoming MPAM-Fb support does not use MMIO primitives to access an
> MSC, but employs a shared-memory/doorbell based firmware protocol.
> Its complexity means that is must be able to handle errors, whereas we
> always assume an MSC access succeeds today.
> 
> Change the __mpam_read_reg() low level accessor function to return the
> requested data through a pointer, and return an error code instead.
> At the moment this is always 0, but this will change with alternative
> MSC access methods.
> Change all users of those MSC read wrappers to comply with the new
> prototype, though at the moment without propagating any errors.
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>

Why do it in this order? It seems like it is a path for some code churn 
from patch to path.  If you started at the outermost calls and
worked in adding error handling at each layer, you wouldn't end up with
the change here to add the parameter, then the same lines changed again
to stash the new return value. Those lines would change just once.
Maybe I'm wrong though and it ends up even messier.

Or as you suggest, maybe just squash the patches, which will also
remove the churn.

Jonathan

> @@ -226,10 +230,11 @@ static inline void _mpam_write_monsel_reg(struct mpam_msc *msc, u16 reg, u32 val
>  
>  static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>  {
> -	u32 aidr = __mpam_read_reg(msc, MPAMF_AIDR);
> -	u32 major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
> -	u32 minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
> +	u32 aidr, major, minor;
>  
> +	__mpam_read_reg(msc, MPAMF_AIDR, &aidr);

This will need updating to handle the error.  If you were to instead do
the patches in the opposite order. So add return value to the outer
most calls that is always 0 then work your way in it should end up
as a fair bit less code churn.

That update is in a later patch.

> +	major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
> +	minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);
>  	/*
>  	 * v0.0 and >v2.x aren't supported, but anything else should be backward
>  	 * compatible to v0.1 or v1.0.