* [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 10:48 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 10:48 UTC (permalink / raw
To: ath10k
Cc: Kalle Valo, kvalo, mohammed, linux-wireless,
Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
Enable support for 'drv_sta_statistics' callback.
Export rx_duration support if available to cfg80211/nl80211
This can also act as a placeholder for any new per STA stats support
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
[v2] fixed condition for ath10k state
Dependencies:
1. cfg80211/nl80211: Add support for NL80211_STA_INFO_RX_DURATION
2. iw: Add support for NL80211_STA_INFO_RX_DURATION netlink
attribute
3. ath10k: Enable debugfs provision to enable Peer Stats feature
drivers/net/wireless/ath/ath10k/debug.h | 4 ++++
drivers/net/wireless/ath/ath10k/debugfs_sta.c | 22 ++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/mac.c | 1 +
3 files changed, 27 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 6206edd..01c500f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -155,6 +155,10 @@ ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, struct dentry *dir);
void ath10k_sta_update_rx_duration(struct ath10k *ar, struct list_head *peer);
+void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct station_info *sinfo);
+
#else
static inline void ath10k_sta_update_rx_duration(struct ath10k *ar,
struct list_head *peer)
diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
index 67ef75b..4c459f2 100644
--- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
+++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
@@ -35,6 +35,28 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar, struct list_head *head)
rcu_read_unlock();
}
+void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct station_info *sinfo)
+{
+ struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+ struct ath10k *ar = arsta->arvif->ar;
+
+ mutex_lock(&ar->conf_mutex);
+
+ if (ar->state != ATH10K_STATE_ON &&
+ ar->state != ATH10K_STATE_RESTARTED)
+ goto out;
+
+ if (!ath10k_peer_stats_enabled(ar))
+ goto out;
+
+ sinfo->rx_duration = arsta->rx_duration;
+ sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
+out:
+ mutex_unlock(&ar->conf_mutex);
+}
+
static ssize_t ath10k_dbg_sta_read_aggr_mode(struct file *file,
char __user *user_buf,
size_t count, loff_t *ppos)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index d5ef201..69a1dcf 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6852,6 +6852,7 @@ static const struct ieee80211_ops ath10k_ops = {
#endif
#ifdef CONFIG_MAC80211_DEBUGFS
.sta_add_debugfs = ath10k_sta_add_debugfs,
+ .sta_statistics = ath10k_sta_statistics,
#endif
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 10:48 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 10:48 UTC (permalink / raw
To: ath10k
Cc: mohammed, Kalle Valo, linux-wireless, kvalo,
Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
Enable support for 'drv_sta_statistics' callback.
Export rx_duration support if available to cfg80211/nl80211
This can also act as a placeholder for any new per STA stats support
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
[v2] fixed condition for ath10k state
Dependencies:
1. cfg80211/nl80211: Add support for NL80211_STA_INFO_RX_DURATION
2. iw: Add support for NL80211_STA_INFO_RX_DURATION netlink
attribute
3. ath10k: Enable debugfs provision to enable Peer Stats feature
drivers/net/wireless/ath/ath10k/debug.h | 4 ++++
drivers/net/wireless/ath/ath10k/debugfs_sta.c | 22 ++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/mac.c | 1 +
3 files changed, 27 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 6206edd..01c500f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -155,6 +155,10 @@ ath10k_debug_get_new_fw_crash_data(struct ath10k *ar)
void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, struct dentry *dir);
void ath10k_sta_update_rx_duration(struct ath10k *ar, struct list_head *peer);
+void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct station_info *sinfo);
+
#else
static inline void ath10k_sta_update_rx_duration(struct ath10k *ar,
struct list_head *peer)
diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
index 67ef75b..4c459f2 100644
--- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
+++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
@@ -35,6 +35,28 @@ void ath10k_sta_update_rx_duration(struct ath10k *ar, struct list_head *head)
rcu_read_unlock();
}
+void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct station_info *sinfo)
+{
+ struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+ struct ath10k *ar = arsta->arvif->ar;
+
+ mutex_lock(&ar->conf_mutex);
+
+ if (ar->state != ATH10K_STATE_ON &&
+ ar->state != ATH10K_STATE_RESTARTED)
+ goto out;
+
+ if (!ath10k_peer_stats_enabled(ar))
+ goto out;
+
+ sinfo->rx_duration = arsta->rx_duration;
+ sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
+out:
+ mutex_unlock(&ar->conf_mutex);
+}
+
static ssize_t ath10k_dbg_sta_read_aggr_mode(struct file *file,
char __user *user_buf,
size_t count, loff_t *ppos)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index d5ef201..69a1dcf 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6852,6 +6852,7 @@ static const struct ieee80211_ops ath10k_ops = {
#endif
#ifdef CONFIG_MAC80211_DEBUGFS
.sta_add_debugfs = ath10k_sta_add_debugfs,
+ .sta_statistics = ath10k_sta_statistics,
#endif
};
--
1.7.9.5
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] ath10k: Remove debugfs support for Per STA total rx duration
2016-03-17 10:48 ` Mohammed Shafi Shajakhan
@ 2016-03-17 10:49 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 10:49 UTC (permalink / raw
To: ath10k
Cc: Kalle Valo, kvalo, mohammed, linux-wireless,
Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
cfg80211/nl80211 interface changes for per STA total rx-duration and
very basic 'ath10k_sta_statistics' mac80211 callback is implemented
to extend support for per station statistics from the driver.
Also provision in 'iw dev wlan#N station dump' to parse rx-duration
is supported. So its safer to remove the debugfs entry for per STA
rx-duration
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
Dependencies:
1. cfg80211/nl80211: Add support for NL80211_STA_INFO_RX_DURATION
2. iw: Add support for NL80211_STA_INFO_RX_DURATION netlink.
3. ath10k: Add support for ath10k_sta_statistics support
4. ath10k: Enable debugfs provision to enable Peer Stats feature
drivers/net/wireless/ath/ath10k/debugfs_sta.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
index 4c459f2..0f24327 100644
--- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
+++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
@@ -271,28 +271,6 @@ static const struct file_operations fops_delba = {
.llseek = default_llseek,
};
-static ssize_t ath10k_dbg_sta_read_rx_duration(struct file *file,
- char __user *user_buf,
- size_t count, loff_t *ppos)
-{
- struct ieee80211_sta *sta = file->private_data;
- struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
- char buf[100];
- int len = 0;
-
- len = scnprintf(buf, sizeof(buf),
- "%llu usecs\n", arsta->rx_duration);
-
- return simple_read_from_buffer(user_buf, count, ppos, buf, len);
-}
-
-static const struct file_operations fops_rx_duration = {
- .read = ath10k_dbg_sta_read_rx_duration,
- .open = simple_open,
- .owner = THIS_MODULE,
- .llseek = default_llseek,
-};
-
void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, struct dentry *dir)
{
@@ -301,6 +279,4 @@ void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
debugfs_create_file("addba", S_IWUSR, dir, sta, &fops_addba);
debugfs_create_file("addba_resp", S_IWUSR, dir, sta, &fops_addba_resp);
debugfs_create_file("delba", S_IWUSR, dir, sta, &fops_delba);
- debugfs_create_file("rx_duration", S_IRUGO, dir, sta,
- &fops_rx_duration);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] ath10k: Remove debugfs support for Per STA total rx duration
@ 2016-03-17 10:49 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 10:49 UTC (permalink / raw
To: ath10k
Cc: mohammed, Kalle Valo, linux-wireless, kvalo,
Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
cfg80211/nl80211 interface changes for per STA total rx-duration and
very basic 'ath10k_sta_statistics' mac80211 callback is implemented
to extend support for per station statistics from the driver.
Also provision in 'iw dev wlan#N station dump' to parse rx-duration
is supported. So its safer to remove the debugfs entry for per STA
rx-duration
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
Dependencies:
1. cfg80211/nl80211: Add support for NL80211_STA_INFO_RX_DURATION
2. iw: Add support for NL80211_STA_INFO_RX_DURATION netlink.
3. ath10k: Add support for ath10k_sta_statistics support
4. ath10k: Enable debugfs provision to enable Peer Stats feature
drivers/net/wireless/ath/ath10k/debugfs_sta.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
index 4c459f2..0f24327 100644
--- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
+++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
@@ -271,28 +271,6 @@ static const struct file_operations fops_delba = {
.llseek = default_llseek,
};
-static ssize_t ath10k_dbg_sta_read_rx_duration(struct file *file,
- char __user *user_buf,
- size_t count, loff_t *ppos)
-{
- struct ieee80211_sta *sta = file->private_data;
- struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
- char buf[100];
- int len = 0;
-
- len = scnprintf(buf, sizeof(buf),
- "%llu usecs\n", arsta->rx_duration);
-
- return simple_read_from_buffer(user_buf, count, ppos, buf, len);
-}
-
-static const struct file_operations fops_rx_duration = {
- .read = ath10k_dbg_sta_read_rx_duration,
- .open = simple_open,
- .owner = THIS_MODULE,
- .llseek = default_llseek,
-};
-
void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, struct dentry *dir)
{
@@ -301,6 +279,4 @@ void ath10k_sta_add_debugfs(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
debugfs_create_file("addba", S_IWUSR, dir, sta, &fops_addba);
debugfs_create_file("addba_resp", S_IWUSR, dir, sta, &fops_addba_resp);
debugfs_create_file("delba", S_IWUSR, dir, sta, &fops_delba);
- debugfs_create_file("rx_duration", S_IRUGO, dir, sta,
- &fops_rx_duration);
}
--
1.7.9.5
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 10:48 ` Mohammed Shafi Shajakhan
@ 2016-03-17 11:04 ` kbuild test robot
-1 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2016-03-17 11:04 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: kbuild-all, ath10k, Kalle Valo, kvalo, mohammed, linux-wireless,
Mohammed Shafi Shajakhan
[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]
Hi Mohammed,
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on next-20160317]
[cannot apply to v4.5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Mohammed-Shafi-Shajakhan/ath10k-Add-support-for-ath10k_sta_statistics-support/20160317-185148
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: xtensa-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/net/wireless/ath/ath10k/debugfs_sta.c: In function 'ath10k_sta_statistics':
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:51:2: error: implicit declaration of function 'ath10k_peer_stats_enabled' [-Werror=implicit-function-declaration]
if (!ath10k_peer_stats_enabled(ar))
^
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:54:7: error: 'struct station_info' has no member named 'rx_duration'
sinfo->rx_duration = arsta->rx_duration;
^
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:55:27: error: 'NL80211_STA_INFO_RX_DURATION' undeclared (first use in this function)
sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
^
drivers/net/wireless/ath/ath10k/debugfs_sta.c:55:27: note: each undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors
vim +/ath10k_peer_stats_enabled +51 drivers/net/wireless/ath/ath10k/debugfs_sta.c
45 mutex_lock(&ar->conf_mutex);
46
47 if (ar->state != ATH10K_STATE_ON &&
48 ar->state != ATH10K_STATE_RESTARTED)
49 goto out;
50
> 51 if (!ath10k_peer_stats_enabled(ar))
52 goto out;
53
> 54 sinfo->rx_duration = arsta->rx_duration;
> 55 sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
56 out:
57 mutex_unlock(&ar->conf_mutex);
58 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44126 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 11:04 ` kbuild test robot
0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2016-03-17 11:04 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: linux-wireless, ath10k, mohammed, Kalle Valo, kbuild-all, kvalo
[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]
Hi Mohammed,
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on next-20160317]
[cannot apply to v4.5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Mohammed-Shafi-Shajakhan/ath10k-Add-support-for-ath10k_sta_statistics-support/20160317-185148
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: xtensa-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/net/wireless/ath/ath10k/debugfs_sta.c: In function 'ath10k_sta_statistics':
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:51:2: error: implicit declaration of function 'ath10k_peer_stats_enabled' [-Werror=implicit-function-declaration]
if (!ath10k_peer_stats_enabled(ar))
^
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:54:7: error: 'struct station_info' has no member named 'rx_duration'
sinfo->rx_duration = arsta->rx_duration;
^
>> drivers/net/wireless/ath/ath10k/debugfs_sta.c:55:27: error: 'NL80211_STA_INFO_RX_DURATION' undeclared (first use in this function)
sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
^
drivers/net/wireless/ath/ath10k/debugfs_sta.c:55:27: note: each undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors
vim +/ath10k_peer_stats_enabled +51 drivers/net/wireless/ath/ath10k/debugfs_sta.c
45 mutex_lock(&ar->conf_mutex);
46
47 if (ar->state != ATH10K_STATE_ON &&
48 ar->state != ATH10K_STATE_RESTARTED)
49 goto out;
50
> 51 if (!ath10k_peer_stats_enabled(ar))
52 goto out;
53
> 54 sinfo->rx_duration = arsta->rx_duration;
> 55 sinfo->filled |= 1ULL << NL80211_STA_INFO_RX_DURATION;
56 out:
57 mutex_unlock(&ar->conf_mutex);
58 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44126 bytes --]
[-- Attachment #3: Type: text/plain, Size: 146 bytes --]
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 10:48 ` Mohammed Shafi Shajakhan
@ 2016-03-17 11:12 ` Michal Kazior
-1 siblings, 0 replies; 16+ messages in thread
From: Michal Kazior @ 2016-03-17 11:12 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: ath10k@lists.infradead.org, Kalle Valo, kvalo@codeaurora.org,
mohammed, linux-wireless
On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
<mohammed@qti.qualcomm.com> wrote:
[...]
> +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + struct station_info *sinfo)
> +{
> + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> + struct ath10k *ar = arsta->arvif->ar;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + if (ar->state != ATH10K_STATE_ON &&
> + ar->state != ATH10K_STATE_RESTARTED)
> + goto out;
Do you really need mutex and ar->state check in this function?
Michał
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 11:12 ` Michal Kazior
0 siblings, 0 replies; 16+ messages in thread
From: Michal Kazior @ 2016-03-17 11:12 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: mohammed, Kalle Valo, linux-wireless, kvalo@codeaurora.org,
ath10k@lists.infradead.org
On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
<mohammed@qti.qualcomm.com> wrote:
[...]
> +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + struct station_info *sinfo)
> +{
> + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> + struct ath10k *ar = arsta->arvif->ar;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + if (ar->state != ATH10K_STATE_ON &&
> + ar->state != ATH10K_STATE_RESTARTED)
> + goto out;
Do you really need mutex and ar->state check in this function?
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 11:12 ` Michal Kazior
@ 2016-03-17 11:20 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 11:20 UTC (permalink / raw
To: Michal Kazior
Cc: Mohammed Shafi Shajakhan, ath10k@lists.infradead.org, Kalle Valo,
kvalo@codeaurora.org, linux-wireless
Hi Michal,
On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> <mohammed@qti.qualcomm.com> wrote:
> [...]
> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > + struct ieee80211_sta *sta,
> > + struct station_info *sinfo)
> > +{
> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> > + struct ath10k *ar = arsta->arvif->ar;
> > +
> > + mutex_lock(&ar->conf_mutex);
> > +
> > + if (ar->state != ATH10K_STATE_ON &&
> > + ar->state != ATH10K_STATE_RESTARTED)
> > + goto out;
>
> Do you really need mutex and ar->state check in this function?
>
[shafi] By default peer stats will be disabled, we are enabling this by debugfs
(hw-restart) so i thought these checks are needed , please advise .. Do you say
they will be never hit
regards
shafi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 11:20 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 11:20 UTC (permalink / raw
To: Michal Kazior
Cc: Kalle Valo, linux-wireless, kvalo@codeaurora.org,
ath10k@lists.infradead.org, Mohammed Shafi Shajakhan
Hi Michal,
On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> <mohammed@qti.qualcomm.com> wrote:
> [...]
> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > + struct ieee80211_sta *sta,
> > + struct station_info *sinfo)
> > +{
> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> > + struct ath10k *ar = arsta->arvif->ar;
> > +
> > + mutex_lock(&ar->conf_mutex);
> > +
> > + if (ar->state != ATH10K_STATE_ON &&
> > + ar->state != ATH10K_STATE_RESTARTED)
> > + goto out;
>
> Do you really need mutex and ar->state check in this function?
>
[shafi] By default peer stats will be disabled, we are enabling this by debugfs
(hw-restart) so i thought these checks are needed , please advise .. Do you say
they will be never hit
regards
shafi
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 11:20 ` Mohammed Shafi Shajakhan
@ 2016-03-17 11:35 ` Michal Kazior
-1 siblings, 0 replies; 16+ messages in thread
From: Michal Kazior @ 2016-03-17 11:35 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: Mohammed Shafi Shajakhan, ath10k@lists.infradead.org, Kalle Valo,
kvalo@codeaurora.org, linux-wireless
On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
<mohammed@codeaurora.org> wrote:
> Hi Michal,
>
> On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
>> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
>> <mohammed@qti.qualcomm.com> wrote:
>> [...]
>> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> > + struct ieee80211_sta *sta,
>> > + struct station_info *sinfo)
>> > +{
>> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
>> > + struct ath10k *ar = arsta->arvif->ar;
>> > +
>> > + mutex_lock(&ar->conf_mutex);
>> > +
>> > + if (ar->state != ATH10K_STATE_ON &&
>> > + ar->state != ATH10K_STATE_RESTARTED)
>> > + goto out;
>>
>> Do you really need mutex and ar->state check in this function?
>>
>
> [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> (hw-restart) so i thought these checks are needed , please advise .. Do you say
> they will be never hit
Hmm.. I think mac80211 shouldn't call sta_statistics() before
sta_state() during recovery (it makes no sense). In practice I think
this isn't enforced in which case it's a mac80211 bug.
Anyway, this isn't much of a problem now. You only read out u64 from
`arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
no way for you to crash the system (it's not a list, spinlock or any
other complex data structure). Worst case userspace will get garbage
rx_duration value if it happens to get_station() while hw-restart is
ongoing.
It'd be good to verify this is actually a problem and - assuming you
want to guarantee correct readouts at all times - to fix the problem
in mac80211.
Michał
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 11:35 ` Michal Kazior
0 siblings, 0 replies; 16+ messages in thread
From: Michal Kazior @ 2016-03-17 11:35 UTC (permalink / raw
To: Mohammed Shafi Shajakhan
Cc: Kalle Valo, linux-wireless, kvalo@codeaurora.org,
ath10k@lists.infradead.org, Mohammed Shafi Shajakhan
On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
<mohammed@codeaurora.org> wrote:
> Hi Michal,
>
> On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
>> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
>> <mohammed@qti.qualcomm.com> wrote:
>> [...]
>> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> > + struct ieee80211_sta *sta,
>> > + struct station_info *sinfo)
>> > +{
>> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
>> > + struct ath10k *ar = arsta->arvif->ar;
>> > +
>> > + mutex_lock(&ar->conf_mutex);
>> > +
>> > + if (ar->state != ATH10K_STATE_ON &&
>> > + ar->state != ATH10K_STATE_RESTARTED)
>> > + goto out;
>>
>> Do you really need mutex and ar->state check in this function?
>>
>
> [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> (hw-restart) so i thought these checks are needed , please advise .. Do you say
> they will be never hit
Hmm.. I think mac80211 shouldn't call sta_statistics() before
sta_state() during recovery (it makes no sense). In practice I think
this isn't enforced in which case it's a mac80211 bug.
Anyway, this isn't much of a problem now. You only read out u64 from
`arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
no way for you to crash the system (it's not a list, spinlock or any
other complex data structure). Worst case userspace will get garbage
rx_duration value if it happens to get_station() while hw-restart is
ongoing.
It'd be good to verify this is actually a problem and - assuming you
want to guarantee correct readouts at all times - to fix the problem
in mac80211.
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 11:35 ` Michal Kazior
@ 2016-03-17 12:30 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 12:30 UTC (permalink / raw
To: Michal Kazior
Cc: Mohammed Shafi Shajakhan, ath10k@lists.infradead.org, Kalle Valo,
kvalo@codeaurora.org, linux-wireless
Hi Michal,
thanks for the comments ..
On Thu, Mar 17, 2016 at 12:35:00PM +0100, Michal Kazior wrote:
> On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
> <mohammed@codeaurora.org> wrote:
> > Hi Michal,
> >
> > On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> >> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> >> <mohammed@qti.qualcomm.com> wrote:
> >> [...]
> >> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >> > + struct ieee80211_sta *sta,
> >> > + struct station_info *sinfo)
> >> > +{
> >> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> >> > + struct ath10k *ar = arsta->arvif->ar;
> >> > +
> >> > + mutex_lock(&ar->conf_mutex);
> >> > +
> >> > + if (ar->state != ATH10K_STATE_ON &&
> >> > + ar->state != ATH10K_STATE_RESTARTED)
> >> > + goto out;
> >>
> >> Do you really need mutex and ar->state check in this function?
> >>
> >
> > [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> > (hw-restart) so i thought these checks are needed , please advise .. Do you say
> > they will be never hit
>
> Hmm.. I think mac80211 shouldn't call sta_statistics() before
> sta_state() during recovery (it makes no sense). In practice I think
> this isn't enforced in which case it's a mac80211 bug.
[shafi] sure i will check this. If the hardware is restarting there should
be no stations connected and station related info.
>
> Anyway, this isn't much of a problem now. You only read out u64 from
> `arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
> no way for you to crash the system (it's not a list, spinlock or any
> other complex data structure). Worst case userspace will get garbage
> rx_duration value if it happens to get_station() while hw-restart is
> ongoing.
[shafi] will check this ..
>
> It'd be good to verify this is actually a problem and - assuming you
> want to guarantee correct readouts at all times - to fix the problem
> in mac80211.
>
[shafi] ok, sure
-shafi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-17 12:30 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-17 12:30 UTC (permalink / raw
To: Michal Kazior
Cc: Kalle Valo, linux-wireless, kvalo@codeaurora.org,
ath10k@lists.infradead.org, Mohammed Shafi Shajakhan
Hi Michal,
thanks for the comments ..
On Thu, Mar 17, 2016 at 12:35:00PM +0100, Michal Kazior wrote:
> On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
> <mohammed@codeaurora.org> wrote:
> > Hi Michal,
> >
> > On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> >> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> >> <mohammed@qti.qualcomm.com> wrote:
> >> [...]
> >> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >> > + struct ieee80211_sta *sta,
> >> > + struct station_info *sinfo)
> >> > +{
> >> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> >> > + struct ath10k *ar = arsta->arvif->ar;
> >> > +
> >> > + mutex_lock(&ar->conf_mutex);
> >> > +
> >> > + if (ar->state != ATH10K_STATE_ON &&
> >> > + ar->state != ATH10K_STATE_RESTARTED)
> >> > + goto out;
> >>
> >> Do you really need mutex and ar->state check in this function?
> >>
> >
> > [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> > (hw-restart) so i thought these checks are needed , please advise .. Do you say
> > they will be never hit
>
> Hmm.. I think mac80211 shouldn't call sta_statistics() before
> sta_state() during recovery (it makes no sense). In practice I think
> this isn't enforced in which case it's a mac80211 bug.
[shafi] sure i will check this. If the hardware is restarting there should
be no stations connected and station related info.
>
> Anyway, this isn't much of a problem now. You only read out u64 from
> `arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
> no way for you to crash the system (it's not a list, spinlock or any
> other complex data structure). Worst case userspace will get garbage
> rx_duration value if it happens to get_station() while hw-restart is
> ongoing.
[shafi] will check this ..
>
> It'd be good to verify this is actually a problem and - assuming you
> want to guarantee correct readouts at all times - to fix the problem
> in mac80211.
>
[shafi] ok, sure
-shafi
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
2016-03-17 12:30 ` Mohammed Shafi Shajakhan
@ 2016-03-24 12:01 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-24 12:01 UTC (permalink / raw
To: Michal Kazior
Cc: Mohammed Shafi Shajakhan, ath10k@lists.infradead.org, Kalle Valo,
kvalo@codeaurora.org, linux-wireless
Hi Michal / Kalle,
>
> On Thu, Mar 17, 2016 at 12:35:00PM +0100, Michal Kazior wrote:
> > On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
> > <mohammed@codeaurora.org> wrote:
> > > Hi Michal,
> > >
> > > On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> > >> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> > >> <mohammed@qti.qualcomm.com> wrote:
> > >> [...]
> > >> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > >> > + struct ieee80211_sta *sta,
> > >> > + struct station_info *sinfo)
> > >> > +{
> > >> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> > >> > + struct ath10k *ar = arsta->arvif->ar;
> > >> > +
> > >> > + mutex_lock(&ar->conf_mutex);
> > >> > +
> > >> > + if (ar->state != ATH10K_STATE_ON &&
> > >> > + ar->state != ATH10K_STATE_RESTARTED)
> > >> > + goto out;
> > >>
> > >> Do you really need mutex and ar->state check in this function?
> > >>
> > >
> > > [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> > > (hw-restart) so i thought these checks are needed , please advise .. Do you say
> > > they will be never hit
> >
> > Hmm.. I think mac80211 shouldn't call sta_statistics() before
> > sta_state() during recovery (it makes no sense). In practice I think
> > this isn't enforced in which case it's a mac80211 bug.
>
> [shafi] sure i will check this. If the hardware is restarting there should
> be no stations connected and station related info.
[shafi] I did not see any checks in mac80211 to prevent this (may be i missed
something), but i did remove the checks and tested it, could not find warnings, crashes
(or) garbage values, so is it safer to remove (unless we find some issue with
this) ?, please suggest Michal.
i was doing this parallely in two terminals
a) while true
do
cat rx_stats | grep duration -A 2
done
b) /sys/kernel/debug/ieee80211/phy0/ath10k# echo 1 > peer_stats
/sys/kernel/debug/ieee80211/phy0/ath10k# echo 0 > peer_stats
(or)
/sys/kernel/debug/ieee80211/phy0/ath10k# echo hw-restart > simulate_fw_crash
>
> >
> > Anyway, this isn't much of a problem now. You only read out u64 from
> > `arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
> > no way for you to crash the system (it's not a list, spinlock or any
> > other complex data structure). Worst case userspace will get garbage
> > rx_duration value if it happens to get_station() while hw-restart is
> > ongoing.
>
> [shafi] will check this ..
[shafi] agree though i could not see any checks in mac80211 to prevent
sta_set_sinfo (or check for the station state), i did not see any issues in my
testing
>
> >
> > It'd be good to verify this is actually a problem and - assuming you
> > want to guarantee correct readouts at all times - to fix the problem
> > in mac80211.
> >
> [shafi] ok, sure
[shafi] I really don't have a deeper understanding of mac80211 code,
i need to go through it thoroughly if i need to have checks for this
if they were not already there.
regards,
shafi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support
@ 2016-03-24 12:01 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 16+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-03-24 12:01 UTC (permalink / raw
To: Michal Kazior
Cc: Kalle Valo, linux-wireless, kvalo@codeaurora.org,
ath10k@lists.infradead.org, Mohammed Shafi Shajakhan
Hi Michal / Kalle,
>
> On Thu, Mar 17, 2016 at 12:35:00PM +0100, Michal Kazior wrote:
> > On 17 March 2016 at 12:20, Mohammed Shafi Shajakhan
> > <mohammed@codeaurora.org> wrote:
> > > Hi Michal,
> > >
> > > On Thu, Mar 17, 2016 at 12:12:31PM +0100, Michal Kazior wrote:
> > >> On 17 March 2016 at 11:48, Mohammed Shafi Shajakhan
> > >> <mohammed@qti.qualcomm.com> wrote:
> > >> [...]
> > >> > +void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > >> > + struct ieee80211_sta *sta,
> > >> > + struct station_info *sinfo)
> > >> > +{
> > >> > + struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> > >> > + struct ath10k *ar = arsta->arvif->ar;
> > >> > +
> > >> > + mutex_lock(&ar->conf_mutex);
> > >> > +
> > >> > + if (ar->state != ATH10K_STATE_ON &&
> > >> > + ar->state != ATH10K_STATE_RESTARTED)
> > >> > + goto out;
> > >>
> > >> Do you really need mutex and ar->state check in this function?
> > >>
> > >
> > > [shafi] By default peer stats will be disabled, we are enabling this by debugfs
> > > (hw-restart) so i thought these checks are needed , please advise .. Do you say
> > > they will be never hit
> >
> > Hmm.. I think mac80211 shouldn't call sta_statistics() before
> > sta_state() during recovery (it makes no sense). In practice I think
> > this isn't enforced in which case it's a mac80211 bug.
>
> [shafi] sure i will check this. If the hardware is restarting there should
> be no stations connected and station related info.
[shafi] I did not see any checks in mac80211 to prevent this (may be i missed
something), but i did remove the checks and tested it, could not find warnings, crashes
(or) garbage values, so is it safer to remove (unless we find some issue with
this) ?, please suggest Michal.
i was doing this parallely in two terminals
a) while true
do
cat rx_stats | grep duration -A 2
done
b) /sys/kernel/debug/ieee80211/phy0/ath10k# echo 1 > peer_stats
/sys/kernel/debug/ieee80211/phy0/ath10k# echo 0 > peer_stats
(or)
/sys/kernel/debug/ieee80211/phy0/ath10k# echo hw-restart > simulate_fw_crash
>
> >
> > Anyway, this isn't much of a problem now. You only read out u64 from
> > `arsta` (sta->drv_priv). Even if it's uninitialized/undefined there's
> > no way for you to crash the system (it's not a list, spinlock or any
> > other complex data structure). Worst case userspace will get garbage
> > rx_duration value if it happens to get_station() while hw-restart is
> > ongoing.
>
> [shafi] will check this ..
[shafi] agree though i could not see any checks in mac80211 to prevent
sta_set_sinfo (or check for the station state), i did not see any issues in my
testing
>
> >
> > It'd be good to verify this is actually a problem and - assuming you
> > want to guarantee correct readouts at all times - to fix the problem
> > in mac80211.
> >
> [shafi] ok, sure
[shafi] I really don't have a deeper understanding of mac80211 code,
i need to go through it thoroughly if i need to have checks for this
if they were not already there.
regards,
shafi
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-03-24 12:02 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-17 10:48 [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support Mohammed Shafi Shajakhan
2016-03-17 10:48 ` Mohammed Shafi Shajakhan
2016-03-17 10:49 ` [PATCH v2 2/2] ath10k: Remove debugfs support for Per STA total rx duration Mohammed Shafi Shajakhan
2016-03-17 10:49 ` Mohammed Shafi Shajakhan
2016-03-17 11:04 ` [PATCH v2 1/2] ath10k: Add support for ath10k_sta_statistics support kbuild test robot
2016-03-17 11:04 ` kbuild test robot
2016-03-17 11:12 ` Michal Kazior
2016-03-17 11:12 ` Michal Kazior
2016-03-17 11:20 ` Mohammed Shafi Shajakhan
2016-03-17 11:20 ` Mohammed Shafi Shajakhan
2016-03-17 11:35 ` Michal Kazior
2016-03-17 11:35 ` Michal Kazior
2016-03-17 12:30 ` Mohammed Shafi Shajakhan
2016-03-17 12:30 ` Mohammed Shafi Shajakhan
2016-03-24 12:01 ` Mohammed Shafi Shajakhan
2016-03-24 12:01 ` Mohammed Shafi Shajakhan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.