Re: [PATCH v3 4/9] platform/surface: aggregator: Add trace points

2020-12-22 Thread Steven Rostedt
On Mon, 21 Dec 2020 19:39:54 +0100
Maximilian Luz  wrote:

> Add trace points to the Surface Aggregator subsystem core. These trace
> points can be used to track packets, requests, and allocations. They are
> further intended for debugging and testing/validation, specifically in
> combination with the error injection capabilities introduced in the
> subsequent commit.
> 
> Signed-off-by: Maximilian Luz 
> Reviewed-by: Hans de Goede 
> ---
> 
> Changes in v1 (from RFC):
>  - add copyright line
>  - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
>  - pack tracing structs
> 
> Changes in v2:
>  - add compiletime check on SSAM_PTR_UID_LEN
>  - fix ssam_trace_get_command_field_u8() macro
>  - use dedicated trace event class for timeout reaper
>  - use printk specifier for hex prefix instead of hard-coding it
>  - unify comment style
>  - run checkpatch --strict, fix warnings and style issues
> 
> 

>From a tracing point of view:

Acked-by: Steven Rostedt (VMware) 

-- Steve


[PATCH v3 4/9] platform/surface: aggregator: Add trace points

2020-12-21 Thread Maximilian Luz
Add trace points to the Surface Aggregator subsystem core. These trace
points can be used to track packets, requests, and allocations. They are
further intended for debugging and testing/validation, specifically in
combination with the error injection capabilities introduced in the
subsequent commit.

Signed-off-by: Maximilian Luz 
Reviewed-by: Hans de Goede 
---

Changes in v1 (from RFC):
 - add copyright line
 - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
 - pack tracing structs

Changes in v2:
 - add compiletime check on SSAM_PTR_UID_LEN
 - fix ssam_trace_get_command_field_u8() macro
 - use dedicated trace event class for timeout reaper
 - use printk specifier for hex prefix instead of hard-coding it
 - unify comment style
 - run checkpatch --strict, fix warnings and style issues

---
 drivers/platform/surface/aggregator/Makefile  |   3 +
 .../platform/surface/aggregator/controller.c  |   5 +
 drivers/platform/surface/aggregator/core.c|   3 +
 .../surface/aggregator/ssh_packet_layer.c |  26 +-
 .../surface/aggregator/ssh_request_layer.c|  18 +
 drivers/platform/surface/aggregator/trace.h   | 601 ++
 6 files changed, 655 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/surface/aggregator/trace.h

diff --git a/drivers/platform/surface/aggregator/Makefile 
b/drivers/platform/surface/aggregator/Makefile
index faad18d4a7f2..b8b24c8ec310 100644
--- a/drivers/platform/surface/aggregator/Makefile
+++ b/drivers/platform/surface/aggregator/Makefile
@@ -1,6 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0+
 # Copyright (C) 2019-2020 Maximilian Luz 
 
+# For include/trace/define_trace.h to include trace.h
+CFLAGS_core.o = -I$(src)
+
 obj-$(CONFIG_SURFACE_AGGREGATOR) += surface_aggregator.o
 
 surface_aggregator-objs := core.o
diff --git a/drivers/platform/surface/aggregator/controller.c 
b/drivers/platform/surface/aggregator/controller.c
index 775a4509bece..5bcb59ed579d 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -32,6 +32,8 @@
 #include "ssh_msgb.h"
 #include "ssh_request_layer.h"
 
+#include "trace.h"
+
 
 /* -- Safe counters.  
*/
 
@@ -568,6 +570,7 @@ static void __ssam_event_item_free_generic(struct 
ssam_event_item *item)
  */
 static void ssam_event_item_free(struct ssam_event_item *item)
 {
+   trace_ssam_event_item_free(item);
item->ops.free(item);
 }
 
@@ -603,6 +606,8 @@ static struct ssam_event_item *ssam_event_item_alloc(size_t 
len, gfp_t flags)
}
 
item->event.length = len;
+
+   trace_ssam_event_item_alloc(item, len);
return item;
 }
 
diff --git a/drivers/platform/surface/aggregator/core.c 
b/drivers/platform/surface/aggregator/core.c
index 37593234fb31..b6a9dea53592 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -24,6 +24,9 @@
 #include 
 #include "controller.h"
 
+#define CREATE_TRACE_POINTS
+#include "trace.h"
+
 
 /* -- Static controller reference. -- 
*/
 
diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c 
b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index 23c2e31e7d0e..c4f082e57372 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -26,6 +26,8 @@
 #include "ssh_packet_layer.h"
 #include "ssh_parser.h"
 
+#include "trace.h"
+
 /*
  * To simplify reasoning about the code below, we define a few concepts. The
  * system below is similar to a state-machine for packets, however, there are
@@ -228,6 +230,8 @@ static void __ssh_ptl_packet_release(struct kref *kref)
 {
struct ssh_packet *p = container_of(kref, struct ssh_packet, refcnt);
 
+   trace_ssam_packet_release(p);
+
ptl_dbg_cond(p->ptl, "ptl: releasing packet %p\n", p);
p->ops->release(p);
 }
@@ -356,6 +360,7 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet,
buffer->ptr = (u8 *)(*packet + 1);
buffer->len = SSH_MSG_LEN_CTRL;
 
+   trace_ssam_ctrl_packet_alloc(*packet, buffer->len);
return 0;
 }
 
@@ -365,6 +370,7 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet,
  */
 static void ssh_ctrl_packet_free(struct ssh_packet *p)
 {
+   trace_ssam_ctrl_packet_free(p);
kmem_cache_free(ssh_ctrl_packet_cache, p);
 }
 
@@ -398,7 +404,12 @@ static void ssh_packet_next_try(struct ssh_packet *p)
 
lockdep_assert_held(>ptl->queue.lock);
 
-   p->priority = __SSH_PACKET_PRIORITY(base, try + 1);
+   /*
+* Ensure that we write the priority in one go via WRITE_ONCE() so we
+* can access it via READ_ONCE() for tracing. Note that other access
+* is guarded by the queue lock, so no need to use READ_ONCE() there.
+*/
+   WRITE_ONCE(p->priority, __SSH_PACKET_PRIORITY(base, try + 1));
 }
 
 /*