From: Abhishek Pandit-Subedi <[email protected]>
Add devcoredump APIs to hci core so that drivers only have to provide
the dump skbs instead of managing the synchronization and timeouts.
The devcoredump APIs should be used in the following manner:
- hci_devcoredump_init is called to allocate the dump.
- hci_devcoredump_append is called to append any skbs with dump data
OR hci_devcoredump_append_pattern is called to insert a pattern.
- hci_devcoredump_complete is called when all dump packets have been
sent OR hci_devcoredump_abort is called to indicate an error and
cancel an ongoing dump collection.
The high level APIs just prepare some skbs with the appropriate data and
queue it for the dump to process. Packets part of the crashdump can be
intercepted in the driver in interrupt context and forwarded directly to
the devcoredump APIs.
Internally, there are 5 states for the dump: idle, active, complete,
abort and timeout. A devcoredump will only be in active state after it
has been initialized. Once active, it accepts data to be appended,
patterns to be inserted (i.e. memset) and a completion event or an abort
event to generate a devcoredump. The timeout is initialized at the same
time the dump is initialized (defaulting to 10s) and will be cleared
either when the timeout occurs or the dump is complete or aborted.
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Signed-off-by: Manish Mandlik <[email protected]>
Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
---
Changes in v3:
- Add attribute to enable/disable and set default state to disabled
Changes in v2:
- Move hci devcoredump implementation to new files
- Move dump queue and dump work to hci_devcoredump struct
- Add CONFIG_DEV_COREDUMP conditional compile
include/net/bluetooth/coredump.h | 110 +++++++
include/net/bluetooth/hci_core.h | 5 +
net/bluetooth/Makefile | 2 +
net/bluetooth/coredump.c | 504 +++++++++++++++++++++++++++++++
net/bluetooth/hci_core.c | 9 +
net/bluetooth/hci_sync.c | 2 +
6 files changed, 632 insertions(+)
create mode 100644 include/net/bluetooth/coredump.h
create mode 100644 net/bluetooth/coredump.c
diff --git a/include/net/bluetooth/coredump.h b/include/net/bluetooth/coredump.h
new file mode 100644
index 000000000000..db286a679eb7
--- /dev/null
+++ b/include/net/bluetooth/coredump.h
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Google Corporation
+ */
+
+#ifndef __COREDUMP_H
+#define __COREDUMP_H
+
+#define DEVCOREDUMP_TIMEOUT msecs_to_jiffies(10000) /* 10 sec */
+
+typedef int (*dmp_hdr_t)(struct hci_dev *hdev, char *buf, size_t size);
+typedef void (*notify_change_t)(struct hci_dev *hdev, int state);
+
+/* struct hci_devcoredump - Devcoredump state
+ *
+ * @supported: Indicates if FW dump collection is supported by driver
+ * @state: Current state of dump collection
+ * @alloc_size: Total size of the dump
+ * @head: Start of the dump
+ * @tail: Pointer to current end of dump
+ * @end: head + alloc_size for easy comparisons
+ *
+ * @dump_q: Dump queue for state machine to process
+ * @dump_rx: Devcoredump state machine work
+ * @dump_timeout: Devcoredump timeout work
+ *
+ * @dmp_hdr: Create a dump header to identify controller/fw/driver info
+ * @notify_change: Notify driver when devcoredump state has changed
+ */
+struct hci_devcoredump {
+ bool enabled;
+ bool supported;
+
+ enum devcoredump_state {
+ HCI_DEVCOREDUMP_IDLE,
+ HCI_DEVCOREDUMP_ACTIVE,
+ HCI_DEVCOREDUMP_DONE,
+ HCI_DEVCOREDUMP_ABORT,
+ HCI_DEVCOREDUMP_TIMEOUT
+ } state;
+
+ u32 alloc_size;
+ char *head;
+ char *tail;
+ char *end;
+
+ struct sk_buff_head dump_q;
+ struct work_struct dump_rx;
+ struct delayed_work dump_timeout;
+
+ dmp_hdr_t dmp_hdr;
+ notify_change_t notify_change;
+};
+
+#ifdef CONFIG_DEV_COREDUMP
+
+void hci_devcoredump_reset(struct hci_dev *hdev);
+void hci_devcoredump_rx(struct work_struct *work);
+void hci_devcoredump_timeout(struct work_struct *work);
+int hci_devcoredump_register(struct hci_dev *hdev, dmp_hdr_t dmp_hdr,
+ notify_change_t notify_change);
+int hci_devcoredump_init(struct hci_dev *hdev, u32 dmp_size);
+int hci_devcoredump_append(struct hci_dev *hdev, struct sk_buff *skb);
+int hci_devcoredump_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
+int hci_devcoredump_complete(struct hci_dev *hdev);
+int hci_devcoredump_abort(struct hci_dev *hdev);
+
+#else
+
+static inline void hci_devcoredump_reset(struct hci_dev *hdev) {}
+static inline void hci_devcoredump_rx(struct work_struct *work) {}
+static inline void hci_devcoredump_timeout(struct work_struct *work) {}
+
+static inline int hci_devcoredump_register(struct hci_dev *hdev,
+ dmp_hdr_t dmp_hdr,
+ notify_change_t notify_change)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hci_devcoredump_init(struct hci_dev *hdev, u32 dmp_size)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hci_devcoredump_append(struct hci_dev *hdev,
+ struct sk_buff *skb)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hci_devcoredump_append_pattern(struct hci_dev *hdev,
+ u8 pattern, u32 len)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hci_devcoredump_complete(struct hci_dev *hdev)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hci_devcoredump_abort(struct hci_dev *hdev)
+{
+ return -EOPNOTSUPP;
+}
+
+#endif /* CONFIG_DEV_COREDUMP */
+
+#endif /* __COREDUMP_H */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 15237ee5f761..9ccc034c8fde 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -32,6 +32,7 @@
#include <net/bluetooth/hci.h>
#include <net/bluetooth/hci_sync.h>
#include <net/bluetooth/hci_sock.h>
+#include <net/bluetooth/coredump.h>
/* HCI priority */
#define HCI_PRIO_MAX 7
@@ -582,6 +583,10 @@ struct hci_dev {
const char *fw_info;
struct dentry *debugfs;
+#ifdef CONFIG_DEV_COREDUMP
+ struct hci_devcoredump dump;
+#endif
+
struct device dev;
struct rfkill *rfkill;
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index a52bba8500e1..4e894e452869 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -17,6 +17,8 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o hci_codec.o \
eir.o hci_sync.o
+bluetooth-$(CONFIG_DEV_COREDUMP) += coredump.o
+
bluetooth-$(CONFIG_BT_BREDR) += sco.o
bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
bluetooth-$(CONFIG_BT_LEDS) += leds.o
diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
new file mode 100644
index 000000000000..aa1ab42e7d4b
--- /dev/null
+++ b/net/bluetooth/coredump.c
@@ -0,0 +1,504 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Google Corporation
+ */
+
+#include <linux/devcoredump.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+enum hci_devcoredump_pkt_type {
+ HCI_DEVCOREDUMP_PKT_INIT,
+ HCI_DEVCOREDUMP_PKT_SKB,
+ HCI_DEVCOREDUMP_PKT_PATTERN,
+ HCI_DEVCOREDUMP_PKT_COMPLETE,
+ HCI_DEVCOREDUMP_PKT_ABORT,
+};
+
+struct hci_devcoredump_skb_cb {
+ u16 pkt_type;
+};
+
+struct hci_devcoredump_skb_pattern {
+ u8 pattern;
+ u32 len;
+} __packed;
+
+#define hci_dmp_cb(skb) ((struct hci_devcoredump_skb_cb *)((skb)->cb))
+
+#define MAX_DEVCOREDUMP_HDR_SIZE 512 /* bytes */
+
+static int hci_devcoredump_update_hdr_state(char *buf, size_t size, int state)
+{
+ if (!buf)
+ return 0;
+
+ return snprintf(buf, size, "Bluetooth devcoredump\nState: %d\n", state);
+}
+
+/* Call with hci_dev_lock only. */
+static int hci_devcoredump_update_state(struct hci_dev *hdev, int state)
+{
+ hdev->dump.state = state;
+
+ return hci_devcoredump_update_hdr_state(hdev->dump.head,
+ hdev->dump.alloc_size, state);
+}
+
+static int hci_devcoredump_mkheader(struct hci_dev *hdev, char *buf,
+ size_t buf_size)
+{
+ char *ptr = buf;
+ size_t rem = buf_size;
+ size_t read = 0;
+
+ read = hci_devcoredump_update_hdr_state(ptr, rem, HCI_DEVCOREDUMP_IDLE);
+ read += 1; /* update_hdr_state adds \0 at the end upon state rewrite */
+ rem -= read;
+ ptr += read;
+
+ if (hdev->dump.dmp_hdr) {
+ /* dmp_hdr() should return number of bytes written */
+ read = hdev->dump.dmp_hdr(hdev, ptr, rem);
+ rem -= read;
+ ptr += read;
+ }
+
+ read = snprintf(ptr, rem, "--- Start dump ---\n");
+ rem -= read;
+ ptr += read;
+
+ return buf_size - rem;
+}
+
+/* Do not call with hci_dev_lock since this calls driver code. */
+static void hci_devcoredump_notify(struct hci_dev *hdev, int state)
+{
+ if (hdev->dump.notify_change)
+ hdev->dump.notify_change(hdev, state);
+}
+
+/* Call with hci_dev_lock only. */
+void hci_devcoredump_reset(struct hci_dev *hdev)
+{
+ hdev->dump.head = NULL;
+ hdev->dump.tail = NULL;
+ hdev->dump.alloc_size = 0;
+
+ hci_devcoredump_update_state(hdev, HCI_DEVCOREDUMP_IDLE);
+
+ cancel_delayed_work(&hdev->dump.dump_timeout);
+ skb_queue_purge(&hdev->dump.dump_q);
+}
+
+/* Call with hci_dev_lock only. */
+static void hci_devcoredump_free(struct hci_dev *hdev)
+{
+ if (hdev->dump.head)
+ vfree(hdev->dump.head);
+
+ hci_devcoredump_reset(hdev);
+}
+
+/* Call with hci_dev_lock only. */
+static int hci_devcoredump_alloc(struct hci_dev *hdev, u32 size)
+{
+ hdev->dump.head = vmalloc(size);
+ if (!hdev->dump.head)
+ return -ENOMEM;
+
+ hdev->dump.alloc_size = size;
+ hdev->dump.tail = hdev->dump.head;
+ hdev->dump.end = hdev->dump.head + size;
+
+ hci_devcoredump_update_state(hdev, HCI_DEVCOREDUMP_IDLE);
+
+ return 0;
+}
+
+/* Call with hci_dev_lock only. */
+static bool hci_devcoredump_copy(struct hci_dev *hdev, char *buf, u32 size)
+{
+ if (hdev->dump.tail + size > hdev->dump.end)
+ return false;
+
+ memcpy(hdev->dump.tail, buf, size);
+ hdev->dump.tail += size;
+
+ return true;
+}
+
+/* Call with hci_dev_lock only. */
+static bool hci_devcoredump_memset(struct hci_dev *hdev, u8 pattern, u32 len)
+{
+ if (hdev->dump.tail + len > hdev->dump.end)
+ return false;
+
+ memset(hdev->dump.tail, pattern, len);
+ hdev->dump.tail += len;
+
+ return true;
+}
+
+/* Call with hci_dev_lock only. */
+static int hci_devcoredump_prepare(struct hci_dev *hdev, u32 dump_size)
+{
+ char *dump_hdr;
+ int dump_hdr_size;
+ u32 size;
+ int err = 0;
+
+ dump_hdr = vmalloc(MAX_DEVCOREDUMP_HDR_SIZE);
+ if (!dump_hdr) {
+ err = -ENOMEM;
+ goto hdr_free;
+ }
+
+ dump_hdr_size = hci_devcoredump_mkheader(hdev, dump_hdr,
+ MAX_DEVCOREDUMP_HDR_SIZE);
+ size = dump_hdr_size + dump_size;
+
+ if (hci_devcoredump_alloc(hdev, size)) {
+ err = -ENOMEM;
+ goto hdr_free;
+ }
+
+ /* Insert the device header */
+ if (!hci_devcoredump_copy(hdev, dump_hdr, dump_hdr_size)) {
+ bt_dev_err(hdev, "Failed to insert header");
+ hci_devcoredump_free(hdev);
+
+ err = -ENOMEM;
+ goto hdr_free;
+ }
+
+hdr_free:
+ if (dump_hdr)
+ vfree(dump_hdr);
+
+ return err;
+}
+
+/* Bluetooth devcoredump state machine.
+ *
+ * Devcoredump states:
+ *
+ * HCI_DEVCOREDUMP_IDLE: The default state.
+ *
+ * HCI_DEVCOREDUMP_ACTIVE: A devcoredump will be in this state once it has
+ * been initialized using hci_devcoredump_init(). Once active, the
+ * driver can append data using hci_devcoredump_append() or insert
+ * a pattern using hci_devcoredump_append_pattern().
+ *
+ * HCI_DEVCOREDUMP_DONE: Once the dump collection is complete, the drive
+ * can signal the completion using hci_devcoredump_complete(). A
+ * devcoredump is generated indicating the completion event and
+ * then the state machine is reset to the default state.
+ *
+ * HCI_DEVCOREDUMP_ABORT: The driver can cancel ongoing dump collection in
+ * case of any error using hci_devcoredump_abort(). A devcoredump
+ * is still generated with the available data indicating the abort
+ * event and then the state machine is reset to the default state.
+ *
+ * HCI_DEVCOREDUMP_TIMEOUT: A timeout timer for HCI_DEVCOREDUMP_TIMEOUT sec
+ * is started during devcoredump initialization. Once the timeout
+ * occurs, the driver is notified, a devcoredump is generated with
+ * the available data indicating the timeout event and then the
+ * state machine is reset to the default state.
+ *
+ * The driver must register using hci_devcoredump_register() before using the
+ * hci devcoredump APIs.
+ */
+void hci_devcoredump_rx(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev, dump.dump_rx);
+ struct sk_buff *skb;
+ struct hci_devcoredump_skb_pattern *pattern;
+ u32 dump_size;
+ int start_state;
+
+#define DBG_UNEXPECTED_STATE() \
+ bt_dev_dbg(hdev, \
+ "Unexpected packet (%d) for state (%d). ", \
+ hci_dmp_cb(skb)->pkt_type, hdev->dump.state)
+
+ while ((skb = skb_dequeue(&hdev->dump.dump_q))) {
+ hci_dev_lock(hdev);
+ start_state = hdev->dump.state;
+
+ switch (hci_dmp_cb(skb)->pkt_type) {
+ case HCI_DEVCOREDUMP_PKT_INIT:
+ if (hdev->dump.state != HCI_DEVCOREDUMP_IDLE) {
+ DBG_UNEXPECTED_STATE();
+ goto loop_continue;
+ }
+
+ if (skb->len != sizeof(dump_size)) {
+ bt_dev_dbg(hdev, "Invalid dump init pkt");
+ goto loop_continue;
+ }
+
+ dump_size = *((u32 *)skb->data);
+ if (!dump_size) {
+ bt_dev_err(hdev, "Zero size dump init pkt");
+ goto loop_continue;
+ }
+
+ if (hci_devcoredump_prepare(hdev, dump_size)) {
+ bt_dev_err(hdev, "Failed to prepare for dump");
+ goto loop_continue;
+ }
+
+ hci_devcoredump_update_state(hdev,
+ HCI_DEVCOREDUMP_ACTIVE);
+ queue_delayed_work(hdev->workqueue,
+ &hdev->dump.dump_timeout,
+ DEVCOREDUMP_TIMEOUT);
+ break;
+
+ case HCI_DEVCOREDUMP_PKT_SKB:
+ if (hdev->dump.state != HCI_DEVCOREDUMP_ACTIVE) {
+ DBG_UNEXPECTED_STATE();
+ goto loop_continue;
+ }
+
+ if (!hci_devcoredump_copy(hdev, skb->data, skb->len))
+ bt_dev_dbg(hdev, "Failed to insert skb");
+ break;
+
+ case HCI_DEVCOREDUMP_PKT_PATTERN:
+ if (hdev->dump.state != HCI_DEVCOREDUMP_ACTIVE) {
+ DBG_UNEXPECTED_STATE();
+ goto loop_continue;
+ }
+
+ if (skb->len != sizeof(*pattern)) {
+ bt_dev_dbg(hdev, "Invalid pattern skb");
+ goto loop_continue;
+ }
+
+ pattern = (void *)skb->data;
+
+ if (!hci_devcoredump_memset(hdev, pattern->pattern,
+ pattern->len))
+ bt_dev_dbg(hdev, "Failed to set pattern");
+ break;
+
+ case HCI_DEVCOREDUMP_PKT_COMPLETE:
+ if (hdev->dump.state != HCI_DEVCOREDUMP_ACTIVE) {
+ DBG_UNEXPECTED_STATE();
+ goto loop_continue;
+ }
+
+ hci_devcoredump_update_state(hdev,
+ HCI_DEVCOREDUMP_DONE);
+ dump_size = hdev->dump.tail - hdev->dump.head;
+
+ bt_dev_info(hdev,
+ "Devcoredump complete with size %u "
+ "(expect %u)",
+ dump_size, hdev->dump.alloc_size);
+
+ dev_coredumpv(&hdev->dev, hdev->dump.head, dump_size,
+ GFP_KERNEL);
+ break;
+
+ case HCI_DEVCOREDUMP_PKT_ABORT:
+ if (hdev->dump.state != HCI_DEVCOREDUMP_ACTIVE) {
+ DBG_UNEXPECTED_STATE();
+ goto loop_continue;
+ }
+
+ hci_devcoredump_update_state(hdev,
+ HCI_DEVCOREDUMP_ABORT);
+ dump_size = hdev->dump.tail - hdev->dump.head;
+
+ bt_dev_info(hdev,
+ "Devcoredump aborted with size %u "
+ "(expect %u)",
+ dump_size, hdev->dump.alloc_size);
+
+ /* Emit a devcoredump with the available data */
+ dev_coredumpv(&hdev->dev, hdev->dump.head, dump_size,
+ GFP_KERNEL);
+ break;
+
+ default:
+ bt_dev_dbg(hdev,
+ "Unknown packet (%d) for state (%d). ",
+ hci_dmp_cb(skb)->pkt_type, hdev->dump.state);
+ break;
+ }
+
+loop_continue:
+ kfree_skb(skb);
+ hci_dev_unlock(hdev);
+
+ if (start_state != hdev->dump.state)
+ hci_devcoredump_notify(hdev, hdev->dump.state);
+
+ hci_dev_lock(hdev);
+ if (hdev->dump.state == HCI_DEVCOREDUMP_DONE ||
+ hdev->dump.state == HCI_DEVCOREDUMP_ABORT)
+ hci_devcoredump_reset(hdev);
+ hci_dev_unlock(hdev);
+ }
+}
+EXPORT_SYMBOL(hci_devcoredump_rx);
+
+void hci_devcoredump_timeout(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev,
+ dump.dump_timeout.work);
+ u32 dump_size;
+
+ hci_devcoredump_notify(hdev, HCI_DEVCOREDUMP_TIMEOUT);
+
+ hci_dev_lock(hdev);
+
+ cancel_work_sync(&hdev->dump.dump_rx);
+
+ hci_devcoredump_update_state(hdev, HCI_DEVCOREDUMP_TIMEOUT);
+ dump_size = hdev->dump.tail - hdev->dump.head;
+ bt_dev_info(hdev, "Devcoredump timeout with size %u (expect %u)",
+ dump_size, hdev->dump.alloc_size);
+
+ /* Emit a devcoredump with the available data */
+ dev_coredumpv(&hdev->dev, hdev->dump.head, dump_size, GFP_KERNEL);
+
+ hci_devcoredump_reset(hdev);
+
+ hci_dev_unlock(hdev);
+}
+EXPORT_SYMBOL(hci_devcoredump_timeout);
+
+int hci_devcoredump_register(struct hci_dev *hdev, dmp_hdr_t dmp_hdr,
+ notify_change_t notify_change)
+{
+ /* driver must implement dmp_hdr() function for bluetooth devcoredump */
+ if (!dmp_hdr)
+ return -EINVAL;
+
+ hci_dev_lock(hdev);
+ hdev->dump.dmp_hdr = dmp_hdr;
+ hdev->dump.notify_change = notify_change;
+ hdev->dump.supported = true;
+ hci_dev_unlock(hdev);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_register);
+
+int hci_devcoredump_init(struct hci_dev *hdev, u32 dmp_size)
+{
+ struct sk_buff *skb = NULL;
+
+ if (!hdev->dump.enabled || !hdev->dump.supported)
+ return -EOPNOTSUPP;
+
+ skb = alloc_skb(sizeof(dmp_size), GFP_ATOMIC);
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump init");
+ return -ENOMEM;
+ }
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_INIT;
+ skb_put_data(skb, &dmp_size, sizeof(dmp_size));
+
+ skb_queue_tail(&hdev->dump.dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump.dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_init);
+
+int hci_devcoredump_append(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ if (!skb)
+ return -ENOMEM;
+
+ if (!hdev->dump.enabled || !hdev->dump.supported) {
+ kfree_skb(skb);
+ return -EOPNOTSUPP;
+ }
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_SKB;
+
+ skb_queue_tail(&hdev->dump.dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump.dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_append);
+
+int hci_devcoredump_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len)
+{
+ struct hci_devcoredump_skb_pattern p;
+ struct sk_buff *skb = NULL;
+
+ if (!hdev->dump.enabled || !hdev->dump.supported)
+ return -EOPNOTSUPP;
+
+ skb = alloc_skb(sizeof(p), GFP_ATOMIC);
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump pattern");
+ return -ENOMEM;
+ }
+
+ p.pattern = pattern;
+ p.len = len;
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_PATTERN;
+ skb_put_data(skb, &p, sizeof(p));
+
+ skb_queue_tail(&hdev->dump.dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump.dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_append_pattern);
+
+int hci_devcoredump_complete(struct hci_dev *hdev)
+{
+ struct sk_buff *skb = NULL;
+
+ if (!hdev->dump.enabled || !hdev->dump.supported)
+ return -EOPNOTSUPP;
+
+ skb = alloc_skb(0, GFP_ATOMIC);
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump complete");
+ return -ENOMEM;
+ }
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_COMPLETE;
+
+ skb_queue_tail(&hdev->dump.dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump.dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_complete);
+
+int hci_devcoredump_abort(struct hci_dev *hdev)
+{
+ struct sk_buff *skb = NULL;
+
+ if (!hdev->dump.enabled || !hdev->dump.supported)
+ return -EOPNOTSUPP;
+
+ skb = alloc_skb(0, GFP_ATOMIC);
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump abort");
+ return -ENOMEM;
+ }
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_ABORT;
+
+ skb_queue_tail(&hdev->dump.dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump.dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_abort);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 05c13f639b94..743c5bdb8daa 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2516,14 +2516,23 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
INIT_WORK(&hdev->tx_work, hci_tx_work);
INIT_WORK(&hdev->power_on, hci_power_on);
INIT_WORK(&hdev->error_reset, hci_error_reset);
+#ifdef CONFIG_DEV_COREDUMP
+ INIT_WORK(&hdev->dump.dump_rx, hci_devcoredump_rx);
+#endif
hci_cmd_sync_init(hdev);
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
+#ifdef CONFIG_DEV_COREDUMP
+ INIT_DELAYED_WORK(&hdev->dump.dump_timeout, hci_devcoredump_timeout);
+#endif
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
skb_queue_head_init(&hdev->raw_q);
+#ifdef CONFIG_DEV_COREDUMP
+ skb_queue_head_init(&hdev->dump.dump_q);
+#endif
init_waitqueue_head(&hdev->req_wait_q);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index e5602e209b63..a3d049b55b70 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3924,6 +3924,8 @@ int hci_dev_open_sync(struct hci_dev *hdev)
goto done;
}
+ hci_devcoredump_reset(hdev);
+
set_bit(HCI_RUNNING, &hdev->flags);
hci_sock_dev_event(hdev, HCI_DEV_OPEN);
--
2.37.0.rc0.161.g10f37bed90-goog
From: Abhishek Pandit-Subedi <[email protected]>
Intercept debug exception events from the controller and put them into
a devcoredump using hci devcoredump APIs. The debug exception contains
data in a TLV format and it will be parsed in userspace.
Signed-off-by: Abhishek Pandit-Subedi <[email protected]>
Signed-off-by: Manish Mandlik <[email protected]>
Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
Reviewed-by: Chethan Tumkur Narayan <[email protected]>
---
(no changes since v2)
Changes in v2:
- Create a local struct to store coredump_info in btintel.c
- Call btintel_register_devcoredump_support() from btintel.c
- Fix strncpy() destination bound warning
drivers/bluetooth/btintel.c | 68 ++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btintel.h | 11 ++++--
drivers/bluetooth/btusb.c | 49 ++++++++++++++++++++++----
3 files changed, 118 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 818681c89db8..8ad77beff17a 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -32,6 +32,13 @@ struct cmd_write_boot_params {
u8 fw_build_yy;
} __packed;
+#define DRIVER_NAME_LEN 16
+static struct {
+ char driver_name[DRIVER_NAME_LEN];
+ u8 hw_variant;
+ u32 fw_build_num;
+} coredump_info;
+
int btintel_check_bdaddr(struct hci_dev *hdev)
{
struct hci_rp_read_bd_addr *bda;
@@ -304,6 +311,9 @@ int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
return -EINVAL;
}
+ coredump_info.hw_variant = ver->hw_variant;
+ coredump_info.fw_build_num = ver->fw_build_num;
+
bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
ver->fw_build_num, ver->fw_build_ww,
@@ -497,6 +507,9 @@ static int btintel_version_info_tlv(struct hci_dev *hdev,
return -EINVAL;
}
+ coredump_info.hw_variant = INTEL_HW_VARIANT(version->cnvi_bt);
+ coredump_info.fw_build_num = version->build_num;
+
bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
2000 + (version->timestamp >> 8), version->timestamp & 0xff,
version->build_type, version->build_num);
@@ -1391,6 +1404,54 @@ int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
}
EXPORT_SYMBOL_GPL(btintel_set_quality_report);
+static int btintel_dmp_hdr(struct hci_dev *hdev, char *buf, size_t size)
+{
+ char *ptr = buf;
+ size_t rem = size;
+ size_t read = 0;
+
+ read = snprintf(ptr, rem, "Controller Name: 0x%X\n",
+ coredump_info.hw_variant);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Firmware Version: 0x%X\n",
+ coredump_info.fw_build_num);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Driver: %s\n", coredump_info.driver_name);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Vendor: Intel\n");
+ rem -= read;
+ ptr += read;
+
+ return size - rem;
+}
+
+static int btintel_register_devcoredump_support(struct hci_dev *hdev)
+{
+ struct intel_debug_features features;
+ int err;
+
+ err = btintel_read_debug_features(hdev, &features);
+ if (err) {
+ bt_dev_info(hdev, "Error reading debug features");
+ return err;
+ }
+
+ if (!(features.page1[0] & 0x3f)) {
+ bt_dev_info(hdev, "Telemetry exception format not supported");
+ return -EOPNOTSUPP;
+ }
+
+ hci_devcoredump_register(hdev, btintel_dmp_hdr, NULL);
+
+ return err;
+}
+
static const struct firmware *btintel_legacy_rom_get_fw(struct hci_dev *hdev,
struct intel_version *ver)
{
@@ -2464,6 +2525,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
btintel_set_msft_opcode(hdev, ver.hw_variant);
err = btintel_bootloader_setup(hdev, &ver);
+ btintel_register_devcoredump_support(hdev);
break;
default:
bt_dev_err(hdev, "Unsupported Intel hw variant (%u)",
@@ -2538,6 +2600,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
btintel_set_msft_opcode(hdev, ver.hw_variant);
err = btintel_bootloader_setup(hdev, &ver);
+ btintel_register_devcoredump_support(hdev);
break;
case 0x17:
case 0x18:
@@ -2560,6 +2623,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
INTEL_HW_VARIANT(ver_tlv.cnvi_bt));
err = btintel_bootloader_setup_tlv(hdev, &ver_tlv);
+ btintel_register_devcoredump_support(hdev);
break;
default:
bt_dev_err(hdev, "Unsupported Intel hw variant (%u)",
@@ -2608,7 +2672,7 @@ static int btintel_shutdown_combined(struct hci_dev *hdev)
return 0;
}
-int btintel_configure_setup(struct hci_dev *hdev)
+int btintel_configure_setup(struct hci_dev *hdev, const char *driver_name)
{
hdev->manufacturer = 2;
hdev->setup = btintel_setup_combined;
@@ -2617,6 +2681,8 @@ int btintel_configure_setup(struct hci_dev *hdev)
hdev->set_diag = btintel_set_diag_combined;
hdev->set_bdaddr = btintel_set_bdaddr;
+ strncpy(coredump_info.driver_name, driver_name, DRIVER_NAME_LEN - 1);
+
return 0;
}
EXPORT_SYMBOL_GPL(btintel_configure_setup);
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index e0060e58573c..8e72e59b5bde 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -137,6 +137,12 @@ struct intel_offload_use_cases {
__u8 preset[8];
} __packed;
+#define INTEL_TLV_TYPE_ID 0x1
+
+#define INTEL_TLV_SYSTEM_EXCEPTION 0x0
+#define INTEL_TLV_FATAL_EXCEPTION 0x1
+#define INTEL_TLV_DEBUG_EXCEPTION 0x2
+
#define INTEL_HW_PLATFORM(cnvx_bt) ((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
#define INTEL_HW_VARIANT(cnvx_bt) ((u8)(((cnvx_bt) & 0x003f0000) >> 16))
#define INTEL_CNVX_TOP_TYPE(cnvx_top) ((cnvx_top) & 0x00000fff)
@@ -206,7 +212,7 @@ int btintel_read_boot_params(struct hci_dev *hdev,
struct intel_boot_params *params);
int btintel_download_firmware(struct hci_dev *dev, struct intel_version *ver,
const struct firmware *fw, u32 *boot_param);
-int btintel_configure_setup(struct hci_dev *hdev);
+int btintel_configure_setup(struct hci_dev *hdev, const char *driver_name);
void btintel_bootup(struct hci_dev *hdev, const void *ptr, unsigned int len);
void btintel_secure_send_result(struct hci_dev *hdev,
const void *ptr, unsigned int len);
@@ -287,7 +293,8 @@ static inline int btintel_download_firmware(struct hci_dev *dev,
return -EOPNOTSUPP;
}
-static inline int btintel_configure_setup(struct hci_dev *hdev)
+static inline int btintel_configure_setup(struct hci_dev *hdev,
+ const char *driver_name)
{
return -ENODEV;
}
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index fb1a67189412..1db719ef62c5 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2170,16 +2170,42 @@ static int btusb_recv_bulk_intel(struct btusb_data *data, void *buffer,
return btusb_recv_bulk(data, buffer, count);
}
+static int btusb_intel_diagnostics(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct intel_tlv *tlv = (void *)&skb->data[5];
+
+ /* The first event is always an event type TLV */
+ if (tlv->type != INTEL_TLV_TYPE_ID)
+ goto recv_frame;
+
+ switch (tlv->val[0]) {
+ case INTEL_TLV_SYSTEM_EXCEPTION:
+ case INTEL_TLV_FATAL_EXCEPTION:
+ case INTEL_TLV_DEBUG_EXCEPTION:
+ /* Generate devcoredump from exception */
+ if (!hci_devcoredump_init(hdev, skb->len)) {
+ hci_devcoredump_append(hdev, skb);
+ hci_devcoredump_complete(hdev);
+ return 0;
+ }
+ break;
+ }
+
+recv_frame:
+ return hci_recv_frame(hdev, skb);
+}
+
static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
{
- if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
- struct hci_event_hdr *hdr = (void *)skb->data;
+ struct hci_event_hdr *hdr = (void *)skb->data;
+ const char diagnostics_hdr[] = { 0x87, 0x80, 0x03 };
- if (skb->len > HCI_EVENT_HDR_SIZE && hdr->evt == 0xff &&
- hdr->plen > 0) {
- const void *ptr = skb->data + HCI_EVENT_HDR_SIZE + 1;
- unsigned int len = skb->len - HCI_EVENT_HDR_SIZE - 1;
+ if (skb->len > HCI_EVENT_HDR_SIZE && hdr->evt == 0xff &&
+ hdr->plen > 0) {
+ const void *ptr = skb->data + HCI_EVENT_HDR_SIZE + 1;
+ unsigned int len = skb->len - HCI_EVENT_HDR_SIZE - 1;
+ if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
switch (skb->data[2]) {
case 0x02:
/* When switching to the operational firmware
@@ -2198,6 +2224,15 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
break;
}
}
+
+ /* Handle all diagnostics events separately. May still call
+ * hci_recv_frame.
+ */
+ if (len >= sizeof(diagnostics_hdr) &&
+ memcmp(&skb->data[2], diagnostics_hdr,
+ sizeof(diagnostics_hdr)) == 0) {
+ return btusb_intel_diagnostics(hdev, skb);
+ }
}
return hci_recv_frame(hdev, skb);
@@ -3770,7 +3805,7 @@ static int btusb_probe(struct usb_interface *intf,
/* Combined Intel Device setup to support multiple setup routine */
if (id->driver_info & BTUSB_INTEL_COMBINED) {
- err = btintel_configure_setup(hdev);
+ err = btintel_configure_setup(hdev, btusb_driver.name);
if (err)
goto out_free_dev;
--
2.37.0.rc0.161.g10f37bed90-goog
Since device/firmware dump is a debugging feature, we may not need it
all the time. Add a sysfs attribute to enable/disable bluetooth
devcoredump capturing. The default state of this feature would be
disabled and it can be enabled by echoing 1 to enable_coredump sysfs
entry as follow:
$ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
Signed-off-by: Manish Mandlik <[email protected]>
---
Changes in v3:
- New patch in the series to enable/disable feature via sysfs entry
net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 4e3e0451b08c..df0d54a5ae3f 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
module_put(THIS_MODULE);
}
+#ifdef CONFIG_DEV_COREDUMP
+static ssize_t enable_coredump_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct hci_dev *hdev = to_hci_dev(dev);
+
+ return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
+}
+
+static ssize_t enable_coredump_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hci_dev *hdev = to_hci_dev(dev);
+
+ /* Consider any non-zero value as true */
+ if (simple_strtol(buf, NULL, 10))
+ hdev->dump.enabled = true;
+ else
+ hdev->dump.enabled = false;
+
+ return count;
+}
+DEVICE_ATTR_RW(enable_coredump);
+#endif
+
+static struct attribute *bt_host_attrs[] = {
+#ifdef CONFIG_DEV_COREDUMP
+ &dev_attr_enable_coredump.attr,
+#endif
+ NULL,
+};
+ATTRIBUTE_GROUPS(bt_host);
+
static const struct device_type bt_host = {
.name = "host",
.release = bt_host_release,
+ .groups = bt_host_groups,
};
void hci_init_sysfs(struct hci_dev *hdev)
--
2.37.0.rc0.161.g10f37bed90-goog
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=657598
---Test result---
Test Summary:
CheckPatch FAIL 4.51 seconds
GitLint PASS 3.02 seconds
SubjectPrefix PASS 2.72 seconds
BuildKernel PASS 32.04 seconds
BuildKernel32 PASS 27.65 seconds
Incremental Build with patchesPASS 52.40 seconds
TestRunner: Setup PASS 474.56 seconds
TestRunner: l2cap-tester PASS 16.85 seconds
TestRunner: bnep-tester PASS 5.82 seconds
TestRunner: mgmt-tester PASS 97.85 seconds
TestRunner: rfcomm-tester PASS 9.35 seconds
TestRunner: sco-tester PASS 9.12 seconds
TestRunner: smp-tester PASS 9.11 seconds
TestRunner: userchan-tester PASS 6.05 seconds
Details
##############################
Test: CheckPatch - FAIL - 4.51 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v3,1/3] Bluetooth: Add support for devcoredump\Traceback (most recent call last):
File "scripts/spdxcheck.py", line 6, in <module>
from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#133:
new file mode 100644
WARNING:SPDX_LICENSE_TAG: Improper SPDX comment style for 'include/net/bluetooth/coredump.h', please use '/*' instead
#138: FILE: include/net/bluetooth/coredump.h:1:
+// SPDX-License-Identifier: GPL-2.0-only
WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#138: FILE: include/net/bluetooth/coredump.h:1:
+// SPDX-License-Identifier: GPL-2.0-only
WARNING:SPLIT_STRING: quoted string split across lines
#589: FILE: net/bluetooth/coredump.c:300:
+ "Devcoredump complete with size %u "
+ "(expect %u)",
WARNING:SPLIT_STRING: quoted string split across lines
#608: FILE: net/bluetooth/coredump.c:319:
+ "Devcoredump aborted with size %u "
+ "(expect %u)",
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#691: FILE: net/bluetooth/coredump.c:402:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump init");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#734: FILE: net/bluetooth/coredump.c:445:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump pattern");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#760: FILE: net/bluetooth/coredump.c:471:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump complete");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#782: FILE: net/bluetooth/coredump.c:493:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump abort");
total: 0 errors, 9 warnings, 0 checks, 670 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/12910303.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[v3,2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump\WARNING:CONSIDER_KSTRTO: simple_strtol is obsolete, use kstrtol instead
#131: FILE: net/bluetooth/hci_sysfs.c:111:
+ if (simple_strtol(buf, NULL, 10))
total: 0 errors, 1 warnings, 0 checks, 45 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/12910304.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
Hi Manish,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth/master]
[also build test WARNING on bluetooth-next/master linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20220708/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/0d785cbd11ed3a6de29aeb05926177440ab26d54
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
git checkout 0d785cbd11ed3a6de29aeb05926177440ab26d54
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> net/bluetooth/hci_sysfs.c:118:1: sparse: sparse: symbol 'dev_attr_enable_coredump' was not declared. Should it be static?
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Manish,
On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <[email protected]> wrote:
>
> Since device/firmware dump is a debugging feature, we may not need it
> all the time. Add a sysfs attribute to enable/disable bluetooth
> devcoredump capturing. The default state of this feature would be
> disabled and it can be enabled by echoing 1 to enable_coredump sysfs
> entry as follow:
>
> $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
>
> Signed-off-by: Manish Mandlik <[email protected]>
> ---
>
> Changes in v3:
> - New patch in the series to enable/disable feature via sysfs entry
>
> net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 4e3e0451b08c..df0d54a5ae3f 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
> module_put(THIS_MODULE);
> }
>
> +#ifdef CONFIG_DEV_COREDUMP
> +static ssize_t enable_coredump_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct hci_dev *hdev = to_hci_dev(dev);
> +
> + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
> +}
> +
> +static ssize_t enable_coredump_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hci_dev *hdev = to_hci_dev(dev);
> +
> + /* Consider any non-zero value as true */
> + if (simple_strtol(buf, NULL, 10))
> + hdev->dump.enabled = true;
> + else
> + hdev->dump.enabled = false;
> +
> + return count;
> +}
> +DEVICE_ATTR_RW(enable_coredump);
> +#endif
> +
> +static struct attribute *bt_host_attrs[] = {
> +#ifdef CONFIG_DEV_COREDUMP
> + &dev_attr_enable_coredump.attr,
> +#endif
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(bt_host);
> +
> static const struct device_type bt_host = {
> .name = "host",
> .release = bt_host_release,
> + .groups = bt_host_groups,
> };
>
> void hci_init_sysfs(struct hci_dev *hdev)
> --
> 2.37.0.rc0.161.g10f37bed90-goog
It seems devcoredump.c creates its own sysfs entries so perhaps this
should be included there and documented in sysfs-devices-coredump.
--
Luiz Augusto von Dentz
Hi Manish,
On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <[email protected]> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <[email protected]> wrote:
>>
>> Hi Manish,
>>
>> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <[email protected]> wrote:
>> >
>> > Since device/firmware dump is a debugging feature, we may not need it
>> > all the time. Add a sysfs attribute to enable/disable bluetooth
>> > devcoredump capturing. The default state of this feature would be
>> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs
>> > entry as follow:
>> >
>> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
>> >
>> > Signed-off-by: Manish Mandlik <[email protected]>
>> > ---
>> >
>> > Changes in v3:
>> > - New patch in the series to enable/disable feature via sysfs entry
>> >
>> > net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 36 insertions(+)
>> >
>> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
>> > index 4e3e0451b08c..df0d54a5ae3f 100644
>> > --- a/net/bluetooth/hci_sysfs.c
>> > +++ b/net/bluetooth/hci_sysfs.c
>> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
>> > module_put(THIS_MODULE);
>> > }
>> >
>> > +#ifdef CONFIG_DEV_COREDUMP
>> > +static ssize_t enable_coredump_show(struct device *dev,
>> > + struct device_attribute *attr,
>> > + char *buf)
>> > +{
>> > + struct hci_dev *hdev = to_hci_dev(dev);
>> > +
>> > + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
>> > +}
>> > +
>> > +static ssize_t enable_coredump_store(struct device *dev,
>> > + struct device_attribute *attr,
>> > + const char *buf, size_t count)
>> > +{
>> > + struct hci_dev *hdev = to_hci_dev(dev);
>> > +
>> > + /* Consider any non-zero value as true */
>> > + if (simple_strtol(buf, NULL, 10))
>> > + hdev->dump.enabled = true;
>> > + else
>> > + hdev->dump.enabled = false;
>> > +
>> > + return count;
>> > +}
>> > +DEVICE_ATTR_RW(enable_coredump);
>> > +#endif
>> > +
>> > +static struct attribute *bt_host_attrs[] = {
>> > +#ifdef CONFIG_DEV_COREDUMP
>> > + &dev_attr_enable_coredump.attr,
>> > +#endif
>> > + NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(bt_host);
>> > +
>> > static const struct device_type bt_host = {
>> > .name = "host",
>> > .release = bt_host_release,
>> > + .groups = bt_host_groups,
>> > };
>> >
>> > void hci_init_sysfs(struct hci_dev *hdev)
>> > --
>> > 2.37.0.rc0.161.g10f37bed90-goog
>>
>> It seems devcoredump.c creates its own sysfs entries so perhaps this
>> should be included there and documented in sysfs-devices-coredump.
>
> I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump.
We should probably check if there is a reason why this is not on per
device and anyway if seem wrong to each subsystem to come up with its
own sysfs entries when it could be easily generalized so the userspace
tools using those entries don't have to look into different entries
depending on the subsystem the device belongs.
>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Regards,
> Manish.
--
Luiz Augusto von Dentz
Hi Manish,
On Fri, Jul 8, 2022 at 3:30 PM Manish Mandlik <[email protected]> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 11:40 AM Luiz Augusto von Dentz <[email protected]> wrote:
>>
>> Hi Manish,
>>
>> On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <[email protected]> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <[email protected]> wrote:
>> >>
>> >> Hi Manish,
>> >>
>> >> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <[email protected]> wrote:
>> >> >
>> >> > Since device/firmware dump is a debugging feature, we may not need it
>> >> > all the time. Add a sysfs attribute to enable/disable bluetooth
>> >> > devcoredump capturing. The default state of this feature would be
>> >> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs
>> >> > entry as follow:
>> >> >
>> >> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
>> >> >
>> >> > Signed-off-by: Manish Mandlik <[email protected]>
>> >> > ---
>> >> >
>> >> > Changes in v3:
>> >> > - New patch in the series to enable/disable feature via sysfs entry
>> >> >
>> >> > net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
>> >> > 1 file changed, 36 insertions(+)
>> >> >
>> >> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
>> >> > index 4e3e0451b08c..df0d54a5ae3f 100644
>> >> > --- a/net/bluetooth/hci_sysfs.c
>> >> > +++ b/net/bluetooth/hci_sysfs.c
>> >> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
>> >> > module_put(THIS_MODULE);
>> >> > }
>> >> >
>> >> > +#ifdef CONFIG_DEV_COREDUMP
>> >> > +static ssize_t enable_coredump_show(struct device *dev,
>> >> > + struct device_attribute *attr,
>> >> > + char *buf)
>> >> > +{
>> >> > + struct hci_dev *hdev = to_hci_dev(dev);
>> >> > +
>> >> > + return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
>> >> > +}
>> >> > +
>> >> > +static ssize_t enable_coredump_store(struct device *dev,
>> >> > + struct device_attribute *attr,
>> >> > + const char *buf, size_t count)
>> >> > +{
>> >> > + struct hci_dev *hdev = to_hci_dev(dev);
>> >> > +
>> >> > + /* Consider any non-zero value as true */
>> >> > + if (simple_strtol(buf, NULL, 10))
>> >> > + hdev->dump.enabled = true;
>> >> > + else
>> >> > + hdev->dump.enabled = false;
>> >> > +
>> >> > + return count;
>> >> > +}
>> >> > +DEVICE_ATTR_RW(enable_coredump);
>> >> > +#endif
>> >> > +
>> >> > +static struct attribute *bt_host_attrs[] = {
>> >> > +#ifdef CONFIG_DEV_COREDUMP
>> >> > + &dev_attr_enable_coredump.attr,
>> >> > +#endif
>> >> > + NULL,
>> >> > +};
>> >> > +ATTRIBUTE_GROUPS(bt_host);
>> >> > +
>> >> > static const struct device_type bt_host = {
>> >> > .name = "host",
>> >> > .release = bt_host_release,
>> >> > + .groups = bt_host_groups,
>> >> > };
>> >> >
>> >> > void hci_init_sysfs(struct hci_dev *hdev)
>> >> > --
>> >> > 2.37.0.rc0.161.g10f37bed90-goog
>> >>
>> >> It seems devcoredump.c creates its own sysfs entries so perhaps this
>> >> should be included there and documented in sysfs-devices-coredump.
>> >
>> > I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump.
>>
>> We should probably check if there is a reason why this is not on per
>> device and anyway if seem wrong to each subsystem to come up with its
>> own sysfs entries when it could be easily generalized so the userspace
>> tools using those entries don't have to look into different entries
>> depending on the subsystem the device belongs.
>
> The purpose of the base devcoredump interface is to only provide a generalized mechanism for drivers to dump the firmware data. It is not aware of which subsystem is using it or what data is being dumped. We want to implement something on top of it only for all bluetooth drivers so that they all can generate firmware dumps in a common way and not worry about the synchronizations and timeouts. That's why it made more sense to me to have a bluetooth specific sysfs entry for enabling/disabling only the bluetooth devcoredump interface without affecting the other users of the base devcoredump.
It looks like we are not understanding each other, you are saying
devcoredump only provides a generalized mechanism for the driver to
dump the firmware coredump, yet we are implementing something extra to
generalize it for bluetooth drivers? Making it bluetooth specific sort
of defeat the purpose of a common layer in my opinion and it is not
like the devcoredump.c don't already have entries inside the device
sysfs directory as documented in sysfs-devices-coredump:
What: /sys/devices/.../coredump
Date: December 2017
Contact: Arend van Spriel <[email protected]>
Description:
The /sys/devices/.../coredump attribute is only present when the
device is bound to a driver, which provides the .coredump()
callback. The attribute is write only. Anything written to this
file will trigger the .coredump() callback.
Available when CONFIG_DEV_COREDUMP is enabled.
What I'm suggesting is in addition to /sys/devices/.../coredump we
also have /sys/devices/.../enable_coredump, perhaps we should include
in the discussion since he is listed as maintainer of DEV_COREDUMP
nowadays.
> Do you suggest we have this sysfs entry for bluetooth class instead? like "/sys/class/bluetooth/enable_coredump" instead of "/sys/class/bluetooth/<device>/enable_coredump"? But the problem with this is that if we have more than one bluetooth controller on the system, disabling one would disable the other as well. So to have the flexibility of controlling it for each device independently I am creating a sysfs entry for each device. IMO, the base devcoredump class sysfs entry is not much of a use anymore as there are already other systems like wifi using it. Let me know your thoughts.
>
>>
>>
>> >
>> >>
>> >> --
>> >> Luiz Augusto von Dentz
>> >
>> >
>> > Regards,
>> > Manish.
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Regards,
> Manish.
--
Luiz Augusto von Dentz