lkml.org 
[lkml]   [2021]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCHv5 4/4] asm-generic/io: Add logging support for MMIO accessors
From
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

\
 
 \ /
  Last update: 2021-12-06 10:53    [W:0.040 / U:0.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site