Re: [PATCH 1/2] iommu/exynos: Abstract getting the fault info

[Sam Protsenko]

Hi Marek,

On Fri, 12 Aug 2022 at 14:25, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>
> Hi Sam,
>

[snip]

> > Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
>
> I'm not very happy with converting the sysmmu_fault_info arrays into the
> decoding functions. If I got the code right, adding v7 is still possible
> with the current approach. The main advantage of the array-based
> approach is readability and keeping all the information together in a
> single place.
>
> I agree for the items listed above as 'minor functional changes',
> though. Those sysmmu_fault_info arrays might be a part of sysmmu hw
> variant to avoid decoding hw version for each fault.
>
> I'm not sure that the linear scan is so problematic with regards to the
> performance. You really don't want your drivers to trigger IOMMU fault
> so often during normal operation. It is just a way to get some debugging
> information or handle some exception.
>
> You mentioned that the transaction type is read from the separate
> register in case of v7, but your code (here and in second patch) still
> relies on the reported interrupt bits.
>
> Could you try to rework all your changes in a such way, that the
> sysmmu_fault_info arrays are still used? V7 is really very similar to
> the v5 already supported by the current driver.
>

That's actually how I implemented this patch on my first attempt.
Really didn't like it, because a half of existing sysmmu_fault_info
structure doesn't make sense for v7, and some functionality of v7 has
to be implemented separately from that structure. I'd argue that
previous abstraction is just broken, and doesn't work for all SysMMU
versions anymore. It's easy to see how much difference between v5 and
v7, just by looking at corresponding get_fault_info() functions I
implemented. For example, the transaction type is probed from
different registers using different version, etc. There is also the
need to handle new VM/non-VM registers on v7. Also there is some extra
functionality that will be added later, like multiple translation
domains support, which is also quite different from how things done
for v5.

I'd show more specifics to demonstrate my statements above, but alas I
already deleted my initial implementation (which was exactly what you
suggest). This callback-style HAL seems to be a perfect choice, and I
spent several days just experimenting with different approaches and
seeing all pros and cons. And from my point of view, this way is the
best for providing actual solid abstraction, which doesn't require
adding any workarounds on top of that. I understand that my patch
changes the very conception of how IRQ is handled in this driver, but
I'm still convinced it's a proper way to do that for all v1/v5/v7,
especially w.r.t. further v7 additions, to keep the abstraction solid.
Not that I'm lazy and don't want to rework things :) But in this
particular case I'd go with unchanged patches.

Do you think it's reasonable to take this series as is? I can try and
collect more particular code snippets to demonstrate my point, if you
like.

Thanks!

[snip]