linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Register read and writes tracing
@ 2020-09-28  0:34 Prasad Sodagudi
  2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
  2020-09-28  5:45 ` [PATCH] Register read and writes tracing Sai Prakash Ranjan
  0 siblings, 2 replies; 6+ messages in thread
From: Prasad Sodagudi @ 2020-09-28  0:34 UTC (permalink / raw)
  To: rostedt, mingo, keescook, saiprakash.ranjan
  Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, gregkh, anton,
	arnd, catalin.marinas, ccross, jbaron, jim.cromie, joe, joel,
	psodagud

Qualcomm team have tried to upstreaming the register trace buffer(RTB) use case earlier - [1]
with pstore approach. In that discussion, there was suggestion to use the ftrace events for
tracking the register reads and writes. In this patch, added register read/write operations
tracing support and also add _no_log variants(for example - readl_relaxed_no_log  to readl_relaxed)
functions, which will help to avoid excessive logging for certain register operations
(continuous writes from a loop).  These tracepoints can be used by modules
to register probes and log the data convenient  for silicon vendor format.

[1] - https://lore.kernel.org/lkml/cover.1535119710.git.saiprakash.ranjan@codeaurora.org/

Qualcomm teams uses these logs for debugging various issues in the product life cycle and
hopping that this logging would help other silicon vendors as this is generic approach.
Please provide your suggestion/comments to bring this patch upstream quality.

Prasad Sodagudi (1):
  tracing: Add register read and write tracing support

 arch/arm64/include/asm/io.h    | 117 ++++++++++++++++++++++++++++++++++++++---
 include/linux/iorw.h           |  20 +++++++
 include/trace/events/rwio.h    |  51 ++++++++++++++++++
 kernel/trace/Kconfig           |  11 ++++
 kernel/trace/Makefile          |   1 +
 kernel/trace/trace_readwrite.c |  30 +++++++++++
 6 files changed, 222 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/iorw.h
 create mode 100644 include/trace/events/rwio.h
 create mode 100644 kernel/trace/trace_readwrite.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] tracing: Add register read and write tracing support
  2020-09-28  0:34 [PATCH] Register read and writes tracing Prasad Sodagudi
@ 2020-09-28  0:34 ` Prasad Sodagudi
  2020-09-28  5:17   ` Greg KH
                     ` (2 more replies)
  2020-09-28  5:45 ` [PATCH] Register read and writes tracing Sai Prakash Ranjan
  1 sibling, 3 replies; 6+ messages in thread
From: Prasad Sodagudi @ 2020-09-28  0:34 UTC (permalink / raw)
  To: rostedt, mingo, keescook, saiprakash.ranjan
  Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, gregkh, anton,
	arnd, catalin.marinas, ccross, jbaron, jim.cromie, joe, joel,
	psodagud

Add register read/write operations tracing support.
ftrace events helps trace register read and write
location details of memory mapped IO registers. Also
add _no_log variants the writel_relaxed/readl_relaed
APIs to avoid excessive logging for certain register
operations.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 arch/arm64/include/asm/io.h    | 117 ++++++++++++++++++++++++++++++++++++++---
 include/linux/iorw.h           |  20 +++++++
 include/trace/events/rwio.h    |  51 ++++++++++++++++++
 kernel/trace/Kconfig           |  11 ++++
 kernel/trace/Makefile          |   1 +
 kernel/trace/trace_readwrite.c |  30 +++++++++++
 6 files changed, 222 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/iorw.h
 create mode 100644 include/trace/events/rwio.h
 create mode 100644 kernel/trace/trace_readwrite.c

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index ff50dd7..db5acff 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -9,6 +9,7 @@
 #define __ASM_IO_H
 
 #include <linux/types.h>
+#include <linux/iorw.h>
 #include <linux/pgtable.h>
 
 #include <asm/byteorder.h>
@@ -24,24 +25,28 @@
 #define __raw_writeb __raw_writeb
 static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
 {
+	log_write_io(addr);
 	asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
 #define __raw_writew __raw_writew
 static inline void __raw_writew(u16 val, volatile void __iomem *addr)
 {
+	log_write_io(addr);
 	asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
 #define __raw_writel __raw_writel
 static __always_inline void __raw_writel(u32 val, volatile void __iomem *addr)
 {
+	log_write_io(addr);
 	asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
 #define __raw_writeq __raw_writeq
 static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
 {
+	log_write_io(addr);
 	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
@@ -49,6 +54,7 @@ static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
 static inline u8 __raw_readb(const volatile void __iomem *addr)
 {
 	u8 val;
+	log_read_io(addr);
 	asm volatile(ALTERNATIVE("ldrb %w0, [%1]",
 				 "ldarb %w0, [%1]",
 				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
@@ -61,6 +67,7 @@ static inline u16 __raw_readw(const volatile void __iomem *addr)
 {
 	u16 val;
 
+	log_read_io(addr);
 	asm volatile(ALTERNATIVE("ldrh %w0, [%1]",
 				 "ldarh %w0, [%1]",
 				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
@@ -72,6 +79,7 @@ static inline u16 __raw_readw(const volatile void __iomem *addr)
 static __always_inline u32 __raw_readl(const volatile void __iomem *addr)
 {
 	u32 val;
+	log_read_io(addr);
 	asm volatile(ALTERNATIVE("ldr %w0, [%1]",
 				 "ldar %w0, [%1]",
 				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
@@ -83,6 +91,79 @@ static __always_inline u32 __raw_readl(const volatile void __iomem *addr)
 static inline u64 __raw_readq(const volatile void __iomem *addr)
 {
 	u64 val;
+	log_read_io(addr);
+	asm volatile(ALTERNATIVE("ldr %0, [%1]",
+				 "ldar %0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+		     : "=r" (val) : "r" (addr));
+	return val;
+}
+
+#define __raw_writeb_no_log __raw_writeb_no_log
+static inline void __raw_writeb_no_log(u8 val, volatile void __iomem *addr)
+{
+	asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
+}
+
+#define __raw_writew_no_log __raw_writew_no_log
+static inline void __raw_writew_no_log(u16 val, volatile void __iomem *addr)
+{
+	asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr));
+}
+
+#define __raw_writel_no_log __raw_writel_no_log
+static inline void __raw_writel_no_log(u32 val, volatile void __iomem *addr)
+{
+	asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr));
+}
+
+#define __raw_writeq_no_log __raw_writeq_no_log
+static inline void __raw_writeq_no_log(u64 val, volatile void __iomem *addr)
+{
+	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
+}
+
+#define __raw_readb_no_log __raw_readb_no_log
+static inline u8 __raw_readb_no_log(const volatile void __iomem *addr)
+{
+	u8 val;
+
+	asm volatile(ALTERNATIVE("ldrb %w0, [%1]",
+				 "ldarb %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+		     : "=r" (val) : "r" (addr));
+	return val;
+}
+
+#define __raw_readw_no_log __raw_readw_no_log
+static inline u16 __raw_readw_no_log(const volatile void __iomem *addr)
+{
+	u16 val;
+
+	asm volatile(ALTERNATIVE("ldrh %w0, [%1]",
+				 "ldarh %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+		     : "=r" (val) : "r" (addr));
+	return val;
+}
+
+#define __raw_readl_no_log __raw_readl_no_log
+static inline u32 __raw_readl_no_log(const volatile void __iomem *addr)
+{
+	u32 val;
+
+	asm volatile(ALTERNATIVE("ldr %w0, [%1]",
+				 "ldar %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+		     : "=r" (val) : "r" (addr));
+	return val;
+}
+
+#define __raw_readq_no_log __raw_readq_no_log
+static inline u64 __raw_readq_no_log(const volatile void __iomem *addr)
+{
+	u64 val;
+
 	asm volatile(ALTERNATIVE("ldr %0, [%1]",
 				 "ldar %0, [%1]",
 				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
@@ -121,10 +202,20 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define readl_relaxed(c)	({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
 #define readq_relaxed(c)	({ u64 __r = le64_to_cpu((__force __le64)__raw_readq(c)); __r; })
 
-#define writeb_relaxed(v,c)	((void)__raw_writeb((v),(c)))
-#define writew_relaxed(v,c)	((void)__raw_writew((__force u16)cpu_to_le16(v),(c)))
-#define writel_relaxed(v,c)	((void)__raw_writel((__force u32)cpu_to_le32(v),(c)))
-#define writeq_relaxed(v,c)	((void)__raw_writeq((__force u64)cpu_to_le64(v),(c)))
+#define writeb_relaxed(v, c)	((void)__raw_writeb((v), (c)))
+#define writew_relaxed(v, c)	((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
+#define writel_relaxed(v, c)	((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
+#define writeq_relaxed(v, c)	((void)__raw_writeq((__force u64)cpu_to_le64(v), (c)))
+
+#define readb_relaxed_no_log(c)	({ u8  __r = __raw_readb_no_log(c); __r; })
+#define readw_relaxed_no_log(c)	({ u16 __r = le16_to_cpu((__force __le16)__raw_readw_no_log(c)); __r; })
+#define readl_relaxed_no_log(c)	({ u32 __r = le32_to_cpu((__force __le32)__raw_readl_no_log(c)); __r; })
+#define readq_relaxed_no_log(c)	({ u64 __r = le64_to_cpu((__force __le64)__raw_readq_no_log(c)); __r; })
+
+#define writeb_relaxed_no_log(v, c)	((void)__raw_writeb_no_log((v), (c)))
+#define writew_relaxed_no_log(v, c)	((void)__raw_writew_no_log((__force u16)cpu_to_le16(v), (c)))
+#define writel_relaxed_no_log(v, c)	((void)__raw_writel_no_log((__force u32)cpu_to_le32(v), (c)))
+#define writeq_relaxed_no_log(v, c)	((void)__raw_writeq_no_log((__force u64)cpu_to_le64(v), (c)))
 
 /*
  * I/O memory access primitives. Reads are ordered relative to any
@@ -136,10 +227,20 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(__v); __v; })
 #define readq(c)		({ u64 __v = readq_relaxed(c); __iormb(__v); __v; })
 
-#define writeb(v,c)		({ __iowmb(); writeb_relaxed((v),(c)); })
-#define writew(v,c)		({ __iowmb(); writew_relaxed((v),(c)); })
-#define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c)); })
-#define writeq(v,c)		({ __iowmb(); writeq_relaxed((v),(c)); })
+#define writeb(v, c)		({ __iowmb(); writeb_relaxed((v), (c)); })
+#define writew(v, c)		({ __iowmb(); writew_relaxed((v), (c)); })
+#define writel(v, c)		({ __iowmb(); writel_relaxed((v), (c)); })
+#define writeq(v, c)		({ __iowmb(); writeq_relaxed((v), (c)); })
+
+#define readb_no_log(c)		({ u8  __v = readb_relaxed_no_log(c); __iormb(__v); __v; })
+#define readw_no_log(c)		({ u16 __v = readw_relaxed_no_log(c); __iormb(__v); __v; })
+#define readl_no_log(c)		({ u32 __v = readl_relaxed_no_log(c); __iormb(__v); __v; })
+#define readq_no_log(c)		({ u64 __v = readq_relaxed_no_log(c); __iormb(__v); __v; })
+
+#define writeb_no_log(v, c)	({ __iowmb(); writeb_relaxed_no_log((v), (c)); })
+#define writew_no_log(v, c)	({ __iowmb(); writew_relaxed_no_log((v), (c)); })
+#define writel_no_log(v, c)	({ __iowmb(); writel_relaxed_no_log((v), (c)); })
+#define writeq_no_log(v, c)	({ __iowmb(); writeq_relaxed_no_log((v), (c)); })
 
 /*
  *  I/O port access primitives.
diff --git a/include/linux/iorw.h b/include/linux/iorw.h
new file mode 100644
index 0000000..c7087b4
--- /dev/null
+++ b/include/linux/iorw.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *
+ */
+#ifndef __LOG_IORW_H__
+#define __LOG_IORW_H__
+
+#include <linux/types.h>
+
+#if IS_ENABLED(CONFIG_TRACE_RW)
+void log_write_io(volatile void __iomem *addr);
+void log_read_io(const volatile void __iomem *addr);
+#else
+static inline void log_write_io(volatile void __iomem *addr)
+{ }
+static inline void log_read_io(const volatile void __iomem *addr)
+{ }
+#endif /* CONFIG_TRACE_RW */
+
+#endif /* __LOG_IORW_H__  */
diff --git a/include/trace/events/rwio.h b/include/trace/events/rwio.h
new file mode 100644
index 0000000..b829629
--- /dev/null
+++ b/include/trace/events/rwio.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM rwio
+
+#if !defined(_TRACE_RWIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_RWIO_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(raw_write,
+
+	TP_PROTO(unsigned long fn, volatile void __iomem *addr),
+
+	TP_ARGS(fn, addr),
+
+	TP_STRUCT__entry(
+		__field(u64, fn)
+		__field(u64, addr)
+	),
+
+	TP_fast_assign(
+		__entry->fn = fn;
+		__entry->addr = (u64)addr;
+	),
+
+	TP_printk("%pS write addr=%p\n", __entry->fn, __entry->addr)
+);
+
+TRACE_EVENT(raw_read,
+
+	TP_PROTO(unsigned long fn, const volatile void __iomem *addr),
+
+	TP_ARGS(fn, addr),
+
+	TP_STRUCT__entry(
+		__field(u64, fn)
+		__field(u64, addr)
+	),
+
+	TP_fast_assign(
+		__entry->fn = fn;
+		__entry->addr = (u64)addr;
+	),
+
+	TP_printk("%pS read addr=%p\n", __entry->fn, __entry->addr)
+);
+
+#endif /* _TRACE_PREEMPTIRQ_H */
+
+#include <trace/define_trace.h>
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index a4020c0..e07bbfd 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -81,6 +81,17 @@ config RING_BUFFER_ALLOW_SWAP
 	 Allow the use of ring_buffer_swap_cpu.
 	 Adds a very slight overhead to tracing when enabled.
 
+config TRACE_RW
+	bool "Register read/write tracing"
+	select TRACING
+	default n
+	help
+	  Create tracepoints for IO read/write operations, so that
+	  modules can register hooks to use them.
+
+	  Disable this option, when there is support from corresponding
+	  architecture.
+
 config PREEMPTIRQ_TRACEPOINTS
 	bool
 	depends on TRACE_PREEMPT_TOGGLE || TRACE_IRQFLAGS
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index e153be3..3a09cc2 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -95,4 +95,5 @@ obj-$(CONFIG_BOOTTIME_TRACING) += trace_boot.o
 
 obj-$(CONFIG_TRACEPOINT_BENCHMARK) += trace_benchmark.o
 
+obj-$(CONFIG_TRACE_RW) += trace_readwrite.o
 libftrace-y := ftrace.o
diff --git a/kernel/trace/trace_readwrite.c b/kernel/trace/trace_readwrite.c
new file mode 100644
index 0000000..37fb67d
--- /dev/null
+++ b/kernel/trace/trace_readwrite.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Register read and write tracepoints
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
+#include <linux/module.h>
+#include <linux/ftrace.h>
+#include <linux/iorw.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/rwio.h>
+
+EXPORT_TRACEPOINT_SYMBOL_GPL(raw_write);
+EXPORT_TRACEPOINT_SYMBOL_GPL(raw_read);
+
+void log_write_io(volatile void __iomem *addr)
+{
+	trace_raw_write(CALLER_ADDR0, addr);
+}
+EXPORT_SYMBOL_GPL(log_write_io);
+
+void log_read_io(const volatile void __iomem *addr)
+{
+	trace_raw_read(CALLER_ADDR0, addr);
+}
+EXPORT_SYMBOL_GPL(log_read_io);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracing: Add register read and write tracing support
  2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
@ 2020-09-28  5:17   ` Greg KH
  2020-09-28  5:20   ` Greg KH
  2020-09-28 14:55   ` Steven Rostedt
  2 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-09-28  5:17 UTC (permalink / raw)
  To: Prasad Sodagudi
  Cc: rostedt, mingo, keescook, saiprakash.ranjan, linux-arm-kernel,
	linux-kernel, linux-arm-msm, anton, arnd, catalin.marinas,
	ccross, jbaron, jim.cromie, joe, joel

On Sun, Sep 27, 2020 at 05:34:50PM -0700, Prasad Sodagudi wrote:
> Add register read/write operations tracing support.
> ftrace events helps trace register read and write
> location details of memory mapped IO registers. Also
> add _no_log variants the writel_relaxed/readl_relaed
> APIs to avoid excessive logging for certain register
> operations.
> 
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> ---
>  arch/arm64/include/asm/io.h    | 117 ++++++++++++++++++++++++++++++++++++++---
>  include/linux/iorw.h           |  20 +++++++
>  include/trace/events/rwio.h    |  51 ++++++++++++++++++
>  kernel/trace/Kconfig           |  11 ++++
>  kernel/trace/Makefile          |   1 +
>  kernel/trace/trace_readwrite.c |  30 +++++++++++
>  6 files changed, 222 insertions(+), 8 deletions(-)
>  create mode 100644 include/linux/iorw.h
>  create mode 100644 include/trace/events/rwio.h
>  create mode 100644 kernel/trace/trace_readwrite.c
> 
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index ff50dd7..db5acff 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -9,6 +9,7 @@
>  #define __ASM_IO_H
>  
>  #include <linux/types.h>
> +#include <linux/iorw.h>
>  #include <linux/pgtable.h>
>  
>  #include <asm/byteorder.h>
> @@ -24,24 +25,28 @@
>  #define __raw_writeb __raw_writeb
>  static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
>  {
> +	log_write_io(addr);
>  	asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));

You are just logging the address, what about the data written or read?
Why throw away half of the information here?

Oh, and sending patches that break the build is generally not a good
thing to do :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracing: Add register read and write tracing support
  2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
  2020-09-28  5:17   ` Greg KH
@ 2020-09-28  5:20   ` Greg KH
  2020-09-28 14:55   ` Steven Rostedt
  2 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-09-28  5:20 UTC (permalink / raw)
  To: Prasad Sodagudi
  Cc: rostedt, mingo, keescook, saiprakash.ranjan, linux-arm-kernel,
	linux-kernel, linux-arm-msm, anton, arnd, catalin.marinas,
	ccross, jbaron, jim.cromie, joe, joel

On Sun, Sep 27, 2020 at 05:34:50PM -0700, Prasad Sodagudi wrote:
> +config TRACE_RW
> +	bool "Register read/write tracing"
> +	select TRACING
> +	default n

n is always the default, no need to list it.

And you only did this for one arch, yet you made a generic kernel config
option, is that really wise?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Register read and writes tracing
  2020-09-28  0:34 [PATCH] Register read and writes tracing Prasad Sodagudi
  2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
@ 2020-09-28  5:45 ` Sai Prakash Ranjan
  1 sibling, 0 replies; 6+ messages in thread
From: Sai Prakash Ranjan @ 2020-09-28  5:45 UTC (permalink / raw)
  To: Prasad Sodagudi
  Cc: rostedt, mingo, keescook, linux-arm-kernel, linux-kernel,
	linux-arm-msm, gregkh, anton, arnd, catalin.marinas, ccross,
	jbaron, jim.cromie, joe, joel, Will Deacon

Hi Prasad,

On 2020-09-28 06:04, Prasad Sodagudi wrote:
> Qualcomm team have tried to upstreaming the register trace buffer(RTB)
> use case earlier - [1]
> with pstore approach. In that discussion, there was suggestion to use
> the ftrace events for
> tracking the register reads and writes. In this patch, added register
> read/write operations
> tracing support and also add _no_log variants(for example -
> readl_relaxed_no_log  to readl_relaxed)
> functions, which will help to avoid excessive logging for certain
> register operations
> (continuous writes from a loop).  These tracepoints can be used by 
> modules
> to register probes and log the data convenient  for silicon vendor 
> format.
> 
> [1] -
> https://lore.kernel.org/lkml/cover.1535119710.git.saiprakash.ranjan@codeaurora.org/
> 

Thanks for picking this up again. This kind of looks like going back to
downstream implementation of having log and nolog variants which was
exactly the thing we wanted to avoid.

I believe the reason for this nolog variants is not just to avoid 
excessive
logging but also to provide filtering i.e, selectively trace the 
register
reads/writes from required drivers or subsystems like for example we
wouldn't want to trace register reads/writes from serial drivers, now if
you use these nolog variants  then it will need to be sprinkled all over
the kernel in various drivers to provide this kind of filtering. That 
was
the reason I did not want to introduce these nolog and log variants, 
instead
introduced a way to use dynamic debug [2] to provide this kind of 
filtering.
Dynamic debug provides an array of filtering capacity such as filter by 
files,
folders and even is applicable for modules making it a prime candidate 
for
these kind of scenarios.

So why not use it instead of all these new variants? Then you don't have 
to
export things like you do in this patch and just have to add 
tracepoints.
Also the patch series[1][2] was almost OK'ed(they didn't give a formal 
review)
by arm folks at the time and even acked by Steve[3] except for the 
pstore part.
We have ways to extract trace events from ramdumps via crash utility or 
STM,
so pstore support is not mandatory and can be done later(it is currently 
being
worked on). Plus the link you mentioned was an RFC and there was a new 
version
posted after that[4]. Please take at the series[4] look once and see if 
you
can use that, only thing required I suppose is decouple the pstore 
patches
and you should be good to go.

[1] https://patchwork.kernel.org/patch/10593175/
[2] https://patchwork.kernel.org/patch/10593175/
[3] https://patchwork.kernel.org/patch/10593173/
[4] https://patchwork.kernel.org/cover/10593159/

> Qualcomm teams uses these logs for debugging various issues in the
> product life cycle and
> hopping that this logging would help other silicon vendors as this is
> generic approach.
> Please provide your suggestion/comments to bring this patch upstream 
> quality.
> 
> Prasad Sodagudi (1):
>   tracing: Add register read and write tracing support
> 
>  arch/arm64/include/asm/io.h    | 117 
> ++++++++++++++++++++++++++++++++++++++---
>  include/linux/iorw.h           |  20 +++++++
>  include/trace/events/rwio.h    |  51 ++++++++++++++++++
>  kernel/trace/Kconfig           |  11 ++++
>  kernel/trace/Makefile          |   1 +
>  kernel/trace/trace_readwrite.c |  30 +++++++++++
>  6 files changed, 222 insertions(+), 8 deletions(-)
>  create mode 100644 include/linux/iorw.h
>  create mode 100644 include/trace/events/rwio.h
>  create mode 100644 kernel/trace/trace_readwrite.c

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tracing: Add register read and write tracing support
  2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
  2020-09-28  5:17   ` Greg KH
  2020-09-28  5:20   ` Greg KH
@ 2020-09-28 14:55   ` Steven Rostedt
  2 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2020-09-28 14:55 UTC (permalink / raw)
  To: Prasad Sodagudi
  Cc: mingo, keescook, saiprakash.ranjan, linux-arm-kernel,
	linux-kernel, linux-arm-msm, gregkh, anton, arnd,
	catalin.marinas, ccross, jbaron, jim.cromie, joe, joel

On Sun, 27 Sep 2020 17:34:50 -0700
Prasad Sodagudi <psodagud@codeaurora.org> wrote:

> Add register read/write operations tracing support.
> ftrace events helps trace register read and write
> location details of memory mapped IO registers. Also
> add _no_log variants the writel_relaxed/readl_relaed
> APIs to avoid excessive logging for certain register
> operations.

As mentioned elsewhere, I don't see a reason for "nolog" variants if it
is just to avoid logging too much. You can easily filter on the
recording side.


> --- /dev/null
> +++ b/include/linux/iorw.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *
> + */
> +#ifndef __LOG_IORW_H__
> +#define __LOG_IORW_H__
> +
> +#include <linux/types.h>
> +
> +#if IS_ENABLED(CONFIG_TRACE_RW)
> +void log_write_io(volatile void __iomem *addr);
> +void log_read_io(const volatile void __iomem *addr);

So basically, this is always doing a function call, even when tracing
is not enabled. You may want to turn this into a macro, and use the new
interface I'm about to push:

 See https://lore.kernel.org/r/20200925211206.423598568@goodmis.org

Although I'm about to push a v3 (found a config that breaks msr.h)

#if IS_ENABLED(CONFIG_TRACE_RW)
#include <linux/atomic.h>
#include <linux/tracepoint-defs.h>

DECLARE_TRACEPOINT(rwio_write);
DECLARE_TRACEPOINT(rwio_read);

void __log_write_io(volatile void __iomem *addr);
void __log_read_io(const volatile void __iomem *addr);

#define log_write_io(addr) \
	if (tracepoint_enabled(rwio_write)
		__log_write_io(addr)

#define log_read_io(addr) \
	if (tracepoint_enabled(rwio_read)
		__log_read_io(addr)


> +#else
> +static inline void log_write_io(volatile void __iomem *addr)
> +{ }
> +static inline void log_read_io(const volatile void __iomem *addr)
> +{ }
> +#endif /* CONFIG_TRACE_RW */
> +
> +#endif /* __LOG_IORW_H__  */
> diff --git a/include/trace/events/rwio.h b/include/trace/events/rwio.h
> new file mode 100644
> index 0000000..b829629
> --- /dev/null
> +++ b/include/trace/events/rwio.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM rwio
> +
> +#if !defined(_TRACE_RWIO_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_RWIO_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(raw_write,

"raw" is too generic. Call this rwio_write.

> +
> +	TP_PROTO(unsigned long fn, volatile void __iomem *addr),
> +
> +	TP_ARGS(fn, addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, fn)
> +		__field(u64, addr)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->fn = fn;
> +		__entry->addr = (u64)addr;
> +	),
> +
> +	TP_printk("%pS write addr=%p\n", __entry->fn, __entry->addr)
> +);
> +
> +TRACE_EVENT(raw_read,

And this "rwio_read"

-- Steve

> +
> +	TP_PROTO(unsigned long fn, const volatile void __iomem *addr),
> +
> +	TP_ARGS(fn, addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, fn)
> +		__field(u64, addr)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->fn = fn;
> +		__entry->addr = (u64)addr;
> +	),
> +
> +	TP_printk("%pS read addr=%p\n", __entry->fn, __entry->addr)
> +);
> +
> +#endif /* _TRACE_PREEMPTIRQ_H */
> +

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-09-28 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28  0:34 [PATCH] Register read and writes tracing Prasad Sodagudi
2020-09-28  0:34 ` [PATCH] tracing: Add register read and write tracing support Prasad Sodagudi
2020-09-28  5:17   ` Greg KH
2020-09-28  5:20   ` Greg KH
2020-09-28 14:55   ` Steven Rostedt
2020-09-28  5:45 ` [PATCH] Register read and writes tracing Sai Prakash Ranjan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).