Re: [PATCHv4 2/2] arm64/io: Add a header for mmio access instrumentation

[Sai Prakash Ranjan <quic_saipraka@quicinc.com>]

On 11/22/2021 8:00 PM, Arnd Bergmann wrote:
> On Mon, Nov 22, 2021 at 3:19 PM Sai Prakash Ranjan
> <quic_saipraka@quicinc.com> wrote:
>> On 11/22/2021 7:29 PM, Arnd Bergmann wrote:
>>> I think this would be a lot less confusing to readers, as it is implemented
>>> exactly in the place that has the normal definition, and it can also have
>>> somewhat more logical semantics by only instrumenting the
>>> normal/relaxed/ioport accessors but not the __raw_* versions that
>>> are meant to be little more than a pointer dereference.
>> But how is this different from logic in atomic-instrumented.h which also
>> has asm-generic version?
>> Initial review few years back mentioned about having something similar
>> to atomic instrumentation
>> and hence it was implemented with the similar approach keeping
>> instrumentation out of arch specific details.
> This is only a cosmetic difference. I usually prefer fewer indirections,
> and I like the way that include/asm-generic/io.h only has all the
> normal 'static inline' definitions spelled out, and calling the __raw_*
> versions. Your version adds an extra layer with the arch_raw_readl(),
> which I'd prefer to avoid.

I'm ok with your preference as long as we have some way to log these 
MMIO accesses.

>> And if we do move this instrumentation to asm-generic/io.h, how will
>> that be executed since
>> the arch specifc read{b,w,l,q} overrides this generic version?
> As I understand it, your version also requires architecture specific 
> changes, so that would be the same: it only works for architectures 
> that get the definition of readl()/readl_relaxed()/inl()/... from 
> include/asm-generic/io.h and only override the __raw version. Arnd

Sorry, I didn't get this part, so  I am trying this on ARM64:

arm64/include/asm/io.h has read{b,l,w,q} defined.
include/asm-generic/io.h has below:
   #ifndef readl
   #define readl readl
   static inline u32 readl(const volatile void __iomem *addr)

and we include asm-generic/io.h in arm64/include/asm/io.h at the end 
after the definitions for arm64 mmio accesors.
So arch implementation here overrides generic ones as I see it, am I 
missing something? I even confirmed this
with some trace_printk to generic and arch specific definitions of readl 
and I see arch specific ones being called.

Thanks,
Sai