Messages in this thread | | | Date | Mon, 6 Dec 2021 15:22:51 +0530 | Subject | Re: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors | From | Sai Prakash Ranjan <> |
| |
On 12/6/2021 2:39 PM, Arnd Bergmann wrote: > 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.
Right, we don't need the tracepoint active check as we declare the tracepoint only when the config is enabled so I can just call log_{read,write}_mmio directly.
> 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. > >
Yes just the trace after read/write won't serve our usecase where we expect crashes/hangs on accessing these registers but internally we did have a log_post_read_mmio() as well, if it is useful then I can add it.
Thanks, Sai
| |