Re: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/6/2021 2:39 PM, Arnd Bergmann wrote:
On Mon, Dec 6, 2021 at 9:28 AM Sai Prakash Ranjan
<quic_saipraka@xxxxxxxxxxx> 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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux