Hi Marek, On Mon, 24 Oct 2022 at 09:43, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote: > > 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! > So, what do you think about this? > [snip]
