All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/8] ath10k: firmware crash dump
@ 2014-08-09 18:07 ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:07 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Next version of ath10k firmware crash dump feature, based on Ben's patches but
heavily modified.

v6:

* fix vzalloc(sizeof(ar->debug.fw_crash_data)), fixes the crash I saw (Ben)

* "Target Register Dump" -> "firmware register dump"

* add ath10k_print_driver_info()

* take timestamp at crash time instead of crash-dump-gather time (Ben)

* fix locking comment in struct ath10k::fw (Ben)

* move "crash_data->crashed_since_read = true" to
  ath10k_debug_get_new_few_crash_data()

* ath10k_pci_hif_dump_area() holds the lock all the time so that we
  can guarantee that changes to ath10k_fw_crash_data are atomic

* take data_lock earlier in ath10k_build_dump_file() so that all
  access to crash_data is protected

* rename debugfs file fw_crash_dump

* fw_crash_dump debugfs files returns -ENODATA if there's no new
  crash dump

* store bss addresses and lengths as u32 in struct ath10k::fw

v5:

* dump_data->tv_sec and tv_nsec to 64 bits (because long can be 32 bits
  on some platforms)

* fix long lines

* renamed ath10k_dbg_save_fw_dbg_buffer() to ath10k_debug_dbglog_add()

* add helpers for ath10k_pci_diag* functions

* refactor and rename ath10k_pci_hif_dump_area()

* latest crash dump is always stored (instead of the oldest unread)

* add ath10k_debug_get_fw_crash_data()

* move fw_r?m_bss_* fields to ar->fw

* struct ath10k_fw_crash_data is allocated with vmalloc()

* atomic allocation in ath10k_pci_dump_bss() is bad, fix that by using vmalloc
  in module initialisation

* separate FW IE entries for BSS regions

* don't use ath10k_err()

* simplify locking and memory allocation for FW IE handling

* add uuid

* move struct ath10k_dump_file_data and enum ath10k_fw_error_dump_type to debug.c

* function and variable naming, using ath10k_fw_crash_ prefix etc

* change warning and debug messages to follow ath10k style

* add ath10k_debug_get_new_fw_crash_data() to avoid ifdefs in pci.c

TODO for the future:

* atomic allocation in ath10k_pci_dump_dbglog() is bad. Should we
  allocate a big buffer with vmalloc and use that?

* dynamic allocation for BSS buffers


---

Ben Greear (5):
      ath10k: provide firmware crash info via debugfs
      ath10k: save firmware debug log messages
      ath10k: save firmware stack upon firmware crash
      ath10k: dump exception stack contents on firmware crash
      ath10k: save firmware RAM and ROM BSS sections on crash

Kalle Valo (3):
      ath10k: add ath10k_pci_diag_* helpers
      ath10k: rename ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed()
      ath10k: print more driver info when firmware crashes


 drivers/net/wireless/ath/ath10k/core.c  |   71 +++++-
 drivers/net/wireless/ath/ath10k/core.h  |   45 ++++
 drivers/net/wireless/ath/ath10k/debug.c |  341 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |   23 ++
 drivers/net/wireless/ath/ath10k/hw.h    |   36 +++
 drivers/net/wireless/ath/ath10k/pci.c   |  257 +++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/pci.h   |    3 
 drivers/net/wireless/ath/ath10k/wmi.c   |   10 +
 8 files changed, 743 insertions(+), 43 deletions(-)


^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH v6 0/8] ath10k: firmware crash dump
@ 2014-08-09 18:07 ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:07 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Next version of ath10k firmware crash dump feature, based on Ben's patches but
heavily modified.

v6:

* fix vzalloc(sizeof(ar->debug.fw_crash_data)), fixes the crash I saw (Ben)

* "Target Register Dump" -> "firmware register dump"

* add ath10k_print_driver_info()

* take timestamp at crash time instead of crash-dump-gather time (Ben)

* fix locking comment in struct ath10k::fw (Ben)

* move "crash_data->crashed_since_read = true" to
  ath10k_debug_get_new_few_crash_data()

* ath10k_pci_hif_dump_area() holds the lock all the time so that we
  can guarantee that changes to ath10k_fw_crash_data are atomic

* take data_lock earlier in ath10k_build_dump_file() so that all
  access to crash_data is protected

* rename debugfs file fw_crash_dump

* fw_crash_dump debugfs files returns -ENODATA if there's no new
  crash dump

* store bss addresses and lengths as u32 in struct ath10k::fw

v5:

* dump_data->tv_sec and tv_nsec to 64 bits (because long can be 32 bits
  on some platforms)

* fix long lines

* renamed ath10k_dbg_save_fw_dbg_buffer() to ath10k_debug_dbglog_add()

* add helpers for ath10k_pci_diag* functions

* refactor and rename ath10k_pci_hif_dump_area()

* latest crash dump is always stored (instead of the oldest unread)

* add ath10k_debug_get_fw_crash_data()

* move fw_r?m_bss_* fields to ar->fw

* struct ath10k_fw_crash_data is allocated with vmalloc()

* atomic allocation in ath10k_pci_dump_bss() is bad, fix that by using vmalloc
  in module initialisation

* separate FW IE entries for BSS regions

* don't use ath10k_err()

* simplify locking and memory allocation for FW IE handling

* add uuid

* move struct ath10k_dump_file_data and enum ath10k_fw_error_dump_type to debug.c

* function and variable naming, using ath10k_fw_crash_ prefix etc

* change warning and debug messages to follow ath10k style

* add ath10k_debug_get_new_fw_crash_data() to avoid ifdefs in pci.c

TODO for the future:

* atomic allocation in ath10k_pci_dump_dbglog() is bad. Should we
  allocate a big buffer with vmalloc and use that?

* dynamic allocation for BSS buffers


---

Ben Greear (5):
      ath10k: provide firmware crash info via debugfs
      ath10k: save firmware debug log messages
      ath10k: save firmware stack upon firmware crash
      ath10k: dump exception stack contents on firmware crash
      ath10k: save firmware RAM and ROM BSS sections on crash

Kalle Valo (3):
      ath10k: add ath10k_pci_diag_* helpers
      ath10k: rename ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed()
      ath10k: print more driver info when firmware crashes


 drivers/net/wireless/ath/ath10k/core.c  |   71 +++++-
 drivers/net/wireless/ath/ath10k/core.h  |   45 ++++
 drivers/net/wireless/ath/ath10k/debug.c |  341 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |   23 ++
 drivers/net/wireless/ath/ath10k/hw.h    |   36 +++
 drivers/net/wireless/ath/ath10k/pci.c   |  257 +++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/pci.h   |    3 
 drivers/net/wireless/ath/ath10k/wmi.c   |   10 +
 8 files changed, 743 insertions(+), 43 deletions(-)


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH v6 1/8] ath10k: add ath10k_pci_diag_* helpers
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:07   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:07 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

ath10k_pci_diag_read32() is for reading u32 from a device and ath10k_pci_diag_read_hi()
is a helper for reading data using "host interest" table.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   55 +++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2d340cc522e3..96ce359349cb 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -466,13 +466,46 @@ done:
 	return ret;
 }
 
+static int ath10k_pci_diag_read32(struct ath10k *ar, u32 address, u32 *value)
+{
+	return ath10k_pci_diag_read_mem(ar, address, value, sizeof(u32));
+}
+
+static int __ath10k_pci_diag_read_hi(struct ath10k *ar, void *dest,
+				     u32 src, u32 len)
+{
+	u32 host_addr, addr;
+	int ret;
+
+	host_addr = host_interest_item_address(src);
+
+	ret = ath10k_pci_diag_read32(ar, host_addr, &addr);
+	if (ret != 0) {
+		ath10k_warn("failed to get memcpy hi address for firmware address %d: %d\n",
+			    src, ret);
+		return ret;
+	}
+
+	ret = ath10k_pci_diag_read_mem(ar, addr, dest, len);
+	if (ret != 0) {
+		ath10k_warn("failed to memcpy firmware memory from %d (%d B): %d\n",
+			    addr, len, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+#define ath10k_pci_diag_read_hi(ar, dest, src, len)		\
+	__ath10k_pci_diag_read_hi(ar, dest, HI_ITEM(src), len);
+
 /* Read 4-byte aligned data from Target memory or register */
 static int ath10k_pci_diag_read_access(struct ath10k *ar, u32 address,
 				       u32 *data)
 {
 	/* Assume range doesn't cross this boundary */
 	if (address >= DRAM_BASE_ADDRESS)
-		return ath10k_pci_diag_read_mem(ar, address, data, sizeof(u32));
+		return ath10k_pci_diag_read32(ar, address, data);
 
 	ath10k_pci_wake(ar);
 	*data = ath10k_pci_read32(ar, address);
@@ -836,9 +869,7 @@ static u16 ath10k_pci_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
 
 static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 {
-	u32 reg_dump_area = 0;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
-	u32 host_addr;
 	int ret;
 	u32 i;
 
@@ -847,21 +878,11 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 		   ar->hw_params.name, ar->target_version);
 	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
 
-	host_addr = host_interest_item_address(HI_ITEM(hi_failure_state));
-	ret = ath10k_pci_diag_read_mem(ar, host_addr,
-				       &reg_dump_area, sizeof(u32));
+	ret = ath10k_pci_diag_read_hi(ar, &reg_dump_values[0],
+				      hi_failure_state,
+				      REG_DUMP_COUNT_QCA988X * sizeof(u32));
 	if (ret) {
-		ath10k_err("failed to read FW dump area address: %d\n", ret);
-		return;
-	}
-
-	ath10k_err("target register Dump Location: 0x%08X\n", reg_dump_area);
-
-	ret = ath10k_pci_diag_read_mem(ar, reg_dump_area,
-				       &reg_dump_values[0],
-				       REG_DUMP_COUNT_QCA988X * sizeof(u32));
-	if (ret != 0) {
-		ath10k_err("failed to read FW dump area: %d\n", ret);
+		ath10k_err("failed to read firmware dump area: %d\n", ret);
 		return;
 	}
 


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 1/8] ath10k: add ath10k_pci_diag_* helpers
@ 2014-08-09 18:07   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:07 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

ath10k_pci_diag_read32() is for reading u32 from a device and ath10k_pci_diag_read_hi()
is a helper for reading data using "host interest" table.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   55 +++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 2d340cc522e3..96ce359349cb 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -466,13 +466,46 @@ done:
 	return ret;
 }
 
+static int ath10k_pci_diag_read32(struct ath10k *ar, u32 address, u32 *value)
+{
+	return ath10k_pci_diag_read_mem(ar, address, value, sizeof(u32));
+}
+
+static int __ath10k_pci_diag_read_hi(struct ath10k *ar, void *dest,
+				     u32 src, u32 len)
+{
+	u32 host_addr, addr;
+	int ret;
+
+	host_addr = host_interest_item_address(src);
+
+	ret = ath10k_pci_diag_read32(ar, host_addr, &addr);
+	if (ret != 0) {
+		ath10k_warn("failed to get memcpy hi address for firmware address %d: %d\n",
+			    src, ret);
+		return ret;
+	}
+
+	ret = ath10k_pci_diag_read_mem(ar, addr, dest, len);
+	if (ret != 0) {
+		ath10k_warn("failed to memcpy firmware memory from %d (%d B): %d\n",
+			    addr, len, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+#define ath10k_pci_diag_read_hi(ar, dest, src, len)		\
+	__ath10k_pci_diag_read_hi(ar, dest, HI_ITEM(src), len);
+
 /* Read 4-byte aligned data from Target memory or register */
 static int ath10k_pci_diag_read_access(struct ath10k *ar, u32 address,
 				       u32 *data)
 {
 	/* Assume range doesn't cross this boundary */
 	if (address >= DRAM_BASE_ADDRESS)
-		return ath10k_pci_diag_read_mem(ar, address, data, sizeof(u32));
+		return ath10k_pci_diag_read32(ar, address, data);
 
 	ath10k_pci_wake(ar);
 	*data = ath10k_pci_read32(ar, address);
@@ -836,9 +869,7 @@ static u16 ath10k_pci_hif_get_free_queue_number(struct ath10k *ar, u8 pipe)
 
 static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 {
-	u32 reg_dump_area = 0;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
-	u32 host_addr;
 	int ret;
 	u32 i;
 
@@ -847,21 +878,11 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 		   ar->hw_params.name, ar->target_version);
 	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
 
-	host_addr = host_interest_item_address(HI_ITEM(hi_failure_state));
-	ret = ath10k_pci_diag_read_mem(ar, host_addr,
-				       &reg_dump_area, sizeof(u32));
+	ret = ath10k_pci_diag_read_hi(ar, &reg_dump_values[0],
+				      hi_failure_state,
+				      REG_DUMP_COUNT_QCA988X * sizeof(u32));
 	if (ret) {
-		ath10k_err("failed to read FW dump area address: %d\n", ret);
-		return;
-	}
-
-	ath10k_err("target register Dump Location: 0x%08X\n", reg_dump_area);
-
-	ret = ath10k_pci_diag_read_mem(ar, reg_dump_area,
-				       &reg_dump_values[0],
-				       REG_DUMP_COUNT_QCA988X * sizeof(u32));
-	if (ret != 0) {
-		ath10k_err("failed to read FW dump area: %d\n", ret);
+		ath10k_err("failed to read firmware dump area: %d\n", ret);
 		return;
 	}
 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |   27 +++
 drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |   22 ++
 drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
 drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
 drivers/net/wireless/ath/ath10k/pci.h   |    3 
 6 files changed, 478 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index d5c95d46e841..77fb36d378c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -22,6 +22,8 @@
 #include <linux/if_ether.h>
 #include <linux/types.h>
 #include <linux/pci.h>
+#include <linux/uuid.h>
+#include <linux/time.h>
 
 #include "htt.h"
 #include "htc.h"
@@ -278,6 +280,29 @@ struct ath10k_vif_iter {
 	struct ath10k_vif *arvif;
 };
 
+/* This will store at least the last 128 entries.  Each dbglog message
+ * is a max of 7 32-bit integers in length, but the length can be less
+ * than that as well.
+ */
+#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * sizeof(u32))
+
+struct ath10k_dbglog_entry_storage {
+	/* where to write next chunk of data */
+	u32 next_idx;
+
+	u8 data[ATH10K_DBGLOG_DATA_LEN];
+};
+
+/* used for crash-dump storage, protected by data-lock */
+struct ath10k_fw_crash_data {
+	bool crashed_since_read;
+
+	uuid_le uuid;
+	struct timespec timestamp;
+	struct ath10k_dbglog_entry_storage dbglog_entry_data;
+	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+};
+
 struct ath10k_debug {
 	struct dentry *debugfs_phy;
 
@@ -295,6 +320,8 @@ struct ath10k_debug {
 
 	u8 htt_max_amsdu;
 	u8 htt_max_ampdu;
+
+	struct ath10k_fw_crash_data *fw_crash_data;
 };
 
 enum ath10k_state {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index c9e35c87edfb..d6b49a9e1585 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -17,6 +17,9 @@
 
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/version.h>
+#include <linux/vermagic.h>
+#include <linux/vmalloc.h>
 
 #include "core.h"
 #include "debug.h"
@@ -24,6 +27,89 @@
 /* ms */
 #define ATH10K_DEBUG_HTT_STATS_INTERVAL 1000
 
+/**
+ * enum ath10k_fw_crash_dump_type - types of data in the dump file
+ * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
+ * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
+ */
+enum ath10k_fw_crash_dump_type {
+	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
+	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
+
+	ATH10K_FW_CRASH_DUMP_MAX,
+};
+
+struct ath10k_tlv_dump_data {
+	/* see ath10k_fw_crash_dump_type above */
+	u32 type;
+
+	/* in bytes */
+	u32 tlv_len;
+
+	/* pad to 32-bit boundaries as needed */
+	u8 tlv_data[];
+} __packed;
+
+struct ath10k_dump_file_data {
+	/* dump file information */
+
+	/* "ATH10K-FW-DUMP" */
+	char df_magic[16];
+
+	u32 len;
+
+	/* 0x1 if host is big-endian */
+	u32 big_endian;
+
+	/* file dump version, 1 for now. */
+	u32 version;
+
+	/* some info we can get from ath10k struct that might help */
+
+	u8 uuid[16];
+
+	u32 chip_id;
+
+	/* 0 for now, in place for later hardware */
+	u32 bus_type;
+
+	u32 target_version;
+	u32 fw_version_major;
+	u32 fw_version_minor;
+	u32 fw_version_release;
+	u32 fw_version_build;
+	u32 phy_capability;
+	u32 hw_min_tx_power;
+	u32 hw_max_tx_power;
+	u32 ht_cap_info;
+	u32 vht_cap_info;
+	u32 num_rf_chains;
+
+	/* firmware version string */
+	char fw_ver[ETHTOOL_FWVERS_LEN];
+
+	/* Kernel related information */
+
+	/* time-of-day stamp */
+	u64 tv_sec;
+
+	/* time-of-day stamp, nano-seconds */
+	u64 tv_nsec;
+
+
+	/* LINUX_VERSION_CODE */
+	u32 kernel_ver_code;
+
+	/* VERMAGIC_STRING */
+	char kernel_ver[64];
+
+	/* room for growth w/out changing binary format */
+	u8 unused[128];
+
+	/* struct ath10k_tlv_dump_data + more */
+	u8 data[0];
+} __packed;
+
 static int ath10k_printk(const char *level, const char *fmt, ...)
 {
 	struct va_format vaf;
@@ -580,6 +666,185 @@ static const struct file_operations fops_chip_id = {
 	.llseek = default_llseek,
 };
 
+struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	crash_data->crashed_since_read = true;
+	uuid_le_gen(&crash_data->uuid);
+	getnstimeofday(&crash_data->timestamp);
+
+	return crash_data;
+}
+EXPORT_SYMBOL(ath10k_debug_get_new_fw_crash_data);
+
+void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer, int len)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+	int i, z;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	z = crash_data->dbglog_entry_data.next_idx;
+
+	for (i = 0; i < len; i++) {
+		crash_data->dbglog_entry_data.data[z] = buffer[i];
+		z++;
+		if (z >= ATH10K_DBGLOG_DATA_LEN)
+			z = 0;
+	}
+
+	crash_data->dbglog_entry_data.next_idx = z;
+}
+EXPORT_SYMBOL(ath10k_debug_dbglog_add);
+
+static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+	struct ath10k_dump_file_data *dump_data;
+	struct ath10k_tlv_dump_data *dump_tlv;
+	int hdr_len = sizeof(*dump_data);
+	unsigned int len, sofar = 0;
+	unsigned char *buf;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	spin_lock_bh(&ar->data_lock);
+
+	if (!crash_data->crashed_since_read) {
+		spin_unlock_bh(&ar->data_lock);
+		return NULL;
+	}
+
+	spin_unlock_bh(&ar->data_lock);
+
+	len = hdr_len;
+	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
+
+	sofar += hdr_len;
+
+	/* 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;
+
+	spin_lock_bh(&ar->data_lock);
+
+	memset(buf, 0, len);
+	dump_data = (struct ath10k_dump_file_data *)(buf);
+	strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
+		sizeof(dump_data->df_magic));
+	dump_data->len = len;
+
+#ifdef __BIG_ENDIAN
+	dump_data->big_endian = 1;
+#else
+	dump_data->big_endian = 0;
+#endif
+
+	dump_data->version = 1;
+	memcpy(dump_data->uuid, &crash_data->uuid, sizeof(dump_data->uuid));
+	dump_data->chip_id = ar->chip_id;
+	dump_data->bus_type = 0;
+	dump_data->target_version = ar->target_version;
+	dump_data->fw_version_major = ar->fw_version_major;
+	dump_data->fw_version_minor = ar->fw_version_minor;
+	dump_data->fw_version_release = ar->fw_version_release;
+	dump_data->fw_version_build = ar->fw_version_build;
+	dump_data->phy_capability = ar->phy_capability;
+	dump_data->hw_min_tx_power = ar->hw_min_tx_power;
+	dump_data->hw_max_tx_power = ar->hw_max_tx_power;
+	dump_data->ht_cap_info = ar->ht_cap_info;
+	dump_data->vht_cap_info = ar->vht_cap_info;
+	dump_data->num_rf_chains = ar->num_rf_chains;
+
+	strlcpy(dump_data->fw_ver, ar->hw->wiphy->fw_version,
+		sizeof(dump_data->fw_ver));
+
+	dump_data->kernel_ver_code = LINUX_VERSION_CODE;
+	strlcpy(dump_data->kernel_ver, VERMAGIC_STRING,
+		sizeof(dump_data->kernel_ver));
+
+	dump_data->tv_sec = crash_data->timestamp.tv_sec;
+	dump_data->tv_nsec = crash_data->timestamp.tv_nsec;
+
+	/* Gather dbg-log */
+	dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+	dump_tlv->type = ATH10K_FW_CRASH_DUMP_DBGLOG;
+	dump_tlv->tlv_len = sizeof(crash_data->dbglog_entry_data);
+	memcpy(dump_tlv->tlv_data, &crash_data->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_CRASH_DUMP_REGDUMP;
+	dump_tlv->tlv_len = sizeof(crash_data->reg_dump_values);
+	memcpy(dump_tlv->tlv_data, &crash_data->reg_dump_values,
+	       dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+	spin_unlock_bh(&ar->data_lock);
+
+	return dump_data;
+}
+
+static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
+{
+	struct ath10k *ar = inode->i_private;
+	struct ath10k_dump_file_data *dump;
+	int ret;
+
+	mutex_lock(&ar->conf_mutex);
+
+	dump = ath10k_build_dump_file(ar);
+	if (!dump) {
+		ret = -ENODATA;
+		goto out;
+	}
+
+	file->private_data = dump;
+	ar->debug.fw_crash_data->crashed_since_read = false;
+	ret = 0;
+
+out:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
+static ssize_t ath10k_fw_crash_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_crash_dump_release(struct inode *inode,
+					struct file *file)
+{
+	vfree(file->private_data);
+
+	return 0;
+}
+
+static const struct file_operations fops_fw_crash_dump = {
+	.open = ath10k_fw_crash_dump_open,
+	.read = ath10k_fw_crash_dump_read,
+	.release = ath10k_fw_crash_dump_release,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 static int ath10k_debug_htt_stats_req(struct ath10k *ar)
 {
 	u64 cookie;
@@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
 
 int ath10k_debug_create(struct ath10k *ar)
 {
+	ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
+	if (!ar->debug.fw_crash_data)
+		return -ENOMEM;
+
 	ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
 						   ar->hw->wiphy->debugfsdir);
 
@@ -933,6 +1202,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_crash_dump", S_IRUSR, ar->debug.debugfs_phy,
+			    ar, &fops_fw_crash_dump);
+
 	debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
 			    ar, &fops_chip_id);
 
@@ -965,6 +1237,7 @@ int ath10k_debug_create(struct ath10k *ar)
 
 void ath10k_debug_destroy(struct ath10k *ar)
 {
+	vfree(ar->debug.fw_crash_data);
 	cancel_delayed_work_sync(&ar->debug.htt_stats_dwork);
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index a5824990bd2a..80ff14e4db9b 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -53,6 +53,10 @@ void ath10k_debug_read_service_map(struct ath10k *ar,
 				   size_t map_size);
 void ath10k_debug_read_target_stats(struct ath10k *ar,
 				    struct wmi_stats_event *ev);
+struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar);
+
+void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer, int len);
 
 #define ATH10K_DFS_STAT_INC(ar, c) (ar->debug.dfs_stats.c++)
 
@@ -86,6 +90,17 @@ static inline void ath10k_debug_read_target_stats(struct ath10k *ar,
 {
 }
 
+static inline void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer,
+					   int len)
+{
+}
+
+static inline struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
+{
+	return NULL;
+}
+
 #define ATH10K_DFS_STAT_INC(ar, c) do { } while (0)
 
 #endif /* CONFIG_ATH10K_DEBUGFS */
@@ -96,6 +111,7 @@ __printf(2, 3) void ath10k_dbg(enum ath10k_debug_mask mask,
 void ath10k_dbg_dump(enum ath10k_debug_mask mask,
 		     const char *msg, const char *prefix,
 		     const void *buf, size_t len);
+
 #else /* CONFIG_ATH10K_DEBUG */
 
 static inline int ath10k_dbg(enum ath10k_debug_mask dbg_mask,
@@ -109,5 +125,11 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
 				   const void *buf, size_t len)
 {
 }
+
+static inline void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar,
+						 u8 *buffer, int len)
+{
+}
 #endif /* CONFIG_ATH10K_DEBUG */
+
 #endif /* _DEBUG_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index ffd04890407e..c391c88096ee 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -39,6 +39,8 @@
 /* includes also the null byte */
 #define ATH10K_FIRMWARE_MAGIC               "QCA-ATH10K"
 
+#define REG_DUMP_COUNT_QCA988X 60
+
 struct ath10k_fw_ie {
 	__le32 id;
 	__le32 len;
@@ -362,4 +364,32 @@ enum ath10k_mcast2ucast_mode {
 
 #define RTC_STATE_V_GET(x) (((x) & RTC_STATE_V_MASK) >> RTC_STATE_V_LSB)
 
+
+/* 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 ath10k_fw_dbglog_buf {
+	/* pointer to dblog_buf_s */
+	u32 next;
+
+	/* pointer to u8 buffer */
+	u32 buffer;
+
+	u32 bufsize;
+	u32 length;
+	u32 count;
+	u32 free;
+} __packed;
+
+struct ath10k_fw_dbglog_hdr {
+	/* pointer to dbglog_buf_s */
+	u32 dbuf;
+
+	u32 dropped;
+} __packed;
+
+
 #endif /* _HW_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 96ce359349cb..99e0bd0c4f4a 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -867,16 +867,103 @@ 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_pci_hif_dump_area(struct ath10k *ar)
+static void ath10k_pci_dump_dbglog(struct ath10k *ar,
+				   struct ath10k_fw_crash_data *crash_data)
+{
+	struct ath10k_fw_dbglog_hdr dbg_hdr;
+	struct ath10k_fw_dbglog_buf dbuf;
+	u8 *buffer;
+	int ret, i;
+	u32 dbufp;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	/* dump the debug logs on the target */
+	ret = ath10k_pci_diag_read_hi(ar, &dbg_hdr,
+				      hi_dbglog_hdr, sizeof(dbg_hdr));
+	if (ret != 0) {
+		ath10k_warn("failed to dump debug log area from hi_dbglog_hdr: %d\n",
+			    ret);
+		return;
+	}
+
+	ath10k_dbg(ATH10K_DBG_PCI,
+		   "pci dbglog header dbuf 0x%x dropped %i\n",
+		   dbg_hdr.dbuf, dbg_hdr.dropped);
+
+	/* pointer in target memory space */
+	dbufp = dbg_hdr.dbuf;
+
+	/* i is for logging purposes and sanity check in case firmware buffers
+	 * are corrupted and will not properly terminate the list.
+	 * In standard firmware, it appears there are no more than 2
+	 * buffers, so 10 should be safe upper limit even if firmware
+	 * changes quite a bit.
+	 */
+	i = 0;
+	while (dbufp && i < 10) {
+		ret = ath10k_pci_diag_read_mem(ar, dbufp,
+					       &dbuf, sizeof(dbuf));
+		if (ret != 0) {
+			ath10k_warn("failed to read debug log area from address 0x%x: %d\n",
+				    dbufp, ret);
+			return;
+		}
+
+		/* we have a buffer of data */
+		ath10k_dbg(ATH10K_DBG_PCI,
+			   "pci dbglog [%i] next 0x%x buf 0x%x size %i len %i count %i free %i\n",
+			   i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
+			   dbuf.count, dbuf.free);
+		if (dbuf.buffer == 0 || dbuf.length == 0)
+			goto next;
+
+		/* Pick arbitrary upper bound in case firmware is corrupted for
+		 * whatever reason.
+		 */
+		if (dbuf.length > 16000) {
+			ath10k_warn("firmware debug log buffer length is out of bounds: %d\n",
+				    dbuf.length);
+			/* do not trust the next pointer either... */
+			return;
+		}
+
+		buffer = kmalloc(dbuf.length, GFP_ATOMIC);
+
+		if (!buffer)
+			goto next;
+
+		ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer, buffer,
+					       dbuf.length);
+		if (ret != 0) {
+			ath10k_warn("failed to read debug log buffer from address 0x%x: %d\n",
+				    dbuf.buffer, ret);
+			kfree(buffer);
+			return;
+		}
+
+		ath10k_debug_dbglog_add(ar, buffer, dbuf.length);
+		kfree(buffer);
+
+next:
+		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 */
+}
+
+static void ath10k_pci_dump_registers(struct ath10k *ar,
+				      struct ath10k_fw_crash_data *crash_data)
 {
-	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
+	u32 i, reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
 	int ret;
-	u32 i;
 
-	ath10k_err("firmware crashed!\n");
-	ath10k_err("hardware name %s version 0x%x\n",
-		   ar->hw_params.name, ar->target_version);
-	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+	lockdep_assert_held(&ar->data_lock);
 
 	ret = ath10k_pci_diag_read_hi(ar, &reg_dump_values[0],
 				      hi_failure_state,
@@ -897,6 +984,38 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 			   reg_dump_values[i + 2],
 			   reg_dump_values[i + 3]);
 
+	memcpy(crash_data->reg_dump_values, reg_dump_values,
+	       sizeof(crash_data->reg_dump_values));
+}
+
+static void ath10k_pci_hif_dump_area(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data;
+	char uuid[50];
+
+	spin_lock_bh(&ar->data_lock);
+
+	crash_data = ath10k_debug_get_new_fw_crash_data(ar);
+
+	if (crash_data)
+		scnprintf(uuid, sizeof(uuid), "%pU", &crash_data->uuid);
+	else
+		scnprintf(uuid, sizeof(uuid), "n/a");
+
+	ath10k_err("firmware crashed! (uuid %s)\n", uuid);
+	ath10k_err("hardware name %s version 0x%x\n",
+		   ar->hw_params.name, ar->target_version);
+	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+
+	if (!crash_data)
+		goto exit;
+
+	ath10k_pci_dump_registers(ar, crash_data);
+	ath10k_pci_dump_dbglog(ar, crash_data);
+
+exit:
+	spin_unlock_bh(&ar->data_lock);
+
 	queue_work(ar->workqueue, &ar->restart_work);
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 940129209990..f72a7cdec4d4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -23,9 +23,6 @@
 #include "hw.h"
 #include "ce.h"
 
-/* FW dump area */
-#define REG_DUMP_COUNT_QCA988X 60
-
 /*
  * maximum number of bytes that can be handled atomically by DiagRead/DiagWrite
  */


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |   27 +++
 drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |   22 ++
 drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
 drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
 drivers/net/wireless/ath/ath10k/pci.h   |    3 
 6 files changed, 478 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index d5c95d46e841..77fb36d378c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -22,6 +22,8 @@
 #include <linux/if_ether.h>
 #include <linux/types.h>
 #include <linux/pci.h>
+#include <linux/uuid.h>
+#include <linux/time.h>
 
 #include "htt.h"
 #include "htc.h"
@@ -278,6 +280,29 @@ struct ath10k_vif_iter {
 	struct ath10k_vif *arvif;
 };
 
+/* This will store at least the last 128 entries.  Each dbglog message
+ * is a max of 7 32-bit integers in length, but the length can be less
+ * than that as well.
+ */
+#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * sizeof(u32))
+
+struct ath10k_dbglog_entry_storage {
+	/* where to write next chunk of data */
+	u32 next_idx;
+
+	u8 data[ATH10K_DBGLOG_DATA_LEN];
+};
+
+/* used for crash-dump storage, protected by data-lock */
+struct ath10k_fw_crash_data {
+	bool crashed_since_read;
+
+	uuid_le uuid;
+	struct timespec timestamp;
+	struct ath10k_dbglog_entry_storage dbglog_entry_data;
+	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+};
+
 struct ath10k_debug {
 	struct dentry *debugfs_phy;
 
@@ -295,6 +320,8 @@ struct ath10k_debug {
 
 	u8 htt_max_amsdu;
 	u8 htt_max_ampdu;
+
+	struct ath10k_fw_crash_data *fw_crash_data;
 };
 
 enum ath10k_state {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index c9e35c87edfb..d6b49a9e1585 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -17,6 +17,9 @@
 
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/version.h>
+#include <linux/vermagic.h>
+#include <linux/vmalloc.h>
 
 #include "core.h"
 #include "debug.h"
@@ -24,6 +27,89 @@
 /* ms */
 #define ATH10K_DEBUG_HTT_STATS_INTERVAL 1000
 
+/**
+ * enum ath10k_fw_crash_dump_type - types of data in the dump file
+ * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
+ * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
+ */
+enum ath10k_fw_crash_dump_type {
+	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
+	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
+
+	ATH10K_FW_CRASH_DUMP_MAX,
+};
+
+struct ath10k_tlv_dump_data {
+	/* see ath10k_fw_crash_dump_type above */
+	u32 type;
+
+	/* in bytes */
+	u32 tlv_len;
+
+	/* pad to 32-bit boundaries as needed */
+	u8 tlv_data[];
+} __packed;
+
+struct ath10k_dump_file_data {
+	/* dump file information */
+
+	/* "ATH10K-FW-DUMP" */
+	char df_magic[16];
+
+	u32 len;
+
+	/* 0x1 if host is big-endian */
+	u32 big_endian;
+
+	/* file dump version, 1 for now. */
+	u32 version;
+
+	/* some info we can get from ath10k struct that might help */
+
+	u8 uuid[16];
+
+	u32 chip_id;
+
+	/* 0 for now, in place for later hardware */
+	u32 bus_type;
+
+	u32 target_version;
+	u32 fw_version_major;
+	u32 fw_version_minor;
+	u32 fw_version_release;
+	u32 fw_version_build;
+	u32 phy_capability;
+	u32 hw_min_tx_power;
+	u32 hw_max_tx_power;
+	u32 ht_cap_info;
+	u32 vht_cap_info;
+	u32 num_rf_chains;
+
+	/* firmware version string */
+	char fw_ver[ETHTOOL_FWVERS_LEN];
+
+	/* Kernel related information */
+
+	/* time-of-day stamp */
+	u64 tv_sec;
+
+	/* time-of-day stamp, nano-seconds */
+	u64 tv_nsec;
+
+
+	/* LINUX_VERSION_CODE */
+	u32 kernel_ver_code;
+
+	/* VERMAGIC_STRING */
+	char kernel_ver[64];
+
+	/* room for growth w/out changing binary format */
+	u8 unused[128];
+
+	/* struct ath10k_tlv_dump_data + more */
+	u8 data[0];
+} __packed;
+
 static int ath10k_printk(const char *level, const char *fmt, ...)
 {
 	struct va_format vaf;
@@ -580,6 +666,185 @@ static const struct file_operations fops_chip_id = {
 	.llseek = default_llseek,
 };
 
+struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	crash_data->crashed_since_read = true;
+	uuid_le_gen(&crash_data->uuid);
+	getnstimeofday(&crash_data->timestamp);
+
+	return crash_data;
+}
+EXPORT_SYMBOL(ath10k_debug_get_new_fw_crash_data);
+
+void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer, int len)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+	int i, z;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	z = crash_data->dbglog_entry_data.next_idx;
+
+	for (i = 0; i < len; i++) {
+		crash_data->dbglog_entry_data.data[z] = buffer[i];
+		z++;
+		if (z >= ATH10K_DBGLOG_DATA_LEN)
+			z = 0;
+	}
+
+	crash_data->dbglog_entry_data.next_idx = z;
+}
+EXPORT_SYMBOL(ath10k_debug_dbglog_add);
+
+static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
+	struct ath10k_dump_file_data *dump_data;
+	struct ath10k_tlv_dump_data *dump_tlv;
+	int hdr_len = sizeof(*dump_data);
+	unsigned int len, sofar = 0;
+	unsigned char *buf;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	spin_lock_bh(&ar->data_lock);
+
+	if (!crash_data->crashed_since_read) {
+		spin_unlock_bh(&ar->data_lock);
+		return NULL;
+	}
+
+	spin_unlock_bh(&ar->data_lock);
+
+	len = hdr_len;
+	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
+
+	sofar += hdr_len;
+
+	/* 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;
+
+	spin_lock_bh(&ar->data_lock);
+
+	memset(buf, 0, len);
+	dump_data = (struct ath10k_dump_file_data *)(buf);
+	strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
+		sizeof(dump_data->df_magic));
+	dump_data->len = len;
+
+#ifdef __BIG_ENDIAN
+	dump_data->big_endian = 1;
+#else
+	dump_data->big_endian = 0;
+#endif
+
+	dump_data->version = 1;
+	memcpy(dump_data->uuid, &crash_data->uuid, sizeof(dump_data->uuid));
+	dump_data->chip_id = ar->chip_id;
+	dump_data->bus_type = 0;
+	dump_data->target_version = ar->target_version;
+	dump_data->fw_version_major = ar->fw_version_major;
+	dump_data->fw_version_minor = ar->fw_version_minor;
+	dump_data->fw_version_release = ar->fw_version_release;
+	dump_data->fw_version_build = ar->fw_version_build;
+	dump_data->phy_capability = ar->phy_capability;
+	dump_data->hw_min_tx_power = ar->hw_min_tx_power;
+	dump_data->hw_max_tx_power = ar->hw_max_tx_power;
+	dump_data->ht_cap_info = ar->ht_cap_info;
+	dump_data->vht_cap_info = ar->vht_cap_info;
+	dump_data->num_rf_chains = ar->num_rf_chains;
+
+	strlcpy(dump_data->fw_ver, ar->hw->wiphy->fw_version,
+		sizeof(dump_data->fw_ver));
+
+	dump_data->kernel_ver_code = LINUX_VERSION_CODE;
+	strlcpy(dump_data->kernel_ver, VERMAGIC_STRING,
+		sizeof(dump_data->kernel_ver));
+
+	dump_data->tv_sec = crash_data->timestamp.tv_sec;
+	dump_data->tv_nsec = crash_data->timestamp.tv_nsec;
+
+	/* Gather dbg-log */
+	dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+	dump_tlv->type = ATH10K_FW_CRASH_DUMP_DBGLOG;
+	dump_tlv->tlv_len = sizeof(crash_data->dbglog_entry_data);
+	memcpy(dump_tlv->tlv_data, &crash_data->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_CRASH_DUMP_REGDUMP;
+	dump_tlv->tlv_len = sizeof(crash_data->reg_dump_values);
+	memcpy(dump_tlv->tlv_data, &crash_data->reg_dump_values,
+	       dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
+	spin_unlock_bh(&ar->data_lock);
+
+	return dump_data;
+}
+
+static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
+{
+	struct ath10k *ar = inode->i_private;
+	struct ath10k_dump_file_data *dump;
+	int ret;
+
+	mutex_lock(&ar->conf_mutex);
+
+	dump = ath10k_build_dump_file(ar);
+	if (!dump) {
+		ret = -ENODATA;
+		goto out;
+	}
+
+	file->private_data = dump;
+	ar->debug.fw_crash_data->crashed_since_read = false;
+	ret = 0;
+
+out:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
+static ssize_t ath10k_fw_crash_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_crash_dump_release(struct inode *inode,
+					struct file *file)
+{
+	vfree(file->private_data);
+
+	return 0;
+}
+
+static const struct file_operations fops_fw_crash_dump = {
+	.open = ath10k_fw_crash_dump_open,
+	.read = ath10k_fw_crash_dump_read,
+	.release = ath10k_fw_crash_dump_release,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 static int ath10k_debug_htt_stats_req(struct ath10k *ar)
 {
 	u64 cookie;
@@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
 
 int ath10k_debug_create(struct ath10k *ar)
 {
+	ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
+	if (!ar->debug.fw_crash_data)
+		return -ENOMEM;
+
 	ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
 						   ar->hw->wiphy->debugfsdir);
 
@@ -933,6 +1202,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_crash_dump", S_IRUSR, ar->debug.debugfs_phy,
+			    ar, &fops_fw_crash_dump);
+
 	debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
 			    ar, &fops_chip_id);
 
@@ -965,6 +1237,7 @@ int ath10k_debug_create(struct ath10k *ar)
 
 void ath10k_debug_destroy(struct ath10k *ar)
 {
+	vfree(ar->debug.fw_crash_data);
 	cancel_delayed_work_sync(&ar->debug.htt_stats_dwork);
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index a5824990bd2a..80ff14e4db9b 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -53,6 +53,10 @@ void ath10k_debug_read_service_map(struct ath10k *ar,
 				   size_t map_size);
 void ath10k_debug_read_target_stats(struct ath10k *ar,
 				    struct wmi_stats_event *ev);
+struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar);
+
+void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer, int len);
 
 #define ATH10K_DFS_STAT_INC(ar, c) (ar->debug.dfs_stats.c++)
 
@@ -86,6 +90,17 @@ static inline void ath10k_debug_read_target_stats(struct ath10k *ar,
 {
 }
 
+static inline void ath10k_debug_dbglog_add(struct ath10k *ar, u8 *buffer,
+					   int len)
+{
+}
+
+static inline struct ath10k_fw_crash_data *
+ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
+{
+	return NULL;
+}
+
 #define ATH10K_DFS_STAT_INC(ar, c) do { } while (0)
 
 #endif /* CONFIG_ATH10K_DEBUGFS */
@@ -96,6 +111,7 @@ __printf(2, 3) void ath10k_dbg(enum ath10k_debug_mask mask,
 void ath10k_dbg_dump(enum ath10k_debug_mask mask,
 		     const char *msg, const char *prefix,
 		     const void *buf, size_t len);
+
 #else /* CONFIG_ATH10K_DEBUG */
 
 static inline int ath10k_dbg(enum ath10k_debug_mask dbg_mask,
@@ -109,5 +125,11 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
 				   const void *buf, size_t len)
 {
 }
+
+static inline void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar,
+						 u8 *buffer, int len)
+{
+}
 #endif /* CONFIG_ATH10K_DEBUG */
+
 #endif /* _DEBUG_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index ffd04890407e..c391c88096ee 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -39,6 +39,8 @@
 /* includes also the null byte */
 #define ATH10K_FIRMWARE_MAGIC               "QCA-ATH10K"
 
+#define REG_DUMP_COUNT_QCA988X 60
+
 struct ath10k_fw_ie {
 	__le32 id;
 	__le32 len;
@@ -362,4 +364,32 @@ enum ath10k_mcast2ucast_mode {
 
 #define RTC_STATE_V_GET(x) (((x) & RTC_STATE_V_MASK) >> RTC_STATE_V_LSB)
 
+
+/* 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 ath10k_fw_dbglog_buf {
+	/* pointer to dblog_buf_s */
+	u32 next;
+
+	/* pointer to u8 buffer */
+	u32 buffer;
+
+	u32 bufsize;
+	u32 length;
+	u32 count;
+	u32 free;
+} __packed;
+
+struct ath10k_fw_dbglog_hdr {
+	/* pointer to dbglog_buf_s */
+	u32 dbuf;
+
+	u32 dropped;
+} __packed;
+
+
 #endif /* _HW_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 96ce359349cb..99e0bd0c4f4a 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -867,16 +867,103 @@ 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_pci_hif_dump_area(struct ath10k *ar)
+static void ath10k_pci_dump_dbglog(struct ath10k *ar,
+				   struct ath10k_fw_crash_data *crash_data)
+{
+	struct ath10k_fw_dbglog_hdr dbg_hdr;
+	struct ath10k_fw_dbglog_buf dbuf;
+	u8 *buffer;
+	int ret, i;
+	u32 dbufp;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	/* dump the debug logs on the target */
+	ret = ath10k_pci_diag_read_hi(ar, &dbg_hdr,
+				      hi_dbglog_hdr, sizeof(dbg_hdr));
+	if (ret != 0) {
+		ath10k_warn("failed to dump debug log area from hi_dbglog_hdr: %d\n",
+			    ret);
+		return;
+	}
+
+	ath10k_dbg(ATH10K_DBG_PCI,
+		   "pci dbglog header dbuf 0x%x dropped %i\n",
+		   dbg_hdr.dbuf, dbg_hdr.dropped);
+
+	/* pointer in target memory space */
+	dbufp = dbg_hdr.dbuf;
+
+	/* i is for logging purposes and sanity check in case firmware buffers
+	 * are corrupted and will not properly terminate the list.
+	 * In standard firmware, it appears there are no more than 2
+	 * buffers, so 10 should be safe upper limit even if firmware
+	 * changes quite a bit.
+	 */
+	i = 0;
+	while (dbufp && i < 10) {
+		ret = ath10k_pci_diag_read_mem(ar, dbufp,
+					       &dbuf, sizeof(dbuf));
+		if (ret != 0) {
+			ath10k_warn("failed to read debug log area from address 0x%x: %d\n",
+				    dbufp, ret);
+			return;
+		}
+
+		/* we have a buffer of data */
+		ath10k_dbg(ATH10K_DBG_PCI,
+			   "pci dbglog [%i] next 0x%x buf 0x%x size %i len %i count %i free %i\n",
+			   i, dbuf.next, dbuf.buffer, dbuf.bufsize, dbuf.length,
+			   dbuf.count, dbuf.free);
+		if (dbuf.buffer == 0 || dbuf.length == 0)
+			goto next;
+
+		/* Pick arbitrary upper bound in case firmware is corrupted for
+		 * whatever reason.
+		 */
+		if (dbuf.length > 16000) {
+			ath10k_warn("firmware debug log buffer length is out of bounds: %d\n",
+				    dbuf.length);
+			/* do not trust the next pointer either... */
+			return;
+		}
+
+		buffer = kmalloc(dbuf.length, GFP_ATOMIC);
+
+		if (!buffer)
+			goto next;
+
+		ret = ath10k_pci_diag_read_mem(ar, dbuf.buffer, buffer,
+					       dbuf.length);
+		if (ret != 0) {
+			ath10k_warn("failed to read debug log buffer from address 0x%x: %d\n",
+				    dbuf.buffer, ret);
+			kfree(buffer);
+			return;
+		}
+
+		ath10k_debug_dbglog_add(ar, buffer, dbuf.length);
+		kfree(buffer);
+
+next:
+		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 */
+}
+
+static void ath10k_pci_dump_registers(struct ath10k *ar,
+				      struct ath10k_fw_crash_data *crash_data)
 {
-	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
+	u32 i, reg_dump_values[REG_DUMP_COUNT_QCA988X] = {};
 	int ret;
-	u32 i;
 
-	ath10k_err("firmware crashed!\n");
-	ath10k_err("hardware name %s version 0x%x\n",
-		   ar->hw_params.name, ar->target_version);
-	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+	lockdep_assert_held(&ar->data_lock);
 
 	ret = ath10k_pci_diag_read_hi(ar, &reg_dump_values[0],
 				      hi_failure_state,
@@ -897,6 +984,38 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 			   reg_dump_values[i + 2],
 			   reg_dump_values[i + 3]);
 
+	memcpy(crash_data->reg_dump_values, reg_dump_values,
+	       sizeof(crash_data->reg_dump_values));
+}
+
+static void ath10k_pci_hif_dump_area(struct ath10k *ar)
+{
+	struct ath10k_fw_crash_data *crash_data;
+	char uuid[50];
+
+	spin_lock_bh(&ar->data_lock);
+
+	crash_data = ath10k_debug_get_new_fw_crash_data(ar);
+
+	if (crash_data)
+		scnprintf(uuid, sizeof(uuid), "%pU", &crash_data->uuid);
+	else
+		scnprintf(uuid, sizeof(uuid), "n/a");
+
+	ath10k_err("firmware crashed! (uuid %s)\n", uuid);
+	ath10k_err("hardware name %s version 0x%x\n",
+		   ar->hw_params.name, ar->target_version);
+	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+
+	if (!crash_data)
+		goto exit;
+
+	ath10k_pci_dump_registers(ar, crash_data);
+	ath10k_pci_dump_dbglog(ar, crash_data);
+
+exit:
+	spin_unlock_bh(&ar->data_lock);
+
 	queue_work(ar->workqueue, &ar->restart_work);
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 940129209990..f72a7cdec4d4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -23,9 +23,6 @@
 #include "hw.h"
 #include "ce.h"
 
-/* FW dump area */
-#define REG_DUMP_COUNT_QCA988X 60
-
 /*
  * maximum number of bytes that can be handled atomically by DiagRead/DiagWrite
  */


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 3/8] ath10k: save firmware debug log messages
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fffb15b1b50b..914be80d12f4 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1210,6 +1210,16 @@ static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
 
 	trace_ath10k_wmi_dbglog(skb->data, skb->len);
 
+	spin_lock_bh(&ar->data_lock);
+
+	/* First 4 bytes are a messages-dropped-due-to-overflow counter,
+	 * and should not be recorded in the dbglog buffer, so we skip
+	 * them.
+	 */
+	ath10k_debug_dbglog_add(ar, skb->data + 4, skb->len - 4);
+
+	spin_unlock_bh(&ar->data_lock);
+
 	return 0;
 }
 


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 3/8] ath10k: save firmware debug log messages
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fffb15b1b50b..914be80d12f4 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1210,6 +1210,16 @@ static int ath10k_wmi_event_debug_mesg(struct ath10k *ar, struct sk_buff *skb)
 
 	trace_ath10k_wmi_dbglog(skb->data, skb->len);
 
+	spin_lock_bh(&ar->data_lock);
+
+	/* First 4 bytes are a messages-dropped-due-to-overflow counter,
+	 * and should not be recorded in the dbglog buffer, so we skip
+	 * them.
+	 */
+	ath10k_debug_dbglog_add(ar, skb->data + 4, skb->len - 4);
+
+	spin_unlock_bh(&ar->data_lock);
+
 	return 0;
 }
 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 4/8] ath10k: save firmware stack upon firmware crash
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |    1 +
 drivers/net/wireless/ath/ath10k/debug.c |   11 +++++++++++
 drivers/net/wireless/ath/ath10k/hw.h    |    1 +
 drivers/net/wireless/ath/ath10k/pci.c   |   13 +++++++++++++
 4 files changed, 26 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 77fb36d378c8..a7634adea320 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -301,6 +301,7 @@ struct ath10k_fw_crash_data {
 	struct timespec timestamp;
 	struct ath10k_dbglog_entry_storage dbglog_entry_data;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+	u8 stack_buf[ATH10K_FW_STACK_SIZE];
 };
 
 struct ath10k_debug {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index d6b49a9e1585..66bec22d6bc3 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -31,10 +31,12 @@
  * enum ath10k_fw_crash_dump_type - types of data in the dump file
  * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
+ * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
+	ATH10K_FW_CRASH_DUMP_STACK = 2,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -724,6 +726,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len = hdr_len;
 	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
 
 	sofar += hdr_len;
 
@@ -790,8 +793,16 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	       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_CRASH_DUMP_STACK;
+	dump_tlv->tlv_len = sizeof(crash_data->stack_buf);
+	memcpy(dump_tlv->tlv_data, crash_data->stack_buf, dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
 	spin_unlock_bh(&ar->data_lock);
 
+	WARN_ON(sofar != len);
 	return dump_data;
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index c391c88096ee..9c602b0d7277 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -40,6 +40,7 @@
 #define ATH10K_FIRMWARE_MAGIC               "QCA-ATH10K"
 
 #define REG_DUMP_COUNT_QCA988X 60
+#define ATH10K_FW_STACK_SIZE 4096
 
 struct ath10k_fw_ie {
 	__le32 id;
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 99e0bd0c4f4a..8fcc219cd1e9 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -867,6 +867,18 @@ 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);
 }
 
+/* Save the main firmware stack */
+static void ath10k_pci_dump_stack(struct ath10k *ar,
+				  struct ath10k_fw_crash_data *crash_data)
+{
+	lockdep_assert_held(&ar->data_lock);
+
+	BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 4);
+
+	ath10k_pci_diag_read_hi(ar, crash_data->stack_buf,
+				hi_stack, ATH10K_FW_STACK_SIZE);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1010,6 +1022,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 	if (!crash_data)
 		goto exit;
 
+	ath10k_pci_dump_stack(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 4/8] ath10k: save firmware stack upon firmware crash
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |    1 +
 drivers/net/wireless/ath/ath10k/debug.c |   11 +++++++++++
 drivers/net/wireless/ath/ath10k/hw.h    |    1 +
 drivers/net/wireless/ath/ath10k/pci.c   |   13 +++++++++++++
 4 files changed, 26 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 77fb36d378c8..a7634adea320 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -301,6 +301,7 @@ struct ath10k_fw_crash_data {
 	struct timespec timestamp;
 	struct ath10k_dbglog_entry_storage dbglog_entry_data;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
+	u8 stack_buf[ATH10K_FW_STACK_SIZE];
 };
 
 struct ath10k_debug {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index d6b49a9e1585..66bec22d6bc3 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -31,10 +31,12 @@
  * enum ath10k_fw_crash_dump_type - types of data in the dump file
  * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
+ * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
+	ATH10K_FW_CRASH_DUMP_STACK = 2,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -724,6 +726,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len = hdr_len;
 	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
 
 	sofar += hdr_len;
 
@@ -790,8 +793,16 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	       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_CRASH_DUMP_STACK;
+	dump_tlv->tlv_len = sizeof(crash_data->stack_buf);
+	memcpy(dump_tlv->tlv_data, crash_data->stack_buf, dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
 	spin_unlock_bh(&ar->data_lock);
 
+	WARN_ON(sofar != len);
 	return dump_data;
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index c391c88096ee..9c602b0d7277 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -40,6 +40,7 @@
 #define ATH10K_FIRMWARE_MAGIC               "QCA-ATH10K"
 
 #define REG_DUMP_COUNT_QCA988X 60
+#define ATH10K_FW_STACK_SIZE 4096
 
 struct ath10k_fw_ie {
 	__le32 id;
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 99e0bd0c4f4a..8fcc219cd1e9 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -867,6 +867,18 @@ 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);
 }
 
+/* Save the main firmware stack */
+static void ath10k_pci_dump_stack(struct ath10k *ar,
+				  struct ath10k_fw_crash_data *crash_data)
+{
+	lockdep_assert_held(&ar->data_lock);
+
+	BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 4);
+
+	ath10k_pci_diag_read_hi(ar, crash_data->stack_buf,
+				hi_stack, ATH10K_FW_STACK_SIZE);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1010,6 +1022,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 	if (!crash_data)
 		goto exit;
 
+	ath10k_pci_dump_stack(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 5/8] ath10k: dump exception stack contents on firmware crash
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |    1 +
 drivers/net/wireless/ath/ath10k/debug.c |   11 +++++++++++
 drivers/net/wireless/ath/ath10k/pci.c   |   13 +++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index a7634adea320..ffee2b5cdf78 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -302,6 +302,7 @@ struct ath10k_fw_crash_data {
 	struct ath10k_dbglog_entry_storage dbglog_entry_data;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
 	u8 stack_buf[ATH10K_FW_STACK_SIZE];
+	u8 exc_stack_buf[ATH10K_FW_STACK_SIZE];
 };
 
 struct ath10k_debug {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 66bec22d6bc3..06e72e799883 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -32,11 +32,13 @@
  * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
  * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
+ * @ATH10K_FW_CRASH_DUMP_EXC_STACK:  Exception stack contents
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
 	ATH10K_FW_CRASH_DUMP_STACK = 2,
+	ATH10K_FW_CRASH_DUMP_EXC_STACK = 3,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -727,6 +729,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->exc_stack_buf);
 
 	sofar += hdr_len;
 
@@ -800,6 +803,14 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	memcpy(dump_tlv->tlv_data, crash_data->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_CRASH_DUMP_EXC_STACK;
+	dump_tlv->tlv_len = sizeof(crash_data->exc_stack_buf);
+	memcpy(dump_tlv->tlv_data, &crash_data->exc_stack_buf,
+	       dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
 	spin_unlock_bh(&ar->data_lock);
 
 	WARN_ON(sofar != len);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 8fcc219cd1e9..7ffa2ebffc94 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -879,6 +879,18 @@ static void ath10k_pci_dump_stack(struct ath10k *ar,
 				hi_stack, ATH10K_FW_STACK_SIZE);
 }
 
+/* Save the firmware exception stack */
+static void ath10k_pci_dump_exc_stack(struct ath10k *ar,
+				      struct ath10k_fw_crash_data *crash_data)
+{
+	lockdep_assert_held(&ar->data_lock);
+
+	BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 4);
+
+	ath10k_pci_diag_read_hi(ar, crash_data->exc_stack_buf,
+				hi_err_stack, ATH10K_FW_STACK_SIZE);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1023,6 +1035,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 		goto exit;
 
 	ath10k_pci_dump_stack(ar, crash_data);
+	ath10k_pci_dump_exc_stack(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 5/8] ath10k: dump exception stack contents on firmware crash
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: 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>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |    1 +
 drivers/net/wireless/ath/ath10k/debug.c |   11 +++++++++++
 drivers/net/wireless/ath/ath10k/pci.c   |   13 +++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index a7634adea320..ffee2b5cdf78 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -302,6 +302,7 @@ struct ath10k_fw_crash_data {
 	struct ath10k_dbglog_entry_storage dbglog_entry_data;
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
 	u8 stack_buf[ATH10K_FW_STACK_SIZE];
+	u8 exc_stack_buf[ATH10K_FW_STACK_SIZE];
 };
 
 struct ath10k_debug {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 66bec22d6bc3..06e72e799883 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -32,11 +32,13 @@
  * @ATH10K_FW_CRASH_DUMP_DBGLOG:  Recent firmware debug log entries
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
  * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
+ * @ATH10K_FW_CRASH_DUMP_EXC_STACK:  Exception stack contents
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
 	ATH10K_FW_CRASH_DUMP_STACK = 2,
+	ATH10K_FW_CRASH_DUMP_EXC_STACK = 3,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -727,6 +729,7 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
+	len += sizeof(*dump_tlv) + sizeof(crash_data->exc_stack_buf);
 
 	sofar += hdr_len;
 
@@ -800,6 +803,14 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	memcpy(dump_tlv->tlv_data, crash_data->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_CRASH_DUMP_EXC_STACK;
+	dump_tlv->tlv_len = sizeof(crash_data->exc_stack_buf);
+	memcpy(dump_tlv->tlv_data, &crash_data->exc_stack_buf,
+	       dump_tlv->tlv_len);
+	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+
 	spin_unlock_bh(&ar->data_lock);
 
 	WARN_ON(sofar != len);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 8fcc219cd1e9..7ffa2ebffc94 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -879,6 +879,18 @@ static void ath10k_pci_dump_stack(struct ath10k *ar,
 				hi_stack, ATH10K_FW_STACK_SIZE);
 }
 
+/* Save the firmware exception stack */
+static void ath10k_pci_dump_exc_stack(struct ath10k *ar,
+				      struct ath10k_fw_crash_data *crash_data)
+{
+	lockdep_assert_held(&ar->data_lock);
+
+	BUILD_BUG_ON(ATH10K_FW_STACK_SIZE % 4);
+
+	ath10k_pci_diag_read_hi(ar, crash_data->exc_stack_buf,
+				hi_err_stack, ATH10K_FW_STACK_SIZE);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1023,6 +1035,7 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 		goto exit;
 
 	ath10k_pci_dump_stack(ar, crash_data);
+	ath10k_pci_dump_exc_stack(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 6/8] ath10k: save firmware RAM and ROM BSS sections on crash
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

From: Ben Greear <greearb@candelatech.com>

This can be used to get a useful back trace out of a firmware
crash that involves an interrupt handler.  For instance, a
null-pointer-exception would be this kind of trace.  A user-space
tool can read the debugfs file and decode things as wished.

This requires a packaged firmware with a new IE to describe the
BSS section starts and length.

Signed-off-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.c  |   54 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/core.h  |   16 +++++++++
 drivers/net/wireless/ath/ath10k/debug.c |   28 ++++++++++++++++
 drivers/net/wireless/ath/ath10k/hw.h    |    5 +++
 drivers/net/wireless/ath/ath10k/pci.c   |   43 +++++++++++++++++++++++++
 5 files changed, 146 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 440c3ff03aec..39b7383b395c 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -479,6 +479,60 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name)
 			ar->otp_len = ie_len;
 
 			break;
+		case ATH10K_FW_IE_RAM_BSS_ADDR:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.ram_bss_addr = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found RAM BSS address 0x%x\n",
+				   ar->fw.ram_bss_addr);
+			break;
+		case ATH10K_FW_IE_RAM_BSS_LEN:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.ram_bss_len = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found RAM BSS length 0x%x\n",
+				   ar->fw.ram_bss_len);
+
+			if (ar->fw.ram_bss_len > ATH10K_RAM_BSS_BUF_LEN) {
+				ath10k_warn("too long firmware RAM BSS length: %d\n",
+					    ar->fw.ram_bss_len);
+				ar->fw.ram_bss_len = 0;
+			}
+
+			break;
+		case ATH10K_FW_IE_ROM_BSS_ADDR:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.rom_bss_addr = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found ROM BSS address 0x%x\n",
+				   ar->fw.rom_bss_addr);
+			break;
+		case ATH10K_FW_IE_ROM_BSS_LEN:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.rom_bss_len = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found ROM BSS length 0x%x\n",
+				   ar->fw.rom_bss_len);
+
+			if (ar->fw.ram_bss_len > ATH10K_RAM_BSS_BUF_LEN) {
+				ath10k_warn("too long firmware RAM BSS length: %d\n",
+					    ar->fw.ram_bss_len);
+				ar->fw.rom_bss_len = 0;
+			}
+
+			break;
 		default:
 			ath10k_warn("Unknown FW IE: %u\n",
 				    le32_to_cpu(hdr->id));
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index ffee2b5cdf78..0d457b9c38d1 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -293,6 +293,10 @@ struct ath10k_dbglog_entry_storage {
 	u8 data[ATH10K_DBGLOG_DATA_LEN];
 };
 
+/* estimated values, hopefully these are enough */
+#define ATH10K_ROM_BSS_BUF_LEN 10000
+#define ATH10K_RAM_BSS_BUF_LEN 30000
+
 /* used for crash-dump storage, protected by data-lock */
 struct ath10k_fw_crash_data {
 	bool crashed_since_read;
@@ -303,6 +307,9 @@ struct ath10k_fw_crash_data {
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
 	u8 stack_buf[ATH10K_FW_STACK_SIZE];
 	u8 exc_stack_buf[ATH10K_FW_STACK_SIZE];
+
+	u8 rom_bss_buf[ATH10K_ROM_BSS_BUF_LEN];
+	u8 ram_bss_buf[ATH10K_RAM_BSS_BUF_LEN];
 };
 
 struct ath10k_debug {
@@ -428,6 +435,15 @@ struct ath10k {
 		} fw;
 	} hw_params;
 
+	/* These are written to only during first firmware load from user
+	 * space so no need for any locking. */
+	struct {
+		u32 ram_bss_addr;
+		u32 ram_bss_len;
+		u32 rom_bss_addr;
+		u32 rom_bss_len;
+	} fw;
+
 	const struct firmware *board;
 	const void *board_data;
 	size_t board_len;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 06e72e799883..a335d29f3528 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -33,12 +33,16 @@
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
  * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
  * @ATH10K_FW_CRASH_DUMP_EXC_STACK:  Exception stack contents
+ * @ATH10K_FW_CRASH_DUMP_RAM_BSS:  BSS area for RAM code
+ * @ATH10K_FW_CRASH_DUMP_ROM_BSS:  BSS area for ROM code
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
 	ATH10K_FW_CRASH_DUMP_STACK = 2,
 	ATH10K_FW_CRASH_DUMP_EXC_STACK = 3,
+	ATH10K_FW_CRASH_DUMP_RAM_BSS = 4,
+	ATH10K_FW_CRASH_DUMP_ROM_BSS = 5,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -731,6 +735,12 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->exc_stack_buf);
 
+	if (ar->fw.ram_bss_addr && ar->fw.ram_bss_len)
+		len += sizeof(*dump_tlv) + ar->fw.ram_bss_len;
+
+	if (ar->fw.rom_bss_addr && ar->fw.rom_bss_len)
+		len += sizeof(*dump_tlv) + ar->fw.rom_bss_len;
+
 	sofar += hdr_len;
 
 	/* This is going to get big when we start dumping FW RAM and such,
@@ -811,6 +821,24 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	       dump_tlv->tlv_len);
 	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
 
+	if (ar->fw.ram_bss_addr && ar->fw.ram_bss_len) {
+		dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+		dump_tlv->type = ATH10K_FW_CRASH_DUMP_RAM_BSS;
+		dump_tlv->tlv_len = ar->fw.ram_bss_len;
+		memcpy(dump_tlv->tlv_data, crash_data->ram_bss_buf,
+		       dump_tlv->tlv_len);
+		sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+	}
+
+	if (ar->fw.rom_bss_addr && ar->fw.rom_bss_len) {
+		dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+		dump_tlv->type = ATH10K_FW_CRASH_DUMP_ROM_BSS;
+		dump_tlv->tlv_len = ar->fw.rom_bss_len;
+		memcpy(dump_tlv->tlv_data, crash_data->rom_bss_buf,
+		       dump_tlv->tlv_len);
+		sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+	}
+
 	spin_unlock_bh(&ar->data_lock);
 
 	WARN_ON(sofar != len);
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 9c602b0d7277..6c275638ddaa 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -54,6 +54,11 @@ enum ath10k_fw_ie_type {
 	ATH10K_FW_IE_FEATURES = 2,
 	ATH10K_FW_IE_FW_IMAGE = 3,
 	ATH10K_FW_IE_OTP_IMAGE = 4,
+	ATH10K_FW_IE_RAM_BSS_ADDR = 5,
+	ATH10K_FW_IE_RAM_BSS_LEN = 6,
+	ATH10K_FW_IE_ROM_BSS_ADDR = 7,
+	ATH10K_FW_IE_ROM_BSS_LEN = 8,
+
 };
 
 /* Known pecularities:
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7ffa2ebffc94..deebd30aa182 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -891,6 +891,47 @@ static void ath10k_pci_dump_exc_stack(struct ath10k *ar,
 				hi_err_stack, ATH10K_FW_STACK_SIZE);
 }
 
+static void ath10k_pci_dump_bss_ram(struct ath10k *ar,
+				    struct ath10k_fw_crash_data *crash_data) {
+	int ret;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	if (!ar->fw.ram_bss_addr)
+		return;
+
+	if (!ar->fw.ram_bss_len)
+		return;
+
+	ret = ath10k_pci_diag_read_mem(ar, ar->fw.ram_bss_addr,
+				       crash_data->ram_bss_buf,
+				       ar->fw.ram_bss_len);
+	if (ret)
+		ath10k_warn("failed to read firmware RAM BSS memory from %d (%d B): %d\n",
+			    ar->fw.ram_bss_addr, ar->fw.ram_bss_len, ret);
+}
+
+static void ath10k_pci_dump_bss_rom(struct ath10k *ar,
+				    struct ath10k_fw_crash_data *crash_data)
+{
+	int ret;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	if (!ar->fw.rom_bss_addr)
+		return;
+
+	if (!ar->fw.rom_bss_len)
+		return;
+
+	ret = ath10k_pci_diag_read_mem(ar, ar->fw.rom_bss_addr,
+				       crash_data->rom_bss_buf,
+				       ar->fw.rom_bss_len);
+	if (ret)
+		ath10k_warn("failed to read firmware ROM BSS memory from %d (%d B): %d\n",
+			    ar->fw.rom_bss_addr, ar->fw.rom_bss_len, ret);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1036,6 +1077,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 
 	ath10k_pci_dump_stack(ar, crash_data);
 	ath10k_pci_dump_exc_stack(ar, crash_data);
+	ath10k_pci_dump_bss_ram(ar, crash_data);
+	ath10k_pci_dump_bss_rom(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 6/8] ath10k: save firmware RAM and ROM BSS sections on crash
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

From: Ben Greear <greearb@candelatech.com>

This can be used to get a useful back trace out of a firmware
crash that involves an interrupt handler.  For instance, a
null-pointer-exception would be this kind of trace.  A user-space
tool can read the debugfs file and decode things as wished.

This requires a packaged firmware with a new IE to describe the
BSS section starts and length.

Signed-off-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.c  |   54 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/core.h  |   16 +++++++++
 drivers/net/wireless/ath/ath10k/debug.c |   28 ++++++++++++++++
 drivers/net/wireless/ath/ath10k/hw.h    |    5 +++
 drivers/net/wireless/ath/ath10k/pci.c   |   43 +++++++++++++++++++++++++
 5 files changed, 146 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 440c3ff03aec..39b7383b395c 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -479,6 +479,60 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name)
 			ar->otp_len = ie_len;
 
 			break;
+		case ATH10K_FW_IE_RAM_BSS_ADDR:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.ram_bss_addr = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found RAM BSS address 0x%x\n",
+				   ar->fw.ram_bss_addr);
+			break;
+		case ATH10K_FW_IE_RAM_BSS_LEN:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.ram_bss_len = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found RAM BSS length 0x%x\n",
+				   ar->fw.ram_bss_len);
+
+			if (ar->fw.ram_bss_len > ATH10K_RAM_BSS_BUF_LEN) {
+				ath10k_warn("too long firmware RAM BSS length: %d\n",
+					    ar->fw.ram_bss_len);
+				ar->fw.ram_bss_len = 0;
+			}
+
+			break;
+		case ATH10K_FW_IE_ROM_BSS_ADDR:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.rom_bss_addr = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found ROM BSS address 0x%x\n",
+				   ar->fw.rom_bss_addr);
+			break;
+		case ATH10K_FW_IE_ROM_BSS_LEN:
+			if (ie_len != sizeof(u32))
+				break;
+
+			ar->fw.rom_bss_len = le32_to_cpup((__le32 *)data);
+
+			ath10k_dbg(ATH10K_DBG_BOOT,
+				   "found ROM BSS length 0x%x\n",
+				   ar->fw.rom_bss_len);
+
+			if (ar->fw.ram_bss_len > ATH10K_RAM_BSS_BUF_LEN) {
+				ath10k_warn("too long firmware RAM BSS length: %d\n",
+					    ar->fw.ram_bss_len);
+				ar->fw.rom_bss_len = 0;
+			}
+
+			break;
 		default:
 			ath10k_warn("Unknown FW IE: %u\n",
 				    le32_to_cpu(hdr->id));
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index ffee2b5cdf78..0d457b9c38d1 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -293,6 +293,10 @@ struct ath10k_dbglog_entry_storage {
 	u8 data[ATH10K_DBGLOG_DATA_LEN];
 };
 
+/* estimated values, hopefully these are enough */
+#define ATH10K_ROM_BSS_BUF_LEN 10000
+#define ATH10K_RAM_BSS_BUF_LEN 30000
+
 /* used for crash-dump storage, protected by data-lock */
 struct ath10k_fw_crash_data {
 	bool crashed_since_read;
@@ -303,6 +307,9 @@ struct ath10k_fw_crash_data {
 	u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
 	u8 stack_buf[ATH10K_FW_STACK_SIZE];
 	u8 exc_stack_buf[ATH10K_FW_STACK_SIZE];
+
+	u8 rom_bss_buf[ATH10K_ROM_BSS_BUF_LEN];
+	u8 ram_bss_buf[ATH10K_RAM_BSS_BUF_LEN];
 };
 
 struct ath10k_debug {
@@ -428,6 +435,15 @@ struct ath10k {
 		} fw;
 	} hw_params;
 
+	/* These are written to only during first firmware load from user
+	 * space so no need for any locking. */
+	struct {
+		u32 ram_bss_addr;
+		u32 ram_bss_len;
+		u32 rom_bss_addr;
+		u32 rom_bss_len;
+	} fw;
+
 	const struct firmware *board;
 	const void *board_data;
 	size_t board_len;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 06e72e799883..a335d29f3528 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -33,12 +33,16 @@
  * @ATH10K_FW_CRASH_DUMP_REGDUMP: Register crash dump in binary format
  * @ATH10K_FW_CRASH_DUMP_STACK:   Stack memory contents.
  * @ATH10K_FW_CRASH_DUMP_EXC_STACK:  Exception stack contents
+ * @ATH10K_FW_CRASH_DUMP_RAM_BSS:  BSS area for RAM code
+ * @ATH10K_FW_CRASH_DUMP_ROM_BSS:  BSS area for ROM code
  */
 enum ath10k_fw_crash_dump_type {
 	ATH10K_FW_CRASH_DUMP_DBGLOG = 0,
 	ATH10K_FW_CRASH_DUMP_REGDUMP = 1,
 	ATH10K_FW_CRASH_DUMP_STACK = 2,
 	ATH10K_FW_CRASH_DUMP_EXC_STACK = 3,
+	ATH10K_FW_CRASH_DUMP_RAM_BSS = 4,
+	ATH10K_FW_CRASH_DUMP_ROM_BSS = 5,
 
 	ATH10K_FW_CRASH_DUMP_MAX,
 };
@@ -731,6 +735,12 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	len += sizeof(*dump_tlv) + sizeof(crash_data->stack_buf);
 	len += sizeof(*dump_tlv) + sizeof(crash_data->exc_stack_buf);
 
+	if (ar->fw.ram_bss_addr && ar->fw.ram_bss_len)
+		len += sizeof(*dump_tlv) + ar->fw.ram_bss_len;
+
+	if (ar->fw.rom_bss_addr && ar->fw.rom_bss_len)
+		len += sizeof(*dump_tlv) + ar->fw.rom_bss_len;
+
 	sofar += hdr_len;
 
 	/* This is going to get big when we start dumping FW RAM and such,
@@ -811,6 +821,24 @@ static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
 	       dump_tlv->tlv_len);
 	sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
 
+	if (ar->fw.ram_bss_addr && ar->fw.ram_bss_len) {
+		dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+		dump_tlv->type = ATH10K_FW_CRASH_DUMP_RAM_BSS;
+		dump_tlv->tlv_len = ar->fw.ram_bss_len;
+		memcpy(dump_tlv->tlv_data, crash_data->ram_bss_buf,
+		       dump_tlv->tlv_len);
+		sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+	}
+
+	if (ar->fw.rom_bss_addr && ar->fw.rom_bss_len) {
+		dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
+		dump_tlv->type = ATH10K_FW_CRASH_DUMP_ROM_BSS;
+		dump_tlv->tlv_len = ar->fw.rom_bss_len;
+		memcpy(dump_tlv->tlv_data, crash_data->rom_bss_buf,
+		       dump_tlv->tlv_len);
+		sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
+	}
+
 	spin_unlock_bh(&ar->data_lock);
 
 	WARN_ON(sofar != len);
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 9c602b0d7277..6c275638ddaa 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -54,6 +54,11 @@ enum ath10k_fw_ie_type {
 	ATH10K_FW_IE_FEATURES = 2,
 	ATH10K_FW_IE_FW_IMAGE = 3,
 	ATH10K_FW_IE_OTP_IMAGE = 4,
+	ATH10K_FW_IE_RAM_BSS_ADDR = 5,
+	ATH10K_FW_IE_RAM_BSS_LEN = 6,
+	ATH10K_FW_IE_ROM_BSS_ADDR = 7,
+	ATH10K_FW_IE_ROM_BSS_LEN = 8,
+
 };
 
 /* Known pecularities:
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7ffa2ebffc94..deebd30aa182 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -891,6 +891,47 @@ static void ath10k_pci_dump_exc_stack(struct ath10k *ar,
 				hi_err_stack, ATH10K_FW_STACK_SIZE);
 }
 
+static void ath10k_pci_dump_bss_ram(struct ath10k *ar,
+				    struct ath10k_fw_crash_data *crash_data) {
+	int ret;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	if (!ar->fw.ram_bss_addr)
+		return;
+
+	if (!ar->fw.ram_bss_len)
+		return;
+
+	ret = ath10k_pci_diag_read_mem(ar, ar->fw.ram_bss_addr,
+				       crash_data->ram_bss_buf,
+				       ar->fw.ram_bss_len);
+	if (ret)
+		ath10k_warn("failed to read firmware RAM BSS memory from %d (%d B): %d\n",
+			    ar->fw.ram_bss_addr, ar->fw.ram_bss_len, ret);
+}
+
+static void ath10k_pci_dump_bss_rom(struct ath10k *ar,
+				    struct ath10k_fw_crash_data *crash_data)
+{
+	int ret;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	if (!ar->fw.rom_bss_addr)
+		return;
+
+	if (!ar->fw.rom_bss_len)
+		return;
+
+	ret = ath10k_pci_diag_read_mem(ar, ar->fw.rom_bss_addr,
+				       crash_data->rom_bss_buf,
+				       ar->fw.rom_bss_len);
+	if (ret)
+		ath10k_warn("failed to read firmware ROM BSS memory from %d (%d B): %d\n",
+			    ar->fw.rom_bss_addr, ar->fw.rom_bss_len, ret);
+}
+
 static void ath10k_pci_dump_dbglog(struct ath10k *ar,
 				   struct ath10k_fw_crash_data *crash_data)
 {
@@ -1036,6 +1077,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 
 	ath10k_pci_dump_stack(ar, crash_data);
 	ath10k_pci_dump_exc_stack(ar, crash_data);
+	ath10k_pci_dump_bss_ram(ar, crash_data);
+	ath10k_pci_dump_bss_rom(ar, crash_data);
 	ath10k_pci_dump_registers(ar, crash_data);
 	ath10k_pci_dump_dbglog(ar, crash_data);
 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 7/8] ath10k: rename ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed()
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Better to have a clear name for the function. While at it, clear up the title
for the register dump.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index deebd30aa182..7fc2f02f53c4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1040,7 +1040,7 @@ static void ath10k_pci_dump_registers(struct ath10k *ar,
 
 	BUILD_BUG_ON(REG_DUMP_COUNT_QCA988X % 4);
 
-	ath10k_err("target Register Dump\n");
+	ath10k_err("firmware register dump:\n");
 	for (i = 0; i < REG_DUMP_COUNT_QCA988X; i += 4)
 		ath10k_err("[%02d]: 0x%08X 0x%08X 0x%08X 0x%08X\n",
 			   i,
@@ -1053,7 +1053,7 @@ static void ath10k_pci_dump_registers(struct ath10k *ar,
 	       sizeof(crash_data->reg_dump_values));
 }
 
-static void ath10k_pci_hif_dump_area(struct ath10k *ar)
+static void ath10k_pci_firmware_crashed(struct ath10k *ar)
 {
 	struct ath10k_fw_crash_data *crash_data;
 	char uuid[50];
@@ -2007,7 +2007,7 @@ static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
 				   fw_indicator & ~FW_IND_EVENT_PENDING);
 
 		if (ar_pci->started) {
-			ath10k_pci_hif_dump_area(ar);
+			ath10k_pci_firmware_crashed(ar);
 		} else {
 			/*
 			 * Probable Target failure before we're prepared
@@ -2458,7 +2458,7 @@ static void ath10k_pci_early_irq_tasklet(unsigned long data)
 	if (fw_ind & FW_IND_EVENT_PENDING) {
 		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
 				   fw_ind & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
+		ath10k_pci_firmware_crashed(ar);
 	}
 
 	ath10k_pci_sleep(ar);
@@ -2737,7 +2737,7 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 		ath10k_warn("device has crashed during init\n");
 		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
 				   val & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
+		ath10k_pci_firmware_crashed(ar);
 		ret = -ECOMM;
 		goto out;
 	}


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 7/8] ath10k: rename ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed()
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Better to have a clear name for the function. While at it, clear up the title
for the register dump.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index deebd30aa182..7fc2f02f53c4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1040,7 +1040,7 @@ static void ath10k_pci_dump_registers(struct ath10k *ar,
 
 	BUILD_BUG_ON(REG_DUMP_COUNT_QCA988X % 4);
 
-	ath10k_err("target Register Dump\n");
+	ath10k_err("firmware register dump:\n");
 	for (i = 0; i < REG_DUMP_COUNT_QCA988X; i += 4)
 		ath10k_err("[%02d]: 0x%08X 0x%08X 0x%08X 0x%08X\n",
 			   i,
@@ -1053,7 +1053,7 @@ static void ath10k_pci_dump_registers(struct ath10k *ar,
 	       sizeof(crash_data->reg_dump_values));
 }
 
-static void ath10k_pci_hif_dump_area(struct ath10k *ar)
+static void ath10k_pci_firmware_crashed(struct ath10k *ar)
 {
 	struct ath10k_fw_crash_data *crash_data;
 	char uuid[50];
@@ -2007,7 +2007,7 @@ static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
 				   fw_indicator & ~FW_IND_EVENT_PENDING);
 
 		if (ar_pci->started) {
-			ath10k_pci_hif_dump_area(ar);
+			ath10k_pci_firmware_crashed(ar);
 		} else {
 			/*
 			 * Probable Target failure before we're prepared
@@ -2458,7 +2458,7 @@ static void ath10k_pci_early_irq_tasklet(unsigned long data)
 	if (fw_ind & FW_IND_EVENT_PENDING) {
 		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
 				   fw_ind & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
+		ath10k_pci_firmware_crashed(ar);
 	}
 
 	ath10k_pci_sleep(ar);
@@ -2737,7 +2737,7 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
 		ath10k_warn("device has crashed during init\n");
 		ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
 				   val & ~FW_IND_EVENT_PENDING);
-		ath10k_pci_hif_dump_area(ar);
+		ath10k_pci_firmware_crashed(ar);
 		ret = -ECOMM;
 		goto out;
 	}


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 8/8] ath10k: print more driver info when firmware crashes
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 18:08   ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.c  |   17 ++---------------
 drivers/net/wireless/ath/ath10k/debug.c |   18 ++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |    1 +
 drivers/net/wireless/ath/ath10k/pci.c   |    4 +---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 39b7383b395c..a34066b85b65 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -870,21 +870,8 @@ int ath10k_core_start(struct ath10k *ar)
 
 	INIT_LIST_HEAD(&ar->arvifs);
 
-	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags)) {
-		ath10k_info("%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
-			    ar->hw_params.name,
-			    ar->target_version,
-			    ar->chip_id,
-			    ar->hw->wiphy->fw_version,
-			    ar->fw_api,
-			    ar->htt.target_version_major,
-			    ar->htt.target_version_minor);
-		ath10k_info("debug %d debugfs %d tracing %d dfs %d\n",
-			    config_enabled(CONFIG_ATH10K_DEBUG),
-			    config_enabled(CONFIG_ATH10K_DEBUGFS),
-			    config_enabled(CONFIG_ATH10K_TRACING),
-			    config_enabled(CONFIG_ATH10K_DFS_CERTIFIED));
-	}
+	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
+		ath10k_print_driver_info(ar);
 
 	__set_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags);
 
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a335d29f3528..96f85a542514 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -154,6 +154,24 @@ int ath10k_info(const char *fmt, ...)
 }
 EXPORT_SYMBOL(ath10k_info);
 
+void ath10k_print_driver_info(struct ath10k *ar)
+{
+	ath10k_info("%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
+		    ar->hw_params.name,
+		    ar->target_version,
+		    ar->chip_id,
+		    ar->hw->wiphy->fw_version,
+		    ar->fw_api,
+		    ar->htt.target_version_major,
+		    ar->htt.target_version_minor);
+	ath10k_info("debug %d debugfs %d tracing %d dfs %d\n",
+		    config_enabled(CONFIG_ATH10K_DEBUG),
+		    config_enabled(CONFIG_ATH10K_DEBUGFS),
+		    config_enabled(CONFIG_ATH10K_TRACING),
+		    config_enabled(CONFIG_ATH10K_DFS_CERTIFIED));
+}
+EXPORT_SYMBOL(ath10k_print_driver_info);
+
 int ath10k_err(const char *fmt, ...)
 {
 	struct va_format vaf = {
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 80ff14e4db9b..494044a89226 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -42,6 +42,7 @@ extern unsigned int ath10k_debug_mask;
 __printf(1, 2) int ath10k_info(const char *fmt, ...);
 __printf(1, 2) int ath10k_err(const char *fmt, ...);
 __printf(1, 2) int ath10k_warn(const char *fmt, ...);
+void ath10k_print_driver_info(struct ath10k *ar);
 
 #ifdef CONFIG_ATH10K_DEBUGFS
 int ath10k_debug_start(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7fc2f02f53c4..a17cb8d103be 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1068,9 +1068,7 @@ static void ath10k_pci_firmware_crashed(struct ath10k *ar)
 		scnprintf(uuid, sizeof(uuid), "n/a");
 
 	ath10k_err("firmware crashed! (uuid %s)\n", uuid);
-	ath10k_err("hardware name %s version 0x%x\n",
-		   ar->hw_params.name, ar->target_version);
-	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+	ath10k_print_driver_info(ar);
 
 	if (!crash_data)
 		goto exit;


^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH v6 8/8] ath10k: print more driver info when firmware crashes
@ 2014-08-09 18:08   ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-09 18:08 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.c  |   17 ++---------------
 drivers/net/wireless/ath/ath10k/debug.c |   18 ++++++++++++++++++
 drivers/net/wireless/ath/ath10k/debug.h |    1 +
 drivers/net/wireless/ath/ath10k/pci.c   |    4 +---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 39b7383b395c..a34066b85b65 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -870,21 +870,8 @@ int ath10k_core_start(struct ath10k *ar)
 
 	INIT_LIST_HEAD(&ar->arvifs);
 
-	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags)) {
-		ath10k_info("%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
-			    ar->hw_params.name,
-			    ar->target_version,
-			    ar->chip_id,
-			    ar->hw->wiphy->fw_version,
-			    ar->fw_api,
-			    ar->htt.target_version_major,
-			    ar->htt.target_version_minor);
-		ath10k_info("debug %d debugfs %d tracing %d dfs %d\n",
-			    config_enabled(CONFIG_ATH10K_DEBUG),
-			    config_enabled(CONFIG_ATH10K_DEBUGFS),
-			    config_enabled(CONFIG_ATH10K_TRACING),
-			    config_enabled(CONFIG_ATH10K_DFS_CERTIFIED));
-	}
+	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
+		ath10k_print_driver_info(ar);
 
 	__set_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags);
 
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a335d29f3528..96f85a542514 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -154,6 +154,24 @@ int ath10k_info(const char *fmt, ...)
 }
 EXPORT_SYMBOL(ath10k_info);
 
+void ath10k_print_driver_info(struct ath10k *ar)
+{
+	ath10k_info("%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
+		    ar->hw_params.name,
+		    ar->target_version,
+		    ar->chip_id,
+		    ar->hw->wiphy->fw_version,
+		    ar->fw_api,
+		    ar->htt.target_version_major,
+		    ar->htt.target_version_minor);
+	ath10k_info("debug %d debugfs %d tracing %d dfs %d\n",
+		    config_enabled(CONFIG_ATH10K_DEBUG),
+		    config_enabled(CONFIG_ATH10K_DEBUGFS),
+		    config_enabled(CONFIG_ATH10K_TRACING),
+		    config_enabled(CONFIG_ATH10K_DFS_CERTIFIED));
+}
+EXPORT_SYMBOL(ath10k_print_driver_info);
+
 int ath10k_err(const char *fmt, ...)
 {
 	struct va_format vaf = {
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 80ff14e4db9b..494044a89226 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -42,6 +42,7 @@ extern unsigned int ath10k_debug_mask;
 __printf(1, 2) int ath10k_info(const char *fmt, ...);
 __printf(1, 2) int ath10k_err(const char *fmt, ...);
 __printf(1, 2) int ath10k_warn(const char *fmt, ...);
+void ath10k_print_driver_info(struct ath10k *ar);
 
 #ifdef CONFIG_ATH10K_DEBUGFS
 int ath10k_debug_start(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7fc2f02f53c4..a17cb8d103be 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1068,9 +1068,7 @@ static void ath10k_pci_firmware_crashed(struct ath10k *ar)
 		scnprintf(uuid, sizeof(uuid), "n/a");
 
 	ath10k_err("firmware crashed! (uuid %s)\n", uuid);
-	ath10k_err("hardware name %s version 0x%x\n",
-		   ar->hw_params.name, ar->target_version);
-	ath10k_err("firmware version: %s\n", ar->hw->wiphy->fw_version);
+	ath10k_print_driver_info(ar);
 
 	if (!crash_data)
 		goto exit;


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 0/8] ath10k: firmware crash dump
  2014-08-09 18:07 ` Kalle Valo
@ 2014-08-09 20:50   ` Ben Greear
  -1 siblings, 0 replies; 28+ messages in thread
From: Ben Greear @ 2014-08-09 20:50 UTC (permalink / raw)
  To: Kalle Valo, ath10k; +Cc: linux-wireless



On 08/09/2014 11:07 AM, Kalle Valo wrote:
> Next version of ath10k firmware crash dump feature, based on Ben's patches but
> heavily modified.
>
> v6:

I see no obvious problems.  If it passes basic testing, poke it upstream
and I'll start making my decode tool able to parse it (and package my CT
firmware with the BSS addrs/lengths..it is currently using a slightly
different IE format.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 0/8] ath10k: firmware crash dump
@ 2014-08-09 20:50   ` Ben Greear
  0 siblings, 0 replies; 28+ messages in thread
From: Ben Greear @ 2014-08-09 20:50 UTC (permalink / raw)
  To: Kalle Valo, ath10k; +Cc: linux-wireless



On 08/09/2014 11:07 AM, Kalle Valo wrote:
> Next version of ath10k firmware crash dump feature, based on Ben's patches but
> heavily modified.
>
> v6:

I see no obvious problems.  If it passes basic testing, poke it upstream
and I'll start making my decode tool able to parse it (and package my CT
firmware with the BSS addrs/lengths..it is currently using a slightly
different IE format.

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] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
  2014-08-09 18:08   ` Kalle Valo
@ 2014-08-18  8:54     ` Michal Kazior
  -1 siblings, 0 replies; 28+ messages in thread
From: Michal Kazior @ 2014-08-18  8:54 UTC (permalink / raw)
  To: Kalle Valo; +Cc: ath10k, linux-wireless

On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>  6 files changed, 478 insertions(+), 10 deletions(-)
[...]

> +struct ath10k_dump_file_data {
> +       /* dump file information */
> +
> +       /* "ATH10K-FW-DUMP" */
> +       char df_magic[16];
> +
> +       u32 len;
> +
> +       /* 0x1 if host is big-endian */
> +       u32 big_endian;

This isn't entirely correct. Depending on host endianess you'll end up
with 0x1 or 0x1000000. This will still work if you do a boolean
evaluation of it in userspace or compare it to 0, but god forbid to
compare it with 0x1.


> +
> +       /* file dump version, 1 for now. */
> +       u32 version;

I think this should have a #define instead of the comment. You'll need
to update 2 values when you bump the version with comment+hardcode
approach.

> +
> +       /* some info we can get from ath10k struct that might help */
> +
> +       u8 uuid[16];
> +
> +       u32 chip_id;
> +
> +       /* 0 for now, in place for later hardware */
> +       u32 bus_type;

Maybe we should have an enum for that instead of using a hardcoded 0?


> +       /* time-of-day stamp, nano-seconds */
> +       u64 tv_nsec;
> +
> +
> +       /* LINUX_VERSION_CODE */
> +       u32 kernel_ver_code;

2 empty newlines?


> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> +{
> +       struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
> +       struct ath10k_dump_file_data *dump_data;
> +       struct ath10k_tlv_dump_data *dump_tlv;
> +       int hdr_len = sizeof(*dump_data);
> +       unsigned int len, sofar = 0;
> +       unsigned char *buf;
> +
> +       lockdep_assert_held(&ar->conf_mutex);
> +
> +       spin_lock_bh(&ar->data_lock);
> +
> +       if (!crash_data->crashed_since_read) {
> +               spin_unlock_bh(&ar->data_lock);
> +               return NULL;
> +       }
> +
> +       spin_unlock_bh(&ar->data_lock);
> +
> +       len = hdr_len;
> +       len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
> +       len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
> +
> +       sofar += hdr_len;
> +
> +       /* 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;
> +
> +       spin_lock_bh(&ar->data_lock);

The current code doesn't seem to allow it, but according to comments
crashed_since_read is protected by data_lock only. As such it might've
changed while the lock was released.

Current code, however, guarantees it remains true while conf_mutex is held.

Perhaps the vmalloc() should be done before spin_lock is acquired
and/or the memory should be allocated outside this function completely
and make it consume the crashed_since_read (i.e. set it to false) once
it's done (while the data_lock is still held).


> +
> +       memset(buf, 0, len);
> +       dump_data = (struct ath10k_dump_file_data *)(buf);
> +       strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
> +               sizeof(dump_data->df_magic));
> +       dump_data->len = len;
> +
> +#ifdef __BIG_ENDIAN
> +       dump_data->big_endian = 1;
> +#else
> +       dump_data->big_endian = 0;
> +#endif

Yuck.


> +static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
> +{
> +       struct ath10k *ar = inode->i_private;
> +       struct ath10k_dump_file_data *dump;
> +       int ret;
> +
> +       mutex_lock(&ar->conf_mutex);
> +
> +       dump = ath10k_build_dump_file(ar);
> +       if (!dump) {
> +               ret = -ENODATA;
> +               goto out;
> +       }
> +
> +       file->private_data = dump;

> +       ar->debug.fw_crash_data->crashed_since_read = false;

According to comments this should be protected by data_lock, but isn't.

> +       ret = 0;
> +
> +out:
> +       mutex_unlock(&ar->conf_mutex);
> +       return ret;
> +}


>  static int ath10k_debug_htt_stats_req(struct ath10k *ar)
>  {
>         u64 cookie;
> @@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
>
>  int ath10k_debug_create(struct ath10k *ar)
>  {
> +       ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
> +       if (!ar->debug.fw_crash_data)
> +               return -ENOMEM;
> +
>         ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
>                                                    ar->hw->wiphy->debugfsdir);

I think there's a check if debug_phy is NULL. If it is it does return
-ENOMEM. This means you leak fw_crash_data.


> +/* 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 ath10k_fw_dbglog_buf {
> +       /* pointer to dblog_buf_s */
> +       u32 next;
> +
> +       /* pointer to u8 buffer */
> +       u32 buffer;
> +
> +       u32 bufsize;
> +       u32 length;
> +       u32 count;
> +       u32 free;
> +} __packed;
> +
> +struct ath10k_fw_dbglog_hdr {
> +       /* pointer to dbglog_buf_s */
> +       u32 dbuf;
> +
> +       u32 dropped;
> +} __packed;
This is confusing.

Target is a 32-bit *Little-Endian* CPU but due to implicit byteswap in
ath10k_pci_diag_* functions everything is already in host endianess.


Michał

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
@ 2014-08-18  8:54     ` Michal Kazior
  0 siblings, 0 replies; 28+ messages in thread
From: Michal Kazior @ 2014-08-18  8:54 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, ath10k

On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>  6 files changed, 478 insertions(+), 10 deletions(-)
[...]

> +struct ath10k_dump_file_data {
> +       /* dump file information */
> +
> +       /* "ATH10K-FW-DUMP" */
> +       char df_magic[16];
> +
> +       u32 len;
> +
> +       /* 0x1 if host is big-endian */
> +       u32 big_endian;

This isn't entirely correct. Depending on host endianess you'll end up
with 0x1 or 0x1000000. This will still work if you do a boolean
evaluation of it in userspace or compare it to 0, but god forbid to
compare it with 0x1.


> +
> +       /* file dump version, 1 for now. */
> +       u32 version;

I think this should have a #define instead of the comment. You'll need
to update 2 values when you bump the version with comment+hardcode
approach.

> +
> +       /* some info we can get from ath10k struct that might help */
> +
> +       u8 uuid[16];
> +
> +       u32 chip_id;
> +
> +       /* 0 for now, in place for later hardware */
> +       u32 bus_type;

Maybe we should have an enum for that instead of using a hardcoded 0?


> +       /* time-of-day stamp, nano-seconds */
> +       u64 tv_nsec;
> +
> +
> +       /* LINUX_VERSION_CODE */
> +       u32 kernel_ver_code;

2 empty newlines?


> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> +{
> +       struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
> +       struct ath10k_dump_file_data *dump_data;
> +       struct ath10k_tlv_dump_data *dump_tlv;
> +       int hdr_len = sizeof(*dump_data);
> +       unsigned int len, sofar = 0;
> +       unsigned char *buf;
> +
> +       lockdep_assert_held(&ar->conf_mutex);
> +
> +       spin_lock_bh(&ar->data_lock);
> +
> +       if (!crash_data->crashed_since_read) {
> +               spin_unlock_bh(&ar->data_lock);
> +               return NULL;
> +       }
> +
> +       spin_unlock_bh(&ar->data_lock);
> +
> +       len = hdr_len;
> +       len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
> +       len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
> +
> +       sofar += hdr_len;
> +
> +       /* 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;
> +
> +       spin_lock_bh(&ar->data_lock);

The current code doesn't seem to allow it, but according to comments
crashed_since_read is protected by data_lock only. As such it might've
changed while the lock was released.

Current code, however, guarantees it remains true while conf_mutex is held.

Perhaps the vmalloc() should be done before spin_lock is acquired
and/or the memory should be allocated outside this function completely
and make it consume the crashed_since_read (i.e. set it to false) once
it's done (while the data_lock is still held).


> +
> +       memset(buf, 0, len);
> +       dump_data = (struct ath10k_dump_file_data *)(buf);
> +       strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
> +               sizeof(dump_data->df_magic));
> +       dump_data->len = len;
> +
> +#ifdef __BIG_ENDIAN
> +       dump_data->big_endian = 1;
> +#else
> +       dump_data->big_endian = 0;
> +#endif

Yuck.


> +static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
> +{
> +       struct ath10k *ar = inode->i_private;
> +       struct ath10k_dump_file_data *dump;
> +       int ret;
> +
> +       mutex_lock(&ar->conf_mutex);
> +
> +       dump = ath10k_build_dump_file(ar);
> +       if (!dump) {
> +               ret = -ENODATA;
> +               goto out;
> +       }
> +
> +       file->private_data = dump;

> +       ar->debug.fw_crash_data->crashed_since_read = false;

According to comments this should be protected by data_lock, but isn't.

> +       ret = 0;
> +
> +out:
> +       mutex_unlock(&ar->conf_mutex);
> +       return ret;
> +}


>  static int ath10k_debug_htt_stats_req(struct ath10k *ar)
>  {
>         u64 cookie;
> @@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
>
>  int ath10k_debug_create(struct ath10k *ar)
>  {
> +       ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
> +       if (!ar->debug.fw_crash_data)
> +               return -ENOMEM;
> +
>         ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
>                                                    ar->hw->wiphy->debugfsdir);

I think there's a check if debug_phy is NULL. If it is it does return
-ENOMEM. This means you leak fw_crash_data.


> +/* 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 ath10k_fw_dbglog_buf {
> +       /* pointer to dblog_buf_s */
> +       u32 next;
> +
> +       /* pointer to u8 buffer */
> +       u32 buffer;
> +
> +       u32 bufsize;
> +       u32 length;
> +       u32 count;
> +       u32 free;
> +} __packed;
> +
> +struct ath10k_fw_dbglog_hdr {
> +       /* pointer to dbglog_buf_s */
> +       u32 dbuf;
> +
> +       u32 dropped;
> +} __packed;
This is confusing.

Target is a 32-bit *Little-Endian* CPU but due to implicit byteswap in
ath10k_pci_diag_* functions everything is already in host endianess.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
  2014-08-18  8:54     ` Michal Kazior
@ 2014-08-18 11:39       ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-18 11:39 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless

Michal Kazior <michal.kazior@tieto.com> writes:

> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>>  6 files changed, 478 insertions(+), 10 deletions(-)
> [...]
>
>> +struct ath10k_dump_file_data {
>> +       /* dump file information */
>> +
>> +       /* "ATH10K-FW-DUMP" */
>> +       char df_magic[16];
>> +
>> +       u32 len;
>> +
>> +       /* 0x1 if host is big-endian */
>> +       u32 big_endian;
>
> This isn't entirely correct. Depending on host endianess you'll end up
> with 0x1 or 0x1000000. This will still work if you do a boolean
> evaluation of it in userspace or compare it to 0, but god forbid to
> compare it with 0x1.

That's true. Didn't you at one point suggest just always using little
endian? I think that's simplest approach here.

>> +
>> +       /* file dump version, 1 for now. */
>> +       u32 version;
>
> I think this should have a #define instead of the comment. You'll need
> to update 2 values when you bump the version with comment+hardcode
> approach.

Good point, I'll add that.

>> +       /* some info we can get from ath10k struct that might help */
>> +
>> +       u8 uuid[16];
>> +
>> +       u32 chip_id;
>> +
>> +       /* 0 for now, in place for later hardware */
>> +       u32 bus_type;
>
> Maybe we should have an enum for that instead of using a hardcoded 0?

We had that but you removed it in 3a0861fffd223 =)

>> +       /* time-of-day stamp, nano-seconds */
>> +       u64 tv_nsec;
>> +
>> +
>> +       /* LINUX_VERSION_CODE */
>> +       u32 kernel_ver_code;
>
> 2 empty newlines?

Will fix.

>> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
>> +{
>> +       struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
>> +       struct ath10k_dump_file_data *dump_data;
>> +       struct ath10k_tlv_dump_data *dump_tlv;
>> +       int hdr_len = sizeof(*dump_data);
>> +       unsigned int len, sofar = 0;
>> +       unsigned char *buf;
>> +
>> +       lockdep_assert_held(&ar->conf_mutex);
>> +
>> +       spin_lock_bh(&ar->data_lock);
>> +
>> +       if (!crash_data->crashed_since_read) {
>> +               spin_unlock_bh(&ar->data_lock);
>> +               return NULL;
>> +       }
>> +
>> +       spin_unlock_bh(&ar->data_lock);
>> +
>> +       len = hdr_len;
>> +       len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
>> +       len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
>> +
>> +       sofar += hdr_len;
>> +
>> +       /* 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;
>> +
>> +       spin_lock_bh(&ar->data_lock);
>
> The current code doesn't seem to allow it, but according to comments
> crashed_since_read is protected by data_lock only. As such it might've
> changed while the lock was released.

Another good point.

> Current code, however, guarantees it remains true while conf_mutex is
> held.
>
> Perhaps the vmalloc() should be done before spin_lock is acquired

That sounds the simple way to do it. We get a useless allocation when
there's no crash data, but that's so bad.

> and/or the memory should be allocated outside this function completely
> and make it consume the crashed_since_read (i.e. set it to false) once
> it's done (while the data_lock is still held).

You mean like allocating the memory in advance, for example during
module probe time? Then we would have a big chunk of memory we don't use
for anything most of the time.

>> +       memset(buf, 0, len);
>> +       dump_data = (struct ath10k_dump_file_data *)(buf);
>> +       strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
>> +               sizeof(dump_data->df_magic));
>> +       dump_data->len = len;
>> +
>> +#ifdef __BIG_ENDIAN
>> +       dump_data->big_endian = 1;
>> +#else
>> +       dump_data->big_endian = 0;
>> +#endif
>
> Yuck.

Yeah. I'm thinking of switching to little endian more and more :)

>> +static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
>> +{
>> +       struct ath10k *ar = inode->i_private;
>> +       struct ath10k_dump_file_data *dump;
>> +       int ret;
>> +
>> +       mutex_lock(&ar->conf_mutex);
>> +
>> +       dump = ath10k_build_dump_file(ar);
>> +       if (!dump) {
>> +               ret = -ENODATA;
>> +               goto out;
>> +       }
>> +
>> +       file->private_data = dump;
>
>> +       ar->debug.fw_crash_data->crashed_since_read = false;
>
> According to comments this should be protected by data_lock, but
> isn't.

Yup. I'll move crashed_since_read handling to ath10k_build_dump_file().

>
>> +       ret = 0;
>> +
>> +out:
>> +       mutex_unlock(&ar->conf_mutex);
>> +       return ret;
>> +}
>
>
>>  static int ath10k_debug_htt_stats_req(struct ath10k *ar)
>>  {
>>         u64 cookie;
>> @@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
>>
>>  int ath10k_debug_create(struct ath10k *ar)
>>  {
>> +       ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>> +       if (!ar->debug.fw_crash_data)
>> +               return -ENOMEM;
>> +
>>         ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
>>                                                    ar->hw->wiphy->debugfsdir);
>
> I think there's a check if debug_phy is NULL. If it is it does return
> -ENOMEM. This means you leak fw_crash_data.

Indeed. I'll fix that.

>> +/* 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 ath10k_fw_dbglog_buf {
>> +       /* pointer to dblog_buf_s */
>> +       u32 next;
>> +
>> +       /* pointer to u8 buffer */
>> +       u32 buffer;
>> +
>> +       u32 bufsize;
>> +       u32 length;
>> +       u32 count;
>> +       u32 free;
>> +} __packed;
>> +
>> +struct ath10k_fw_dbglog_hdr {
>> +       /* pointer to dbglog_buf_s */
>> +       u32 dbuf;
>> +
>> +       u32 dropped;
>> +} __packed;
>
> This is confusing.

Sorry, what are referring to here? I don't understand your comment.

> Target is a 32-bit *Little-Endian* CPU but due to implicit byteswap in
> ath10k_pci_diag_* functions everything is already in host endianess.

I think I'll that as a comment here.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
@ 2014-08-18 11:39       ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-18 11:39 UTC (permalink / raw)
  To: Michal Kazior; +Cc: linux-wireless, ath10k

Michal Kazior <michal.kazior@tieto.com> writes:

> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>>  6 files changed, 478 insertions(+), 10 deletions(-)
> [...]
>
>> +struct ath10k_dump_file_data {
>> +       /* dump file information */
>> +
>> +       /* "ATH10K-FW-DUMP" */
>> +       char df_magic[16];
>> +
>> +       u32 len;
>> +
>> +       /* 0x1 if host is big-endian */
>> +       u32 big_endian;
>
> This isn't entirely correct. Depending on host endianess you'll end up
> with 0x1 or 0x1000000. This will still work if you do a boolean
> evaluation of it in userspace or compare it to 0, but god forbid to
> compare it with 0x1.

That's true. Didn't you at one point suggest just always using little
endian? I think that's simplest approach here.

>> +
>> +       /* file dump version, 1 for now. */
>> +       u32 version;
>
> I think this should have a #define instead of the comment. You'll need
> to update 2 values when you bump the version with comment+hardcode
> approach.

Good point, I'll add that.

>> +       /* some info we can get from ath10k struct that might help */
>> +
>> +       u8 uuid[16];
>> +
>> +       u32 chip_id;
>> +
>> +       /* 0 for now, in place for later hardware */
>> +       u32 bus_type;
>
> Maybe we should have an enum for that instead of using a hardcoded 0?

We had that but you removed it in 3a0861fffd223 =)

>> +       /* time-of-day stamp, nano-seconds */
>> +       u64 tv_nsec;
>> +
>> +
>> +       /* LINUX_VERSION_CODE */
>> +       u32 kernel_ver_code;
>
> 2 empty newlines?

Will fix.

>> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
>> +{
>> +       struct ath10k_fw_crash_data *crash_data = ar->debug.fw_crash_data;
>> +       struct ath10k_dump_file_data *dump_data;
>> +       struct ath10k_tlv_dump_data *dump_tlv;
>> +       int hdr_len = sizeof(*dump_data);
>> +       unsigned int len, sofar = 0;
>> +       unsigned char *buf;
>> +
>> +       lockdep_assert_held(&ar->conf_mutex);
>> +
>> +       spin_lock_bh(&ar->data_lock);
>> +
>> +       if (!crash_data->crashed_since_read) {
>> +               spin_unlock_bh(&ar->data_lock);
>> +               return NULL;
>> +       }
>> +
>> +       spin_unlock_bh(&ar->data_lock);
>> +
>> +       len = hdr_len;
>> +       len += sizeof(*dump_tlv) + sizeof(crash_data->reg_dump_values);
>> +       len += sizeof(*dump_tlv) + sizeof(crash_data->dbglog_entry_data);
>> +
>> +       sofar += hdr_len;
>> +
>> +       /* 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;
>> +
>> +       spin_lock_bh(&ar->data_lock);
>
> The current code doesn't seem to allow it, but according to comments
> crashed_since_read is protected by data_lock only. As such it might've
> changed while the lock was released.

Another good point.

> Current code, however, guarantees it remains true while conf_mutex is
> held.
>
> Perhaps the vmalloc() should be done before spin_lock is acquired

That sounds the simple way to do it. We get a useless allocation when
there's no crash data, but that's so bad.

> and/or the memory should be allocated outside this function completely
> and make it consume the crashed_since_read (i.e. set it to false) once
> it's done (while the data_lock is still held).

You mean like allocating the memory in advance, for example during
module probe time? Then we would have a big chunk of memory we don't use
for anything most of the time.

>> +       memset(buf, 0, len);
>> +       dump_data = (struct ath10k_dump_file_data *)(buf);
>> +       strlcpy(dump_data->df_magic, "ATH10K-FW-DUMP",
>> +               sizeof(dump_data->df_magic));
>> +       dump_data->len = len;
>> +
>> +#ifdef __BIG_ENDIAN
>> +       dump_data->big_endian = 1;
>> +#else
>> +       dump_data->big_endian = 0;
>> +#endif
>
> Yuck.

Yeah. I'm thinking of switching to little endian more and more :)

>> +static int ath10k_fw_crash_dump_open(struct inode *inode, struct file *file)
>> +{
>> +       struct ath10k *ar = inode->i_private;
>> +       struct ath10k_dump_file_data *dump;
>> +       int ret;
>> +
>> +       mutex_lock(&ar->conf_mutex);
>> +
>> +       dump = ath10k_build_dump_file(ar);
>> +       if (!dump) {
>> +               ret = -ENODATA;
>> +               goto out;
>> +       }
>> +
>> +       file->private_data = dump;
>
>> +       ar->debug.fw_crash_data->crashed_since_read = false;
>
> According to comments this should be protected by data_lock, but
> isn't.

Yup. I'll move crashed_since_read handling to ath10k_build_dump_file().

>
>> +       ret = 0;
>> +
>> +out:
>> +       mutex_unlock(&ar->conf_mutex);
>> +       return ret;
>> +}
>
>
>>  static int ath10k_debug_htt_stats_req(struct ath10k *ar)
>>  {
>>         u64 cookie;
>> @@ -913,6 +1178,10 @@ static const struct file_operations fops_dfs_stats = {
>>
>>  int ath10k_debug_create(struct ath10k *ar)
>>  {
>> +       ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>> +       if (!ar->debug.fw_crash_data)
>> +               return -ENOMEM;
>> +
>>         ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
>>                                                    ar->hw->wiphy->debugfsdir);
>
> I think there's a check if debug_phy is NULL. If it is it does return
> -ENOMEM. This means you leak fw_crash_data.

Indeed. I'll fix that.

>> +/* 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 ath10k_fw_dbglog_buf {
>> +       /* pointer to dblog_buf_s */
>> +       u32 next;
>> +
>> +       /* pointer to u8 buffer */
>> +       u32 buffer;
>> +
>> +       u32 bufsize;
>> +       u32 length;
>> +       u32 count;
>> +       u32 free;
>> +} __packed;
>> +
>> +struct ath10k_fw_dbglog_hdr {
>> +       /* pointer to dbglog_buf_s */
>> +       u32 dbuf;
>> +
>> +       u32 dropped;
>> +} __packed;
>
> This is confusing.

Sorry, what are referring to here? I don't understand your comment.

> Target is a 32-bit *Little-Endian* CPU but due to implicit byteswap in
> ath10k_pci_diag_* functions everything is already in host endianess.

I think I'll that as a comment here.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
  2014-08-18 11:39       ` Kalle Valo
@ 2014-08-18 12:36         ` Michal Kazior
  -1 siblings, 0 replies; 28+ messages in thread
From: Michal Kazior @ 2014-08-18 12:36 UTC (permalink / raw)
  To: Kalle Valo; +Cc: ath10k, linux-wireless

On 18 August 2014 13:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
>>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>>> ---
>>>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>>>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>>>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>>>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>>>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>>>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>>>  6 files changed, 478 insertions(+), 10 deletions(-)
>> [...]
>>
>>> +struct ath10k_dump_file_data {
>>> +       /* dump file information */
>>> +
>>> +       /* "ATH10K-FW-DUMP" */
>>> +       char df_magic[16];
>>> +
>>> +       u32 len;
>>> +
>>> +       /* 0x1 if host is big-endian */
>>> +       u32 big_endian;
>>
>> This isn't entirely correct. Depending on host endianess you'll end up
>> with 0x1 or 0x1000000. This will still work if you do a boolean
>> evaluation of it in userspace or compare it to 0, but god forbid to
>> compare it with 0x1.
>
> That's true. Didn't you at one point suggest just always using little
> endian? I think that's simplest approach here.

Yes. I did point that out at some time ago.


>>> +       /* some info we can get from ath10k struct that might help */
>>> +
>>> +       u8 uuid[16];
>>> +
>>> +       u32 chip_id;
>>> +
>>> +       /* 0 for now, in place for later hardware */
>>> +       u32 bus_type;
>>
>> Maybe we should have an enum for that instead of using a hardcoded 0?
>
> We had that but you removed it in 3a0861fffd223 =)

.. right :-) I suppose we could just remove the bus_type then? We do
have an unused[128] for future expansion, don't we?


>> and/or the memory should be allocated outside this function completely
>> and make it consume the crashed_since_read (i.e. set it to false) once
>> it's done (while the data_lock is still held).
>
> You mean like allocating the memory in advance, for example during
> module probe time? Then we would have a big chunk of memory we don't use
> for anything most of the time.

I meant just moving it up in the call stack, e.g. to
ath10k_fw_crash_dump_open().


Michał

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
@ 2014-08-18 12:36         ` Michal Kazior
  0 siblings, 0 replies; 28+ messages in thread
From: Michal Kazior @ 2014-08-18 12:36 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, ath10k

On 18 August 2014 13:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.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>
>>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>>> ---
>>>  drivers/net/wireless/ath/ath10k/core.h  |   27 +++
>>>  drivers/net/wireless/ath/ath10k/debug.c |  273 +++++++++++++++++++++++++++++++
>>>  drivers/net/wireless/ath/ath10k/debug.h |   22 ++
>>>  drivers/net/wireless/ath/ath10k/hw.h    |   30 +++
>>>  drivers/net/wireless/ath/ath10k/pci.c   |  133 ++++++++++++++-
>>>  drivers/net/wireless/ath/ath10k/pci.h   |    3
>>>  6 files changed, 478 insertions(+), 10 deletions(-)
>> [...]
>>
>>> +struct ath10k_dump_file_data {
>>> +       /* dump file information */
>>> +
>>> +       /* "ATH10K-FW-DUMP" */
>>> +       char df_magic[16];
>>> +
>>> +       u32 len;
>>> +
>>> +       /* 0x1 if host is big-endian */
>>> +       u32 big_endian;
>>
>> This isn't entirely correct. Depending on host endianess you'll end up
>> with 0x1 or 0x1000000. This will still work if you do a boolean
>> evaluation of it in userspace or compare it to 0, but god forbid to
>> compare it with 0x1.
>
> That's true. Didn't you at one point suggest just always using little
> endian? I think that's simplest approach here.

Yes. I did point that out at some time ago.


>>> +       /* some info we can get from ath10k struct that might help */
>>> +
>>> +       u8 uuid[16];
>>> +
>>> +       u32 chip_id;
>>> +
>>> +       /* 0 for now, in place for later hardware */
>>> +       u32 bus_type;
>>
>> Maybe we should have an enum for that instead of using a hardcoded 0?
>
> We had that but you removed it in 3a0861fffd223 =)

.. right :-) I suppose we could just remove the bus_type then? We do
have an unused[128] for future expansion, don't we?


>> and/or the memory should be allocated outside this function completely
>> and make it consume the crashed_since_read (i.e. set it to false) once
>> it's done (while the data_lock is still held).
>
> You mean like allocating the memory in advance, for example during
> module probe time? Then we would have a big chunk of memory we don't use
> for anything most of the time.

I meant just moving it up in the call stack, e.g. to
ath10k_fw_crash_dump_open().


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
  2014-08-18 12:36         ` Michal Kazior
@ 2014-08-18 12:53           ` Kalle Valo
  -1 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-18 12:53 UTC (permalink / raw)
  To: Michal Kazior; +Cc: ath10k, linux-wireless

Michal Kazior <michal.kazior@tieto.com> writes:

> On 18 August 2014 13:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>>
>>>> +struct ath10k_dump_file_data {
>>>> +       /* dump file information */
>>>> +
>>>> +       /* "ATH10K-FW-DUMP" */
>>>> +       char df_magic[16];
>>>> +
>>>> +       u32 len;
>>>> +
>>>> +       /* 0x1 if host is big-endian */
>>>> +       u32 big_endian;
>>>
>>> This isn't entirely correct. Depending on host endianess you'll end up
>>> with 0x1 or 0x1000000. This will still work if you do a boolean
>>> evaluation of it in userspace or compare it to 0, but god forbid to
>>> compare it with 0x1.
>>
>> That's true. Didn't you at one point suggest just always using little
>> endian? I think that's simplest approach here.
>
> Yes. I did point that out at some time ago.

Ok. I started converting to use little endian already.

>>>> +       /* some info we can get from ath10k struct that might help */
>>>> +
>>>> +       u8 uuid[16];
>>>> +
>>>> +       u32 chip_id;
>>>> +
>>>> +       /* 0 for now, in place for later hardware */
>>>> +       u32 bus_type;
>>>
>>> Maybe we should have an enum for that instead of using a hardcoded 0?
>>
>> We had that but you removed it in 3a0861fffd223 =)
>
> .. right :-) 

Sorry, I couldn't resist :)

> I suppose we could just remove the bus_type then? We do have an
> unused[128] for future expansion, don't we?

We could, but then we would have to modify the crash dump tools. I would
rather be prepared for this, it's only a u32 anyway. If we have to do
something, I would prefer to get back the enum.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs
@ 2014-08-18 12:53           ` Kalle Valo
  0 siblings, 0 replies; 28+ messages in thread
From: Kalle Valo @ 2014-08-18 12:53 UTC (permalink / raw)
  To: Michal Kazior; +Cc: linux-wireless, ath10k

Michal Kazior <michal.kazior@tieto.com> writes:

> On 18 August 2014 13:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> On 9 August 2014 20:08, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>>
>>>> +struct ath10k_dump_file_data {
>>>> +       /* dump file information */
>>>> +
>>>> +       /* "ATH10K-FW-DUMP" */
>>>> +       char df_magic[16];
>>>> +
>>>> +       u32 len;
>>>> +
>>>> +       /* 0x1 if host is big-endian */
>>>> +       u32 big_endian;
>>>
>>> This isn't entirely correct. Depending on host endianess you'll end up
>>> with 0x1 or 0x1000000. This will still work if you do a boolean
>>> evaluation of it in userspace or compare it to 0, but god forbid to
>>> compare it with 0x1.
>>
>> That's true. Didn't you at one point suggest just always using little
>> endian? I think that's simplest approach here.
>
> Yes. I did point that out at some time ago.

Ok. I started converting to use little endian already.

>>>> +       /* some info we can get from ath10k struct that might help */
>>>> +
>>>> +       u8 uuid[16];
>>>> +
>>>> +       u32 chip_id;
>>>> +
>>>> +       /* 0 for now, in place for later hardware */
>>>> +       u32 bus_type;
>>>
>>> Maybe we should have an enum for that instead of using a hardcoded 0?
>>
>> We had that but you removed it in 3a0861fffd223 =)
>
> .. right :-) 

Sorry, I couldn't resist :)

> I suppose we could just remove the bus_type then? We do have an
> unused[128] for future expansion, don't we?

We could, but then we would have to modify the crash dump tools. I would
rather be prepared for this, it's only a u32 anyway. If we have to do
something, I would prefer to get back the enum.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 28+ messages in thread
end of thread, other threads:[~2014-08-18 12:54 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-09 18:07 [PATCH v6 0/8] ath10k: firmware crash dump Kalle Valo
2014-08-09 18:07 ` Kalle Valo
2014-08-09 18:07 ` [PATCH v6 1/8] ath10k: add ath10k_pci_diag_* helpers Kalle Valo
2014-08-09 18:07   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 2/8] ath10k: provide firmware crash info via debugfs Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-18  8:54   ` Michal Kazior
2014-08-18  8:54     ` Michal Kazior
2014-08-18 11:39     ` Kalle Valo
2014-08-18 11:39       ` Kalle Valo
2014-08-18 12:36       ` Michal Kazior
2014-08-18 12:36         ` Michal Kazior
2014-08-18 12:53         ` Kalle Valo
2014-08-18 12:53           ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 3/8] ath10k: save firmware debug log messages Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 4/8] ath10k: save firmware stack upon firmware crash Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 5/8] ath10k: dump exception stack contents on " Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 6/8] ath10k: save firmware RAM and ROM BSS sections on crash Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 7/8] ath10k: rename ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed() Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 18:08 ` [PATCH v6 8/8] ath10k: print more driver info when firmware crashes Kalle Valo
2014-08-09 18:08   ` Kalle Valo
2014-08-09 20:50 ` [PATCH v6 0/8] ath10k: firmware crash dump Ben Greear
2014-08-09 20:50   ` Ben Greear

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.