Messages in this thread | | | From | Arnd Bergmann <> | Date | Mon, 6 Dec 2021 10:09:57 +0100 | Subject | Re: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors |
| |
On Mon, Dec 6, 2021 at 9:28 AM Sai Prakash Ranjan <quic_saipraka@quicinc.com> wrote: > +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) > +#include <linux/tracepoint-defs.h> > + > +DECLARE_TRACEPOINT(rwmmio_write); > +DECLARE_TRACEPOINT(rwmmio_read); > + > +#define rwmmio_tracepoint_active(t) tracepoint_enabled(t) > +void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr); > +void log_read_mmio(u8 width, const volatile void __iomem *addr); > + > +#else > + > +#define rwmmio_tracepoint_active(t) false > +static inline void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr) {} > +static inline void log_read_mmio(u8 width, const volatile void __iomem *addr) {} > + > +#endif /* CONFIG_TRACE_MMIO_ACCESS */ > > /* > * __raw_{read,write}{b,w,l,q}() access memory in native endianness. > @@ -149,6 +166,8 @@ static inline u8 readb(const volatile void __iomem *addr) > { > u8 val; > > + if (rwmmio_tracepoint_active(rwmmio_read)) > + log_read_mmio(8, addr); > __io_br(); > val = __raw_readb(addr); > __io_ar(val);
For readability, it may be nicer to fold the two lines you add for each helper into one, such as
void __log_write_mmio(u64 val, u8 width, volatile void __iomem *addr); #define log_write_mmio(val, widtg, addr) do { \ if (tracepoint_enabled(rwmmio_read)) \ __log_write_mmio((val), (width), (addr)); \ } while (0)
I wonder if it may even be better to not check for tracepoint_active() in the inline function at all but always enter the external function when built-in. This means we do run into the branch, but it also reduces the i-cache footprint.
For general functionality, I think it would be better to trace the returned value from the read, but I don't know if that defeats the purpose you are interested in, since it requires the tracing to come after the __raw_read.
Arnd
| |