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

[Sai Prakash Ranjan]

On 11/22/2021 8:00 PM, Arnd Bergmann wrote:
> On Mon, Nov 22, 2021 at 3:19 PM Sai Prakash Ranjan
> <quic_saipraka@xxxxxxxxxxx> 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