greearb@xxxxxxxxxxxxxxx writes:
> From: Ben Greear <greearb@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
My first comments, more later. But my first impression is that I like
this, we just need to sort out some implementation issues.
kbuild found some warnings:
drivers/net/wireless/ath/ath10k/debug.c:725:2: error: implicit declaration of function 'vfree' [-Werror=implicit-function-declaration]
drivers/net/wireless/ath/ath10k/debug.c:624:2: error: implicit declaration of function 'vmalloc' [-Werror=implicit-function-declaration]
drivers/net/wireless/ath/ath10k/debug.c:624:6: warning: assignment makes pointer from integer without a cast [enabled by default]
The naming of structures and functions are a bit too long, but I'll
comment on that later. That's easy to change as the last thing.
> +/**
> + * enum ath10k_fw_error_dump_type - types of data in the dump file
> + * @ATH10K_FW_ERROR_DUMP_DBGLOG: Recent firmware debug log entries
> + * @ATH10K_FW_ERROR_DUMP_CRASH: Crash dump in binary format
> + */
> +enum ath10k_fw_error_dump_type {
> + ATH10K_FW_ERROR_DUMP_DBGLOG = 0,
> + ATH10K_FW_ERROR_DUMP_REGDUMP = 1,
> +
> + ATH10K_FW_ERROR_DUMP_MAX,
> +};
> +
> +
Extra newline.
> +struct ath10k_tlv_dump_data {
> + u32 type; /* see ath10k_fw_error_dump_type above */
> + u32 tlv_len; /* in bytes */
> + u8 tlv_data[]; /* Pad to 32-bit boundaries as needed. */
> +} __packed;
> +
> +struct ath10k_dump_file_data {
> + /* Dump file information */
> + u32 len;
> + u32 magic; /* 0x01020304, tells us byte-order of host if we care */
> + u32 version; /* File dump version, 1 for now. */
Actually I would prefer to have magic first and have ASCII string as
string, for example "ATH10K-FW-DUMP".
> + /* Some info we can get from ath10k struct that might help. */
> + u32 chip_id;
> + u32 target_version;
bus_type or something like that would be good to add already now.
> + u8 fw_version_major;
> + u8 unused8; /* pad fw_version_major */
> + u16 unused16; /* pad fw_version_major */
I don't see any point of using u8 or u16. Would it be simpler just to
use u32 for everything?
> + u32 fw_version_minor;
> + u16 fw_version_release;
> + u16 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;
> + char fw_ver[64]; /* Firmware version string */
Can we reuse ETHTOOL_FWVERS_LEN? cfg80211 already uses that.
#define ETHTOOL_FWVERS_LEN 32
> + /* Kernel related information */
> + u32 kernel_ver_code; /* LINUX_VERSION_CODE */
> + char kernel_ver[64]; /* VERMAGIC_STRING */
> +
> + u8 unused[64]; /* Room for growth w/out changing binary format */
Maybe 128 bytes, just so that we don't need to change it for some time?
> + struct ath10k_tlv_dump_data data; /* more may follow */
I would prefer:
u8 data[0];
So that this struct is only about the header.
> +} __packed;
> +
> +/* 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 * 4)
Empty line after the define.
> @@ -488,6 +555,12 @@ struct ath10k {
>
> struct dfs_pattern_detector *dfs_detector;
>
> + /* Used for crash-dump storage */
> + /* Don't over-write dump info until someone reads the data. */
> + bool crashed_since_read;
> + struct ath10k_dbglog_entry_storage dbglog_entry_data;
> + u32 reg_dump_values[REG_DUMP_COUNT_QCA988X];
I think these should be in struct ath10k_debug.
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 1b7ff4b..a7d7877 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -17,6 +17,8 @@
>
> #include <linux/module.h>
> #include <linux/debugfs.h>
> +#include <linux/version.h>
> +#include <linux/vermagic.h>
>
> #include "core.h"
> #include "debug.h"
> @@ -577,6 +579,145 @@ static const struct file_operations fops_chip_id = {
> .llseek = default_llseek,
> };
>
> +void ath10k_dbg_save_fw_dbg_buffer(struct ath10k *ar, u8 *buffer, int len)
> +{
> + int i;
> + int z = ar->dbglog_entry_data.next_idx;
> +
> + /* Don't save any new logs until user-space reads this. */
> + if (ar->crashed_since_read)
> + return;
Locking? If this functions depends on something, please document that
with lockdep_assert_held().
> + for (i = 0; i < len; i++) {
> + ar->dbglog_entry_data.data[z] = buffer[i];
> + z++;
> + if (z >= ATH10K_DBGLOG_DATA_LEN)
> + z = 0;
> + }
Empty line here.
> + ar->dbglog_entry_data.next_idx = z;
> +}
> +EXPORT_SYMBOL(ath10k_dbg_save_fw_dbg_buffer);
> +
> +static struct ath10k_dump_file_data *ath10k_build_dump_file(struct ath10k *ar)
> +{
> + unsigned int len = (sizeof(ar->dbglog_entry_data)
Unneeded parenthesis.
> + + sizeof(ar->reg_dump_values));
> + unsigned int sofar = 0;
> + char *buf;
> + struct ath10k_tlv_dump_data *dump_tlv;
> + struct ath10k_dump_file_data *dump_data;
> + int hdr_len = sizeof(*dump_data) - sizeof(dump_data->data);
I prefer separating variable declarations and definitions.
> +
> + lockdep_assert_held(&ar->conf_mutex);
> +
> + len += hdr_len;
> + sofar += hdr_len;
> +
> + /* So we can add headers to the data dump */
> + len += 2 * sizeof(*dump_tlv);
This is a bit awkward way of defining the length. Something like this
would be easier to read:
len = sizeof (*dump_tlv) + sizeof(ar->dbglog_entry_data);
len += sizeof (*dump_tlv) + sizeof(ar->reg_dump_values));
> +
> + /* 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;
> +
> + memset(buf, 0, len);
> + dump_data = (struct ath10k_dump_file_data *)(buf);
> + dump_data->len = len;
> + dump_data->magic = 0x01020304;
> + dump_data->version = 1;
> + dump_data->chip_id = ar->chip_id;
> + 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;
> +
> + strncpy(dump_data->fw_ver, ar->hw->wiphy->fw_version,
> + sizeof(dump_data->fw_ver) - 1);
BUILD_BUG_ON(sizeof(dump_data->fw_ver) != sizeof(ar->hw->wiphy->fw_version))?
And wouldn't strlcpy() be safer?
> + dump_data->kernel_ver_code = LINUX_VERSION_CODE;
checkpatch actually complains about this:
drivers/net/wireless/ath/ath10k/debug.c:649: WARNING: LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
But I guess there's nothing we can do for that.
> + strncpy(dump_data->kernel_ver, VERMAGIC_STRING,
> + sizeof(dump_data->kernel_ver) - 1);
strlcpy()?
> + /* Gather dbg-log */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = ATH10K_FW_ERROR_DUMP_DBGLOG;
> + dump_tlv->tlv_len = sizeof(ar->dbglog_entry_data);
> + memcpy(dump_tlv->tlv_data, &ar->dbglog_entry_data, dump_tlv->tlv_len);
> + sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
> +
> + /* Gather crash-dump */
> + dump_tlv = (struct ath10k_tlv_dump_data *)(buf + sofar);
> + dump_tlv->type = ATH10K_FW_ERROR_DUMP_REGDUMP;
> + dump_tlv->tlv_len = sizeof(ar->reg_dump_values);
> + memcpy(dump_tlv->tlv_data, &ar->reg_dump_values, dump_tlv->tlv_len);
> + sofar += sizeof(*dump_tlv) + dump_tlv->tlv_len;
> +
> + return dump_data;
> +}
> +
> +static int ath10k_fw_error_dump_open(struct inode *inode, struct file *file)
> +{
> + struct ath10k *ar = inode->i_private;
> + int ret;
> + struct ath10k_dump_file_data *dump;
> +
> + if (!ar)
> + return -EINVAL;
Is this cheak necessary? In what cases is i_private NULL?
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -19,6 +19,7 @@
> #define _DEBUG_H_
>
> #include <linux/types.h>
> +#include "pci.h"
debug.h shouldn't access pci.h directly. All access should happen
through hif.
>
> enum ath10k_debug_mask {
> @@ -110,4 +111,28 @@ static inline void ath10k_dbg_dump(enum ath10k_debug_mask mask,
> {
> }
> #endif /* CONFIG_ATH10K_DEBUG */
> +
> +
> +/* Target debug log related defines and structs */
> +
> +/* Target is 32-bit CPU, so we just use u32 for
> + * the pointers. The memory space is relative to the
> + * target, not the host.
> + */
> +struct ath10k_fw_dbglog_buf {
> + u32 next; /* pointer to dblog_buf_s. */
> + u32 buffer; /* pointer to u8 buffer */
> + u32 bufsize;
> + u32 length;
> + u32 count;
> + u32 free;
> +} __packed;
> +
> +struct ath10k_fw_dbglog_hdr {
> + u32 dbuf; /* pointer to dbglog_buf_s */
> + u32 dropped;
> +} __packed;
Should these be in hw.h?
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -19,7 +19,7 @@
> #include <linux/module.h>
> #include <linux/interrupt.h>
> #include <linux/spinlock.h>
> -#include <linux/bitops.h>
Why remove bitops.h?
Have to stop here, more later.
--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
