Subject: [PATCH 1/3] ath6kl: Add initial debugfs changes

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/Kconfig | 6 ++++++
drivers/net/wireless/ath/ath6kl/core.h | 1 +
drivers/net/wireless/ath/ath6kl/debug.c | 13 +++++++++++++
drivers/net/wireless/ath/ath6kl/debug.h | 9 +++++++++
drivers/net/wireless/ath/ath6kl/init.c | 6 ++++++
5 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/Kconfig b/drivers/net/wireless/ath/ath6kl/Kconfig
index 3d5f8be..ea9387b 100644
--- a/drivers/net/wireless/ath/ath6kl/Kconfig
+++ b/drivers/net/wireless/ath/ath6kl/Kconfig
@@ -13,3 +13,9 @@ config ATH6KL_DEBUG
depends on ATH6KL
---help---
Enables debug support
+config ATH6KL_DEBUGFS
+ bool "ath6kl debugfs"
+ depends on ATH6KL_DEBUG && DEBUG_FS
+ ---help---
+ Debugfs file entries to dump various target and host stats, like tx, rx,
+ credit disctribution, etc.
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 4405ab5..c5537b3 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -467,6 +467,7 @@ struct ath6kl {
struct workqueue_struct *ath6kl_wq;

struct ath6kl_node_table scan_table;
+ struct dentry *debugfs_phy;
};

static inline void *ath6kl_priv(struct net_device *dev)
diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 316136c..30888d1 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -148,3 +148,16 @@ void dump_cred_dist_stats(struct htc_target *target)
}

#endif
+
+#ifdef CONFIG_ATH6KL_DEBUGFS
+int ath6kl_init_debugfs(struct ath6kl *ar)
+{
+ ar->debugfs_phy = debugfs_create_dir("ath6kl",
+ ar->wdev->wiphy->debugfsdir);
+ if (!ar->debugfs_phy)
+ return -ENOMEM;
+
+ /* TODO: Create debugfs file entries for various target/host stats */
+ return 0;
+}
+#endif
diff --git a/drivers/net/wireless/ath/ath6kl/debug.h b/drivers/net/wireless/ath/ath6kl/debug.h
index 66b3999..ac03506 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.h
+++ b/drivers/net/wireless/ath/ath6kl/debug.h
@@ -102,4 +102,13 @@ static inline void dump_cred_dist_stats(struct htc_target *target)
}
#endif

+#ifdef CONFIG_ATH6KL_DEBUGFS
+int ath6kl_init_debugfs(struct ath6kl *ar);
+#else
+static inline int ath6kl_init_debugfs(struct ath6kl *ar)
+{
+ return 0;
+}
+#endif
+
#endif
diff --git a/drivers/net/wireless/ath/ath6kl/init.c b/drivers/net/wireless/ath/ath6kl/init.c
index 75230ac..b1d6fca 100644
--- a/drivers/net/wireless/ath/ath6kl/init.c
+++ b/drivers/net/wireless/ath/ath6kl/init.c
@@ -573,6 +573,12 @@ struct ath6kl *ath6kl_core_alloc(struct device *sdev)
ar->wdev = wdev;
wdev->iftype = NL80211_IFTYPE_STATION;

+ if (ath6kl_init_debugfs(ar)) {
+ ath6kl_err("Failed to initialize debugfs\n");
+ ath6kl_cfg80211_deinit(ar);
+ return NULL;
+ }
+
dev = alloc_netdev(0, "wlan%d", ether_setup);
if (!dev) {
ath6kl_err("no memory for network device instance\n");
--
1.7.0.4



2011-08-24 17:24:18

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:44 PM, Vasanthakumar Thiagarajan wrote:
> On Wed, Aug 24, 2011 at 05:33:30PM +0300, Kalle Valo wrote:
>>
>> Actually do we really need ATH6KL_DEBUGFS? I would think that if both
>> ATH6KL_DEBUG and DEBUG_FS are enabled we should just enable debugfs code
>> in ath6kl. I don't see the need to have separate kconfig option control
>> that.
>
> You are right. We can directly use CONFIG_DEBUG_FS to enable ath6kl
> debugfs functionalities, also as I said, this does not need to
> depend on ATH6KL_DEBUG. One purpose that I can think of introducing
> a config option for debugfs is to reduce the size of the binary,
> may be it is very insignificant.

Yeah, I think we don't need to worry that. The user can always disable
CONFIG_DEBUG_FS if he doesn't want to use debugfs, just disabling
debugfs for ath6kl doesn't make sense.

Kalle

2011-08-24 14:46:31

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
> +#ifdef CONFIG_ATH6KL_DEBUGFS
> +int ath6kl_init_debugfs(struct ath6kl *ar)
> +{
> + ar->debugfs_phy = debugfs_create_dir("ath6kl",
> + ar->wdev->wiphy->debugfsdir);

What if we have multiple devices on same host? Then this won't work,
right? Maybe we should have some kind of subdirectory under ath6kl or
something like that.

Would interface name ("wlan0") be a good choise? Ah, but that can
change. Any better options?

Kalle

2011-08-24 17:31:35

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:46 PM, Vasanthakumar Thiagarajan wrote:
> On Wed, Aug 24, 2011 at 05:39:48PM +0300, Kalle Valo wrote:
>> On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
>>> +#ifdef CONFIG_ATH6KL_DEBUGFS
>>> +int ath6kl_init_debugfs(struct ath6kl *ar);
>>> +#else
>>> +static inline int ath6kl_init_debugfs(struct ath6kl *ar)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>
>> I would prefer to call this ath6kl_debug_init(). That way we can put all
>> debug initialisation into that, not just debugfs code. And then the
>> ifdef should check for CONFIG_ATH6KL_DEBUG.
>
> May be, but I think we can't always enable debugfs based on only
> CONFIG_ATH6KL_DEBUG as the general debug does not depend on
> CONFIG_DEBUG_FS.

I don't see why we need to check for CONFIG_DEBUG_FS.
debugfs_create_file() should handle the case then CONFIG_DEBUG_FS is
disabled as it will just return ERR_PTR(-ENODEV).

Kalle

Subject: [PATCH 3/3] ath6kl: Add debugfs file entry to dump credit distribution stats

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/debug.c | 65 +++++++++++++++++++++++++++++++
1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 014cc18..a729793 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -293,6 +293,68 @@ static const struct file_operations fops_tgt_stats = {
.llseek = default_llseek,
};

+static ssize_t read_file_credit_dist_stats(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath6kl *ar = file->private_data;
+ struct htc_target *target = ar->htc_target;
+ struct htc_endpoint_credit_dist *ep_list;
+ char *buf;
+ unsigned int buf_len = 128, len = 0;
+ ssize_t ret_cnt;
+
+ buf_len += get_queue_depth(&target->cred_dist_list) * 128;
+ buf = kzalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ len += scnprintf(buf + len, buf_len - len,
+ " Epid Flags Cred_norm Cred_min Credits Cred_assngd"
+ " Seek_cred Cred_sz Cred_per_msg Cred_to_dist"
+ " qdepth\n");
+
+ list_for_each_entry(ep_list, &target->cred_dist_list, list) {
+ len += scnprintf(buf + len, buf_len - len, " %2d",
+ ep_list->endpoint);
+ len += scnprintf(buf + len, buf_len - len, "%10x",
+ ep_list->dist_flags);
+ len += scnprintf(buf + len, buf_len - len, "%8d",
+ ep_list->cred_norm);
+ len += scnprintf(buf + len, buf_len - len, "%9d",
+ ep_list->cred_min);
+ len += scnprintf(buf + len, buf_len - len, "%9d",
+ ep_list->credits);
+ len += scnprintf(buf + len, buf_len - len, "%10d",
+ ep_list->cred_assngd);
+ len += scnprintf(buf + len, buf_len - len, "%13d",
+ ep_list->seek_cred);
+ len += scnprintf(buf + len, buf_len - len, "%12d",
+ ep_list->cred_sz);
+ len += scnprintf(buf + len, buf_len - len, "%9d",
+ ep_list->cred_per_msg);
+ len += scnprintf(buf + len, buf_len - len, "%14d",
+ ep_list->cred_to_dist);
+ len += scnprintf(buf + len, buf_len - len, "%12d\n",
+ get_queue_depth(&((struct htc_endpoint *)
+ ep_list->htc_rsvd)->txq));
+ }
+
+ if (len > buf_len)
+ len = buf_len;
+
+ ret_cnt = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+ kfree(buf);
+ return ret_cnt;
+}
+
+static const struct file_operations fops_credit_dist_stats = {
+ .read = read_file_credit_dist_stats,
+ .open = ath6kl_debugfs_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
int ath6kl_init_debugfs(struct ath6kl *ar)
{
ar->debugfs_phy = debugfs_create_dir("ath6kl",
@@ -303,6 +365,9 @@ int ath6kl_init_debugfs(struct ath6kl *ar)
debugfs_create_file("tgt_stats", S_IRUSR, ar->debugfs_phy, ar,
&fops_tgt_stats);

+ debugfs_create_file("credit_dist_stats", S_IRUSR, ar->debugfs_phy, ar,
+ &fops_credit_dist_stats);
+
return 0;
}
#endif
--
1.7.0.4


Subject: [PATCH 2/3] ath6kl: Add debugfs entry to dump target stats

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/debug.c | 147 ++++++++++++++++++++++++++++++-
1 files changed, 146 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 30888d1..014cc18 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -150,6 +150,149 @@ void dump_cred_dist_stats(struct htc_target *target)
#endif

#ifdef CONFIG_ATH6KL_DEBUGFS
+static int ath6kl_debugfs_open(struct inode *inode, struct file *file)
+{
+ file->private_data = inode->i_private;
+ return 0;
+}
+
+static ssize_t read_file_tgt_stats(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ath6kl *ar = file->private_data;
+ struct target_stats *tgt_stats = &ar->target_stats;
+ char *buf;
+ unsigned int len = 0, buf_len = 1500;
+ int i;
+ long left;
+ ssize_t ret_cnt;
+
+ buf = kzalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ if (down_interruptible(&ar->sem)) {
+ kfree(buf);
+ return -EBUSY;
+ }
+
+ set_bit(STATS_UPDATE_PEND, &ar->flag);
+
+ if (ath6kl_wmi_get_stats_cmd(ar->wmi)) {
+ up(&ar->sem);
+ kfree(buf);
+ return -EIO;
+ }
+
+ left = wait_event_interruptible_timeout(ar->event_wq,
+ !test_bit(STATS_UPDATE_PEND,
+ &ar->flag), WMI_TIMEOUT);
+
+ up(&ar->sem);
+
+ if (left <= 0) {
+ kfree(buf);
+ return -ETIMEDOUT;
+ }
+
+ len += scnprintf(buf + len, buf_len - len, "\n");
+ len += scnprintf(buf + len, buf_len - len, "%25s\n",
+ "Target Tx stats");
+ len += scnprintf(buf + len, buf_len - len, "%25s\n\n",
+ "=================");
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Ucast packets", tgt_stats->tx_ucast_pkt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Bcast packets", tgt_stats->tx_bcast_pkt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Ucast byte", tgt_stats->tx_ucast_byte);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Bcast byte", tgt_stats->tx_bcast_byte);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Rts success cnt", tgt_stats->tx_rts_success_cnt);
+ for (i = 0; i < 4; i++)
+ len += scnprintf(buf + len, buf_len - len,
+ "%18s %d %10llu\n", "PER on ac",
+ i, tgt_stats->tx_pkt_per_ac[i]);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Error", tgt_stats->tx_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Fail count", tgt_stats->tx_fail_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Retry count", tgt_stats->tx_retry_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Multi retry cnt", tgt_stats->tx_mult_retry_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Rts fail cnt", tgt_stats->tx_rts_fail_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%25s %10llu\n\n",
+ "TKIP counter measure used",
+ tgt_stats->tkip_cnter_measures_invoked);
+
+ len += scnprintf(buf + len, buf_len - len, "%25s\n",
+ "Target Rx stats");
+ len += scnprintf(buf + len, buf_len - len, "%25s\n",
+ "=================");
+
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Ucast packets", tgt_stats->rx_ucast_pkt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10d\n",
+ "Ucast Rate", tgt_stats->rx_ucast_rate);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Bcast packets", tgt_stats->rx_bcast_pkt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Ucast byte", tgt_stats->rx_ucast_byte);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Bcast byte", tgt_stats->rx_bcast_byte);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Fragmented pkt", tgt_stats->rx_frgment_pkt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Error", tgt_stats->rx_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "CRC Err", tgt_stats->rx_crc_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Key chache miss", tgt_stats->rx_key_cache_miss);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Decrypt Err", tgt_stats->rx_decrypt_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Duplicate frame", tgt_stats->rx_dupl_frame);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Tkip Mic failure", tgt_stats->tkip_local_mic_fail);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "TKIP format err", tgt_stats->tkip_fmt_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "CCMP format Err", tgt_stats->ccmp_fmt_err);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n\n",
+ "CCMP Replay Err", tgt_stats->ccmp_replays);
+
+ len += scnprintf(buf + len, buf_len - len, "%25s\n",
+ "Misc Target stats");
+ len += scnprintf(buf + len, buf_len - len, "%25s\n",
+ "=================");
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Beacon Miss count", tgt_stats->cs_bmiss_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Num Connects", tgt_stats->cs_connect_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10llu\n",
+ "Num disconnects", tgt_stats->cs_discon_cnt);
+ len += scnprintf(buf + len, buf_len - len, "%20s %10d\n",
+ "Beacon avg rssi", tgt_stats->cs_ave_beacon_rssi);
+
+ if (len > buf_len)
+ len = buf_len;
+
+ ret_cnt = simple_read_from_buffer(user_buf, count, ppos, buf, len);
+
+ kfree(buf);
+ return ret_cnt;
+}
+
+static const struct file_operations fops_tgt_stats = {
+ .read = read_file_tgt_stats,
+ .open = ath6kl_debugfs_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
int ath6kl_init_debugfs(struct ath6kl *ar)
{
ar->debugfs_phy = debugfs_create_dir("ath6kl",
@@ -157,7 +300,9 @@ int ath6kl_init_debugfs(struct ath6kl *ar)
if (!ar->debugfs_phy)
return -ENOMEM;

- /* TODO: Create debugfs file entries for various target/host stats */
+ debugfs_create_file("tgt_stats", S_IRUSR, ar->debugfs_phy, ar,
+ &fops_tgt_stats);
+
return 0;
}
#endif
--
1.7.0.4


2011-08-24 14:33:43

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:30 PM, Vasanthakumar Thiagarajan wrote:
> On Wed, Aug 24, 2011 at 07:55:34PM +0530, Vasanthakumar Thiagarajan wrote:
>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath6kl/Kconfig | 6 ++++++
>> drivers/net/wireless/ath/ath6kl/core.h | 1 +
>> drivers/net/wireless/ath/ath6kl/debug.c | 13 +++++++++++++
>> drivers/net/wireless/ath/ath6kl/debug.h | 9 +++++++++
>> drivers/net/wireless/ath/ath6kl/init.c | 6 ++++++
>> 5 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/Kconfig b/drivers/net/wireless/ath/ath6kl/Kconfig
>> index 3d5f8be..ea9387b 100644
>> --- a/drivers/net/wireless/ath/ath6kl/Kconfig
>> +++ b/drivers/net/wireless/ath/ath6kl/Kconfig
>> @@ -13,3 +13,9 @@ config ATH6KL_DEBUG
>> depends on ATH6KL
>> ---help---
>> Enables debug support
>> +config ATH6KL_DEBUGFS
>> + bool "ath6kl debugfs"
>> + depends on ATH6KL_DEBUG && DEBUG_FS
>
> ATH6KL_DEBUGFS can be independent of ATH6KL_DEBUG. I'll send v2
> fixing this.

Actually do we really need ATH6KL_DEBUGFS? I would think that if both
ATH6KL_DEBUG and DEBUG_FS are enabled we should just enable debugfs code
in ath6kl. I don't see the need to have separate kconfig option control
that.

But, as usual, I could be just too blind to see the need :)

Kalle

Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On Wed, Aug 24, 2011 at 05:39:48PM +0300, Kalle Valo wrote:
> On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
> > +#ifdef CONFIG_ATH6KL_DEBUGFS
> > +int ath6kl_init_debugfs(struct ath6kl *ar);
> > +#else
> > +static inline int ath6kl_init_debugfs(struct ath6kl *ar)
> > +{
> > + return 0;
> > +}
> > +#endif
>
> I would prefer to call this ath6kl_debug_init(). That way we can put all
> debug initialisation into that, not just debugfs code. And then the
> ifdef should check for CONFIG_ATH6KL_DEBUG.

May be, but I think we can't always enable debugfs based on only
CONFIG_ATH6KL_DEBUG as the general debug does not depend on
CONFIG_DEBUG_FS.

Vasanth

Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On Wed, Aug 24, 2011 at 05:33:30PM +0300, Kalle Valo wrote:
> On 08/24/2011 05:30 PM, Vasanthakumar Thiagarajan wrote:
> > On Wed, Aug 24, 2011 at 07:55:34PM +0530, Vasanthakumar Thiagarajan wrote:
> >> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> >> ---
> >> drivers/net/wireless/ath/ath6kl/Kconfig | 6 ++++++
> >> drivers/net/wireless/ath/ath6kl/core.h | 1 +
> >> drivers/net/wireless/ath/ath6kl/debug.c | 13 +++++++++++++
> >> drivers/net/wireless/ath/ath6kl/debug.h | 9 +++++++++
> >> drivers/net/wireless/ath/ath6kl/init.c | 6 ++++++
> >> 5 files changed, 35 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/ath/ath6kl/Kconfig b/drivers/net/wireless/ath/ath6kl/Kconfig
> >> index 3d5f8be..ea9387b 100644
> >> --- a/drivers/net/wireless/ath/ath6kl/Kconfig
> >> +++ b/drivers/net/wireless/ath/ath6kl/Kconfig
> >> @@ -13,3 +13,9 @@ config ATH6KL_DEBUG
> >> depends on ATH6KL
> >> ---help---
> >> Enables debug support
> >> +config ATH6KL_DEBUGFS
> >> + bool "ath6kl debugfs"
> >> + depends on ATH6KL_DEBUG && DEBUG_FS
> >
> > ATH6KL_DEBUGFS can be independent of ATH6KL_DEBUG. I'll send v2
> > fixing this.
>
> Actually do we really need ATH6KL_DEBUGFS? I would think that if both
> ATH6KL_DEBUG and DEBUG_FS are enabled we should just enable debugfs code
> in ath6kl. I don't see the need to have separate kconfig option control
> that.

You are right. We can directly use CONFIG_DEBUG_FS to enable ath6kl
debugfs functionalities, also as I said, this does not need to
depend on ATH6KL_DEBUG. One purpose that I can think of introducing
a config option for debugfs is to reduce the size of the binary,
may be it is very insignificant.

Vasanth

Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On Wed, Aug 24, 2011 at 05:46:21PM +0300, Kalle Valo wrote:
> On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
> > +#ifdef CONFIG_ATH6KL_DEBUGFS
> > +int ath6kl_init_debugfs(struct ath6kl *ar)
> > +{
> > + ar->debugfs_phy = debugfs_create_dir("ath6kl",
> > + ar->wdev->wiphy->debugfsdir);
>
> What if we have multiple devices on same host? Then this won't work,
> right? Maybe we should have some kind of subdirectory under ath6kl or
> something like that.

I don't think we need to worry about multiple device case, it is
entirely different wiphy. May be multiple virtual interface needs to be
taken care?. Anyway, currently ath6kl does not support more than
one virtual interface, can be addressed later.

Vasanth

2011-08-24 17:37:19

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:52 PM, Vasanthakumar Thiagarajan wrote:
> On Wed, Aug 24, 2011 at 05:46:21PM +0300, Kalle Valo wrote:
>> On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
>>> +#ifdef CONFIG_ATH6KL_DEBUGFS
>>> +int ath6kl_init_debugfs(struct ath6kl *ar)
>>> +{
>>> + ar->debugfs_phy = debugfs_create_dir("ath6kl",
>>> + ar->wdev->wiphy->debugfsdir);
>>
>> What if we have multiple devices on same host? Then this won't work,
>> right? Maybe we should have some kind of subdirectory under ath6kl or
>> something like that.
>
> I don't think we need to worry about multiple device case, it is
> entirely different wiphy.

But when were are two devices, both will try create
/sys/kernel/debug/athk6kl and second one is bound to fail. Ah, but now I
see where I'm wrong. The directory will be under wiphy debugfsdir, eg.
/sys/kernel/debug/phy0/, and there's no issue with multiple devices.

Sorry, I missed this. So forget all I said :)

> May be multiple virtual interface needs to be
> taken care? Anyway, currently ath6kl does not support more than
> one virtual interface, can be addressed later.

Yeah, no need to worry that now.

Kalle

Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On Wed, Aug 24, 2011 at 07:55:34PM +0530, Vasanthakumar Thiagarajan wrote:
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> ---
> drivers/net/wireless/ath/ath6kl/Kconfig | 6 ++++++
> drivers/net/wireless/ath/ath6kl/core.h | 1 +
> drivers/net/wireless/ath/ath6kl/debug.c | 13 +++++++++++++
> drivers/net/wireless/ath/ath6kl/debug.h | 9 +++++++++
> drivers/net/wireless/ath/ath6kl/init.c | 6 ++++++
> 5 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/Kconfig b/drivers/net/wireless/ath/ath6kl/Kconfig
> index 3d5f8be..ea9387b 100644
> --- a/drivers/net/wireless/ath/ath6kl/Kconfig
> +++ b/drivers/net/wireless/ath/ath6kl/Kconfig
> @@ -13,3 +13,9 @@ config ATH6KL_DEBUG
> depends on ATH6KL
> ---help---
> Enables debug support
> +config ATH6KL_DEBUGFS
> + bool "ath6kl debugfs"
> + depends on ATH6KL_DEBUG && DEBUG_FS

ATH6KL_DEBUGFS can be independent of ATH6KL_DEBUG. I'll send v2
fixing this.

Vasanth

2011-08-24 14:40:17

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/3] ath6kl: Add initial debugfs changes

On 08/24/2011 05:25 PM, Vasanthakumar Thiagarajan wrote:
> +#ifdef CONFIG_ATH6KL_DEBUGFS
> +int ath6kl_init_debugfs(struct ath6kl *ar);
> +#else
> +static inline int ath6kl_init_debugfs(struct ath6kl *ar)
> +{
> + return 0;
> +}
> +#endif

I would prefer to call this ath6kl_debug_init(). That way we can put all
debug initialisation into that, not just debugfs code. And then the
ifdef should check for CONFIG_ATH6KL_DEBUG.

Kalle