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

[Ben Horgan <ben.horgan@arm.com>]

Hi Andre,

On 7/10/26 22:40, Andre Przywara wrote:
> Hi Jonathan,
> 
> many thanks for the time you spent on this - though it wasn't particularly pretty, I guess.
> 
> This is just a quick reply, unfortunately (well...) I will be on holidays next week, so cannot reply
> in detail now.
> 
> On 7/10/26 20:11, Jonathan Cameron wrote:
>> On Fri, 10 Jul 2026 16:45:05 +0200
>> Andre Przywara <andre.przywara@arm.com> 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@arm.com>
>>
>> Why do it in this order? It seems like it is a path for some code churn
>> from patch to path.
> 
> Well, this is pain however you do this - as you figured. And I knew this before, that's why I
> avoided this in v1, but then found we have to bite the bullet at some point anyway, so we could as
> well just do it now.
> 
> So I did the change in one go, and the diff was completely unreadable and even more so unreviewable.
> Then I figured to start with the actual root cause: the low level accessors, to show the motivation,
> then split the rest up in reasonably small chunks. And with the unavoidable dependencies, this led
> to the structure you see, with the added churn of requested changes and refactors between v2 and v3.
> 

From my point view, I think the order of the patches is ok and I'd consider it a bonus if you do
find a clean way to avoid changing the same line twice in this patch set. What I'm more interested
in is that the code itself is clean and consistent but with correctness trumping everything. I
hadn't discovered the ACQUIRE() and ACQUIRE_ERR() before but I do expect that, as Jonathan points
out, they could be used judiciously to make the code clearer.

Thanks,

Ben