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