* [PATCH 1/2] Bluetooth: Add support for devcoredump
@ 2022-03-21 1:34 Manish Mandlik
2022-03-21 1:34 ` [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support Manish Mandlik
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Manish Mandlik @ 2022-03-21 1:34 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, Manish Mandlik, David S. Miller,
Jakub Kicinski, Johan Hedberg, Paolo Abeni, linux-kernel, netdev
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
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 <abhishekpandit@chromium.org>
Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
include/net/bluetooth/hci_core.h | 51 ++++
net/bluetooth/hci_core.c | 496 +++++++++++++++++++++++++++++++
net/bluetooth/hci_sync.c | 2 +
3 files changed, 549 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d5377740e99c..818ba3a43e8c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -118,6 +118,43 @@ enum suspended_state {
BT_SUSPEND_CONFIGURE_WAKE,
};
+#define DEVCOREDUMP_TIMEOUT msecs_to_jiffies(100000) /* 100 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
+ *
+ * @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 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;
+
+ dmp_hdr_t dmp_hdr;
+ notify_change_t notify_change;
+};
+
struct hci_conn_hash {
struct list_head list;
unsigned int acl_num;
@@ -568,6 +605,11 @@ struct hci_dev {
const char *fw_info;
struct dentry *debugfs;
+ struct hci_devcoredump dump;
+ struct sk_buff_head dump_q;
+ struct work_struct dump_rx;
+ struct delayed_work dump_timeout;
+
struct device dev;
struct rfkill *rfkill;
@@ -1308,6 +1350,15 @@ static inline void hci_set_aosp_capable(struct hci_dev *hdev)
#endif
}
+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);
+void hci_devcoredump_reset(struct hci_dev *hdev);
+
int hci_dev_open(__u16 dev);
int hci_dev_close(__u16 dev);
int hci_dev_do_close(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b4782a6c1025..76dbb6b28870 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -28,6 +28,7 @@
#include <linux/export.h>
#include <linux/rfkill.h>
#include <linux/debugfs.h>
+#include <linux/devcoredump.h>
#include <linux/crypto.h>
#include <linux/property.h>
#include <linux/suspend.h>
@@ -2404,6 +2405,498 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
return NOTIFY_DONE;
}
+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_timeout);
+ skb_queue_purge(&hdev->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.
+ */
+static void hci_devcoredump_rx(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev, 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_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_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);
+ }
+}
+
+static void hci_devcoredump_timeout(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev,
+ dump_timeout.work);
+ u32 dump_size;
+
+ hci_devcoredump_notify(hdev, HCI_DEVCOREDUMP_TIMEOUT);
+
+ hci_dev_lock(hdev);
+
+ cancel_work_sync(&hdev->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);
+}
+
+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.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_q, skb);
+ queue_work(hdev->workqueue, &hdev->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.supported) {
+ kfree_skb(skb);
+ return -EOPNOTSUPP;
+ }
+
+ hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_SKB;
+
+ skb_queue_tail(&hdev->dump_q, skb);
+ queue_work(hdev->workqueue, &hdev->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.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_q, skb);
+ queue_work(hdev->workqueue, &hdev->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.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_q, skb);
+ queue_work(hdev->workqueue, &hdev->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.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_q, skb);
+ queue_work(hdev->workqueue, &hdev->dump_rx);
+
+ return 0;
+}
+EXPORT_SYMBOL(hci_devcoredump_abort);
+
/* Alloc HCI device */
struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
{
@@ -2511,14 +3004,17 @@ 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);
+ INIT_WORK(&hdev->dump_rx, hci_devcoredump_rx);
hci_cmd_sync_init(hdev);
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
+ INIT_DELAYED_WORK(&hdev->dump_timeout, hci_devcoredump_timeout);
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
skb_queue_head_init(&hdev->raw_q);
+ skb_queue_head_init(&hdev->dump_q);
init_waitqueue_head(&hdev->req_wait_q);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 8f4c5698913d..ec03fa871ef0 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3877,6 +3877,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.35.1.894.gb6a874cedc-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support
2022-03-21 1:34 [PATCH 1/2] Bluetooth: Add support for devcoredump Manish Mandlik
@ 2022-03-21 1:34 ` Manish Mandlik
2022-03-21 7:38 ` kernel test robot
2022-03-21 2:34 ` [1/2] Bluetooth: Add support for devcoredump bluez.test.bot
2022-03-21 16:46 ` [PATCH 1/2] " Luiz Augusto von Dentz
2 siblings, 1 reply; 9+ messages in thread
From: Manish Mandlik @ 2022-03-21 1:34 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, Manish Mandlik, Chethan Tumkur Narayan,
Johan Hedberg, linux-kernel
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
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 <abhishekpandit@chromium.org>
Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Chethan Tumkur Narayan <chethan.tumkur.narayan@intel.com>
---
drivers/bluetooth/btintel.c | 60 +++++++++++++++++++++++++++++++++++++
drivers/bluetooth/btintel.h | 14 +++++++++
drivers/bluetooth/btusb.c | 47 +++++++++++++++++++++++++----
3 files changed, 115 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 06514ed66022..9ac7a4441b38 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -32,6 +32,11 @@ struct cmd_write_boot_params {
u8 fw_build_yy;
} __packed;
+#define DRIVER_NAME_LEN 16
+static char driver_name[DRIVER_NAME_LEN];
+static u8 hw_variant;
+static u32 fw_build_num;
+
int btintel_check_bdaddr(struct hci_dev *hdev)
{
struct hci_rp_read_bd_addr *bda;
@@ -304,6 +309,9 @@ int btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
return -EINVAL;
}
+ hw_variant = ver->hw_variant;
+ 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 +505,9 @@ static int btintel_version_info_tlv(struct hci_dev *hdev,
return -EINVAL;
}
+ hw_variant = INTEL_HW_VARIANT(version->cnvi_bt);
+ 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 +1402,55 @@ 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", hw_variant);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Firmware Version: 0x%X\n", fw_build_num);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Driver: %s\n", driver_name);
+ rem -= read;
+ ptr += read;
+
+ read = snprintf(ptr, rem, "Vendor: Intel\n");
+ rem -= read;
+ ptr += read;
+
+ return size - rem;
+}
+
+int btintel_register_devcoredump_support(struct hci_dev *hdev,
+ const char *driver)
+{
+ 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;
+ }
+
+ strncpy(driver_name, driver, DRIVER_NAME_LEN);
+ hci_devcoredump_register(hdev, btintel_dmp_hdr, NULL);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(btintel_register_devcoredump_support);
+
static const struct firmware *btintel_legacy_rom_get_fw(struct hci_dev *hdev,
struct intel_version *ver)
{
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index e0060e58573c..785c63cc6c1d 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,6 +212,8 @@ 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_register_devcoredump_support(struct hci_dev *hdev,
+ const char *driver);
int btintel_configure_setup(struct hci_dev *hdev);
void btintel_bootup(struct hci_dev *hdev, const void *ptr, unsigned int len);
void btintel_secure_send_result(struct hci_dev *hdev,
@@ -302,6 +310,12 @@ static inline void btintel_secure_send_result(struct hci_dev *hdev,
{
}
+static int btintel_register_devcoredump_support(struct hci_dev *hdev,
+ const char *driver)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
{
return -ENODEV;
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 50df417207af..42a3d90180a4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2155,16 +2155,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
@@ -2183,6 +2209,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);
--
2.35.1.894.gb6a874cedc-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [1/2] Bluetooth: Add support for devcoredump
2022-03-21 1:34 [PATCH 1/2] Bluetooth: Add support for devcoredump Manish Mandlik
2022-03-21 1:34 ` [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support Manish Mandlik
@ 2022-03-21 2:34 ` bluez.test.bot
2022-03-21 16:46 ` [PATCH 1/2] " Luiz Augusto von Dentz
2 siblings, 0 replies; 9+ messages in thread
From: bluez.test.bot @ 2022-03-21 2:34 UTC (permalink / raw)
To: linux-bluetooth, mmandlik
[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]
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=624997
---Test result---
Test Summary:
CheckPatch FAIL 1.78 seconds
GitLint PASS 0.89 seconds
SubjectPrefix PASS 0.64 seconds
BuildKernel PASS 38.17 seconds
BuildKernel32 PASS 33.98 seconds
Incremental Build with patchesPASS 53.60 seconds
TestRunner: Setup PASS 594.46 seconds
TestRunner: l2cap-tester PASS 18.39 seconds
TestRunner: bnep-tester PASS 7.43 seconds
TestRunner: mgmt-tester PASS 125.35 seconds
TestRunner: rfcomm-tester PASS 9.63 seconds
TestRunner: sco-tester PASS 9.35 seconds
TestRunner: smp-tester PASS 9.48 seconds
TestRunner: userchan-tester PASS 7.82 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.78 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[1/2] Bluetooth: Add support for devcoredump\WARNING:SPLIT_STRING: quoted string split across lines
#498: FILE: net/bluetooth/hci_core.c:2696:
+ "Devcoredump complete with size %u "
+ "(expect %u)",
WARNING:SPLIT_STRING: quoted string split across lines
#517: FILE: net/bluetooth/hci_core.c:2715:
+ "Devcoredump aborted with size %u "
+ "(expect %u)",
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#598: FILE: net/bluetooth/hci_core.c:2796:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump init");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#641: FILE: net/bluetooth/hci_core.c:2839:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump pattern");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#667: FILE: net/bluetooth/hci_core.c:2865:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump complete");
WARNING:OOM_MESSAGE: Possible unnecessary 'out of memory' message
#689: FILE: net/bluetooth/hci_core.c:2887:
+ if (!skb) {
+ bt_dev_err(hdev, "Failed to allocate devcoredump abort");
total: 0 errors, 6 warnings, 0 checks, 599 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/12786731.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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support
2022-03-21 1:34 ` [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support Manish Mandlik
@ 2022-03-21 7:38 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-03-21 7:38 UTC (permalink / raw)
To: Manish Mandlik, marcel, luiz.dentz
Cc: kbuild-all, chromeos-bluetooth-upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, Manish Mandlik, Chethan Tumkur Narayan,
Johan Hedberg, linux-kernel
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 v5.17 next-20220318]
[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]
url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: parisc-randconfig-r023-20220320 (https://download.01.org/0day-ci/archive/20220321/202203211500.z1bvoo6A-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/677f482cb7027ab030842015cfd6c188568df39f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
git checkout 677f482cb7027ab030842015cfd6c188568df39f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=parisc SHELL=/bin/bash drivers/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/bluetooth/btintel.c: In function 'btintel_register_devcoredump_support':
>> drivers/bluetooth/btintel.c:1447:9: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
1447 | strncpy(driver_name, driver, DRIVER_NAME_LEN);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/strncpy +1447 drivers/bluetooth/btintel.c
1429
1430 int btintel_register_devcoredump_support(struct hci_dev *hdev,
1431 const char *driver)
1432 {
1433 struct intel_debug_features features;
1434 int err;
1435
1436 err = btintel_read_debug_features(hdev, &features);
1437 if (err) {
1438 bt_dev_info(hdev, "Error reading debug features");
1439 return err;
1440 }
1441
1442 if (!(features.page1[0] & 0x3f)) {
1443 bt_dev_info(hdev, "Telemetry exception format not supported");
1444 return -EOPNOTSUPP;
1445 }
1446
> 1447 strncpy(driver_name, driver, DRIVER_NAME_LEN);
1448 hci_devcoredump_register(hdev, btintel_dmp_hdr, NULL);
1449
1450 return err;
1451 }
1452 EXPORT_SYMBOL_GPL(btintel_register_devcoredump_support);
1453
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add support for devcoredump
2022-03-21 1:34 [PATCH 1/2] Bluetooth: Add support for devcoredump Manish Mandlik
2022-03-21 1:34 ` [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support Manish Mandlik
2022-03-21 2:34 ` [1/2] Bluetooth: Add support for devcoredump bluez.test.bot
@ 2022-03-21 16:46 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLDu5_ES5CcsUU3qHLWHkzAV=FUU8rns8Twh6C5FVSeEUQ@mail.gmail.com>
2 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-03-21 16:46 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Manish,
On Sun, Mar 20, 2022 at 6:34 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> 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.
If this sort of information comes from telemetry support it can be
intercepted by other means as well e.g. btmon, in fact I think that
would have been better since we already dump the backtrace of
bluetoothd crashes into the monitor as well. Anyway the kernel
devcoredump support seems to just be dumping the data into sysfs so we
could be fetching that information from there as well but I'm not
really sure what we would be gaining with that since we are just
adding yet another kernel dependency for something that userspace
could already be doing by itself.
> 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.
Is there some userspace component parsing these dumps? Or we will need
to add some support for the likes of btmon to collect the logs from
sysfs?
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> include/net/bluetooth/hci_core.h | 51 ++++
> net/bluetooth/hci_core.c | 496 +++++++++++++++++++++++++++++++
> net/bluetooth/hci_sync.c | 2 +
> 3 files changed, 549 insertions(+)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d5377740e99c..818ba3a43e8c 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -118,6 +118,43 @@ enum suspended_state {
> BT_SUSPEND_CONFIGURE_WAKE,
> };
>
> +#define DEVCOREDUMP_TIMEOUT msecs_to_jiffies(100000) /* 100 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
> + *
> + * @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 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;
> +
> + dmp_hdr_t dmp_hdr;
> + notify_change_t notify_change;
> +};
> +
> struct hci_conn_hash {
> struct list_head list;
> unsigned int acl_num;
> @@ -568,6 +605,11 @@ struct hci_dev {
> const char *fw_info;
> struct dentry *debugfs;
>
> + struct hci_devcoredump dump;
> + struct sk_buff_head dump_q;
> + struct work_struct dump_rx;
> + struct delayed_work dump_timeout;
> +
> struct device dev;
>
> struct rfkill *rfkill;
> @@ -1308,6 +1350,15 @@ static inline void hci_set_aosp_capable(struct hci_dev *hdev)
> #endif
> }
>
> +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);
> +void hci_devcoredump_reset(struct hci_dev *hdev);
> +
> int hci_dev_open(__u16 dev);
> int hci_dev_close(__u16 dev);
> int hci_dev_do_close(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b4782a6c1025..76dbb6b28870 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -28,6 +28,7 @@
> #include <linux/export.h>
> #include <linux/rfkill.h>
> #include <linux/debugfs.h>
> +#include <linux/devcoredump.h>
> #include <linux/crypto.h>
> #include <linux/property.h>
> #include <linux/suspend.h>
> @@ -2404,6 +2405,498 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
> return NOTIFY_DONE;
> }
>
> +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_timeout);
> + skb_queue_purge(&hdev->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.
> + */
> +static void hci_devcoredump_rx(struct work_struct *work)
> +{
> + struct hci_dev *hdev = container_of(work, struct hci_dev, 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_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_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);
> + }
> +}
> +
> +static void hci_devcoredump_timeout(struct work_struct *work)
> +{
> + struct hci_dev *hdev = container_of(work, struct hci_dev,
> + dump_timeout.work);
> + u32 dump_size;
> +
> + hci_devcoredump_notify(hdev, HCI_DEVCOREDUMP_TIMEOUT);
> +
> + hci_dev_lock(hdev);
> +
> + cancel_work_sync(&hdev->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);
> +}
> +
> +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.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_q, skb);
> + queue_work(hdev->workqueue, &hdev->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.supported) {
> + kfree_skb(skb);
> + return -EOPNOTSUPP;
> + }
> +
> + hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_SKB;
> +
> + skb_queue_tail(&hdev->dump_q, skb);
> + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
> + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
> + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
> + queue_work(hdev->workqueue, &hdev->dump_rx);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hci_devcoredump_abort);
> +
> /* Alloc HCI device */
> struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
> {
> @@ -2511,14 +3004,17 @@ 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);
> + INIT_WORK(&hdev->dump_rx, hci_devcoredump_rx);
>
> hci_cmd_sync_init(hdev);
>
> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
> + INIT_DELAYED_WORK(&hdev->dump_timeout, hci_devcoredump_timeout);
>
> skb_queue_head_init(&hdev->rx_q);
> skb_queue_head_init(&hdev->cmd_q);
> skb_queue_head_init(&hdev->raw_q);
> + skb_queue_head_init(&hdev->dump_q);
>
> init_waitqueue_head(&hdev->req_wait_q);
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 8f4c5698913d..ec03fa871ef0 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3877,6 +3877,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.35.1.894.gb6a874cedc-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add support for devcoredump
[not found] ` <CAGPPCLDu5_ES5CcsUU3qHLWHkzAV=FUU8rns8Twh6C5FVSeEUQ@mail.gmail.com>
@ 2022-05-04 0:04 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLBp7kOXquyJzHaYxsH8=GkJ6M6gC1twwc=2iMJvB+n=qQ@mail.gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-04 0:04 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Manish,
On Tue, May 3, 2022 at 4:00 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Mon, Mar 21, 2022 at 9:46 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Manish,
>>
>> On Sun, Mar 20, 2022 at 6:34 PM Manish Mandlik <mmandlik@google.com> wrote:
>> >
>> > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> >
>> > 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.
>>
>> If this sort of information comes from telemetry support it can be
>> intercepted by other means as well e.g. btmon, in fact I think that
>> would have been better since we already dump the backtrace of
>> bluetoothd crashes into the monitor as well. Anyway the kernel
>> devcoredump support seems to just be dumping the data into sysfs so we
>> could be fetching that information from there as well but I'm not
>> really sure what we would be gaining with that since we are just
>> adding yet another kernel dependency for something that userspace
>> could already be doing by itself.
>
> Currently every vendor has their own way of collecting firmware dumps. The main intention of adding devcoredump support for bluetooth is to provide a unified way to collect firmware crashdumps across all vendors/controllers. This will generate a dump file in the sysfs and vendors can use it for further debugging. There is no plan to include any information from these dumps into the btmon logs.
>
>>
>> > 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.
>>
>> Is there some userspace component parsing these dumps? Or we will need
>> to add some support for the likes of btmon to collect the logs from
>> sysfs?
>
> Yes, user space component is required to parse these dumps. But these tools will be vendor specific and could be proprietary. However, ChromeOS would be using these dumps to identify bluetooth firmware crashes. I'll share more information on ChromeOS use case offline.
Fair enough, but we do need some way to exercise this code by the CI,
so at very least we need emulator support otherwise it needs to be
behind an experimental UUID until proper tests are introduced.
>>
>>
>> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> > Signed-off-by: Manish Mandlik <mmandlik@google.com>
>> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> > ---
>> >
>> > include/net/bluetooth/hci_core.h | 51 ++++
>> > net/bluetooth/hci_core.c | 496 +++++++++++++++++++++++++++++++
>> > net/bluetooth/hci_sync.c | 2 +
>> > 3 files changed, 549 insertions(+)
>> >
>> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> > index d5377740e99c..818ba3a43e8c 100644
>> > --- a/include/net/bluetooth/hci_core.h
>> > +++ b/include/net/bluetooth/hci_core.h
>> > @@ -118,6 +118,43 @@ enum suspended_state {
>> > BT_SUSPEND_CONFIGURE_WAKE,
>> > };
>> >
>> > +#define DEVCOREDUMP_TIMEOUT msecs_to_jiffies(100000) /* 100 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
>> > + *
>> > + * @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 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;
>> > +
>> > + dmp_hdr_t dmp_hdr;
>> > + notify_change_t notify_change;
>> > +};
>> > +
>> > struct hci_conn_hash {
>> > struct list_head list;
>> > unsigned int acl_num;
>> > @@ -568,6 +605,11 @@ struct hci_dev {
>> > const char *fw_info;
>> > struct dentry *debugfs;
>> >
>> > + struct hci_devcoredump dump;
>> > + struct sk_buff_head dump_q;
>> > + struct work_struct dump_rx;
>> > + struct delayed_work dump_timeout;
>> > +
>> > struct device dev;
>> >
>> > struct rfkill *rfkill;
>> > @@ -1308,6 +1350,15 @@ static inline void hci_set_aosp_capable(struct hci_dev *hdev)
>> > #endif
>> > }
>> >
>> > +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);
>> > +void hci_devcoredump_reset(struct hci_dev *hdev);
>> > +
>> > int hci_dev_open(__u16 dev);
>> > int hci_dev_close(__u16 dev);
>> > int hci_dev_do_close(struct hci_dev *hdev);
>> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> > index b4782a6c1025..76dbb6b28870 100644
>> > --- a/net/bluetooth/hci_core.c
>> > +++ b/net/bluetooth/hci_core.c
>> > @@ -28,6 +28,7 @@
>> > #include <linux/export.h>
>> > #include <linux/rfkill.h>
>> > #include <linux/debugfs.h>
>> > +#include <linux/devcoredump.h>
>> > #include <linux/crypto.h>
>> > #include <linux/property.h>
>> > #include <linux/suspend.h>
>> > @@ -2404,6 +2405,498 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
>> > return NOTIFY_DONE;
>> > }
>> >
>> > +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,
>> > +};
I'd move the documentation of the states here, also perhaps we should
have the handling of devcoredump in its own file e.g. coredump.c,
since hci_core.c is quite big already adding things that are not part
of HCI/core specification makes it even bigger.
>> > +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_timeout);
>> > + skb_queue_purge(&hdev->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.
>> > + */
>> > +static void hci_devcoredump_rx(struct work_struct *work)
>> > +{
>> > + struct hci_dev *hdev = container_of(work, struct hci_dev, 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_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_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);
>> > + }
>> > +}
>> > +
>> > +static void hci_devcoredump_timeout(struct work_struct *work)
>> > +{
>> > + struct hci_dev *hdev = container_of(work, struct hci_dev,
>> > + dump_timeout.work);
>> > + u32 dump_size;
>> > +
>> > + hci_devcoredump_notify(hdev, HCI_DEVCOREDUMP_TIMEOUT);
>> > +
>> > + hci_dev_lock(hdev);
>> > +
>> > + cancel_work_sync(&hdev->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);
>> > +}
>> > +
>> > +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.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_q, skb);
>> > + queue_work(hdev->workqueue, &hdev->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.supported) {
>> > + kfree_skb(skb);
>> > + return -EOPNOTSUPP;
>> > + }
>> > +
>> > + hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_SKB;
>> > +
>> > + skb_queue_tail(&hdev->dump_q, skb);
>> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> > + queue_work(hdev->workqueue, &hdev->dump_rx);
>> > +
>> > + return 0;
>> > +}
>> > +EXPORT_SYMBOL(hci_devcoredump_abort);
>> > +
>> > /* Alloc HCI device */
>> > struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
>> > {
>> > @@ -2511,14 +3004,17 @@ 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);
>> > + INIT_WORK(&hdev->dump_rx, hci_devcoredump_rx);
>> >
>> > hci_cmd_sync_init(hdev);
>> >
>> > INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>> > + INIT_DELAYED_WORK(&hdev->dump_timeout, hci_devcoredump_timeout);
>> >
>> > skb_queue_head_init(&hdev->rx_q);
>> > skb_queue_head_init(&hdev->cmd_q);
>> > skb_queue_head_init(&hdev->raw_q);
>> > + skb_queue_head_init(&hdev->dump_q);
Perhaps it is a better idea to have this fields as part of a dedicated
struct for coredumps e.g. struct hci_devcoredump, also perhaps they
need to be conditional to CONFIG_DEV_COREDUMP otherwise it just adds
things like a delayed work that will never going to be used, this just
reinforces the need of proper CI tests so we can exercise this code
with the likes of the emulator.
>> > init_waitqueue_head(&hdev->req_wait_q);
>> >
>> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>> > index 8f4c5698913d..ec03fa871ef0 100644
>> > --- a/net/bluetooth/hci_sync.c
>> > +++ b/net/bluetooth/hci_sync.c
>> > @@ -3877,6 +3877,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.35.1.894.gb6a874cedc-goog
>> >
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Regards,
> Manish.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add support for devcoredump
[not found] ` <CAGPPCLBp7kOXquyJzHaYxsH8=GkJ6M6gC1twwc=2iMJvB+n=qQ@mail.gmail.com>
@ 2022-06-29 23:56 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-06-29 23:56 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Abhishek Pandit-Subedi, David S. Miller, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Manish,
On Thu, Jun 23, 2022 at 12:43 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, May 3, 2022 at 5:04 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Manish,
>>
>> On Tue, May 3, 2022 at 4:00 PM Manish Mandlik <mmandlik@google.com> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Mon, Mar 21, 2022 at 9:46 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> >>
>> >> Hi Manish,
>> >>
>> >> On Sun, Mar 20, 2022 at 6:34 PM Manish Mandlik <mmandlik@google.com> wrote:
>> >> >
>> >> > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> >> >
>> >> > 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.
>> >>
>> >> If this sort of information comes from telemetry support it can be
>> >> intercepted by other means as well e.g. btmon, in fact I think that
>> >> would have been better since we already dump the backtrace of
>> >> bluetoothd crashes into the monitor as well. Anyway the kernel
>> >> devcoredump support seems to just be dumping the data into sysfs so we
>> >> could be fetching that information from there as well but I'm not
>> >> really sure what we would be gaining with that since we are just
>> >> adding yet another kernel dependency for something that userspace
>> >> could already be doing by itself.
>> >
>> > Currently every vendor has their own way of collecting firmware dumps. The main intention of adding devcoredump support for bluetooth is to provide a unified way to collect firmware crashdumps across all vendors/controllers. This will generate a dump file in the sysfs and vendors can use it for further debugging. There is no plan to include any information from these dumps into the btmon logs.
>> >
>> >>
>> >> > 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.
>> >>
>> >> Is there some userspace component parsing these dumps? Or we will need
>> >> to add some support for the likes of btmon to collect the logs from
>> >> sysfs?
>> >
>> > Yes, user space component is required to parse these dumps. But these tools will be vendor specific and could be proprietary. However, ChromeOS would be using these dumps to identify bluetooth firmware crashes. I'll share more information on ChromeOS use case offline.
>>
>> Fair enough, but we do need some way to exercise this code by the CI,
>> so at very least we need emulator support otherwise it needs to be
>> behind an experimental UUID until proper tests are introduced.
>
> I can add emulator support. Can you please share some instructions/documentation on how to create and run emulator tests? I'll send a separate patch for adding emulator support.
They are the same tests run in CI, have a look at:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/test-runner.txt
>>
>>
>> >>
>> >>
>> >> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> >> > Signed-off-by: Manish Mandlik <mmandlik@google.com>
>> >> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> >> > ---
>> >> >
>> >> > include/net/bluetooth/hci_core.h | 51 ++++
>> >> > net/bluetooth/hci_core.c | 496 +++++++++++++++++++++++++++++++
>> >> > net/bluetooth/hci_sync.c | 2 +
>> >> > 3 files changed, 549 insertions(+)
>> >> >
>> >> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> >> > index d5377740e99c..818ba3a43e8c 100644
>> >> > --- a/include/net/bluetooth/hci_core.h
>> >> > +++ b/include/net/bluetooth/hci_core.h
>> >> > @@ -118,6 +118,43 @@ enum suspended_state {
>> >> > BT_SUSPEND_CONFIGURE_WAKE,
>> >> > };
>> >> >
>> >> > +#define DEVCOREDUMP_TIMEOUT msecs_to_jiffies(100000) /* 100 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
>> >> > + *
>> >> > + * @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 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;
>> >> > +
>> >> > + dmp_hdr_t dmp_hdr;
>> >> > + notify_change_t notify_change;
>> >> > +};
>> >> > +
>> >> > struct hci_conn_hash {
>> >> > struct list_head list;
>> >> > unsigned int acl_num;
>> >> > @@ -568,6 +605,11 @@ struct hci_dev {
>> >> > const char *fw_info;
>> >> > struct dentry *debugfs;
>> >> >
>> >> > + struct hci_devcoredump dump;
>> >> > + struct sk_buff_head dump_q;
>> >> > + struct work_struct dump_rx;
>> >> > + struct delayed_work dump_timeout;
>> >> > +
>> >> > struct device dev;
>> >> >
>> >> > struct rfkill *rfkill;
>> >> > @@ -1308,6 +1350,15 @@ static inline void hci_set_aosp_capable(struct hci_dev *hdev)
>> >> > #endif
>> >> > }
>> >> >
>> >> > +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);
>> >> > +void hci_devcoredump_reset(struct hci_dev *hdev);
>> >> > +
>> >> > int hci_dev_open(__u16 dev);
>> >> > int hci_dev_close(__u16 dev);
>> >> > int hci_dev_do_close(struct hci_dev *hdev);
>> >> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> >> > index b4782a6c1025..76dbb6b28870 100644
>> >> > --- a/net/bluetooth/hci_core.c
>> >> > +++ b/net/bluetooth/hci_core.c
>> >> > @@ -28,6 +28,7 @@
>> >> > #include <linux/export.h>
>> >> > #include <linux/rfkill.h>
>> >> > #include <linux/debugfs.h>
>> >> > +#include <linux/devcoredump.h>
>> >> > #include <linux/crypto.h>
>> >> > #include <linux/property.h>
>> >> > #include <linux/suspend.h>
>> >> > @@ -2404,6 +2405,498 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
>> >> > return NOTIFY_DONE;
>> >> > }
>> >> >
>> >> > +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,
>> >> > +};
>>
>> I'd move the documentation of the states here, also perhaps we should
>> have the handling of devcoredump in its own file e.g. coredump.c,
>> since hci_core.c is quite big already adding things that are not part
>> of HCI/core specification makes it even bigger.
>
> Since these are just packet types, I'm keeping the documentation of the states with the state machine function hci_devcoredump_rx(). However, I have moved all hci_devcoredump code to a new file - net/bluetooth/coredump.c.
>
>>
>> >> > +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_timeout);
>> >> > + skb_queue_purge(&hdev->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.
>> >> > + */
>> >> > +static void hci_devcoredump_rx(struct work_struct *work)
>> >> > +{
>> >> > + struct hci_dev *hdev = container_of(work, struct hci_dev, 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_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_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);
>> >> > + }
>> >> > +}
>> >> > +
>> >> > +static void hci_devcoredump_timeout(struct work_struct *work)
>> >> > +{
>> >> > + struct hci_dev *hdev = container_of(work, struct hci_dev,
>> >> > + dump_timeout.work);
>> >> > + u32 dump_size;
>> >> > +
>> >> > + hci_devcoredump_notify(hdev, HCI_DEVCOREDUMP_TIMEOUT);
>> >> > +
>> >> > + hci_dev_lock(hdev);
>> >> > +
>> >> > + cancel_work_sync(&hdev->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);
>> >> > +}
>> >> > +
>> >> > +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.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_q, skb);
>> >> > + queue_work(hdev->workqueue, &hdev->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.supported) {
>> >> > + kfree_skb(skb);
>> >> > + return -EOPNOTSUPP;
>> >> > + }
>> >> > +
>> >> > + hci_dmp_cb(skb)->pkt_type = HCI_DEVCOREDUMP_PKT_SKB;
>> >> > +
>> >> > + skb_queue_tail(&hdev->dump_q, skb);
>> >> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> >> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> >> > + queue_work(hdev->workqueue, &hdev->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.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_q, skb);
>> >> > + queue_work(hdev->workqueue, &hdev->dump_rx);
>> >> > +
>> >> > + return 0;
>> >> > +}
>> >> > +EXPORT_SYMBOL(hci_devcoredump_abort);
>> >> > +
>> >> > /* Alloc HCI device */
>> >> > struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
>> >> > {
>> >> > @@ -2511,14 +3004,17 @@ 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);
>> >> > + INIT_WORK(&hdev->dump_rx, hci_devcoredump_rx);
>> >> >
>> >> > hci_cmd_sync_init(hdev);
>> >> >
>> >> > INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>> >> > + INIT_DELAYED_WORK(&hdev->dump_timeout, hci_devcoredump_timeout);
>> >> >
>> >> > skb_queue_head_init(&hdev->rx_q);
>> >> > skb_queue_head_init(&hdev->cmd_q);
>> >> > skb_queue_head_init(&hdev->raw_q);
>> >> > + skb_queue_head_init(&hdev->dump_q);
>>
>> Perhaps it is a better idea to have this fields as part of a dedicated
>> struct for coredumps e.g. struct hci_devcoredump, also perhaps they
>> need to be conditional to CONFIG_DEV_COREDUMP otherwise it just adds
>> things like a delayed work that will never going to be used, this just
>> reinforces the need of proper CI tests so we can exercise this code
>> with the likes of the emulator.
>
> Done. I just submitted a v2 patch series with all above changes. Please take a look.
>
>>
>>
>> >> > init_waitqueue_head(&hdev->req_wait_q);
>> >> >
>> >> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>> >> > index 8f4c5698913d..ec03fa871ef0 100644
>> >> > --- a/net/bluetooth/hci_sync.c
>> >> > +++ b/net/bluetooth/hci_sync.c
>> >> > @@ -3877,6 +3877,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.35.1.894.gb6a874cedc-goog
>> >> >
>> >>
>> >>
>> >> --
>> >> Luiz Augusto von Dentz
>> >
>> > Regards,
>> > Manish.
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Regards,
> Manish.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add support for devcoredump
@ 2022-03-25 8:04 kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-03-25 8:04 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 19921 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220320183225.1.Iaf638bb9f885f5880ab1b4e7ae2f73dd53a54661@changeid>
References: <20220320183225.1.Iaf638bb9f885f5880ab1b4e7ae2f73dd53a54661@changeid>
TO: Manish Mandlik <mmandlik@google.com>
TO: marcel(a)holtmann.org
TO: luiz.dentz(a)gmail.com
CC: chromeos-bluetooth-upstreaming(a)chromium.org
CC: linux-bluetooth(a)vger.kernel.org
CC: "Abhishek Pandit-Subedi" <abhishekpandit@chromium.org>
CC: Manish Mandlik <mmandlik@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Johan Hedberg <johan.hedberg@gmail.com>
CC: Paolo Abeni <pabeni@redhat.com>
CC: linux-kernel(a)vger.kernel.org
CC: netdev(a)vger.kernel.org
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 v5.17 next-20220324]
[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]
url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220325/202203251548.aVjcdELg-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 85e9b2687a13d1908aa86d1b89c5ce398a06cd39)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ac170a5ea7c81e445f903d14dce2a5f92155d1ef
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
git checkout ac170a5ea7c81e445f903d14dce2a5f92155d1ef
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/md/bcache/btree.c:764:2: note: Loop condition is true. Entering loop body
while (!list_empty(&c->btree_cache_freed)) {
^
drivers/md/bcache/btree.c:767:3: note: Calling 'list_del'
list_del(&b->list);
^~~~~~~~~~~~~~~~~~
include/linux/list.h:149:14: note: Use of memory after it is freed
entry->next = LIST_POISON1;
~~~~~~~~~~~ ^
1 warning generated.
drivers/nvme/target/fcloop.c:1226:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
ret = -EINVAL;
^ ~~~~~~~
drivers/nvme/target/fcloop.c:1226:3: note: Value stored to 'ret' is never read
ret = -EINVAL;
^ ~~~~~~~
1 warning generated.
drivers/i3c/master/mipi-i3c-hci/ext_caps.c:78:7: warning: Value stored to 'mode_entry' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
u32 mode_entry = readl(base);
^~~~~~~~~~
drivers/i3c/master/mipi-i3c-hci/ext_caps.c:78:7: note: Value stored to 'mode_entry' during its initialization is never read
u32 mode_entry = readl(base);
^~~~~~~~~~
1 warning generated.
sound/firewire/motu/motu-pcm.c:363:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(pcm->name, motu->card->shortname);
^~~~~~
sound/firewire/motu/motu-pcm.c:363:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(pcm->name, motu->card->shortname);
^~~~~~
1 warning generated.
drivers/accessibility/speakup/speakup_soft.c:174:2: warning: Value stored to 'cp' is never read [clang-analyzer-deadcode.DeadStores]
cp = cp + scnprintf(cp, len, "\n");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/accessibility/speakup/speakup_soft.c:174:2: note: Value stored to 'cp' is never read
cp = cp + scnprintf(cp, len, "\n");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
drivers/bcma/main.c:674:3: warning: Value stored to 'err' is never read [clang-analyzer-deadcode.DeadStores]
err = 0;
^ ~
drivers/bcma/main.c:674:3: note: Value stored to 'err' is never read
err = 0;
^ ~
3 warnings generated.
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c:536:2: warning: Value stored to 'err' is never read [clang-analyzer-deadcode.DeadStores]
err = anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c:536:2: note: Value stored to 'err' is never read
err = anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Suppressed 2 warnings (2 with check filters).
1 warning generated.
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:424:2: warning: Value stored to 'reg' is never read [clang-analyzer-deadcode.DeadStores]
reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
^
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:424:2: note: Value stored to 'reg' is never read
1 warning generated.
drivers/mmc/core/sdio_cis.c:63:3: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(string, buf);
^~~~~~
drivers/mmc/core/sdio_cis.c:63:3: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(string, buf);
^~~~~~
1 warning generated.
drivers/w1/slaves/w1_ds2438.c:69:3: warning: Value stored to 'crc' is never read [clang-analyzer-deadcode.DeadStores]
crc = 0;
^ ~
drivers/w1/slaves/w1_ds2438.c:69:3: note: Value stored to 'crc' is never read
crc = 0;
^ ~
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
4 warnings generated.
net/bluetooth/hci_core.c:891:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(di.name, hdev->name);
^~~~~~
net/bluetooth/hci_core.c:891:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(di.name, hdev->name);
^~~~~~
>> net/bluetooth/hci_core.c:2467:2: warning: Value stored to 'ptr' is never read [clang-analyzer-deadcode.DeadStores]
ptr += read;
^ ~~~~
net/bluetooth/hci_core.c:2467:2: note: Value stored to 'ptr' is never read
ptr += read;
^ ~~~~
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
sound/drivers/mpu401/mpu401.c:67:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(card->shortname, card->driver);
^~~~~~
sound/drivers/mpu401/mpu401.c:67:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(card->shortname, card->driver);
^~~~~~
sound/drivers/mpu401/mpu401.c:72:3: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcat(card->longname, "polled");
^~~~~~
sound/drivers/mpu401/mpu401.c:72:3: note: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119
strcat(card->longname, "polled");
^~~~~~
1 warning generated.
sound/isa/es18xx.c:1777:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(card->mixername, chip->pcm->name);
^~~~~~
sound/isa/es18xx.c:1777:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(card->mixername, chip->pcm->name);
^~~~~~
1 warning generated.
sound/isa/opl3sa2.c:242:2: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcat(card->shortname, str);
^~~~~~
sound/isa/opl3sa2.c:242:2: note: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119
strcat(card->shortname, str);
^~~~~~
1 warning generated.
sound/isa/sscape.c:1052:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(card->shortname, name);
^~~~~~
sound/isa/sscape.c:1052:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(card->shortname, name);
^~~~~~
1 warning generated.
drivers/nvme/target/core.c:1124:10: warning: Although the value stored to 'cc_css' is used in the enclosing expression, the value is never actually read from 'cc_css' [clang-analyzer-deadcode.DeadStores]
switch (cc_css <<= NVME_CC_CSS_SHIFT) {
^ ~~~~~~~~~~~~~~~~~
drivers/nvme/target/core.c:1124:10: note: Although the value stored to 'cc_css' is used in the enclosing expression, the value is never actually read from 'cc_css'
switch (cc_css <<= NVME_CC_CSS_SHIFT) {
^ ~~~~~~~~~~~~~~~~~
2 warnings generated.
drivers/media/i2c/adv7170.c:361:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7170_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:361:3: note: Value stored to 'i' is never read
i = adv7170_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:362:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7170_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:362:3: note: Value stored to 'i' is never read
i = adv7170_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
drivers/media/i2c/adv7175.c:416:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:416:3: note: Value stored to 'i' is never read
i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:417:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7175_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:417:3: note: Value stored to 'i' is never read
i = adv7175_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
vim +/ptr +2467 net/bluetooth/hci_core.c
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2445
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2446 static int hci_devcoredump_mkheader(struct hci_dev *hdev, char *buf,
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2447 size_t buf_size)
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2448 {
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2449 char *ptr = buf;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2450 size_t rem = buf_size;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2451 size_t read = 0;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2452
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2453 read = hci_devcoredump_update_hdr_state(ptr, rem, HCI_DEVCOREDUMP_IDLE);
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2454 read += 1; /* update_hdr_state adds \0@the end upon state rewrite */
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2455 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2456 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2457
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2458 if (hdev->dump.dmp_hdr) {
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2459 /* dmp_hdr() should return number of bytes written */
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2460 read = hdev->dump.dmp_hdr(hdev, ptr, rem);
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2461 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2462 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2463 }
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2464
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2465 read = snprintf(ptr, rem, "--- Start dump ---\n");
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2466 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 @2467 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2468
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2469 return buf_size - rem;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2470 }
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2471
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add support for devcoredump
@ 2022-03-24 4:31 kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-03-24 4:31 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 20322 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220320183225.1.Iaf638bb9f885f5880ab1b4e7ae2f73dd53a54661@changeid>
References: <20220320183225.1.Iaf638bb9f885f5880ab1b4e7ae2f73dd53a54661@changeid>
TO: Manish Mandlik <mmandlik@google.com>
TO: marcel(a)holtmann.org
TO: luiz.dentz(a)gmail.com
CC: chromeos-bluetooth-upstreaming(a)chromium.org
CC: linux-bluetooth(a)vger.kernel.org
CC: "Abhishek Pandit-Subedi" <abhishekpandit@chromium.org>
CC: Manish Mandlik <mmandlik@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Johan Hedberg <johan.hedberg@gmail.com>
CC: Paolo Abeni <pabeni@redhat.com>
CC: linux-kernel(a)vger.kernel.org
CC: netdev(a)vger.kernel.org
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 v5.17 next-20220323]
[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]
url: https://github.com/0day-ci/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220324/202203241228.EDfAR8zW-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 85e9b2687a13d1908aa86d1b89c5ce398a06cd39)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ac170a5ea7c81e445f903d14dce2a5f92155d1ef
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220321-093553
git checkout ac170a5ea7c81e445f903d14dce2a5f92155d1ef
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
strcpy(motu->card->mixername, motu->spec->name);
^~~~~~
sound/firewire/motu/motu.c:46:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(motu->card->mixername, motu->spec->name);
^~~~~~
1 warning generated.
sound/firewire/motu/motu-pcm.c:363:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(pcm->name, motu->card->shortname);
^~~~~~
sound/firewire/motu/motu-pcm.c:363:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(pcm->name, motu->card->shortname);
^~~~~~
2 warnings generated.
sound/firewire/fireface/ff.c:31:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(ff->card->shortname, name);
^~~~~~
sound/firewire/fireface/ff.c:31:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(ff->card->shortname, name);
^~~~~~
sound/firewire/fireface/ff.c:32:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(ff->card->mixername, name);
^~~~~~
sound/firewire/fireface/ff.c:32:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(ff->card->mixername, name);
^~~~~~
1 warning generated.
sound/firewire/fireface/ff-hwdep.c:181:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(hwdep->name, ff->card->driver);
^~~~~~
sound/firewire/fireface/ff-hwdep.c:181:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(hwdep->name, ff->card->driver);
^~~~~~
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings generated.
fs/xfs/xfs_reflink.c:1154:3: warning: Value stored to 'qdelta' is never read [clang-analyzer-deadcode.DeadStores]
qdelta += dmap->br_blockcount;
^ ~~~~~~~~~~~~~~~~~~~
fs/xfs/xfs_reflink.c:1154:3: note: Value stored to 'qdelta' is never read
qdelta += dmap->br_blockcount;
^ ~~~~~~~~~~~~~~~~~~~
Suppressed 2 warnings (1 in non-user code, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
drivers/net/can/softing/softing_fw.c:531:3: warning: Value stored to 'error_reporting' is never read [clang-analyzer-deadcode.DeadStores]
error_reporting += softing_error_reporting(netdev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/can/softing/softing_fw.c:531:3: note: Value stored to 'error_reporting' is never read
error_reporting += softing_error_reporting(netdev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings generated.
net/bluetooth/hci_core.c:891:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(di.name, hdev->name);
^~~~~~
net/bluetooth/hci_core.c:891:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(di.name, hdev->name);
^~~~~~
>> net/bluetooth/hci_core.c:2467:2: warning: Value stored to 'ptr' is never read [clang-analyzer-deadcode.DeadStores]
ptr += read;
^ ~~~~
net/bluetooth/hci_core.c:2467:2: note: Value stored to 'ptr' is never read
ptr += read;
^ ~~~~
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c:372:4: warning: Value stored to 'err' is never read [clang-analyzer-deadcode.DeadStores]
err = 0;
^ ~
drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c:372:4: note: Value stored to 'err' is never read
err = 0;
^ ~
1 warning generated.
drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c:152:34: warning: Value stored to 'tx_ring' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
const struct mcp251xfd_tx_ring *tx_ring = priv->tx;
^~~~~~~ ~~~~~~~~
drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c:152:34: note: Value stored to 'tx_ring' during its initialization is never read
const struct mcp251xfd_tx_ring *tx_ring = priv->tx;
^~~~~~~ ~~~~~~~~
1 warning generated.
sound/soc/codecs/da7213.c:1427:3: warning: Value stored to 'indiv' is never read [clang-analyzer-deadcode.DeadStores]
indiv = DA7213_PLL_INDIV_9_TO_18_MHZ_VAL;
^
sound/soc/codecs/da7213.c:1427:3: note: Value stored to 'indiv' is never read
1 warning generated.
drivers/media/i2c/imx258.c:781:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/imx258.c:781:3: note: Value stored to 'ret' is never read
ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
sound/firewire/fireworks/fireworks.c:94:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(efw->card->shortname, hwinfo->model_name);
^~~~~~
sound/firewire/fireworks/fireworks.c:94:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(efw->card->shortname, hwinfo->model_name);
^~~~~~
sound/firewire/fireworks/fireworks.c:95:2: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcpy(efw->card->mixername, hwinfo->model_name);
^~~~~~
sound/firewire/fireworks/fireworks.c:95:2: note: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
strcpy(efw->card->mixername, hwinfo->model_name);
^~~~~~
1 warning generated.
drivers/input/rmi4/rmi_f11.c:1129:2: warning: Value stored to 'query_offset' is never read [clang-analyzer-deadcode.DeadStores]
query_offset += rc;
^ ~~
drivers/input/rmi4/rmi_f11.c:1129:2: note: Value stored to 'query_offset' is never read
query_offset += rc;
^ ~~
3 warnings generated.
drivers/input/rmi4/rmi_f12.c:133:3: warning: Value stored to 'offset' is never read [clang-analyzer-deadcode.DeadStores]
offset += 1;
^ ~
drivers/input/rmi4/rmi_f12.c:133:3: note: Value stored to 'offset' is never read
offset += 1;
^ ~
drivers/input/rmi4/rmi_f12.c:401:2: warning: Value stored to 'query_addr' is never read [clang-analyzer-deadcode.DeadStores]
query_addr += 3;
^ ~
drivers/input/rmi4/rmi_f12.c:401:2: note: Value stored to 'query_addr' is never read
query_addr += 3;
^ ~
drivers/input/rmi4/rmi_f12.c:520:3: warning: Value stored to 'data_offset' is never read [clang-analyzer-deadcode.DeadStores]
data_offset += item->reg_size;
^ ~~~~~~~~~~~~~~
drivers/input/rmi4/rmi_f12.c:520:3: note: Value stored to 'data_offset' is never read
data_offset += item->reg_size;
^ ~~~~~~~~~~~~~~
2 warnings generated.
drivers/media/i2c/adv7170.c:361:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7170_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:361:3: note: Value stored to 'i' is never read
i = adv7170_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:362:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7170_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7170.c:362:3: note: Value stored to 'i' is never read
i = adv7170_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
drivers/media/i2c/adv7175.c:416:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:416:3: note: Value stored to 'i' is never read
i = adv7175_write(sd, 0x07, TR0MODE | TR0RST);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:417:3: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
i = adv7175_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/i2c/adv7175.c:417:3: note: Value stored to 'i' is never read
i = adv7175_write(sd, 0x07, TR0MODE);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
arch/x86/include/asm/paravirt.h:55:2: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign]
vim +/ptr +2467 net/bluetooth/hci_core.c
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2445
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2446 static int hci_devcoredump_mkheader(struct hci_dev *hdev, char *buf,
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2447 size_t buf_size)
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2448 {
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2449 char *ptr = buf;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2450 size_t rem = buf_size;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2451 size_t read = 0;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2452
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2453 read = hci_devcoredump_update_hdr_state(ptr, rem, HCI_DEVCOREDUMP_IDLE);
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2454 read += 1; /* update_hdr_state adds \0 at the end upon state rewrite */
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2455 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2456 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2457
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2458 if (hdev->dump.dmp_hdr) {
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2459 /* dmp_hdr() should return number of bytes written */
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2460 read = hdev->dump.dmp_hdr(hdev, ptr, rem);
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2461 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2462 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2463 }
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2464
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2465 read = snprintf(ptr, rem, "--- Start dump ---\n");
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2466 rem -= read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 @2467 ptr += read;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2468
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2469 return buf_size - rem;
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2470 }
ac170a5ea7c81e Abhishek Pandit-Subedi 2022-03-20 2471
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-06-29 23:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-21 1:34 [PATCH 1/2] Bluetooth: Add support for devcoredump Manish Mandlik
2022-03-21 1:34 ` [PATCH 2/2] Bluetooth: btusb: Add Intel devcoredump support Manish Mandlik
2022-03-21 7:38 ` kernel test robot
2022-03-21 2:34 ` [1/2] Bluetooth: Add support for devcoredump bluez.test.bot
2022-03-21 16:46 ` [PATCH 1/2] " Luiz Augusto von Dentz
[not found] ` <CAGPPCLDu5_ES5CcsUU3qHLWHkzAV=FUU8rns8Twh6C5FVSeEUQ@mail.gmail.com>
2022-05-04 0:04 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLBp7kOXquyJzHaYxsH8=GkJ6M6gC1twwc=2iMJvB+n=qQ@mail.gmail.com>
2022-06-29 23:56 ` Luiz Augusto von Dentz
2022-03-24 4:31 kernel test robot
2022-03-25 8:04 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.