2014-09-18 13:35:56

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 0/3] ath10k: debug improvements

Hi,

This adds extra prints and information for
debugging purposes.


Michal Kazior (3):
ath10k: print wmi version info
ath10k: dump hex bytes with dev string prefix
ath10k: add debug dump for pci rx

drivers/net/wireless/ath/ath10k/debug.c | 29 ++++++++++++++++++++++++++---
drivers/net/wireless/ath/ath10k/pci.c | 6 ++++++
2 files changed, 32 insertions(+), 3 deletions(-)

--
1.8.5.3



2014-09-22 10:05:57

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath10k: print wmi version info

Michal Kazior <[email protected]> writes:

> HTT version is already printed so print WMI
> version as well for consistency.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/debug.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 3756feb..f1e5916 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -126,14 +126,18 @@ EXPORT_SYMBOL(ath10k_info);
>
> void ath10k_print_driver_info(struct ath10k *ar)
> {
> - ath10k_info(ar, "%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
> + ath10k_info(ar, "%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d wmi %d.%d.%d.%d\n",

Heh, I have been thinking about to add something similar.

But if in the future we add the u32 FW IE for WMI version I would like
to print that as well. I guess we could do that with "%d/%d.%d.%d.%d",
the %d being the u32. The idea here being that both version numbers are
good to know.

But of course for now the above is good, just thinking aloud here.

--
Kalle Valo

2014-09-18 13:35:58

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 2/3] ath10k: dump hex bytes with dev string prefix

This makes it easier to debug hex dumps on systems
with more than a single ath10k device.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/debug.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index f1e5916..9f8dddc 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1264,11 +1264,30 @@ void ath10k_dbg_dump(struct ath10k *ar,
const char *msg, const char *prefix,
const void *buf, size_t len)
{
+ char linebuf[256];
+ unsigned int linebuflen;
+ const void *ptr;
+
if (ath10k_debug_mask & mask) {
if (msg)
ath10k_dbg(ar, mask, "%s\n", msg);

- print_hex_dump_bytes(prefix, DUMP_PREFIX_OFFSET, buf, len);
+ for (ptr = buf; (ptr - buf) < len; ptr += 16) {
+ linebuflen = 0;
+ if (prefix)
+ linebuflen += scnprintf(linebuf + linebuflen,
+ sizeof(linebuf) -
+ linebuflen,
+ "%s", prefix);
+ linebuflen += scnprintf(linebuf + linebuflen,
+ sizeof(linebuf) - linebuflen,
+ "%08x: ",
+ (unsigned int)(ptr - buf));
+ hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
+ linebuf + linebuflen,
+ sizeof(linebuf) - linebuflen, true);
+ dev_printk(KERN_DEBUG, ar->dev, "%s\n", linebuf);
+ }
}

/* tracing code doesn't like null strings :/ */
--
1.8.5.3


2014-09-22 12:03:49

by Michal Kazior

[permalink] [raw]
Subject: Re: [PATCH 2/3] ath10k: dump hex bytes with dev string prefix

On 22 September 2014 12:52, Kalle Valo <[email protected]> wrote:
> Michal Kazior <[email protected]> writes:
>
>> This makes it easier to debug hex dumps on systems
>> with more than a single ath10k device.
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>
> [...]
>
>> - print_hex_dump_bytes(prefix, DUMP_PREFIX_OFFSET, buf, len);
>> + for (ptr = buf; (ptr - buf) < len; ptr += 16) {
>> + linebuflen = 0;
>> + if (prefix)
>> + linebuflen += scnprintf(linebuf + linebuflen,
>> + sizeof(linebuf) -
>> + linebuflen,
>> + "%s", prefix);
>> + linebuflen += scnprintf(linebuf + linebuflen,
>> + sizeof(linebuf) - linebuflen,
>> + "%08x: ",
>> + (unsigned int)(ptr - buf));
>> + hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
>> + linebuf + linebuflen,
>> + sizeof(linebuf) - linebuflen, true);
>> + dev_printk(KERN_DEBUG, ar->dev, "%s\n", linebuf);
>> + }
>
> Would it be possible to simplify this to one scnprintf()? Something
> like:
>
> linebuflen += scnprintf(linebuf + linebuflen,
> sizeof(linebuf) - linebuflen,
> "%s%08x: ",
> prefix ? prefix : "",
> (unsigned int)(ptr - buf));

It should be fine. You want me to re-send it?


MichaƂ

2014-09-18 13:35:57

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 1/3] ath10k: print wmi version info

HTT version is already printed so print WMI
version as well for consistency.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/debug.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 3756feb..f1e5916 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -126,14 +126,18 @@ EXPORT_SYMBOL(ath10k_info);

void ath10k_print_driver_info(struct ath10k *ar)
{
- ath10k_info(ar, "%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d\n",
+ ath10k_info(ar, "%s (0x%08x, 0x%08x) fw %s api %d htt %d.%d wmi %d.%d.%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);
+ ar->htt.target_version_minor,
+ ar->fw_version_major,
+ ar->fw_version_minor,
+ ar->fw_version_release,
+ ar->fw_version_build);
ath10k_info(ar, "debug %d debugfs %d tracing %d dfs %d testmode %d\n",
config_enabled(CONFIG_ATH10K_DEBUG),
config_enabled(CONFIG_ATH10K_DEBUGFS),
--
1.8.5.3


2014-09-18 13:35:58

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 3/3] ath10k: add debug dump for pci rx

This makes it easier to debug the device-target
communication at a very low level.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 59e0ea8..8319d83 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -861,6 +861,12 @@ static void ath10k_pci_ce_recv_data(struct ath10k_ce_pipe *ce_state)
}

skb_put(skb, nbytes);
+
+ ath10k_dbg(ar, ATH10K_DBG_PCI, "pci rx ce pipe %d len %d\n",
+ ce_state->id, skb->len);
+ ath10k_dbg_dump(ar, ATH10K_DBG_PCI_DUMP, NULL, "pci rx: ",
+ skb->data, skb->len);
+
cb->rx_completion(ar, skb, pipe_info->pipe_num);
}

--
1.8.5.3


2014-09-22 10:52:37

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 2/3] ath10k: dump hex bytes with dev string prefix

Michal Kazior <[email protected]> writes:

> This makes it easier to debug hex dumps on systems
> with more than a single ath10k device.
>
> Signed-off-by: Michal Kazior <[email protected]>

[...]

> - print_hex_dump_bytes(prefix, DUMP_PREFIX_OFFSET, buf, len);
> + for (ptr = buf; (ptr - buf) < len; ptr += 16) {
> + linebuflen = 0;
> + if (prefix)
> + linebuflen += scnprintf(linebuf + linebuflen,
> + sizeof(linebuf) -
> + linebuflen,
> + "%s", prefix);
> + linebuflen += scnprintf(linebuf + linebuflen,
> + sizeof(linebuf) - linebuflen,
> + "%08x: ",
> + (unsigned int)(ptr - buf));
> + hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
> + linebuf + linebuflen,
> + sizeof(linebuf) - linebuflen, true);
> + dev_printk(KERN_DEBUG, ar->dev, "%s\n", linebuf);
> + }

Would it be possible to simplify this to one scnprintf()? Something
like:

linebuflen += scnprintf(linebuf + linebuflen,
sizeof(linebuf) - linebuflen,
"%s%08x: ",
prefix ? prefix : "",
(unsigned int)(ptr - buf));

--
Kalle Valo

2014-09-22 12:07:47

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 2/3] ath10k: dump hex bytes with dev string prefix

Michal Kazior <[email protected]> writes:

> On 22 September 2014 12:52, Kalle Valo <[email protected]> wrote:
>> Michal Kazior <[email protected]> writes:
>>
>>> - print_hex_dump_bytes(prefix, DUMP_PREFIX_OFFSET, buf, len);
>>> + for (ptr = buf; (ptr - buf) < len; ptr += 16) {
>>> + linebuflen = 0;
>>> + if (prefix)
>>> + linebuflen += scnprintf(linebuf + linebuflen,
>>> + sizeof(linebuf) -
>>> + linebuflen,
>>> + "%s", prefix);
>>> + linebuflen += scnprintf(linebuf + linebuflen,
>>> + sizeof(linebuf) - linebuflen,
>>> + "%08x: ",
>>> + (unsigned int)(ptr - buf));
>>> + hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
>>> + linebuf + linebuflen,
>>> + sizeof(linebuf) - linebuflen, true);
>>> + dev_printk(KERN_DEBUG, ar->dev, "%s\n", linebuf);
>>> + }
>>
>> Would it be possible to simplify this to one scnprintf()? Something
>> like:
>>
>> linebuflen += scnprintf(linebuf + linebuflen,
>> sizeof(linebuf) - linebuflen,
>> "%s%08x: ",
>> prefix ? prefix : "",
>> (unsigned int)(ptr - buf));
>
> It should be fine. You want me to re-send it?

Yes, that would be good. I don't feel comfortable editing patches with
logic changes like this.

--
Kalle Valo