* [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-03 16:25 ` greearb
0 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Ben Greear
From: Ben Greear <greearb@candelatech.com>
Store the firmware crash registers and last 128 or so
firmware debug-log ids and present them to user-space
via debugfs.
Should help with figuring out why the firmware crashed.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
This series is compile-tested only at this point. Hoping
for feedback on general approach at least.
drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
4 files changed, 273 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 68ceef6..4068910 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -41,6 +41,8 @@
#define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
#define ATH10K_NUM_CHANS 38
+#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
+
/* Antenna noise floor */
#define ATH10K_DEFAULT_NOISE_FLOOR -95
@@ -338,6 +340,39 @@ enum ath10k_dev_flags {
ATH10K_FLAG_CORE_REGISTERED,
};
+/**
+ * enum ath10k_fw_error_dump_type - types of data in the dump file
+ * @ATH10K_FW_ERROR_DUMP_DBGLOG: Recent firmware debug log entries
+ * @ATH10K_FW_ERROR_DUMP_CRASH: Crash dump in binary format
+ */
+enum ath10k_fw_error_dump_type {
+ ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
+ ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
+
+ ATH10K_FW_ERROR_DUMP_MAX,
+};
+
+
+struct ath10k_tlv_dump_data {
+ u32 type; /* see ath10k_fw_error_dump_type above */
+ u32 tlv_len; /* in bytes */
+ u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
+} __packed;
+
+struct ath10k_dump_file_data {
+ u32 len;
+ u32 magic; /* 0x01020304, tells us byte-order of host if we care */
+ struct ath10k_tlv_dump_data data; /* more may follow */
+} __packed;
+
+/* This will store at least the last 128 entries. */
+#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
+struct ath10k_dbglog_entry_storage {
+ u32 next_idx; /* Where to write next chunk of data */
+ u8 data[ATH10K_DBGLOG_DATA_LEN];
+};
+
+
struct ath10k {
struct ath_common ath_common;
struct ieee80211_hw *hw;
@@ -488,6 +523,12 @@ struct ath10k {
struct dfs_pattern_detector *dfs_detector;
+ /* Used for crash-dump storage */
+ /* Don't over-write dump info until someone reads the data. */
+ bool crashed_since_read;
+ struct ath10k_dbglog_entry_storage dbglog_entry_data;
+ u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
#endif
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 1b7ff4b..1f18412 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -577,6 +577,126 @@ static const struct file_operations fops_chip_id = {
.llseek = default_llseek,
};
+void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar, u8 *buffer, int len)
+{
+ int i;
+ int z = ar->dbglog_entry_data.next_idx;
+
+ /* Don't save any new logs until user-space reads this. */
+ if (ar->crashed_since_read)
+ return;
+
+ for (i = 0; i < len; i++) {
+ ar->dbglog_entry_data.data[z] = buffer[i];
+ z++;
+ if (z >= ATH10K_DBGLOG_DATA_LEN)
+ z = 0;
+ }
+ ar->dbglog_entry_data.next_idx = z;
+}
+EXPORT_SYMBOL(ath10k_dbg_save_fw_dbg_buffer);
+
+static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
+{
+ unsigned int len = (sizeof(ar->dbglog_entry_data)
+ + sizeof(ar->reg_dump_values));
+ unsigned int sofar = 0;
+ char *buf;
+ struct ath10k_tlv_dump_data *dump_tlv;
+ struct ath10k_dump_file_data *dump_data;
+ int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
+
+ len += hdr_len;
+ sofar += hdr_len;
+
+ /* So we can add headers to the data dump */
+ len += 2 * sizeof(*dump_tlv);
+
+ /* This is going to get big when we start dumping FW RAM and such,
+ * so go ahead and use vmalloc.
+ */
+ buf = vmalloc(len);
+ if (!buf)
+ return NULL;
+
+ dump_data = (struct ath10k_dump_file_data *)(buf);
+ dump_data->len = len;
+ dump_data->magic = 0x01020304;
+
+ /* Gather dbg-log */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_DBGLOG;
+ dump_tlv->tlv_len = sizeof(ar->dbglog_entry_data);
+ memcpy(dump_tlv->tlv_data, &ar->dbglog_entry_data, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ /* Gather crash-dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_REGDUMP;
+ dump_tlv->tlv_len = sizeof(ar->reg_dump_values);
+ memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ return dump_data;
+}
+
+
+
+static int ath10k_fw_error_dump_open(struct inode *inode, struct file *file)
+{
+ struct ath10k *ar = inode->i_private;
+ int ret;
+ struct ath10k_dump_file_data *dump;
+
+ if (!ar)
+ return -EINVAL;
+
+ mutex_lock(&ar->conf_mutex);
+
+ dump = ath10k_build_dump_file(ar);
+
+ if (!dump) {
+ ret = -ENODATA;
+ goto out;
+ }
+
+ file->private_data = dump;
+ ar->crashed_since_read = false;
+ ret = 0;
+
+out:
+ mutex_unlock(&ar->conf_mutex);
+ return ret;
+}
+
+
+static ssize_t ath10k_fw_error_dump_read(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath10k_dump_file_data *dump_file = file->private_data;
+
+ return simple_read_from_buffer(user_buf, count, ppos,
+ dump_file,
+ dump_file->len);
+}
+
+static int ath10k_fw_error_dump_release(struct inode *inode,
+ struct file *file)
+{
+ vfree(file->private_data);
+
+ return 0;
+}
+
+static const struct file_operations fops_fw_error_dump = {
+ .open = ath10k_fw_error_dump_open,
+ .read = ath10k_fw_error_dump_read,
+ .release = ath10k_fw_error_dump_release,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
static int ath10k_debug_htt_stats_req(struct ath10k *ar)
{
u64 cookie;
@@ -861,6 +981,9 @@ int ath10k_debug_create(struct ath10k *ar)
debugfs_create_file("simulate_fw_crash", S_IRUSR, ar->debug.debugfs_phy,
ar, &fops_simulate_fw_crash);
+ debugfs_create_file("fw_error_dump", S_IRUSR, ar->debug.debugfs_phy,
+ ar, &fops_fw_error_dump);
+
debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
ar, &fops_chip_id);
@@ -931,4 +1054,5 @@ void ath10k_dbg_dump(enum ath10k_debug_mask mask,
}
EXPORT_SYMBOL(ath10k_dbg_dump);
+
#endif /* CONFIG_ATH10K_DEBUG */
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index a582499..d9629f9 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -19,6 +19,7 @@
#define _DEBUG_H_
#include <linux/types.h>
+#include "pci.h"
#include "trace.h"
enum ath10k_debug_mask {
@@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
{
}
#endif /* CONFIG_ATH10K_DEBUG */
+
+
+/* Target debug log related defines and structs */
+
+/* Target is 32-bit CPU, so we just use u32 for
+ * the pointers. The memory space is relative to the
+ * target, not the host.
+ */
+struct dbglog_buf_s {
+ u32 next; /* pointer to dblog_buf_s. */
+ u32 buffer; /* pointer to u8 buffer */
+ u32 bufsize;
+ u32 length;
+ u32 count;
+ u32 free;
+} __packed;
+
+struct dbglog_hdr_s {
+ u32 dbuf; /* pointer to dbglog_buf_s */
+ u32 dropped;
+} __packed;
+
+void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar, u8 *buffer, int len);
+
#endif /* _DEBUG_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index d0004d5..36da9a5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -19,7 +19,7 @@
#include <linux/module.h>
#include <linux/interrupt.h>
#include <linux/spinlock.h>
-#include <linux/bitops.h>
+#include <linux/ctype.h>
#include "core.h"
#include "debug.h"
@@ -840,6 +840,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
u32 host_addr;
int ret;
u32 i;
+ struct dbglog_hdr_s dbg_hdr;
+ u32 dbufp; /* pointer in target memory space */
ath10k_err("firmware crashed!\n");
ath10k_err("hardware name %s version 0x%x\n",
@@ -851,7 +853,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
®_dump_area, sizeof(u32));
if (ret) {
ath10k_err("failed to read FW dump area address: %d\n", ret);
- return;
+ goto do_restart;
}
ath10k_err("target register Dump Location: 0x%08X\n", reg_dump_area);
@@ -861,7 +863,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
REG_DUMP_COUNT_QCA988X * sizeof(u32));
if (ret != 0) {
ath10k_err("failed to read FW dump area: %d\n", ret);
- return;
+ goto do_restart;
}
BUILD_BUG_ON(REG_DUMP_COUNT_QCA988X % 4);
@@ -875,6 +877,84 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
+ /* Dump the debug logs on the target */
+ host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
+ if (ath10k_pci_diag_read_mem(ar, host_addr,
+ ®_dump_area, sizeof(u32)) != 0) {
+ ath10k_warn("could not read hi_dbglog_hdr\n");
+ goto save_regs_and_restart;
+ }
+
+ ath10k_err("target register Debug Log Location: 0x%08X\n",
+ reg_dump_area);
+
+ ret = ath10k_pci_diag_read_mem(ar, reg_dump_area,
+ &dbg_hdr, sizeof(dbg_hdr));
+ if (ret != 0) {
+ ath10k_err("could not dump Debug Log Area\n");
+ goto save_regs_and_restart;
+ }
+
+ ath10k_err("Debug Log Header, dbuf: 0x%x dropped: %i\n",
+ dbg_hdr.dbuf, dbg_hdr.dropped);
+ dbufp = dbg_hdr.dbuf;
+ i = 0;
+ while (dbufp) {
+ struct dbglog_buf_s dbuf;
+
+ ret = ath10k_pci_diag_read_mem(ar, dbufp,
+ &dbuf, sizeof(dbuf));
+ if (ret != 0) {
+ ath10k_err("could not read Debug Log Area: 0x%x\n",
+ dbufp);
+ goto save_regs_and_restart;
+ }
+
+ /* We have a buffer of data */
+ /* TODO: Do we need to worry about bit order on some
+ * architectures? This seems to work fine with
+ * x86-64 host, at least.
+ */
+ ath10k_err("[%i] next: 0x%x buf: 0x%x sz: %i len: %i count: %i free: %i\n",
+ i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
+ dbuf.count, dbuf.free);
+ if (dbuf.buffer && dbuf.length) {
+ u8 *buffer = kmalloc(dbuf.length, GFP_ATOMIC);
+
+ if (buffer) {
+ ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer,
+ buffer,
+ dbuf.length);
+ if (ret != 0) {
+ ath10k_err("could not read Debug Log buffer: 0x%x\n",
+ dbuf.buffer);
+ kfree(buffer);
+ goto save_regs_and_restart;
+ }
+
+ ath10k_dbg_save_fw_dbg_buffer(ar, buffer,
+ dbuf.length);
+ kfree(buffer);
+ }
+ }
+ dbufp = dbuf.next;
+ if (dbufp == dbg_hdr.dbuf) {
+ /* It is a circular buffer it seems, bail if next
+ * is head
+ */
+ break;
+ }
+ i++;
+ } /* While we have a debug buffer to read */
+
+save_regs_and_restart:
+ if (!ar->crashed_since_read) {
+ ar->crashed_since_read = true;
+ memcpy(ar->reg_dump_values, reg_dump_values,
+ sizeof(ar->reg_dump_values));
+ }
+
+do_restart:
queue_work(ar->workqueue, &ar->restart_work);
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-03 16:25 ` greearb
0 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: Ben Greear, linux-wireless
From: Ben Greear <greearb@candelatech.com>
Store the firmware crash registers and last 128 or so
firmware debug-log ids and present them to user-space
via debugfs.
Should help with figuring out why the firmware crashed.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
This series is compile-tested only at this point. Hoping
for feedback on general approach at least.
drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
4 files changed, 273 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 68ceef6..4068910 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -41,6 +41,8 @@
#define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
#define ATH10K_NUM_CHANS 38
+#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
+
/* Antenna noise floor */
#define ATH10K_DEFAULT_NOISE_FLOOR -95
@@ -338,6 +340,39 @@ enum ath10k_dev_flags {
ATH10K_FLAG_CORE_REGISTERED,
};
+/**
+ * enum ath10k_fw_error_dump_type - types of data in the dump file
+ * @ATH10K_FW_ERROR_DUMP_DBGLOG: Recent firmware debug log entries
+ * @ATH10K_FW_ERROR_DUMP_CRASH: Crash dump in binary format
+ */
+enum ath10k_fw_error_dump_type {
+ ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
+ ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
+
+ ATH10K_FW_ERROR_DUMP_MAX,
+};
+
+
+struct ath10k_tlv_dump_data {
+ u32 type; /* see ath10k_fw_error_dump_type above */
+ u32 tlv_len; /* in bytes */
+ u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
+} __packed;
+
+struct ath10k_dump_file_data {
+ u32 len;
+ u32 magic; /* 0x01020304, tells us byte-order of host if we care */
+ struct ath10k_tlv_dump_data data; /* more may follow */
+} __packed;
+
+/* This will store at least the last 128 entries. */
+#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
+struct ath10k_dbglog_entry_storage {
+ u32 next_idx; /* Where to write next chunk of data */
+ u8 data[ATH10K_DBGLOG_DATA_LEN];
+};
+
+
struct ath10k {
struct ath_common ath_common;
struct ieee80211_hw *hw;
@@ -488,6 +523,12 @@ struct ath10k {
struct dfs_pattern_detector *dfs_detector;
+ /* Used for crash-dump storage */
+ /* Don't over-write dump info until someone reads the data. */
+ bool crashed_since_read;
+ struct ath10k_dbglog_entry_storage dbglog_entry_data;
+ u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
#endif
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 1b7ff4b..1f18412 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -577,6 +577,126 @@ static const struct file_operations fops_chip_id = {
.llseek = default_llseek,
};
+void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar, u8 *buffer, int len)
+{
+ int i;
+ int z = ar->dbglog_entry_data.next_idx;
+
+ /* Don't save any new logs until user-space reads this. */
+ if (ar->crashed_since_read)
+ return;
+
+ for (i = 0; i < len; i++) {
+ ar->dbglog_entry_data.data[z] = buffer[i];
+ z++;
+ if (z >= ATH10K_DBGLOG_DATA_LEN)
+ z = 0;
+ }
+ ar->dbglog_entry_data.next_idx = z;
+}
+EXPORT_SYMBOL(ath10k_dbg_save_fw_dbg_buffer);
+
+static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
+{
+ unsigned int len = (sizeof(ar->dbglog_entry_data)
+ + sizeof(ar->reg_dump_values));
+ unsigned int sofar = 0;
+ char *buf;
+ struct ath10k_tlv_dump_data *dump_tlv;
+ struct ath10k_dump_file_data *dump_data;
+ int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
+
+ len += hdr_len;
+ sofar += hdr_len;
+
+ /* So we can add headers to the data dump */
+ len += 2 * sizeof(*dump_tlv);
+
+ /* This is going to get big when we start dumping FW RAM and such,
+ * so go ahead and use vmalloc.
+ */
+ buf = vmalloc(len);
+ if (!buf)
+ return NULL;
+
+ dump_data = (struct ath10k_dump_file_data *)(buf);
+ dump_data->len = len;
+ dump_data->magic = 0x01020304;
+
+ /* Gather dbg-log */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_DBGLOG;
+ dump_tlv->tlv_len = sizeof(ar->dbglog_entry_data);
+ memcpy(dump_tlv->tlv_data, &ar->dbglog_entry_data, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ /* Gather crash-dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_REGDUMP;
+ dump_tlv->tlv_len = sizeof(ar->reg_dump_values);
+ memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ return dump_data;
+}
+
+
+
+static int ath10k_fw_error_dump_open(struct inode *inode, struct file *file)
+{
+ struct ath10k *ar = inode->i_private;
+ int ret;
+ struct ath10k_dump_file_data *dump;
+
+ if (!ar)
+ return -EINVAL;
+
+ mutex_lock(&ar->conf_mutex);
+
+ dump = ath10k_build_dump_file(ar);
+
+ if (!dump) {
+ ret = -ENODATA;
+ goto out;
+ }
+
+ file->private_data = dump;
+ ar->crashed_since_read = false;
+ ret = 0;
+
+out:
+ mutex_unlock(&ar->conf_mutex);
+ return ret;
+}
+
+
+static ssize_t ath10k_fw_error_dump_read(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath10k_dump_file_data *dump_file = file->private_data;
+
+ return simple_read_from_buffer(user_buf, count, ppos,
+ dump_file,
+ dump_file->len);
+}
+
+static int ath10k_fw_error_dump_release(struct inode *inode,
+ struct file *file)
+{
+ vfree(file->private_data);
+
+ return 0;
+}
+
+static const struct file_operations fops_fw_error_dump = {
+ .open = ath10k_fw_error_dump_open,
+ .read = ath10k_fw_error_dump_read,
+ .release = ath10k_fw_error_dump_release,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
static int ath10k_debug_htt_stats_req(struct ath10k *ar)
{
u64 cookie;
@@ -861,6 +981,9 @@ int ath10k_debug_create(struct ath10k *ar)
debugfs_create_file("simulate_fw_crash", S_IRUSR, ar->debug.debugfs_phy,
ar, &fops_simulate_fw_crash);
+ debugfs_create_file("fw_error_dump", S_IRUSR, ar->debug.debugfs_phy,
+ ar, &fops_fw_error_dump);
+
debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
ar, &fops_chip_id);
@@ -931,4 +1054,5 @@ void ath10k_dbg_dump(enum ath10k_debug_mask mask,
}
EXPORT_SYMBOL(ath10k_dbg_dump);
+
#endif /* CONFIG_ATH10K_DEBUG */
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index a582499..d9629f9 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -19,6 +19,7 @@
#define _DEBUG_H_
#include <linux/types.h>
+#include "pci.h"
#include "trace.h"
enum ath10k_debug_mask {
@@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
{
}
#endif /* CONFIG_ATH10K_DEBUG */
+
+
+/* Target debug log related defines and structs */
+
+/* Target is 32-bit CPU, so we just use u32 for
+ * the pointers. The memory space is relative to the
+ * target, not the host.
+ */
+struct dbglog_buf_s {
+ u32 next; /* pointer to dblog_buf_s. */
+ u32 buffer; /* pointer to u8 buffer */
+ u32 bufsize;
+ u32 length;
+ u32 count;
+ u32 free;
+} __packed;
+
+struct dbglog_hdr_s {
+ u32 dbuf; /* pointer to dbglog_buf_s */
+ u32 dropped;
+} __packed;
+
+void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar, u8 *buffer, int len);
+
#endif /* _DEBUG_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index d0004d5..36da9a5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -19,7 +19,7 @@
#include <linux/module.h>
#include <linux/interrupt.h>
#include <linux/spinlock.h>
-#include <linux/bitops.h>
+#include <linux/ctype.h>
#include "core.h"
#include "debug.h"
@@ -840,6 +840,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
u32 host_addr;
int ret;
u32 i;
+ struct dbglog_hdr_s dbg_hdr;
+ u32 dbufp; /* pointer in target memory space */
ath10k_err("firmware crashed!\n");
ath10k_err("hardware name %s version 0x%x\n",
@@ -851,7 +853,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
®_dump_area, sizeof(u32));
if (ret) {
ath10k_err("failed to read FW dump area address: %d\n", ret);
- return;
+ goto do_restart;
}
ath10k_err("target register Dump Location: 0x%08X\n", reg_dump_area);
@@ -861,7 +863,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
REG_DUMP_COUNT_QCA988X * sizeof(u32));
if (ret != 0) {
ath10k_err("failed to read FW dump area: %d\n", ret);
- return;
+ goto do_restart;
}
BUILD_BUG_ON(REG_DUMP_COUNT_QCA988X % 4);
@@ -875,6 +877,84 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
+ /* Dump the debug logs on the target */
+ host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
+ if (ath10k_pci_diag_read_mem(ar, host_addr,
+ ®_dump_area, sizeof(u32)) != 0) {
+ ath10k_warn("could not read hi_dbglog_hdr\n");
+ goto save_regs_and_restart;
+ }
+
+ ath10k_err("target register Debug Log Location: 0x%08X\n",
+ reg_dump_area);
+
+ ret = ath10k_pci_diag_read_mem(ar, reg_dump_area,
+ &dbg_hdr, sizeof(dbg_hdr));
+ if (ret != 0) {
+ ath10k_err("could not dump Debug Log Area\n");
+ goto save_regs_and_restart;
+ }
+
+ ath10k_err("Debug Log Header, dbuf: 0x%x dropped: %i\n",
+ dbg_hdr.dbuf, dbg_hdr.dropped);
+ dbufp = dbg_hdr.dbuf;
+ i = 0;
+ while (dbufp) {
+ struct dbglog_buf_s dbuf;
+
+ ret = ath10k_pci_diag_read_mem(ar, dbufp,
+ &dbuf, sizeof(dbuf));
+ if (ret != 0) {
+ ath10k_err("could not read Debug Log Area: 0x%x\n",
+ dbufp);
+ goto save_regs_and_restart;
+ }
+
+ /* We have a buffer of data */
+ /* TODO: Do we need to worry about bit order on some
+ * architectures? This seems to work fine with
+ * x86-64 host, at least.
+ */
+ ath10k_err("[%i] next: 0x%x buf: 0x%x sz: %i len: %i count: %i free: %i\n",
+ i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
+ dbuf.count, dbuf.free);
+ if (dbuf.buffer && dbuf.length) {
+ u8 *buffer = kmalloc(dbuf.length, GFP_ATOMIC);
+
+ if (buffer) {
+ ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer,
+ buffer,
+ dbuf.length);
+ if (ret != 0) {
+ ath10k_err("could not read Debug Log buffer: 0x%x\n",
+ dbuf.buffer);
+ kfree(buffer);
+ goto save_regs_and_restart;
+ }
+
+ ath10k_dbg_save_fw_dbg_buffer(ar, buffer,
+ dbuf.length);
+ kfree(buffer);
+ }
+ }
+ dbufp = dbuf.next;
+ if (dbufp == dbg_hdr.dbuf) {
+ /* It is a circular buffer it seems, bail if next
+ * is head
+ */
+ break;
+ }
+ i++;
+ } /* While we have a debug buffer to read */
+
+save_regs_and_restart:
+ if (!ar->crashed_since_read) {
+ ar->crashed_since_read = true;
+ memcpy(ar->reg_dump_values, reg_dump_values,
+ sizeof(ar->reg_dump_values));
+ }
+
+do_restart:
queue_work(ar->workqueue, &ar->restart_work);
}
--
1.7.11.7
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 2/4] ath10k: save firmware debug log messages.
2014-06-03 16:25 ` greearb
@ 2014-06-03 16:25 ` greearb
-1 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Ben Greear
From: Ben Greear <greearb@candelatech.com>
They may be dumped through the firmware dump debugfs
file.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/wmi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 4b7782a..7637618 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
{
+ u32 *m = (u32 *)(skb->data);
ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
skb->len);
trace_ath10k_wmi_dbglog(skb->data, skb->len);
+ ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
+
return 0;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 2/4] ath10k: save firmware debug log messages.
@ 2014-06-03 16:25 ` greearb
0 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: Ben Greear, linux-wireless
From: Ben Greear <greearb@candelatech.com>
They may be dumped through the firmware dump debugfs
file.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/wmi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 4b7782a..7637618 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
{
+ u32 *m = (u32 *)(skb->data);
ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
skb->len);
trace_ath10k_wmi_dbglog(skb->data, skb->len);
+ ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
+
return 0;
}
--
1.7.11.7
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 3/4] ath10k: save firmware stack upon firmware crash.
2014-06-03 16:25 ` greearb
@ 2014-06-03 16:25 ` greearb
-1 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Ben Greear
From: Ben Greear <greearb@candelatech.com>
Should help debug firmware crashes, and give users a way
to provide some useful debug reports to firmware developers.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/core.h | 3 +++
drivers/net/wireless/ath/ath10k/debug.c | 11 ++++++++++-
drivers/net/wireless/ath/ath10k/pci.c | 35 +++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4068910..b331942 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -42,6 +42,7 @@
#define ATH10K_NUM_CHANS 38
#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
+#define ATH10K_FW_STACK_SIZE 4096
/* Antenna noise floor */
#define ATH10K_DEFAULT_NOISE_FLOOR -95
@@ -348,6 +349,7 @@ enum ath10k_dev_flags {
enum ath10k_fw_error_dump_type {
ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
+ ATH10K_FW_ERROR_DUMP_STACK = 2,
ATH10K_FW_ERROR_DUMP_MAX,
};
@@ -528,6 +530,7 @@ struct ath10k {
bool crashed_since_read;
struct ath10k_dbglog_entry_storage dbglog_entry_data;
u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+ unsigned char stack_buf[ATH10K_FW_STACK_SIZE];
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 1f18412..9c9456d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -599,6 +599,7 @@ EXPORT_SYMBOL(ath10k_dbg_save_fw_dbg_buffer);
static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
{
unsigned int len = (sizeof(ar->dbglog_entry_data)
+ + sizeof(ar->stack_buf)
+ sizeof(ar->reg_dump_values));
unsigned int sofar = 0;
char *buf;
@@ -610,7 +611,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
sofar += hdr_len;
/* So we can add headers to the data dump */
- len += 2 * sizeof(*dump_tlv);
+ len += 3 * sizeof(*dump_tlv);
/* This is going to get big when we start dumping FW RAM and such,
* so go ahead and use vmalloc.
@@ -637,6 +638,14 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+ /* Gather firmware stack dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_STACK;
+ dump_tlv->tlv_len = sizeof(ar->stack_buf);
+ memcpy(dump_tlv->tlv_data, &ar->stack_buf, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ WARN_ON(sofar != len);
return dump_data;
}
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 36da9a5..821a216 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -833,6 +833,38 @@ static u16 ath10k_pci_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
return ath10k_ce_num_free_src_entries(ar_pci->pipe_info[pipe].ce_hdl);
}
+static void ath10k_save_firmware_mem_helper(struct ath10k *ar, u32 sz,
+ unsigned char *to, u32 addr)
+{
+ u32 host_addr;
+ u32 reg_dump_area;
+ int ret;
+
+ host_addr = host_interest_item_address(addr);
+
+ ret = ath10k_pci_diag_read_mem(ar, host_addr, ®_dump_area,
+ sizeof(u32));
+ if (ret != 0) {
+ ath10k_warn("failed to read FW addr: %d (addr %d)\n",
+ ret, addr);
+ return;
+ }
+
+ ret = ath10k_pci_diag_read_mem(ar, reg_dump_area, to, sz);
+ if (ret != 0)
+ ath10k_err("failed to read FW memory: %d (addr %d sz %d)\n",
+ ret, reg_dump_area, sz);
+}
+
+/* Save the main firmware stack */
+static void ath10k_save_firmware_stack(struct ath10k *ar)
+{
+ BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 8);
+ ath10k_save_firmware_mem_helper(ar, ATH10K_FW_STACK_SIZE,
+ ar->stack_buf, HI_ITEM(hi_stack));
+}
+
+
static void ath10k_pci_hif_dump_area(struct ath10k *ar)
{
u32 reg_dump_area = 0;
@@ -877,6 +909,9 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
+ if (!ar->crashed_since_read)
+ ath10k_save_firmware_stack(ar);
+
/* Dump the debug logs on the target */
host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
if (ath10k_pci_diag_read_mem(ar, host_addr,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 3/4] ath10k: save firmware stack upon firmware crash.
@ 2014-06-03 16:25 ` greearb
0 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: Ben Greear, linux-wireless
From: Ben Greear <greearb@candelatech.com>
Should help debug firmware crashes, and give users a way
to provide some useful debug reports to firmware developers.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/core.h | 3 +++
drivers/net/wireless/ath/ath10k/debug.c | 11 ++++++++++-
drivers/net/wireless/ath/ath10k/pci.c | 35 +++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4068910..b331942 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -42,6 +42,7 @@
#define ATH10K_NUM_CHANS 38
#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
+#define ATH10K_FW_STACK_SIZE 4096
/* Antenna noise floor */
#define ATH10K_DEFAULT_NOISE_FLOOR -95
@@ -348,6 +349,7 @@ enum ath10k_dev_flags {
enum ath10k_fw_error_dump_type {
ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
+ ATH10K_FW_ERROR_DUMP_STACK = 2,
ATH10K_FW_ERROR_DUMP_MAX,
};
@@ -528,6 +530,7 @@ struct ath10k {
bool crashed_since_read;
struct ath10k_dbglog_entry_storage dbglog_entry_data;
u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+ unsigned char stack_buf[ATH10K_FW_STACK_SIZE];
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 1f18412..9c9456d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -599,6 +599,7 @@ EXPORT_SYMBOL(ath10k_dbg_save_fw_dbg_buffer);
static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
{
unsigned int len = (sizeof(ar->dbglog_entry_data)
+ + sizeof(ar->stack_buf)
+ sizeof(ar->reg_dump_values));
unsigned int sofar = 0;
char *buf;
@@ -610,7 +611,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
sofar += hdr_len;
/* So we can add headers to the data dump */
- len += 2 * sizeof(*dump_tlv);
+ len += 3 * sizeof(*dump_tlv);
/* This is going to get big when we start dumping FW RAM and such,
* so go ahead and use vmalloc.
@@ -637,6 +638,14 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+ /* Gather firmware stack dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_STACK;
+ dump_tlv->tlv_len = sizeof(ar->stack_buf);
+ memcpy(dump_tlv->tlv_data, &ar->stack_buf, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+ WARN_ON(sofar != len);
return dump_data;
}
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 36da9a5..821a216 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -833,6 +833,38 @@ static u16 ath10k_pci_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
return ath10k_ce_num_free_src_entries(ar_pci->pipe_info[pipe].ce_hdl);
}
+static void ath10k_save_firmware_mem_helper(struct ath10k *ar, u32 sz,
+ unsigned char *to, u32 addr)
+{
+ u32 host_addr;
+ u32 reg_dump_area;
+ int ret;
+
+ host_addr = host_interest_item_address(addr);
+
+ ret = ath10k_pci_diag_read_mem(ar, host_addr, ®_dump_area,
+ sizeof(u32));
+ if (ret != 0) {
+ ath10k_warn("failed to read FW addr: %d (addr %d)\n",
+ ret, addr);
+ return;
+ }
+
+ ret = ath10k_pci_diag_read_mem(ar, reg_dump_area, to, sz);
+ if (ret != 0)
+ ath10k_err("failed to read FW memory: %d (addr %d sz %d)\n",
+ ret, reg_dump_area, sz);
+}
+
+/* Save the main firmware stack */
+static void ath10k_save_firmware_stack(struct ath10k *ar)
+{
+ BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 8);
+ ath10k_save_firmware_mem_helper(ar, ATH10K_FW_STACK_SIZE,
+ ar->stack_buf, HI_ITEM(hi_stack));
+}
+
+
static void ath10k_pci_hif_dump_area(struct ath10k *ar)
{
u32 reg_dump_area = 0;
@@ -877,6 +909,9 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
+ if (!ar->crashed_since_read)
+ ath10k_save_firmware_stack(ar);
+
/* Dump the debug logs on the target */
host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
if (ath10k_pci_diag_read_mem(ar, host_addr,
--
1.7.11.7
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 4/4] ath10k: Dump exception stack contents on firmware crash.
2014-06-03 16:25 ` greearb
@ 2014-06-03 16:25 ` greearb
-1 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Ben Greear
From: Ben Greear <greearb@candelatech.com>
Firmware developers can decode this and maybe figure out
why the firmware crashed.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/core.h | 2 ++
drivers/net/wireless/ath/ath10k/debug.c | 10 +++++++++-
drivers/net/wireless/ath/ath10k/pci.c | 11 ++++++++++-
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index b331942..f389bde 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -350,6 +350,7 @@ enum ath10k_fw_error_dump_type {
ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
ATH10K_FW_ERROR_DUMP_STACK = 2,
+ ATH10K_FW_ERROR_DUMP_EXC_STACK = 3,
ATH10K_FW_ERROR_DUMP_MAX,
};
@@ -531,6 +532,7 @@ struct ath10k {
struct ath10k_dbglog_entry_storage dbglog_entry_data;
u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
unsigned char stack_buf[ATH10K_FW_STACK_SIZE];
+ unsigned char exc_stack_buf[ATH10K_FW_STACK_SIZE];
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 9c9456d..0c12c9d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -600,6 +600,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
{
unsigned int len = (sizeof(ar->dbglog_entry_data)
+ sizeof(ar->stack_buf)
+ + sizeof(ar->exc_stack_buf)
+ sizeof(ar->reg_dump_values));
unsigned int sofar = 0;
char *buf;
@@ -611,7 +612,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
sofar += hdr_len;
/* So we can add headers to the data dump */
- len += 3 * sizeof(*dump_tlv);
+ len += 4 * sizeof(*dump_tlv);
/* This is going to get big when we start dumping FW RAM and such,
* so go ahead and use vmalloc.
@@ -645,6 +646,13 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
memcpy(dump_tlv->tlv_data, &ar->stack_buf, dump_tlv->tlv_len);
sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+ /* Gather firmware exception (irq) stack dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_EXC_STACK;
+ dump_tlv->tlv_len = sizeof(ar->exc_stack_buf);
+ memcpy(dump_tlv->tlv_data, &ar->exc_stack_buf, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
WARN_ON(sofar != len);
return dump_data;
}
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 821a216..bf258e1 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -864,6 +864,13 @@ static void ath10k_save_firmware_stack(struct ath10k *ar)
ar->stack_buf, HI_ITEM(hi_stack));
}
+/* Save the firmware exception stack */
+static void ath10k_save_firmware_exc_stack(struct ath10k *ar)
+{
+ ath10k_save_firmware_mem_helper(ar, ATH10K_FW_STACK_SIZE,
+ ar->exc_stack_buf,
+ HI_ITEM(hi_err_stack));
+}
static void ath10k_pci_hif_dump_area(struct ath10k *ar)
{
@@ -909,8 +916,10 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
- if (!ar->crashed_since_read)
+ if (!ar->crashed_since_read) {
ath10k_save_firmware_stack(ar);
+ ath10k_save_firmware_exc_stack(ar);
+ }
/* Dump the debug logs on the target */
host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
--
1.7.11.7
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [RFC 4/4] ath10k: Dump exception stack contents on firmware crash.
@ 2014-06-03 16:25 ` greearb
0 siblings, 0 replies; 26+ messages in thread
From: greearb @ 2014-06-03 16:25 UTC (permalink / raw)
To: ath10k; +Cc: Ben Greear, linux-wireless
From: Ben Greear <greearb@candelatech.com>
Firmware developers can decode this and maybe figure out
why the firmware crashed.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
drivers/net/wireless/ath/ath10k/core.h | 2 ++
drivers/net/wireless/ath/ath10k/debug.c | 10 +++++++++-
drivers/net/wireless/ath/ath10k/pci.c | 11 ++++++++++-
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index b331942..f389bde 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -350,6 +350,7 @@ enum ath10k_fw_error_dump_type {
ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
ATH10K_FW_ERROR_DUMP_STACK = 2,
+ ATH10K_FW_ERROR_DUMP_EXC_STACK = 3,
ATH10K_FW_ERROR_DUMP_MAX,
};
@@ -531,6 +532,7 @@ struct ath10k {
struct ath10k_dbglog_entry_storage dbglog_entry_data;
u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
unsigned char stack_buf[ATH10K_FW_STACK_SIZE];
+ unsigned char exc_stack_buf[ATH10K_FW_STACK_SIZE];
#ifdef CONFIG_ATH10K_DEBUGFS
struct ath10k_debug debug;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 9c9456d..0c12c9d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -600,6 +600,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
{
unsigned int len = (sizeof(ar->dbglog_entry_data)
+ sizeof(ar->stack_buf)
+ + sizeof(ar->exc_stack_buf)
+ sizeof(ar->reg_dump_values));
unsigned int sofar = 0;
char *buf;
@@ -611,7 +612,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
sofar += hdr_len;
/* So we can add headers to the data dump */
- len += 3 * sizeof(*dump_tlv);
+ len += 4 * sizeof(*dump_tlv);
/* This is going to get big when we start dumping FW RAM and such,
* so go ahead and use vmalloc.
@@ -645,6 +646,13 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
memcpy(dump_tlv->tlv_data, &ar->stack_buf, dump_tlv->tlv_len);
sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+ /* Gather firmware exception (irq) stack dump */
+ dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+ dump_tlv->type = ATH10K_FW_ERROR_DUMP_EXC_STACK;
+ dump_tlv->tlv_len = sizeof(ar->exc_stack_buf);
+ memcpy(dump_tlv->tlv_data, &ar->exc_stack_buf, dump_tlv->tlv_len);
+ sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
WARN_ON(sofar != len);
return dump_data;
}
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 821a216..bf258e1 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -864,6 +864,13 @@ static void ath10k_save_firmware_stack(struct ath10k *ar)
ar->stack_buf, HI_ITEM(hi_stack));
}
+/* Save the firmware exception stack */
+static void ath10k_save_firmware_exc_stack(struct ath10k *ar)
+{
+ ath10k_save_firmware_mem_helper(ar, ATH10K_FW_STACK_SIZE,
+ ar->exc_stack_buf,
+ HI_ITEM(hi_err_stack));
+}
static void ath10k_pci_hif_dump_area(struct ath10k *ar)
{
@@ -909,8 +916,10 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
reg_dump_values[i + 2],
reg_dump_values[i + 3]);
- if (!ar->crashed_since_read)
+ if (!ar->crashed_since_read) {
ath10k_save_firmware_stack(ar);
+ ath10k_save_firmware_exc_stack(ar);
+ }
/* Dump the debug logs on the target */
host_addr = host_interest_item_address(HI_ITEM(hi_dbglog_hdr));
--
1.7.11.7
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC 2/4] ath10k: save firmware debug log messages.
2014-06-03 16:25 ` greearb
@ 2014-06-03 16:45 ` Joe Perches
-1 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2014-06-03 16:45 UTC (permalink / raw)
To: greearb; +Cc: ath10k, linux-wireless
On Tue, 2014-06-03 at 09:25 -0700, greearb@candelatech.com wrote:
> They may be dumped through the firmware dump debugfs
> file.
[]
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
[]
> @@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
>
> static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
> {
> + u32 *m = (u32 *)(skb->data);
> ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
> skb->len);
>
> trace_ath10k_wmi_dbglog(skb->data, skb->len);
>
> + ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
> +
> return 0;
> }
>
That last line looks an overly complicated way of writing
ath10k_dbg_save_fw_dbg_buffer(ar, skb->data + 4, skb->len - 4);
btw: why skip the first 4 bytes?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 2/4] ath10k: save firmware debug log messages.
@ 2014-06-03 16:45 ` Joe Perches
0 siblings, 0 replies; 26+ messages in thread
From: Joe Perches @ 2014-06-03 16:45 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless, ath10k
On Tue, 2014-06-03 at 09:25 -0700, greearb@candelatech.com wrote:
> They may be dumped through the firmware dump debugfs
> file.
[]
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
[]
> @@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
>
> static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
> {
> + u32 *m = (u32 *)(skb->data);
> ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
> skb->len);
>
> trace_ath10k_wmi_dbglog(skb->data, skb->len);
>
> + ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
> +
> return 0;
> }
>
That last line looks an overly complicated way of writing
ath10k_dbg_save_fw_dbg_buffer(ar, skb->data + 4, skb->len - 4);
btw: why skip the first 4 bytes?
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 2/4] ath10k: save firmware debug log messages.
2014-06-03 16:45 ` Joe Perches
@ 2014-06-03 16:49 ` Ben Greear
-1 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-03 16:49 UTC (permalink / raw)
To: Joe Perches; +Cc: ath10k, linux-wireless
On 06/03/2014 09:45 AM, Joe Perches wrote:
> On Tue, 2014-06-03 at 09:25 -0700, greearb@candelatech.com wrote:
>> They may be dumped through the firmware dump debugfs
>> file.
> []
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> []
>> @@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
>>
>> static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
>> {
>> + u32 *m = (u32 *)(skb->data);
>> ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
>> skb->len);
>>
>> trace_ath10k_wmi_dbglog(skb->data, skb->len);
>>
>> + ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
>> +
>> return 0;
>> }
>>
>
> That last line looks an overly complicated way of writing
>
> ath10k_dbg_save_fw_dbg_buffer(ar, skb->data + 4, skb->len - 4);
>
> btw: why skip the first 4 bytes?
First 4 bytes is a counter meaning something like 'messages lost due to overflow',
and is not really part of the debug messages.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 2/4] ath10k: save firmware debug log messages.
@ 2014-06-03 16:49 ` Ben Greear
0 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-03 16:49 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-wireless, ath10k
On 06/03/2014 09:45 AM, Joe Perches wrote:
> On Tue, 2014-06-03 at 09:25 -0700, greearb@candelatech.com wrote:
>> They may be dumped through the firmware dump debugfs
>> file.
> []
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> []
>> @@ -1084,11 +1084,14 @@ static void ath10k_wmi_event_echo(struct ath10k *ar, struct sk_buff *skb)
>>
>> static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
>> {
>> + u32 *m = (u32 *)(skb->data);
>> ath10k_dbg(ATH10K_DBG_WMI, "wmi event debug mesg len %d\n",
>> skb->len);
>>
>> trace_ath10k_wmi_dbglog(skb->data, skb->len);
>>
>> + ath10k_dbg_save_fw_dbg_buffer(ar, (u8 *)(&(m[1])), skb->len - 4);
>> +
>> return 0;
>> }
>>
>
> That last line looks an overly complicated way of writing
>
> ath10k_dbg_save_fw_dbg_buffer(ar, skb->data + 4, skb->len - 4);
>
> btw: why skip the first 4 bytes?
First 4 bytes is a counter meaning something like 'messages lost due to overflow',
and is not really part of the debug messages.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-03 16:25 ` greearb
@ 2014-06-04 9:04 ` Michal Kazior
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Kazior @ 2014-06-04 9:04 UTC (permalink / raw)
To: Ben Greear; +Cc: ath10k, linux-wireless
On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Store the firmware crash registers and last 128 or so
> firmware debug-log ids and present them to user-space
> via debugfs.
>
> Should help with figuring out why the firmware crashed.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>
> This series is compile-tested only at this point. Hoping
> for feedback on general approach at least.
>
>
> drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
> drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
> drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
> 4 files changed, 273 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 68ceef6..4068910 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -41,6 +41,8 @@
> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
> #define ATH10K_NUM_CHANS 38
>
> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
I don't like this.
[...]
> +struct ath10k_tlv_dump_data {
> + u32 type; /* see ath10k_fw_error_dump_type above */
> + u32 tlv_len; /* in bytes */
> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
> +} __packed;
> +
> +struct ath10k_dump_file_data {
> + u32 len;
> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
> + struct ath10k_tlv_dump_data data; /* more may follow */
> +} __packed;
> +
> +/* This will store at least the last 128 entries. */
> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
The 128 should probably be a separate #define?
[...]
> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> +{
> + unsigned int len = (sizeof(ar->dbglog_entry_data)
> + + sizeof(ar->reg_dump_values));
> + unsigned int sofar = 0;
> + char *buf;
> + struct ath10k_tlv_dump_data *dump_tlv;
> + struct ath10k_dump_file_data *dump_data;
> + int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
You call this with conf_mutex held so it's a good idea to have a
lockdep here, isn't it?
[...]
> + /* Gather crash-dump */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = ATH10K_FW_ERROR_DUMP_REGDUMP;
> + dump_tlv->tlv_len = sizeof(ar->reg_dump_values);
> + memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
> + sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
> +
> + return dump_data;
> +}
> +
> +
> +
> +static int ath10k_fw_error_dump_open(struct inode *inode, struct file *file)
3 newlines?
> +{
> + struct ath10k *ar = inode->i_private;
> + int ret;
> + struct ath10k_dump_file_data *dump;
> +
> + if (!ar)
> + return -EINVAL;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + dump = ath10k_build_dump_file(ar);
> +
Maybe it's just me, but this newline bugs me :)
> + if (!dump) {
> + ret = -ENODATA;
> + goto out;
> + }
> +
> + file->private_data = dump;
> + ar->crashed_since_read = false;
> + ret = 0;
> +
> +out:
> + mutex_unlock(&ar->conf_mutex);
> + return ret;
> +}
> +
> +
> +static ssize_t ath10k_fw_error_dump_read(struct file *file,
2 newlines?
[...]
> static int ath10k_debug_htt_stats_req(struct ath10k *ar)
> {
> u64 cookie;
> @@ -861,6 +981,9 @@ int ath10k_debug_create(struct ath10k *ar)
> debugfs_create_file("simulate_fw_crash", S_IRUSR, ar->debug.debugfs_phy,
> ar, &fops_simulate_fw_crash);
>
> + debugfs_create_file("fw_error_dump", S_IRUSR, ar->debug.debugfs_phy,
> + ar, &fops_fw_error_dump);
> +
> debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
> ar, &fops_chip_id);
>
> @@ -931,4 +1054,5 @@ void ath10k_dbg_dump(enum ath10k_debug_mask mask,
> }
> EXPORT_SYMBOL(ath10k_dbg_dump);
>
> +
What is this newline doing here?
> #endif /* CONFIG_ATH10K_DEBUG */
> diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
> index a582499..d9629f9 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -19,6 +19,7 @@
> #define _DEBUG_H_
>
> #include <linux/types.h>
> +#include "pci.h"
> #include "trace.h"
Not really sure if we should be mixing pci in debug?
This actually gets me thinking we could have some stuff in hw.h that
isn't really related to the transport. I can imagine usb transport
would have pretty much the same hardware at the other side.
> enum ath10k_debug_mask {
> @@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
> {
> }
> #endif /* CONFIG_ATH10K_DEBUG */
> +
> +
> +/* Target debug log related defines and structs */
> +
> +/* Target is 32-bit CPU, so we just use u32 for
> + * the pointers. The memory space is relative to the
> + * target, not the host.
> + */
> +struct dbglog_buf_s {
> + u32 next; /* pointer to dblog_buf_s. */
> + u32 buffer; /* pointer to u8 buffer */
> + u32 bufsize;
> + u32 length;
> + u32 count;
> + u32 free;
> +} __packed;
> +
> +struct dbglog_hdr_s {
> + u32 dbuf; /* pointer to dbglog_buf_s */
> + u32 dropped;
> +} __packed;
These structure names look strange. Why the _s suffix? Shouldn't these
be called simply ath10k_dbglog_buf and ath10k_dbglog_hdr? Or
ath10k_{fw/hw/dev/target}_dbglog_buf/hdr? (In case you want to have a
clearer distinction these structures aren't ath10k's per se).
[...]
> + while (dbufp) {
> + struct dbglog_buf_s dbuf;
> +
> + ret = ath10k_pci_diag_read_mem(ar, dbufp,
> + &dbuf, sizeof(dbuf));
> + if (ret != 0) {
> + ath10k_err("could not read Debug Log Area: 0x%x\n",
> + dbufp);
> + goto save_regs_and_restart;
> + }
> +
> + /* We have a buffer of data */
> + /* TODO: Do we need to worry about bit order on some
> + * architectures? This seems to work fine with
> + * x86-64 host, at least.
> + */
ath10k_pci_diag* performs byte-swap so you're good as long as you read
word-like data from the target. I suspect this might not work so great
if you read something like, say, a mac address which might be stored
as a raw byte stream instead.
What kind of data can we expect from the dbglog?
> + ath10k_err("[%i] next: 0x%x buf: 0x%x sz: %i len: %i count: %i free: %i\n",
> + i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
> + dbuf.count, dbuf.free);
> + if (dbuf.buffer && dbuf.length) {
> + u8 *buffer = kmalloc(dbuf.length, GFP_ATOMIC);
> +
> + if (buffer) {
> + ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer,
> + buffer,
> + dbuf.length);
> + if (ret != 0) {
> + ath10k_err("could not read Debug Log buffer: 0x%x\n",
> + dbuf.buffer);
> + kfree(buffer);
> + goto save_regs_and_restart;
> + }
> +
> + ath10k_dbg_save_fw_dbg_buffer(ar, buffer,
> + dbuf.length);
> + kfree(buffer);
> + }
> + }
> + dbufp = dbuf.next;
> + if (dbufp == dbg_hdr.dbuf) {
> + /* It is a circular buffer it seems, bail if next
> + * is head
> + */
Hmm, we seem to be mixing comment styles in ath10k now I guess (this
applies to other instances of multi-line comments in your patch). What
multi-line comment style should we really be using in ath10k? Kalle?
Michał
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-04 9:04 ` Michal Kazior
0 siblings, 0 replies; 26+ messages in thread
From: Michal Kazior @ 2014-06-04 9:04 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless, ath10k
On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Store the firmware crash registers and last 128 or so
> firmware debug-log ids and present them to user-space
> via debugfs.
>
> Should help with figuring out why the firmware crashed.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>
> This series is compile-tested only at this point. Hoping
> for feedback on general approach at least.
>
>
> drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
> drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
> drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
> 4 files changed, 273 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 68ceef6..4068910 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -41,6 +41,8 @@
> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
> #define ATH10K_NUM_CHANS 38
>
> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
I don't like this.
[...]
> +struct ath10k_tlv_dump_data {
> + u32 type; /* see ath10k_fw_error_dump_type above */
> + u32 tlv_len; /* in bytes */
> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
> +} __packed;
> +
> +struct ath10k_dump_file_data {
> + u32 len;
> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
> + struct ath10k_tlv_dump_data data; /* more may follow */
> +} __packed;
> +
> +/* This will store at least the last 128 entries. */
> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
The 128 should probably be a separate #define?
[...]
> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> +{
> + unsigned int len = (sizeof(ar->dbglog_entry_data)
> + + sizeof(ar->reg_dump_values));
> + unsigned int sofar = 0;
> + char *buf;
> + struct ath10k_tlv_dump_data *dump_tlv;
> + struct ath10k_dump_file_data *dump_data;
> + int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
You call this with conf_mutex held so it's a good idea to have a
lockdep here, isn't it?
[...]
> + /* Gather crash-dump */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = ATH10K_FW_ERROR_DUMP_REGDUMP;
> + dump_tlv->tlv_len = sizeof(ar->reg_dump_values);
> + memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
> + sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
> +
> + return dump_data;
> +}
> +
> +
> +
> +static int ath10k_fw_error_dump_open(struct inode *inode, struct file *file)
3 newlines?
> +{
> + struct ath10k *ar = inode->i_private;
> + int ret;
> + struct ath10k_dump_file_data *dump;
> +
> + if (!ar)
> + return -EINVAL;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + dump = ath10k_build_dump_file(ar);
> +
Maybe it's just me, but this newline bugs me :)
> + if (!dump) {
> + ret = -ENODATA;
> + goto out;
> + }
> +
> + file->private_data = dump;
> + ar->crashed_since_read = false;
> + ret = 0;
> +
> +out:
> + mutex_unlock(&ar->conf_mutex);
> + return ret;
> +}
> +
> +
> +static ssize_t ath10k_fw_error_dump_read(struct file *file,
2 newlines?
[...]
> static int ath10k_debug_htt_stats_req(struct ath10k *ar)
> {
> u64 cookie;
> @@ -861,6 +981,9 @@ int ath10k_debug_create(struct ath10k *ar)
> debugfs_create_file("simulate_fw_crash", S_IRUSR, ar->debug.debugfs_phy,
> ar, &fops_simulate_fw_crash);
>
> + debugfs_create_file("fw_error_dump", S_IRUSR, ar->debug.debugfs_phy,
> + ar, &fops_fw_error_dump);
> +
> debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
> ar, &fops_chip_id);
>
> @@ -931,4 +1054,5 @@ void ath10k_dbg_dump(enum ath10k_debug_mask mask,
> }
> EXPORT_SYMBOL(ath10k_dbg_dump);
>
> +
What is this newline doing here?
> #endif /* CONFIG_ATH10K_DEBUG */
> diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
> index a582499..d9629f9 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -19,6 +19,7 @@
> #define _DEBUG_H_
>
> #include <linux/types.h>
> +#include "pci.h"
> #include "trace.h"
Not really sure if we should be mixing pci in debug?
This actually gets me thinking we could have some stuff in hw.h that
isn't really related to the transport. I can imagine usb transport
would have pretty much the same hardware at the other side.
> enum ath10k_debug_mask {
> @@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
> {
> }
> #endif /* CONFIG_ATH10K_DEBUG */
> +
> +
> +/* Target debug log related defines and structs */
> +
> +/* Target is 32-bit CPU, so we just use u32 for
> + * the pointers. The memory space is relative to the
> + * target, not the host.
> + */
> +struct dbglog_buf_s {
> + u32 next; /* pointer to dblog_buf_s. */
> + u32 buffer; /* pointer to u8 buffer */
> + u32 bufsize;
> + u32 length;
> + u32 count;
> + u32 free;
> +} __packed;
> +
> +struct dbglog_hdr_s {
> + u32 dbuf; /* pointer to dbglog_buf_s */
> + u32 dropped;
> +} __packed;
These structure names look strange. Why the _s suffix? Shouldn't these
be called simply ath10k_dbglog_buf and ath10k_dbglog_hdr? Or
ath10k_{fw/hw/dev/target}_dbglog_buf/hdr? (In case you want to have a
clearer distinction these structures aren't ath10k's per se).
[...]
> + while (dbufp) {
> + struct dbglog_buf_s dbuf;
> +
> + ret = ath10k_pci_diag_read_mem(ar, dbufp,
> + &dbuf, sizeof(dbuf));
> + if (ret != 0) {
> + ath10k_err("could not read Debug Log Area: 0x%x\n",
> + dbufp);
> + goto save_regs_and_restart;
> + }
> +
> + /* We have a buffer of data */
> + /* TODO: Do we need to worry about bit order on some
> + * architectures? This seems to work fine with
> + * x86-64 host, at least.
> + */
ath10k_pci_diag* performs byte-swap so you're good as long as you read
word-like data from the target. I suspect this might not work so great
if you read something like, say, a mac address which might be stored
as a raw byte stream instead.
What kind of data can we expect from the dbglog?
> + ath10k_err("[%i] next: 0x%x buf: 0x%x sz: %i len: %i count: %i free: %i\n",
> + i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
> + dbuf.count, dbuf.free);
> + if (dbuf.buffer && dbuf.length) {
> + u8 *buffer = kmalloc(dbuf.length, GFP_ATOMIC);
> +
> + if (buffer) {
> + ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer,
> + buffer,
> + dbuf.length);
> + if (ret != 0) {
> + ath10k_err("could not read Debug Log buffer: 0x%x\n",
> + dbuf.buffer);
> + kfree(buffer);
> + goto save_regs_and_restart;
> + }
> +
> + ath10k_dbg_save_fw_dbg_buffer(ar, buffer,
> + dbuf.length);
> + kfree(buffer);
> + }
> + }
> + dbufp = dbuf.next;
> + if (dbufp == dbg_hdr.dbuf) {
> + /* It is a circular buffer it seems, bail if next
> + * is head
> + */
Hmm, we seem to be mixing comment styles in ath10k now I guess (this
applies to other instances of multi-line comments in your patch). What
multi-line comment style should we really be using in ath10k? Kalle?
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-04 9:04 ` Michal Kazior
@ 2014-06-04 16:48 ` Ben Greear
-1 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-04 16:48 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k, linux-wireless
On 06/04/2014 02:04 AM, Michal Kazior wrote:
> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Store the firmware crash registers and last 128 or so
>> firmware debug-log ids and present them to user-space
>> via debugfs.
>>
>> Should help with figuring out why the firmware crashed.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>
>> This series is compile-tested only at this point. Hoping
>> for feedback on general approach at least.
>>
>>
>> drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
>> drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
>> drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
>> drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
>> 4 files changed, 273 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index 68ceef6..4068910 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -41,6 +41,8 @@
>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>> #define ATH10K_NUM_CHANS 38
>>
>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>
> I don't like this.
You want me to make a different define for this that duplicates
the '60' value? At least with my method above, we should get compile
errors if the values ever diverge in the two files.
>> +struct ath10k_tlv_dump_data {
>> + u32 type; /* see ath10k_fw_error_dump_type above */
>> + u32 tlv_len; /* in bytes */
>> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
>> +} __packed;
>> +
>> +struct ath10k_dump_file_data {
>> + u32 len;
>> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
>> + struct ath10k_tlv_dump_data data; /* more may follow */
>> +} __packed;
>> +
>> +/* This will store at least the last 128 entries. */
>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>
> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
It is from the guts of how the firmware does debug logs.
Each entry is a max of 7 32-bit integers in length.
> The 128 should probably be a separate #define?
I don't see why...dbglog messages are variable number of 32-bit
integers in length, so the 128 is fairly arbitrary.
>> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
>> +{
>> + unsigned int len = (sizeof(ar->dbglog_entry_data)
>> + + sizeof(ar->reg_dump_values));
>> + unsigned int sofar = 0;
>> + char *buf;
>> + struct ath10k_tlv_dump_data *dump_tlv;
>> + struct ath10k_dump_file_data *dump_data;
>> + int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
>
> You call this with conf_mutex held so it's a good idea to have a
> lockdep here, isn't it?
ok.
>> --- a/drivers/net/wireless/ath/ath10k/debug.h
>> +++ b/drivers/net/wireless/ath/ath10k/debug.h
>> @@ -19,6 +19,7 @@
>> #define _DEBUG_H_
>>
>> #include <linux/types.h>
>> +#include "pci.h"
>> #include "trace.h"
>
> Not really sure if we should be mixing pci in debug?
>
> This actually gets me thinking we could have some stuff in hw.h that
> isn't really related to the transport. I can imagine usb transport
> would have pretty much the same hardware at the other side.
I don't see much benefit to the separate modules for ath10k anyway,
seems like it's currently a lot of overhead for no advantage. Maybe
with a usb NIC is released it will make more sense.
>> enum ath10k_debug_mask {
>> @@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
>> {
>> }
>> #endif /* CONFIG_ATH10K_DEBUG */
>> +
>> +
>> +/* Target debug log related defines and structs */
>> +
>> +/* Target is 32-bit CPU, so we just use u32 for
>> + * the pointers. The memory space is relative to the
>> + * target, not the host.
>> + */
>> +struct dbglog_buf_s {
>> + u32 next; /* pointer to dblog_buf_s. */
>> + u32 buffer; /* pointer to u8 buffer */
>> + u32 bufsize;
>> + u32 length;
>> + u32 count;
>> + u32 free;
>> +} __packed;
>> +
>> +struct dbglog_hdr_s {
>> + u32 dbuf; /* pointer to dbglog_buf_s */
>> + u32 dropped;
>> +} __packed;
>
> These structure names look strange. Why the _s suffix? Shouldn't these
> be called simply ath10k_dbglog_buf and ath10k_dbglog_hdr? Or
> ath10k_{fw/hw/dev/target}_dbglog_buf/hdr? (In case you want to have a
> clearer distinction these structures aren't ath10k's per se).
I can rename them, don't recall why I named them thus (similar patch has
been in my tree since last fall.)
> [...]
>> + while (dbufp) {
>> + struct dbglog_buf_s dbuf;
>> +
>> + ret = ath10k_pci_diag_read_mem(ar, dbufp,
>> + &dbuf, sizeof(dbuf));
>> + if (ret != 0) {
>> + ath10k_err("could not read Debug Log Area: 0x%x\n",
>> + dbufp);
>> + goto save_regs_and_restart;
>> + }
>> +
>> + /* We have a buffer of data */
>> + /* TODO: Do we need to worry about bit order on some
>> + * architectures? This seems to work fine with
>> + * x86-64 host, at least.
>> + */
>
> ath10k_pci_diag* performs byte-swap so you're good as long as you read
> word-like data from the target. I suspect this might not work so great
> if you read something like, say, a mac address which might be stored
> as a raw byte stream instead.
>
> What kind of data can we expect from the dbglog?
Any and all, it is up to user-space to decode because it requires
intimate knowledge of the firmware. I think you might have seen
the tool I gave to QCA that decodes dbglog messages printed in ascii-hex
in kernel logs? Same tool with tweak can decode this binary info.
Plz ask me off list if you want the tool and do not have it...I can
try to get it forwarded to you trough my QCA contacts. The debug tool
cannot be made public w/out QCA's explicit consent, and no idea if
that would ever happen...but at least firmware devs under NDA can use it
or something similar...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-04 16:48 ` Ben Greear
0 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-04 16:48 UTC (permalink / raw)
To: Michal Kazior; +Cc: linux-wireless, ath10k
On 06/04/2014 02:04 AM, Michal Kazior wrote:
> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Store the firmware crash registers and last 128 or so
>> firmware debug-log ids and present them to user-space
>> via debugfs.
>>
>> Should help with figuring out why the firmware crashed.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>
>> This series is compile-tested only at this point. Hoping
>> for feedback on general approach at least.
>>
>>
>> drivers/net/wireless/ath/ath10k/core.h | 41 +++++++++++
>> drivers/net/wireless/ath/ath10k/debug.c | 124 ++++++++++++++++++++++++++++++++
>> drivers/net/wireless/ath/ath10k/debug.h | 25 +++++++
>> drivers/net/wireless/ath/ath10k/pci.c | 86 +++++++++++++++++++++-
>> 4 files changed, 273 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index 68ceef6..4068910 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -41,6 +41,8 @@
>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>> #define ATH10K_NUM_CHANS 38
>>
>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>
> I don't like this.
You want me to make a different define for this that duplicates
the '60' value? At least with my method above, we should get compile
errors if the values ever diverge in the two files.
>> +struct ath10k_tlv_dump_data {
>> + u32 type; /* see ath10k_fw_error_dump_type above */
>> + u32 tlv_len; /* in bytes */
>> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
>> +} __packed;
>> +
>> +struct ath10k_dump_file_data {
>> + u32 len;
>> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
>> + struct ath10k_tlv_dump_data data; /* more may follow */
>> +} __packed;
>> +
>> +/* This will store at least the last 128 entries. */
>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>
> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
It is from the guts of how the firmware does debug logs.
Each entry is a max of 7 32-bit integers in length.
> The 128 should probably be a separate #define?
I don't see why...dbglog messages are variable number of 32-bit
integers in length, so the 128 is fairly arbitrary.
>> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
>> +{
>> + unsigned int len = (sizeof(ar->dbglog_entry_data)
>> + + sizeof(ar->reg_dump_values));
>> + unsigned int sofar = 0;
>> + char *buf;
>> + struct ath10k_tlv_dump_data *dump_tlv;
>> + struct ath10k_dump_file_data *dump_data;
>> + int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
>
> You call this with conf_mutex held so it's a good idea to have a
> lockdep here, isn't it?
ok.
>> --- a/drivers/net/wireless/ath/ath10k/debug.h
>> +++ b/drivers/net/wireless/ath/ath10k/debug.h
>> @@ -19,6 +19,7 @@
>> #define _DEBUG_H_
>>
>> #include <linux/types.h>
>> +#include "pci.h"
>> #include "trace.h"
>
> Not really sure if we should be mixing pci in debug?
>
> This actually gets me thinking we could have some stuff in hw.h that
> isn't really related to the transport. I can imagine usb transport
> would have pretty much the same hardware at the other side.
I don't see much benefit to the separate modules for ath10k anyway,
seems like it's currently a lot of overhead for no advantage. Maybe
with a usb NIC is released it will make more sense.
>> enum ath10k_debug_mask {
>> @@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
>> {
>> }
>> #endif /* CONFIG_ATH10K_DEBUG */
>> +
>> +
>> +/* Target debug log related defines and structs */
>> +
>> +/* Target is 32-bit CPU, so we just use u32 for
>> + * the pointers. The memory space is relative to the
>> + * target, not the host.
>> + */
>> +struct dbglog_buf_s {
>> + u32 next; /* pointer to dblog_buf_s. */
>> + u32 buffer; /* pointer to u8 buffer */
>> + u32 bufsize;
>> + u32 length;
>> + u32 count;
>> + u32 free;
>> +} __packed;
>> +
>> +struct dbglog_hdr_s {
>> + u32 dbuf; /* pointer to dbglog_buf_s */
>> + u32 dropped;
>> +} __packed;
>
> These structure names look strange. Why the _s suffix? Shouldn't these
> be called simply ath10k_dbglog_buf and ath10k_dbglog_hdr? Or
> ath10k_{fw/hw/dev/target}_dbglog_buf/hdr? (In case you want to have a
> clearer distinction these structures aren't ath10k's per se).
I can rename them, don't recall why I named them thus (similar patch has
been in my tree since last fall.)
> [...]
>> + while (dbufp) {
>> + struct dbglog_buf_s dbuf;
>> +
>> + ret = ath10k_pci_diag_read_mem(ar, dbufp,
>> + &dbuf, sizeof(dbuf));
>> + if (ret != 0) {
>> + ath10k_err("could not read Debug Log Area: 0x%x\n",
>> + dbufp);
>> + goto save_regs_and_restart;
>> + }
>> +
>> + /* We have a buffer of data */
>> + /* TODO: Do we need to worry about bit order on some
>> + * architectures? This seems to work fine with
>> + * x86-64 host, at least.
>> + */
>
> ath10k_pci_diag* performs byte-swap so you're good as long as you read
> word-like data from the target. I suspect this might not work so great
> if you read something like, say, a mac address which might be stored
> as a raw byte stream instead.
>
> What kind of data can we expect from the dbglog?
Any and all, it is up to user-space to decode because it requires
intimate knowledge of the firmware. I think you might have seen
the tool I gave to QCA that decodes dbglog messages printed in ascii-hex
in kernel logs? Same tool with tweak can decode this binary info.
Plz ask me off list if you want the tool and do not have it...I can
try to get it forwarded to you trough my QCA contacts. The debug tool
cannot be made public w/out QCA's explicit consent, and no idea if
that would ever happen...but at least firmware devs under NDA can use it
or something similar...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-04 9:04 ` Michal Kazior
@ 2014-06-04 17:11 ` Ben Greear
-1 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-04 17:11 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k, linux-wireless
On 06/04/2014 02:04 AM, Michal Kazior wrote:
>> + if (dbufp == dbg_hdr.dbuf) {
>> + /* It is a circular buffer it seems, bail if next
>> + * is head
>> + */
>
> Hmm, we seem to be mixing comment styles in ath10k now I guess (this
> applies to other instances of multi-line comments in your patch). What
> multi-line comment style should we really be using in ath10k? Kalle?
Check-patch bitches if you do it differently than what is above, so I say
we keep it as I have it. Not worth cleaning up other comments to match
unless someone actually cares enough to do it...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-04 17:11 ` Ben Greear
0 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-04 17:11 UTC (permalink / raw)
To: Michal Kazior; +Cc: linux-wireless, ath10k
On 06/04/2014 02:04 AM, Michal Kazior wrote:
>> + if (dbufp == dbg_hdr.dbuf) {
>> + /* It is a circular buffer it seems, bail if next
>> + * is head
>> + */
>
> Hmm, we seem to be mixing comment styles in ath10k now I guess (this
> applies to other instances of multi-line comments in your patch). What
> multi-line comment style should we really be using in ath10k? Kalle?
Check-patch bitches if you do it differently than what is above, so I say
we keep it as I have it. Not worth cleaning up other comments to match
unless someone actually cares enough to do it...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-04 16:48 ` Ben Greear
@ 2014-06-05 11:29 ` Kalle Valo
-1 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-05 11:29 UTC (permalink / raw)
To: Ben Greear; +Cc: Michal Kazior, linux-wireless, ath10k
Ben Greear <greearb@candelatech.com> writes:
> On 06/04/2014 02:04 AM, Michal Kazior wrote:
>> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>>
>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>> @@ -41,6 +41,8 @@
>>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>>> #define ATH10K_NUM_CHANS 38
>>>
>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>>
>> I don't like this.
Me neither.
> You want me to make a different define for this that duplicates
> the '60' value? At least with my method above, we should get compile
> errors if the values ever diverge in the two files.
There should be only one define. Somewhere (forgot where) Michal
suggested hw.h, I think this define would be a good candidate to move
there too.
>>> +struct ath10k_tlv_dump_data {
>>> + u32 type; /* see ath10k_fw_error_dump_type above */
>>> + u32 tlv_len; /* in bytes */
>>> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
>>> +} __packed;
>>> +
>>> +struct ath10k_dump_file_data {
>>> + u32 len;
>>> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
>>> + struct ath10k_tlv_dump_data data; /* more may follow */
>>> +} __packed;
>>> +
>>> +/* This will store at least the last 128 entries. */
>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>
>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>
> It is from the guts of how the firmware does debug logs.
>
> Each entry is a max of 7 32-bit integers in length.
>
>> The 128 should probably be a separate #define?
>
> I don't see why...
To make the code more readable.
> dbglog messages are variable number of 32-bit integers in length, so
> the 128 is fairly arbitrary.
A person should immeaditely understand where the numbers are coming from
and not start googling about it. The minimum is to put your description
above to the comment. And it would be clearer to replace 4 with
sizeof(u32).
--
Kalle Valo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-05 11:29 ` Kalle Valo
0 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-05 11:29 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless, Michal Kazior, ath10k
Ben Greear <greearb@candelatech.com> writes:
> On 06/04/2014 02:04 AM, Michal Kazior wrote:
>> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>>
>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>> @@ -41,6 +41,8 @@
>>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>>> #define ATH10K_NUM_CHANS 38
>>>
>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>>
>> I don't like this.
Me neither.
> You want me to make a different define for this that duplicates
> the '60' value? At least with my method above, we should get compile
> errors if the values ever diverge in the two files.
There should be only one define. Somewhere (forgot where) Michal
suggested hw.h, I think this define would be a good candidate to move
there too.
>>> +struct ath10k_tlv_dump_data {
>>> + u32 type; /* see ath10k_fw_error_dump_type above */
>>> + u32 tlv_len; /* in bytes */
>>> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
>>> +} __packed;
>>> +
>>> +struct ath10k_dump_file_data {
>>> + u32 len;
>>> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
>>> + struct ath10k_tlv_dump_data data; /* more may follow */
>>> +} __packed;
>>> +
>>> +/* This will store at least the last 128 entries. */
>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>
>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>
> It is from the guts of how the firmware does debug logs.
>
> Each entry is a max of 7 32-bit integers in length.
>
>> The 128 should probably be a separate #define?
>
> I don't see why...
To make the code more readable.
> dbglog messages are variable number of 32-bit integers in length, so
> the 128 is fairly arbitrary.
A person should immeaditely understand where the numbers are coming from
and not start googling about it. The minimum is to put your description
above to the comment. And it would be clearer to replace 4 with
sizeof(u32).
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-04 9:04 ` Michal Kazior
@ 2014-06-05 11:33 ` Kalle Valo
-1 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-05 11:33 UTC (permalink / raw)
To: Michal Kazior; +Cc: Ben Greear, linux-wireless, ath10k
Michal Kazior <michal.kazior@tieto.com> writes:
>> + dbufp = dbuf.next;
>> + if (dbufp == dbg_hdr.dbuf) {
>> + /* It is a circular buffer it seems, bail if next
>> + * is head
>> + */
>
> Hmm, we seem to be mixing comment styles in ath10k now I guess (this
> applies to other instances of multi-line comments in your patch). What
> multi-line comment style should we really be using in ath10k? Kalle?
Yeah, checkpatch is really annoying here and started to use that
different style of multiline comments. I just disabled that check on my
own setup. Personally I don't really care, but I guess we should switch
to use this "new" style.
--
Kalle Valo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-05 11:33 ` Kalle Valo
0 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-05 11:33 UTC (permalink / raw)
To: Michal Kazior; +Cc: Ben Greear, linux-wireless, ath10k
Michal Kazior <michal.kazior@tieto.com> writes:
>> + dbufp = dbuf.next;
>> + if (dbufp == dbg_hdr.dbuf) {
>> + /* It is a circular buffer it seems, bail if next
>> + * is head
>> + */
>
> Hmm, we seem to be mixing comment styles in ath10k now I guess (this
> applies to other instances of multi-line comments in your patch). What
> multi-line comment style should we really be using in ath10k? Kalle?
Yeah, checkpatch is really annoying here and started to use that
different style of multiline comments. I just disabled that check on my
own setup. Personally I don't really care, but I guess we should switch
to use this "new" style.
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-05 11:29 ` Kalle Valo
@ 2014-06-05 15:49 ` Ben Greear
-1 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-05 15:49 UTC (permalink / raw)
To: Kalle Valo; +Cc: Michal Kazior, linux-wireless, ath10k
On 06/05/2014 04:29 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 06/04/2014 02:04 AM, Michal Kazior wrote:
>>> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>>>
>>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>>> @@ -41,6 +41,8 @@
>>>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>>>> #define ATH10K_NUM_CHANS 38
>>>>
>>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>>>
>>> I don't like this.
>
> Me neither.
>
>> You want me to make a different define for this that duplicates
>> the '60' value? At least with my method above, we should get compile
>> errors if the values ever diverge in the two files.
>
> There should be only one define. Somewhere (forgot where) Michal
> suggested hw.h, I think this define would be a good candidate to move
> there too.
Ok, I'll move it to hw.h
>>>> +/* This will store at least the last 128 entries. */
>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>
>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>
>> It is from the guts of how the firmware does debug logs.
>>
>> Each entry is a max of 7 32-bit integers in length.
>>
>>> The 128 should probably be a separate #define?
>>
>> I don't see why...
>
> To make the code more readable.
>
>> dbglog messages are variable number of 32-bit integers in length, so
>> the 128 is fairly arbitrary.
>
> A person should immeaditely understand where the numbers are coming from
> and not start googling about it. The minimum is to put your description
> above to the comment. And it would be clearer to replace 4 with
> sizeof(u32).
Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
be sufficient, or do you want actual defines? To really understand
this code you need details from how the firmware encodes the messages.
I would rather a QCA person add that info just in case it is considered
protected info...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-05 15:49 ` Ben Greear
0 siblings, 0 replies; 26+ messages in thread
From: Ben Greear @ 2014-06-05 15:49 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Michal Kazior, ath10k
On 06/05/2014 04:29 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 06/04/2014 02:04 AM, Michal Kazior wrote:
>>> On 3 June 2014 18:25, <greearb@candelatech.com> wrote:
>>>
>>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>>> @@ -41,6 +41,8 @@
>>>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ)
>>>> #define ATH10K_NUM_CHANS 38
>>>>
>>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */
>>>
>>> I don't like this.
>
> Me neither.
>
>> You want me to make a different define for this that duplicates
>> the '60' value? At least with my method above, we should get compile
>> errors if the values ever diverge in the two files.
>
> There should be only one define. Somewhere (forgot where) Michal
> suggested hw.h, I think this define would be a good candidate to move
> there too.
Ok, I'll move it to hw.h
>>>> +/* This will store at least the last 128 entries. */
>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>
>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>
>> It is from the guts of how the firmware does debug logs.
>>
>> Each entry is a max of 7 32-bit integers in length.
>>
>>> The 128 should probably be a separate #define?
>>
>> I don't see why...
>
> To make the code more readable.
>
>> dbglog messages are variable number of 32-bit integers in length, so
>> the 128 is fairly arbitrary.
>
> A person should immeaditely understand where the numbers are coming from
> and not start googling about it. The minimum is to put your description
> above to the comment. And it would be clearer to replace 4 with
> sizeof(u32).
Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
be sufficient, or do you want actual defines? To really understand
this code you need details from how the firmware encodes the messages.
I would rather a QCA person add that info just in case it is considered
protected info...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
2014-06-05 15:49 ` Ben Greear
@ 2014-06-06 6:13 ` Kalle Valo
-1 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-06 6:13 UTC (permalink / raw)
To: Ben Greear; +Cc: Michal Kazior, linux-wireless, ath10k
Ben Greear <greearb@candelatech.com> writes:
>>>>> +/* This will store at least the last 128 entries. */
>>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>>
>>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>>
>>> It is from the guts of how the firmware does debug logs.
>>>
>>> Each entry is a max of 7 32-bit integers in length.
>>>
>>>> The 128 should probably be a separate #define?
>>>
>>> I don't see why...
>>
>> To make the code more readable.
>>
>>> dbglog messages are variable number of 32-bit integers in length, so
>>> the 128 is fairly arbitrary.
>>
>> A person should immeaditely understand where the numbers are coming from
>> and not start googling about it. The minimum is to put your description
>> above to the comment. And it would be clearer to replace 4 with
>> sizeof(u32).
>
> Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
> be sufficient, or do you want actual defines?
What you wrote about should be enough, I think you can skip the defines
for now. Something like:
/* Each entry is a max of 7 32-bit integers in length, let's choose
* arbitrarily 128 entries */
--
Kalle Valo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.
@ 2014-06-06 6:13 ` Kalle Valo
0 siblings, 0 replies; 26+ messages in thread
From: Kalle Valo @ 2014-06-06 6:13 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless, Michal Kazior, ath10k
Ben Greear <greearb@candelatech.com> writes:
>>>>> +/* This will store at least the last 128 entries. */
>>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>>
>>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>>
>>> It is from the guts of how the firmware does debug logs.
>>>
>>> Each entry is a max of 7 32-bit integers in length.
>>>
>>>> The 128 should probably be a separate #define?
>>>
>>> I don't see why...
>>
>> To make the code more readable.
>>
>>> dbglog messages are variable number of 32-bit integers in length, so
>>> the 128 is fairly arbitrary.
>>
>> A person should immeaditely understand where the numbers are coming from
>> and not start googling about it. The minimum is to put your description
>> above to the comment. And it would be clearer to replace 4 with
>> sizeof(u32).
>
> Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
> be sufficient, or do you want actual defines?
What you wrote about should be enough, I think you can skip the defines
for now. Something like:
/* Each entry is a max of 7 32-bit integers in length, let's choose
* arbitrarily 128 entries */
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2014-06-06 6:19 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-03 16:25 [RFC 1/4] ath10k: provide firmware crash info via debugfs greearb
2014-06-03 16:25 ` greearb
2014-06-03 16:25 ` [RFC 2/4] ath10k: save firmware debug log messages greearb
2014-06-03 16:25 ` greearb
2014-06-03 16:45 ` Joe Perches
2014-06-03 16:45 ` Joe Perches
2014-06-03 16:49 ` Ben Greear
2014-06-03 16:49 ` Ben Greear
2014-06-03 16:25 ` [RFC 3/4] ath10k: save firmware stack upon firmware crash greearb
2014-06-03 16:25 ` greearb
2014-06-03 16:25 ` [RFC 4/4] ath10k: Dump exception stack contents on " greearb
2014-06-03 16:25 ` greearb
2014-06-04 9:04 ` [RFC 1/4] ath10k: provide firmware crash info via debugfs Michal Kazior
2014-06-04 9:04 ` Michal Kazior
2014-06-04 16:48 ` Ben Greear
2014-06-04 16:48 ` Ben Greear
2014-06-05 11:29 ` Kalle Valo
2014-06-05 11:29 ` Kalle Valo
2014-06-05 15:49 ` Ben Greear
2014-06-05 15:49 ` Ben Greear
2014-06-06 6:13 ` Kalle Valo
2014-06-06 6:13 ` Kalle Valo
2014-06-04 17:11 ` Ben Greear
2014-06-04 17:11 ` Ben Greear
2014-06-05 11:33 ` Kalle Valo
2014-06-05 11:33 ` Kalle Valo
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.