2017-05-10 16:26:17

by Adrian Chadd

[permalink] [raw]
Subject: [PATCH] ath10k: add configurable debugging.

This adds a few configurable debugging options:

* driver debugging and tracing is now configurable per device
* driver debugging and tracing is now configurable at runtime
* the debugging / tracing is not run at all (besides a mask check)
unless the specific debugging bitmap field is configured.

Signed-off-by: Adrian Chadd <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 2 +
drivers/net/wireless/ath/ath10k/core.h | 2 +
drivers/net/wireless/ath/ath10k/debug.c | 101 ++++++++++++++++++++++++++++----
drivers/net/wireless/ath/ath10k/debug.h | 44 +++++++++-----
4 files changed, 125 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index eea111d704c5..fcb068cb0248 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2444,6 +2444,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
ar->hw_rev = hw_rev;
ar->hif.ops = hif_ops;
ar->hif.bus = bus;
+ ar->debug_mask = ath10k_debug_mask;
+ ar->trace_debug_mask = ath10k_debug_mask;

switch (hw_rev) {
case ATH10K_HW_QCA988X:
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 8fc08a5043db..07e392a377d0 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -762,6 +762,8 @@ struct ath10k {
struct device *dev;
u8 mac_addr[ETH_ALEN];

+ u32 debug_mask;
+ u32 trace_debug_mask;
enum ath10k_hw_rev hw_rev;
u16 dev_id;
u32 chip_id;
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 389fcb7a9fd0..017360a26b40 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -2418,6 +2418,79 @@ int ath10k_debug_create(struct ath10k *ar)
return 0;
}

+#ifdef CONFIG_ATH10K_DEBUGFS
+static ssize_t ath10k_write_debug_mask(struct file *file,
+ const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ struct ath10k *ar = file->private_data;
+ int ret;
+ u32 val;
+
+ if (kstrtou32_from_user(ubuf, count, 0, &val))
+ return -EINVAL;
+
+ ar->debug_mask = val;
+ ret = count;
+
+ return ret;
+}
+
+static ssize_t ath10k_read_debug_mask(struct file *file, char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ char buf[32];
+ struct ath10k *ar = file->private_data;
+ int len = 0;
+
+ len = scnprintf(buf, sizeof(buf) - len, "0x%x\n", ar->debug_mask);
+ return simple_read_from_buffer(ubuf, count, ppos, buf, len);
+}
+
+static const struct file_operations fops_debug_mask = {
+ .read = ath10k_read_debug_mask,
+ .write = ath10k_write_debug_mask,
+ .open = simple_open
+};
+
+static ssize_t ath10k_write_trace_debug_mask(struct file *file,
+ const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ struct ath10k *ar = file->private_data;
+ int ret;
+ u32 val;
+
+ if (kstrtou32_from_user(ubuf, count, 0, &val))
+ return -EINVAL;
+
+ ar->trace_debug_mask = val;
+ ret = count;
+
+ return ret;
+}
+
+static ssize_t ath10k_read_trace_debug_mask(struct file *file,
+ char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ char buf[32];
+ struct ath10k *ar = file->private_data;
+ int len = 0;
+
+ len = scnprintf(buf, sizeof(buf) - len, "0x%x\n",
+ ar->trace_debug_mask);
+ return simple_read_from_buffer(ubuf, count, ppos, buf, len);
+}
+
+static const struct file_operations fops_trace_debug_mask = {
+ .read = ath10k_read_trace_debug_mask,
+ .write = ath10k_write_trace_debug_mask,
+ .open = simple_open
+};
+#endif /* CONFIG_ATH10K_DEBUGFS */
+
+
void ath10k_debug_destroy(struct ath10k *ar)
{
vfree(ar->debug.fw_crash_data);
@@ -2448,6 +2521,13 @@ int ath10k_debug_register(struct ath10k *ar)
init_completion(&ar->debug.tpc_complete);
init_completion(&ar->debug.fw_stats_complete);

+#ifdef CONFIG_ATH10K_DEBUG
+ debugfs_create_file("debug", S_IRUSR, ar->debug.debugfs_phy, ar,
+ &fops_debug_mask);
+ debugfs_create_file("trace_debug", S_IRUSR, ar->debug.debugfs_phy, ar,
+ &fops_trace_debug_mask);
+#endif
+
debugfs_create_file("fw_stats", 0400, ar->debug.debugfs_phy, ar,
&fops_fw_stats);

@@ -2536,7 +2616,7 @@ void ath10k_debug_unregister(struct ath10k *ar)
#endif /* CONFIG_ATH10K_DEBUGFS */

#ifdef CONFIG_ATH10K_DEBUG
-void ath10k_dbg(struct ath10k *ar, enum ath10k_debug_mask mask,
+void _ath10k_dbg(struct ath10k *ar, enum ath10k_debug_mask mask,
const char *fmt, ...)
{
struct va_format vaf;
@@ -2547,16 +2627,16 @@ void ath10k_dbg(struct ath10k *ar, enum ath10k_debug_mask mask,
vaf.fmt = fmt;
vaf.va = &args;

- if (ath10k_debug_mask & mask)
+ if (ar->debug_mask & mask)
dev_printk(KERN_DEBUG, ar->dev, "%pV", &vaf);
-
- trace_ath10k_log_dbg(ar, mask, &vaf);
+ if (ar->trace_debug_mask & mask)
+ trace_ath10k_log_dbg(ar, mask, &vaf);

va_end(args);
}
-EXPORT_SYMBOL(ath10k_dbg);
+EXPORT_SYMBOL(_ath10k_dbg);

-void ath10k_dbg_dump(struct ath10k *ar,
+void _ath10k_dbg_dump(struct ath10k *ar,
enum ath10k_debug_mask mask,
const char *msg, const char *prefix,
const void *buf, size_t len)
@@ -2565,7 +2645,7 @@ void ath10k_dbg_dump(struct ath10k *ar,
size_t linebuflen;
const void *ptr;

- if (ath10k_debug_mask & mask) {
+ if (ar->debug_mask & mask) {
if (msg)
ath10k_dbg(ar, mask, "%s\n", msg);

@@ -2584,9 +2664,10 @@ void ath10k_dbg_dump(struct ath10k *ar,
}

/* tracing code doesn't like null strings :/ */
- trace_ath10k_log_dbg_dump(ar, msg ? msg : "", prefix ? prefix : "",
- buf, len);
+ if (ar->trace_debug_mask & mask)
+ trace_ath10k_log_dbg_dump(ar, msg ? msg : "", prefix ? prefix : "",
+ buf, len);
}
-EXPORT_SYMBOL(ath10k_dbg_dump);
+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 257d10985c6e..7bd461927029 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -200,27 +200,43 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar,
#endif /* CONFIG_MAC80211_DEBUGFS */

#ifdef CONFIG_ATH10K_DEBUG
-__printf(3, 4) void ath10k_dbg(struct ath10k *ar,
+static inline int
+_ath10k_do_dbg(struct ath10k *ar, enum ath10k_debug_mask mask)
+{
+ if (ar->trace_debug_mask & mask)
+ return (1);
+ if (ar->debug_mask & mask)
+ return (1);
+ return (0);
+}
+
+void _ath10k_dbg(struct ath10k *ar,
enum ath10k_debug_mask mask,
const char *fmt, ...);
-void ath10k_dbg_dump(struct ath10k *ar,
+
+void _ath10k_dbg_dump(struct ath10k *ar,
enum ath10k_debug_mask mask,
const char *msg, const char *prefix,
const void *buf, size_t len);
+
+#define ath10k_dbg(ar, mask, ...) \
+ do { \
+ if (_ath10k_do_dbg(ar, mask)) { \
+ _ath10k_dbg((ar), (mask), __VA_ARGS__); \
+ }; \
+ } while (0)
+
+#define ath10k_dbg_dump(ar, mask, msg, pfx, buf, len) \
+ do { \
+ if (_ath10k_do_dbg(ar, mask)) { \
+ _ath10k_dbg_dump((ar), (mask), (msg), (pfx), (buf), (len)); \
+ }; \
+ } while (0)
+
#else /* CONFIG_ATH10K_DEBUG */

-static inline int ath10k_dbg(struct ath10k *ar,
- enum ath10k_debug_mask dbg_mask,
- const char *fmt, ...)
-{
- return 0;
-}
+#define ath10k_dbg(ar, mask, fmt, ...)
+#define ath10k_dbg_dump(ar, mask, msg, pfx, buf, len)

-static inline void ath10k_dbg_dump(struct ath10k *ar,
- enum ath10k_debug_mask mask,
- const char *msg, const char *prefix,
- const void *buf, size_t len)
-{
-}
#endif /* CONFIG_ATH10K_DEBUG */
#endif /* _DEBUG_H_ */
--
2.12.1


2017-05-10 16:44:25

by Steve deRosier

[permalink] [raw]
Subject: Re: [PATCH] ath10k: add configurable debugging.

Hi Adrian,

On Wed, May 10, 2017 at 9:25 AM, Adrian Chadd <[email protected]> wrote:

> diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
> index 257d10985c6e..7bd461927029 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.h
> +++ b/drivers/net/wireless/ath/ath10k/debug.h
> @@ -200,27 +200,43 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar,
> #endif /* CONFIG_MAC80211_DEBUGFS */
>
> #ifdef CONFIG_ATH10K_DEBUG
> -__printf(3, 4) void ath10k_dbg(struct ath10k *ar,
> +static inline int
> +_ath10k_do_dbg(struct ath10k *ar, enum ath10k_debug_mask mask)
> +{
> + if (ar->trace_debug_mask & mask)
> + return (1);
> + if (ar->debug_mask & mask)
> + return (1);
> + return (0);
> +}
> +
> +void _ath10k_dbg(struct ath10k *ar,
> enum ath10k_debug_mask mask,
> const char *fmt, ...);
> -void ath10k_dbg_dump(struct ath10k *ar,
> +
> +void _ath10k_dbg_dump(struct ath10k *ar,
> enum ath10k_debug_mask mask,
> const char *msg, const char *prefix,
> const void *buf, size_t len);
> +
> +#define ath10k_dbg(ar, mask, ...) \
> + do { \
> + if (_ath10k_do_dbg(ar, mask)) { \
> + _ath10k_dbg((ar), (mask), __VA_ARGS__); \
> + }; \
> + } while (0)
> +

Looks to me you dropped the "__printf(3, 4)" safety check. Was that intentional?

Thanks,
- Steve

2017-05-19 09:47:58

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath10k: add configurable debugging.

Adrian Chadd <[email protected]> writes:

> This adds a few configurable debugging options:
>
> * driver debugging and tracing is now configurable per device
> * driver debugging and tracing is now configurable at runtime
> * the debugging / tracing is not run at all (besides a mask check)
> unless the specific debugging bitmap field is configured.
>
> Signed-off-by: Adrian Chadd <[email protected]>

ath10k-check found some trivial whitespace problems:

drivers/net/wireless/ath/ath10k/debug.h:207: return is not a function, pare=
ntheses are not required
drivers/net/wireless/ath/ath10k/debug.h:209: return is not a function, pare=
ntheses are not required
drivers/net/wireless/ath/ath10k/debug.h:210: return is not a function, pare=
ntheses are not required
drivers/net/wireless/ath/ath10k/debug.h:214: Alignment should match open pa=
renthesis
drivers/net/wireless/ath/ath10k/debug.h:218: Alignment should match open pa=
renthesis
drivers/net/wireless/ath/ath10k/debug.c:2430: code indent should use tabs w=
here possible
drivers/net/wireless/ath/ath10k/debug.c:2430: please, no spaces at the star=
t of a line
drivers/net/wireless/ath/ath10k/debug.c:2431: code indent should use tabs w=
here possible
drivers/net/wireless/ath/ath10k/debug.c:2431: please, no spaces at the star=
t of a line
drivers/net/wireless/ath/ath10k/debug.c:2464: code indent should use tabs w=
here possible
drivers/net/wireless/ath/ath10k/debug.c:2464: please, no spaces at the star=
t of a line
drivers/net/wireless/ath/ath10k/debug.c:2465: code indent should use tabs w=
here possible
drivers/net/wireless/ath/ath10k/debug.c:2465: please, no spaces at the star=
t of a line
drivers/net/wireless/ath/ath10k/debug.c:2493: Please don't use multiple bla=
nk lines
drivers/net/wireless/ath/ath10k/debug.c:2525: Symbolic permissions 'S_IRUSR=
' are not preferred. Consider using octal permissions '0400'.
drivers/net/wireless/ath/ath10k/debug.c:2527: Symbolic permissions 'S_IRUSR=
' are not preferred. Consider using octal permissions '0400'.
drivers/net/wireless/ath/ath10k/debug.c:2620: Alignment should match open p=
arenthesis
drivers/net/wireless/ath/ath10k/debug.c:2640: Alignment should match open p=
arenthesis

I fixed those in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=3Dp=
ending&id=3Dbd8c3bdce70adc201037b2eb7eda0a83911ef375

I'll look at this more closely later.

--=20
Kalle Valo=

2017-05-10 16:50:40

by Adrian Chadd

[permalink] [raw]
Subject: Re: [PATCH] ath10k: add configurable debugging.

grr, no. lemme go re-add that and resubmit.

thanks!


-a


On 10 May 2017 at 09:44, Steve deRosier <[email protected]> wrote:
> Hi Adrian,
>
> On Wed, May 10, 2017 at 9:25 AM, Adrian Chadd <[email protected]> wrote:
>
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
>> index 257d10985c6e..7bd461927029 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.h
>> +++ b/drivers/net/wireless/ath/ath10k/debug.h
>> @@ -200,27 +200,43 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar,
>> #endif /* CONFIG_MAC80211_DEBUGFS */
>>
>> #ifdef CONFIG_ATH10K_DEBUG
>> -__printf(3, 4) void ath10k_dbg(struct ath10k *ar,
>> +static inline int
>> +_ath10k_do_dbg(struct ath10k *ar, enum ath10k_debug_mask mask)
>> +{
>> + if (ar->trace_debug_mask & mask)
>> + return (1);
>> + if (ar->debug_mask & mask)
>> + return (1);
>> + return (0);
>> +}
>> +
>> +void _ath10k_dbg(struct ath10k *ar,
>> enum ath10k_debug_mask mask,
>> const char *fmt, ...);
>> -void ath10k_dbg_dump(struct ath10k *ar,
>> +
>> +void _ath10k_dbg_dump(struct ath10k *ar,
>> enum ath10k_debug_mask mask,
>> const char *msg, const char *prefix,
>> const void *buf, size_t len);
>> +
>> +#define ath10k_dbg(ar, mask, ...) \
>> + do { \
>> + if (_ath10k_do_dbg(ar, mask)) { \
>> + _ath10k_dbg((ar), (mask), __VA_ARGS__); \
>> + }; \
>> + } while (0)
>> +
>
> Looks to me you dropped the "__printf(3, 4)" safety check. Was that intentional?
>
> Thanks,
> - Steve