| Message ID | 6673a2e73d3dd4c7aa01fee9b26cc4a52176ba7a.1651139070.git.quic_saipraka@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | lib/rwmmio/arm64: Add support to trace register reads/writes | expand |
On Thu, Apr 28, 2022 at 03:25:30PM +0530, Sai Prakash Ranjan wrote: > Add logging support for MMIO high level accessors such as read{b,w,l,q} > and their relaxed versions to aid in debugging unexpected crashes/hangs > caused by the corresponding MMIO operation. > > Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> > --- > include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 78 insertions(+), 4 deletions(-) > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 7ce93aaf69f8..99090722cb4b 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -10,6 +10,7 @@ > #include <asm/page.h> /* I/O is all done through memory accesses */ > #include <linux/string.h> /* for memset() and memcpy() */ > #include <linux/types.h> > +#include <linux/instruction_pointer.h> > > #ifdef CONFIG_GENERIC_IOMAP > #include <asm-generic/iomap.h> > @@ -61,6 +62,35 @@ > #define __io_par(v) __io_ar(v) > #endif > > +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) Shouldn't you document __DISABLE_TRACE_MMIO__ somewhere? It's not even in the changelog. Put a big comment above this for what is is for and how to use it. Otherwise you will forget all about this in 6 months :) thanks, greg k-h
On 4/28/2022 4:21 PM, Greg KH wrote: > On Thu, Apr 28, 2022 at 03:25:30PM +0530, Sai Prakash Ranjan wrote: >> Add logging support for MMIO high level accessors such as read{b,w,l,q} >> and their relaxed versions to aid in debugging unexpected crashes/hangs >> caused by the corresponding MMIO operation. >> >> Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> >> --- >> include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 78 insertions(+), 4 deletions(-) >> >> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h >> index 7ce93aaf69f8..99090722cb4b 100644 >> --- a/include/asm-generic/io.h >> +++ b/include/asm-generic/io.h >> @@ -10,6 +10,7 @@ >> #include <asm/page.h> /* I/O is all done through memory accesses */ >> #include <linux/string.h> /* for memset() and memcpy() */ >> #include <linux/types.h> >> +#include <linux/instruction_pointer.h> >> >> #ifdef CONFIG_GENERIC_IOMAP >> #include <asm-generic/iomap.h> >> @@ -61,6 +62,35 @@ >> #define __io_par(v) __io_ar(v) >> #endif >> >> +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) > Shouldn't you document __DISABLE_TRACE_MMIO__ somewhere? It's not even > in the changelog. Put a big comment above this for what is is for and > how to use it. Otherwise you will forget all about this in 6 months :) > > thanks, > > greg k-h Didn't you ask me to split the patch to the one actually adding the flag and the one using it. So I have added the description in that patch which adds the flag since we can't add description everywhere we use the flag right :) Thanks, Sai
On Thu, Apr 28, 2022 at 04:51:49PM +0530, Sai Prakash Ranjan wrote: > On 4/28/2022 4:21 PM, Greg KH wrote: > > On Thu, Apr 28, 2022 at 03:25:30PM +0530, Sai Prakash Ranjan wrote: > > > Add logging support for MMIO high level accessors such as read{b,w,l,q} > > > and their relaxed versions to aid in debugging unexpected crashes/hangs > > > caused by the corresponding MMIO operation. > > > > > > Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> > > > --- > > > include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 78 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 7ce93aaf69f8..99090722cb4b 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -10,6 +10,7 @@ > > > #include <asm/page.h> /* I/O is all done through memory accesses */ > > > #include <linux/string.h> /* for memset() and memcpy() */ > > > #include <linux/types.h> > > > +#include <linux/instruction_pointer.h> > > > #ifdef CONFIG_GENERIC_IOMAP > > > #include <asm-generic/iomap.h> > > > @@ -61,6 +62,35 @@ > > > #define __io_par(v) __io_ar(v) > > > #endif > > > +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) > > Shouldn't you document __DISABLE_TRACE_MMIO__ somewhere? It's not even > > in the changelog. Put a big comment above this for what is is for and > > how to use it. Otherwise you will forget all about this in 6 months :) > > > > thanks, > > > > greg k-h > > Didn't you ask me to split the patch to the one actually adding the flag and the one using it. Yes, and isn't this the commit that adds the flag? Or was that on an earlier one that I missed? Ah, it's in patch 6/9 But you should also document it here in the .h file, otherwise the only place it is described is in some random kvm Makefile that no one will ever notice :) thanks, greg k-h
On 4/28/2022 5:32 PM, Greg KH wrote: > On Thu, Apr 28, 2022 at 04:51:49PM +0530, Sai Prakash Ranjan wrote: >> On 4/28/2022 4:21 PM, Greg KH wrote: >>> On Thu, Apr 28, 2022 at 03:25:30PM +0530, Sai Prakash Ranjan wrote: >>>> Add logging support for MMIO high level accessors such as read{b,w,l,q} >>>> and their relaxed versions to aid in debugging unexpected crashes/hangs >>>> caused by the corresponding MMIO operation. >>>> >>>> Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> >>>> --- >>>> include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 78 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h >>>> index 7ce93aaf69f8..99090722cb4b 100644 >>>> --- a/include/asm-generic/io.h >>>> +++ b/include/asm-generic/io.h >>>> @@ -10,6 +10,7 @@ >>>> #include <asm/page.h> /* I/O is all done through memory accesses */ >>>> #include <linux/string.h> /* for memset() and memcpy() */ >>>> #include <linux/types.h> >>>> +#include <linux/instruction_pointer.h> >>>> #ifdef CONFIG_GENERIC_IOMAP >>>> #include <asm-generic/iomap.h> >>>> @@ -61,6 +62,35 @@ >>>> #define __io_par(v) __io_ar(v) >>>> #endif >>>> +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) >>> Shouldn't you document __DISABLE_TRACE_MMIO__ somewhere? It's not even >>> in the changelog. Put a big comment above this for what is is for and >>> how to use it. Otherwise you will forget all about this in 6 months :) >>> >>> thanks, >>> >>> greg k-h >> Didn't you ask me to split the patch to the one actually adding the flag and the one using it. > Yes, and isn't this the commit that adds the flag? Or was that on an > earlier one that I missed? > > Ah, it's in patch 6/9 > > But you should also document it here in the .h file, otherwise the only > place it is described is in some random kvm Makefile that no one will > ever notice :) > > Sure it makes sense, will add it. Thanks, Sai
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ce93aaf69f8..99090722cb4b 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -10,6 +10,7 @@ #include <asm/page.h> /* I/O is all done through memory accesses */ #include <linux/string.h> /* for memset() and memcpy() */ #include <linux/types.h> +#include <linux/instruction_pointer.h> #ifdef CONFIG_GENERIC_IOMAP #include <asm-generic/iomap.h> @@ -61,6 +62,35 @@ #define __io_par(v) __io_ar(v) #endif +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__)) +#include <linux/tracepoint-defs.h> + +DECLARE_TRACEPOINT(rwmmio_write); +DECLARE_TRACEPOINT(rwmmio_post_write); +DECLARE_TRACEPOINT(rwmmio_read); +DECLARE_TRACEPOINT(rwmmio_post_read); + +void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr, + unsigned long caller_addr); +void log_post_write_mmio(u64 val, u8 width, volatile void __iomem *addr, + unsigned long caller_addr); +void log_read_mmio(u8 width, const volatile void __iomem *addr, + unsigned long caller_addr); +void log_post_read_mmio(u64 val, u8 width, const volatile void __iomem *addr, + unsigned long caller_addr); + +#else + +static inline void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr, + unsigned long caller_addr) {} +static inline void log_post_write_mmio(u64 val, u8 width, volatile void __iomem *addr, + unsigned long caller_addr) {} +static inline void log_read_mmio(u8 width, const volatile void __iomem *addr, + unsigned long caller_addr) {} +static inline void log_post_read_mmio(u64 val, u8 width, const volatile void __iomem *addr, + unsigned long caller_addr) {} + +#endif /* CONFIG_TRACE_MMIO_ACCESS */ /* * __raw_{read,write}{b,w,l,q}() access memory in native endianness. @@ -149,9 +179,11 @@ static inline u8 readb(const volatile void __iomem *addr) { u8 val; + log_read_mmio(8, addr, _THIS_IP_); __io_br(); val = __raw_readb(addr); __io_ar(val); + log_post_read_mmio(val, 8, addr, _THIS_IP_); return val; } #endif @@ -162,9 +194,11 @@ static inline u16 readw(const volatile void __iomem *addr) { u16 val; + log_read_mmio(16, addr, _THIS_IP_); __io_br(); val = __le16_to_cpu((__le16 __force)__raw_readw(addr)); __io_ar(val); + log_post_read_mmio(val, 16, addr, _THIS_IP_); return val; } #endif @@ -175,9 +209,11 @@ static inline u32 readl(const volatile void __iomem *addr) { u32 val; + log_read_mmio(32, addr, _THIS_IP_); __io_br(); val = __le32_to_cpu((__le32 __force)__raw_readl(addr)); __io_ar(val); + log_post_read_mmio(val, 32, addr, _THIS_IP_); return val; } #endif @@ -189,9 +225,11 @@ static inline u64 readq(const volatile void __iomem *addr) { u64 val; + log_read_mmio(64, addr, _THIS_IP_); __io_br(); val = __le64_to_cpu(__raw_readq(addr)); __io_ar(val); + log_post_read_mmio(val, 64, addr, _THIS_IP_); return val; } #endif @@ -201,9 +239,11 @@ static inline u64 readq(const volatile void __iomem *addr) #define writeb writeb static inline void writeb(u8 value, volatile void __iomem *addr) { + log_write_mmio(value, 8, addr, _THIS_IP_); __io_bw(); __raw_writeb(value, addr); __io_aw(); + log_post_write_mmio(value, 8, addr, _THIS_IP_); } #endif @@ -211,9 +251,11 @@ static inline void writeb(u8 value, volatile void __iomem *addr) #define writew writew static inline void writew(u16 value, volatile void __iomem *addr) { + log_write_mmio(value, 16, addr, _THIS_IP_); __io_bw(); __raw_writew((u16 __force)cpu_to_le16(value), addr); __io_aw(); + log_post_write_mmio(value, 16, addr, _THIS_IP_); } #endif @@ -221,9 +263,11 @@ static inline void writew(u16 value, volatile void __iomem *addr) #define writel writel static inline void writel(u32 value, volatile void __iomem *addr) { + log_write_mmio(value, 32, addr, _THIS_IP_); __io_bw(); __raw_writel((u32 __force)__cpu_to_le32(value), addr); __io_aw(); + log_post_write_mmio(value, 32, addr, _THIS_IP_); } #endif @@ -232,9 +276,11 @@ static inline void writel(u32 value, volatile void __iomem *addr) #define writeq writeq static inline void writeq(u64 value, volatile void __iomem *addr) { + log_write_mmio(value, 64, addr, _THIS_IP_); __io_bw(); __raw_writeq(__cpu_to_le64(value), addr); __io_aw(); + log_post_write_mmio(value, 64, addr, _THIS_IP_); } #endif #endif /* CONFIG_64BIT */ @@ -248,7 +294,12 @@ static inline void writeq(u64 value, volatile void __iomem *addr) #define readb_relaxed readb_relaxed static inline u8 readb_relaxed(const volatile void __iomem *addr) { - return __raw_readb(addr); + u8 val; + + log_read_mmio(8, addr, _THIS_IP_); + val = __raw_readb(addr); + log_post_read_mmio(val, 8, addr, _THIS_IP_); + return val; } #endif @@ -256,7 +307,12 @@ static inline u8 readb_relaxed(const volatile void __iomem *addr) #define readw_relaxed readw_relaxed static inline u16 readw_relaxed(const volatile void __iomem *addr) { - return __le16_to_cpu(__raw_readw(addr)); + u16 val; + + log_read_mmio(16, addr, _THIS_IP_); + val = __le16_to_cpu(__raw_readw(addr)); + log_post_read_mmio(val, 16, addr, _THIS_IP_); + return val; } #endif @@ -264,7 +320,12 @@ static inline u16 readw_relaxed(const volatile void __iomem *addr) #define readl_relaxed readl_relaxed static inline u32 readl_relaxed(const volatile void __iomem *addr) { - return __le32_to_cpu(__raw_readl(addr)); + u32 val; + + log_read_mmio(32, addr, _THIS_IP_); + val = __le32_to_cpu(__raw_readl(addr)); + log_post_read_mmio(val, 32, addr, _THIS_IP_); + return val; } #endif @@ -272,7 +333,12 @@ static inline u32 readl_relaxed(const volatile void __iomem *addr) #define readq_relaxed readq_relaxed static inline u64 readq_relaxed(const volatile void __iomem *addr) { - return __le64_to_cpu(__raw_readq(addr)); + u64 val; + + log_read_mmio(64, addr, _THIS_IP_); + val = __le64_to_cpu(__raw_readq(addr)); + log_post_read_mmio(val, 64, addr, _THIS_IP_); + return val; } #endif @@ -280,7 +346,9 @@ static inline u64 readq_relaxed(const volatile void __iomem *addr) #define writeb_relaxed writeb_relaxed static inline void writeb_relaxed(u8 value, volatile void __iomem *addr) { + log_write_mmio(value, 8, addr, _THIS_IP_); __raw_writeb(value, addr); + log_post_write_mmio(value, 8, addr, _THIS_IP_); } #endif @@ -288,7 +356,9 @@ static inline void writeb_relaxed(u8 value, volatile void __iomem *addr) #define writew_relaxed writew_relaxed static inline void writew_relaxed(u16 value, volatile void __iomem *addr) { + log_write_mmio(value, 16, addr, _THIS_IP_); __raw_writew(cpu_to_le16(value), addr); + log_post_write_mmio(value, 16, addr, _THIS_IP_); } #endif @@ -296,7 +366,9 @@ static inline void writew_relaxed(u16 value, volatile void __iomem *addr) #define writel_relaxed writel_relaxed static inline void writel_relaxed(u32 value, volatile void __iomem *addr) { + log_write_mmio(value, 32, addr, _THIS_IP_); __raw_writel(__cpu_to_le32(value), addr); + log_post_write_mmio(value, 32, addr, _THIS_IP_); } #endif @@ -304,7 +376,9 @@ static inline void writel_relaxed(u32 value, volatile void __iomem *addr) #define writeq_relaxed writeq_relaxed static inline void writeq_relaxed(u64 value, volatile void __iomem *addr) { + log_write_mmio(value, 64, addr, _THIS_IP_); __raw_writeq(__cpu_to_le64(value), addr); + log_post_write_mmio(value, 64, addr, _THIS_IP_); } #endif
Add logging support for MMIO high level accessors such as read{b,w,l,q} and their relaxed versions to aid in debugging unexpected crashes/hangs caused by the corresponding MMIO operation. Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> --- include/asm-generic/io.h | 82 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 4 deletions(-)