Re: [RFC 1/2] Bluetooth: hci_conn: Add tracepoints for debugging

[Paul Menzel]

[Cc: +linux-trace-kernel@vger.kernel.org]

Dear Yutan,

Thank you for your patch.

Am 08.12.25 um 13:32 schrieb wang.ya...@zte.com.cn:
From: Qiu Yutan <qiu.yu...@zte.com.cn>

Add tracepoints for tracking hci_conn_hold, hci_conn_drop, and hci_conn_del
to facilitate debugging and viewing call stacks.

I’d add a blank line between paragraphs.

The existing Bluetooth debugging method, BT_DBG, cannot trace call stacks.

It’d be great if you added an example output, how to use hte new trace 
points.

Signed-off-by: Qiu Yutan <qiu.yu...@zte.com.cn>
Signed-off-by: Wang Yaxin <wang.ya...@zte.com.cn>
Signed-off-by: xu xin <xu.xi...@zte.com.cn>

Please start each name with a capital letter.

Signed-off-by: Chen Junlin <chen.jun...@zte.com.cn>
---
  include/net/bluetooth/hci_core.h |  3 ++
  include/trace/events/bluetooth.h | 80 ++++++++++++++++++++++++++++++++
  net/bluetooth/hci_conn.c         |  4 ++
  3 files changed, 87 insertions(+)
  create mode 100644 include/trace/events/bluetooth.h

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2b261e74e2c4..5e01e6c501c1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -29,6 +29,7 @@
  #include <linux/idr.h>
  #include <linux/leds.h>
  #include <linux/rculist.h>
+#include <trace/events/bluetooth.h>

  #include <net/bluetooth/hci.h>
  #include <net/bluetooth/hci_drv.h>
@@ -1620,6 +1621,7 @@ static inline void hci_conn_put(struct hci_conn *conn)

  static inline struct hci_conn *hci_conn_hold(struct hci_conn *conn)
  {
+       trace_hci_conn_hold(conn);
        BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));

        atomic_inc(&conn->refcnt);
@@ -1630,6 +1632,7 @@ static inline struct hci_conn *hci_conn_hold(struct 
hci_conn *conn)

  static inline void hci_conn_drop(struct hci_conn *conn)
  {
+       trace_hci_conn_drop(conn);
        BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));

        if (atomic_dec_and_test(&conn->refcnt)) {
diff --git a/include/trace/events/bluetooth.h b/include/trace/events/bluetooth.h
new file mode 100644
index 000000000000..dd6446263e83
--- /dev/null
+++ b/include/trace/events/bluetooth.h
@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM bluetooth
+
+#if !defined(_TRACE_BLUETOOTH_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_BLUETOOTH_H
+
+#include <linux/tracepoint.h>
+#include <net/bluetooth/hci_core.h>
+
+TRACE_EVENT(hci_conn_hold,
+
+       TP_PROTO(void *conn_ptr),
+
+       TP_ARGS(conn_ptr),
+
+       TP_STRUCT__entry(
+               __field(void *, conn_addr)
+               __field(int, refcnt)
+       ),
+
+       TP_fast_assign(
+               struct hci_conn *conn = (struct hci_conn *)conn_ptr;
+
+               __entry->conn_addr = conn;
+               __entry->refcnt = atomic_read(&conn->refcnt);
+       ),
+
+       TP_printk("conn_addr=%p, orig refcnt=%d",
+                       __entry->conn_addr, __entry->refcnt)
+)
+
+TRACE_EVENT(hci_conn_drop,
+
+       TP_PROTO(void *conn_ptr),
+
+       TP_ARGS(conn_ptr),
+
+       TP_STRUCT__entry(
+               __field(void *, conn_addr)
+               __field(int, refcnt)
+       ),
+
+       TP_fast_assign(
+               struct hci_conn *conn = (struct hci_conn *)conn_ptr;
+
+               __entry->conn_addr = conn;
+               __entry->refcnt = atomic_read(&conn->refcnt);
+       ),
+
+       TP_printk("conn_addr=%p, orig refcnt=%d",
+                       __entry->conn_addr, __entry->refcnt)
+)
+
+TRACE_EVENT(hci_conn_del,
+
+       TP_PROTO(void *conn_ptr),
+
+       TP_ARGS(conn_ptr),
+
+       TP_STRUCT__entry(
+               __field(void *, conn_addr)
+                __field(int, refcnt)
+       ),
+
+       TP_fast_assign(
+               struct hci_conn *conn = (struct hci_conn *)conn_ptr;
+
+               __entry->conn_addr = conn;
+               __entry->refcnt = atomic_read(&conn->refcnt);
+       ),
+
+       TP_printk("conn_addr=%p, orig refcnt=%d",
+                       __entry->conn_addr, __entry->refcnt)
+)

All definitions look the same besides the name. Any idea, how the 
duplication can be avoided?

+
+#endif /* _TRACE_BLUETOOTH_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 99efeed6a766..74a02cf7ba14 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -35,6 +35,9 @@
  #include <net/bluetooth/iso.h>
  #include <net/bluetooth/mgmt.h>

+#define CREATE_TRACE_POINTS
+#include <trace/events/bluetooth.h>
+
  #include "smp.h"
  #include "eir.h"

@@ -1129,6 +1132,7 @@ static void hci_conn_unlink(struct hci_conn *conn)

  void hci_conn_del(struct hci_conn *conn)
  {
+       trace_hci_conn_del(conn);
        struct hci_dev *hdev = conn->hdev;

        BT_DBG("%s hcon %p handle %d", hdev->name, conn, conn->handle);

Thank you for working on that. I’t great to see.


Kind regards,

Paul