| Message ID | 20161217174634.4531-1-chunkeey@googlemail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi Christian, On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > The 10.4 firmware adds extended peer information to the > firmware's statistics payload. This additional info is > stored as a separate data field. During review of > "ath10k: add accounting for the extended peer statistics" [0] > > Mohammed Shafi Shajakhan commented that the extended peer statistics > lists are of little use:"... there is not much use in appending > the extended peer stats (which gets periodically updated) to the > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > rid of the below (and the required cleanup as well) > > list_splice_tail_init(&stats.peers_extd, > &ar->debug.fw_stats.peers_extd); > > since rx_duration is getting updated periodically to the per sta > information." [shafi] thanks ! > > This patch replaces the extended peers list with a lookup and > puts the retrieved data (rx_duration) into the existing > ath10k_fw_stats_peer entry that was created earlier. [shafi] Its good to maintain the extended stats variable and retain the two different functions to update rx_duration. a) extended peer stats supported - mainly for 10.4 b) extender peer stats not supported - for 10.2 > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > --- > drivers/net/wireless/ath/ath10k/core.h | 2 -- > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > 4 files changed, 28 insertions(+), 57 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > index 09ff8b8a6441..3fffbbb18c25 100644 > --- a/drivers/net/wireless/ath/ath10k/core.h > +++ b/drivers/net/wireless/ath/ath10k/core.h > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > }; > > struct ath10k_fw_stats { > - bool extended; > struct list_head pdevs; > struct list_head vdevs; > struct list_head peers; > - struct list_head peers_extd; > }; > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > index 82a4c67f3672..89f7fde77cdf 100644 > --- a/drivers/net/wireless/ath/ath10k/debug.c > +++ b/drivers/net/wireless/ath/ath10k/debug.c > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > } > } > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > -{ > - struct ath10k_fw_extd_stats_peer *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > - > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > { > spin_lock_bh(&ar->data_lock); > ar->debug.fw_stats_done = false; > - ar->debug.fw_stats.extended = false; [shafi] this looks fine, but not removing the 'extended' variable from ath10k_fw_stats structure, I see the design for 'rx_duration' arguably some what convoluted ! *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' {certainly 'stats' object is for this particular update only, and freed up later) *Update immediately using 'ath10k_sta_update_rx_duration' 'ath10k_wmi_pull_fw_stats' has a slightly different implementation for 10.2 and 10.4 (the later supporting extended peer stats) > ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers); > - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd); > spin_unlock_bh(&ar->data_lock); > } > > @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > INIT_LIST_HEAD(&stats.pdevs); > INIT_LIST_HEAD(&stats.vdevs); > INIT_LIST_HEAD(&stats.peers); > - INIT_LIST_HEAD(&stats.peers_extd); > > spin_lock_bh(&ar->data_lock); > ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats); > @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers); > list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs); > - list_splice_tail_init(&stats.peers_extd, > - &ar->debug.fw_stats.peers_extd); > } > > complete(&ar->debug.fw_stats_complete); > @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > ath10k_fw_stats_pdevs_free(&stats.pdevs); > ath10k_fw_stats_vdevs_free(&stats.vdevs); > ath10k_fw_stats_peers_free(&stats.peers); > - ath10k_fw_extd_stats_peers_free(&stats.peers_extd); > > spin_unlock_bh(&ar->data_lock); > } > @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar) > INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > INIT_LIST_HEAD(&ar->debug.fw_stats.peers); > - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd); > > return 0; > } > diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c > index fce6f8137d33..bf2d49cbb3bb 100644 > --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c > +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c > @@ -18,27 +18,8 @@ > #include "wmi-ops.h" > #include "debug.h" > > -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar, > - struct ath10k_fw_stats *stats) > -{ > - struct ath10k_fw_extd_stats_peer *peer; > - struct ieee80211_sta *sta; > - struct ath10k_sta *arsta; > - > - rcu_read_lock(); > - list_for_each_entry(peer, &stats->peers_extd, list) { > - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr, > - NULL); > - if (!sta) > - continue; > - arsta = (struct ath10k_sta *)sta->drv_priv; > - arsta->rx_duration += (u64)peer->rx_duration; > - } > - rcu_read_unlock(); > -} > - > -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, > - struct ath10k_fw_stats *stats) > +void ath10k_sta_update_rx_duration(struct ath10k *ar, > + struct ath10k_fw_stats *stats) > { > struct ath10k_fw_stats_peer *peer; > struct ieee80211_sta *sta; > @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, > rcu_read_unlock(); > } > > -void ath10k_sta_update_rx_duration(struct ath10k *ar, > - struct ath10k_fw_stats *stats) > -{ > - if (stats->extended) > - ath10k_sta_update_extd_stats_rx_duration(ar, stats); > - else > - ath10k_sta_update_stats_rx_duration(ar, stats); > -} > - > void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > struct ieee80211_sta *sta, > struct station_info *sinfo) > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index c893314a191f..c7ec7b9e9b55 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > return 0; > > - stats->extended = true; > - > for (i = 0; i < num_peer_stats; i++) { > const struct wmi_10_4_peer_extd_stats *src; > - struct ath10k_fw_extd_stats_peer *dst; > + struct ath10k_fw_stats_peer *dst; > > src = (void *)skb->data; > if (!skb_pull(skb, sizeof(*src))) > return -EPROTO; > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > - if (!dst) > - continue; > + /* Because the stat data may exceed htc-wmi buffer > + * limit the firmware might split the stats data > + * and delivers it in multiple update events. > + * if we can't find the entry in the current event > + * payload, we have to look in main list as well. > + */ > + list_for_each_entry(dst, &stats->peers, list) { > + if (ether_addr_equal(dst->peer_macaddr, > + src->peer_macaddr.addr)) > + goto found; > + } > + > +#ifdef CONFIG_ATH10K_DEBUGFS > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > + if (ether_addr_equal(dst->peer_macaddr, > + src->peer_macaddr.addr)) > + goto found; > + } > +#endif > + > + ath10k_dbg(ar, ATH10K_DBG_WMI, > + "Orphaned extended stats entry for station %pM.\n", > + src->peer_macaddr.addr); > + continue; > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > +found: > dst->rx_duration = __le32_to_cpu(src->rx_duration); > - list_add_tail(&dst->list, &stats->peers_extd); > } > > return 0; [shafi] Yes i am bit concerned about this change making 10.4 to update over the existing peer_stats structure, the idea is to maintain uniformity between the structures shared between ath10k and its corresponding to avoid confusion/ bugs in the future. Kindly let me know your thoughts, feel free to correct me if any of my analysis is incorrect. thank you ! regards, shafi
On Mon, Dec 19, 2016 at 10:19:57PM +0530, Mohammed Shafi Shajakhan wrote: > Hi Christian, > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > > The 10.4 firmware adds extended peer information to the > > firmware's statistics payload. This additional info is > > stored as a separate data field. During review of > > "ath10k: add accounting for the extended peer statistics" [0] > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics > > lists are of little use:"... there is not much use in appending > > the extended peer stats (which gets periodically updated) to the > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > > rid of the below (and the required cleanup as well) > > > > list_splice_tail_init(&stats.peers_extd, > > &ar->debug.fw_stats.peers_extd); > > > > since rx_duration is getting updated periodically to the per sta > > information." > > [shafi] thanks ! > > > > > This patch replaces the extended peers list with a lookup and > > puts the retrieved data (rx_duration) into the existing > > ath10k_fw_stats_peer entry that was created earlier. > > [shafi] Its good to maintain the extended stats variable > and retain the two different functions to update rx_duration. > > a) extended peer stats supported - mainly for 10.4 > b) extender peer stats not supported - for 10.2 > > > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > > --- > > drivers/net/wireless/ath/ath10k/core.h | 2 -- > > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > > 4 files changed, 28 insertions(+), 57 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > > index 09ff8b8a6441..3fffbbb18c25 100644 > > --- a/drivers/net/wireless/ath/ath10k/core.h > > +++ b/drivers/net/wireless/ath/ath10k/core.h > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > > }; > > > > struct ath10k_fw_stats { > > - bool extended; > > struct list_head pdevs; > > struct list_head vdevs; > > struct list_head peers; > > - struct list_head peers_extd; > > }; > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > > index 82a4c67f3672..89f7fde77cdf 100644 > > --- a/drivers/net/wireless/ath/ath10k/debug.c > > +++ b/drivers/net/wireless/ath/ath10k/debug.c > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > > } > > } > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > > -{ > > - struct ath10k_fw_extd_stats_peer *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > > { > > spin_lock_bh(&ar->data_lock); > > ar->debug.fw_stats_done = false; > > - ar->debug.fw_stats.extended = false; > > [shafi] this looks fine, but not removing the 'extended' variable > from ath10k_fw_stats structure, I see the design for 'rx_duration' > arguably some what convoluted ! > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' > {certainly 'stats' object is for this particular update only, and freed > up later) > *Update immediately using 'ath10k_sta_update_rx_duration' > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation > for 10.2 and 10.4 (the later supporting extended peer stats) > > > > > ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > > ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers); > > - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd); > > spin_unlock_bh(&ar->data_lock); > > } > > > > @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > INIT_LIST_HEAD(&stats.pdevs); > > INIT_LIST_HEAD(&stats.vdevs); > > INIT_LIST_HEAD(&stats.peers); > > - INIT_LIST_HEAD(&stats.peers_extd); > > > > spin_lock_bh(&ar->data_lock); > > ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats); > > @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > > > list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers); > > list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs); > > - list_splice_tail_init(&stats.peers_extd, > > - &ar->debug.fw_stats.peers_extd); > > } > > > > complete(&ar->debug.fw_stats_complete); > > @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > ath10k_fw_stats_pdevs_free(&stats.pdevs); > > ath10k_fw_stats_vdevs_free(&stats.vdevs); > > ath10k_fw_stats_peers_free(&stats.peers); > > - ath10k_fw_extd_stats_peers_free(&stats.peers_extd); > > > > spin_unlock_bh(&ar->data_lock); > > } > > @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar) > > INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > > INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > > INIT_LIST_HEAD(&ar->debug.fw_stats.peers); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd); > > > > return 0; > > } > > diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c > > index fce6f8137d33..bf2d49cbb3bb 100644 > > --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c > > +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c > > @@ -18,27 +18,8 @@ > > #include "wmi-ops.h" > > #include "debug.h" > > > > -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar, > > - struct ath10k_fw_stats *stats) > > -{ > > - struct ath10k_fw_extd_stats_peer *peer; > > - struct ieee80211_sta *sta; > > - struct ath10k_sta *arsta; > > - > > - rcu_read_lock(); > > - list_for_each_entry(peer, &stats->peers_extd, list) { > > - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr, > > - NULL); > > - if (!sta) > > - continue; > > - arsta = (struct ath10k_sta *)sta->drv_priv; > > - arsta->rx_duration += (u64)peer->rx_duration; > > - } > > - rcu_read_unlock(); > > -} > > - > > -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, > > - struct ath10k_fw_stats *stats) > > +void ath10k_sta_update_rx_duration(struct ath10k *ar, > > + struct ath10k_fw_stats *stats) > > { > > struct ath10k_fw_stats_peer *peer; > > struct ieee80211_sta *sta; > > @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, > > rcu_read_unlock(); > > } > > > > -void ath10k_sta_update_rx_duration(struct ath10k *ar, > > - struct ath10k_fw_stats *stats) > > -{ > > - if (stats->extended) > > - ath10k_sta_update_extd_stats_rx_duration(ar, stats); > > - else > > - ath10k_sta_update_stats_rx_duration(ar, stats); > > -} > > - > > void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > > struct ieee80211_sta *sta, > > struct station_info *sinfo) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > > index c893314a191f..c7ec7b9e9b55 100644 > > --- a/drivers/net/wireless/ath/ath10k/wmi.c > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > > return 0; > > > > - stats->extended = true; > > - > > for (i = 0; i < num_peer_stats; i++) { > > const struct wmi_10_4_peer_extd_stats *src; > > - struct ath10k_fw_extd_stats_peer *dst; > > + struct ath10k_fw_stats_peer *dst; > > > > src = (void *)skb->data; > > if (!skb_pull(skb, sizeof(*src))) > > return -EPROTO; > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > > - if (!dst) > > - continue; > > + /* Because the stat data may exceed htc-wmi buffer > > + * limit the firmware might split the stats data > > + * and delivers it in multiple update events. > > + * if we can't find the entry in the current event > > + * payload, we have to look in main list as well. > > + */ > > + list_for_each_entry(dst, &stats->peers, list) { > > + if (ether_addr_equal(dst->peer_macaddr, > > + src->peer_macaddr.addr)) > > + goto found; > > + } > > + > > +#ifdef CONFIG_ATH10K_DEBUGFS > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > > + if (ether_addr_equal(dst->peer_macaddr, > > + src->peer_macaddr.addr)) > > + goto found; > > + } > > +#endif > > + > > + ath10k_dbg(ar, ATH10K_DBG_WMI, > > + "Orphaned extended stats entry for station %pM.\n", > > + src->peer_macaddr.addr); > > + continue; > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > > +found: > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > - list_add_tail(&dst->list, &stats->peers_extd); > > } > > > > return 0; > > [shafi] Yes i am bit concerned about this change making 10.4 to update > over the existing peer_stats structure, the idea is to maintain uniformity > between the structures shared between ath10k and its corresponding *firmware* to avoid > bugs in the future. Kindly let me know your thoughts, feel free > to correct me if any of my analysis is incorrect. thank you ! > > regards, > shafi > > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k
Hello Shafi, On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote: > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > > The 10.4 firmware adds extended peer information to the > > firmware's statistics payload. This additional info is > > stored as a separate data field. During review of > > "ath10k: add accounting for the extended peer statistics" [0] > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics > > lists are of little use:"... there is not much use in appending > > the extended peer stats (which gets periodically updated) to the > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > > rid of the below (and the required cleanup as well) > > > > list_splice_tail_init(&stats.peers_extd, > > &ar->debug.fw_stats.peers_extd); > > > > since rx_duration is getting updated periodically to the per sta > > information." > > > > This patch replaces the extended peers list with a lookup and > > puts the retrieved data (rx_duration) into the existing > > ath10k_fw_stats_peer entry that was created earlier. > > [shafi] Its good to maintain the extended stats variable > and retain the two different functions to update rx_duration. > > a) extended peer stats supported - mainly for 10.4 > b) extender peer stats not supported - for 10.2 Well, I have to ask why you want to retain the two different functions to update the same arsta->rx_duration? I think a little bit of code that helps to explain what's on your mind (or how you would do it) would help immensely in this case. Since I have the feeling that this is the problem here. So please explain it in C(lang). > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > > --- > > drivers/net/wireless/ath/ath10k/core.h | 2 -- > > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > > 4 files changed, 28 insertions(+), 57 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > > index 09ff8b8a6441..3fffbbb18c25 100644 > > --- a/drivers/net/wireless/ath/ath10k/core.h > > +++ b/drivers/net/wireless/ath/ath10k/core.h > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > > }; > > > > struct ath10k_fw_stats { > > - bool extended; > > struct list_head pdevs; > > struct list_head vdevs; > > struct list_head peers; > > - struct list_head peers_extd; > > }; > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > > index 82a4c67f3672..89f7fde77cdf 100644 > > --- a/drivers/net/wireless/ath/ath10k/debug.c > > +++ b/drivers/net/wireless/ath/ath10k/debug.c > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > > } > > } > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > > -{ > > - struct ath10k_fw_extd_stats_peer *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > > { > > spin_lock_bh(&ar->data_lock); > > ar->debug.fw_stats_done = false; > > - ar->debug.fw_stats.extended = false; > > [shafi] this looks fine, but not removing the 'extended' variable > from ath10k_fw_stats structure, I see the design for 'rx_duration' > arguably some what convoluted ! I removed extended because it is now a write-only variable. So I figured, there's no point in keeping it around? I don't have access to the firmware interface documentation, so I don't know if there's a reason why it would be good to have it later. So please tell me, what information do we gain from it? > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' > {certainly 'stats' object is for this particular update only, and freed > up later) > *Update immediately using 'ath10k_sta_update_rx_duration' > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation > for 10.2 and 10.4 (the later supporting extended peer stats) I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats() passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats element which is basically a carbon-copy of wmi_10_2_4_peer_stats (but with one extra __le32 for the rx_duration at the end.) This rx_duration is then used to set the rx_duration field in the generated ath10k_fw_stats_peer struct. 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for the communicating the rx_duration to the driver. Thing is, both function have the same signature. They produce the same struct ath10k_fw_stats_peer for the given data in the sk_buff input. So why does 10.4 need to have it's peer_extd infrastructure, when it can use the existing rx_duration field in the *universal* ath10k_fw_stats_peer? What's with the extra layers / HAL here? Because it looks like it's merged back together in the same manner into the same arsta->rx_duration? [ath10k_sta_update_stats_rx_duration() vs. ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too] > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > > index c893314a191f..c7ec7b9e9b55 100644 > > --- a/drivers/net/wireless/ath/ath10k/wmi.c > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > > return 0; > > > > - stats->extended = true; > > - > > for (i = 0; i < num_peer_stats; i++) { > > const struct wmi_10_4_peer_extd_stats *src; > > - struct ath10k_fw_extd_stats_peer *dst; > > + struct ath10k_fw_stats_peer *dst; > > > > src = (void *)skb->data; > > if (!skb_pull(skb, sizeof(*src))) > > return -EPROTO; > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > > - if (!dst) > > - continue; > > + /* Because the stat data may exceed htc-wmi buffer > > + * limit the firmware might split the stats data > > + * and delivers it in multiple update events. > > + * if we can't find the entry in the current event > > + * payload, we have to look in main list as well. > > + */ > > + list_for_each_entry(dst, &stats->peers, list) { > > + if (ether_addr_equal(dst->peer_macaddr, > > + src->peer_macaddr.addr)) > > + goto found; > > + } > > + > > +#ifdef CONFIG_ATH10K_DEBUGFS > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > > + if (ether_addr_equal(dst->peer_macaddr, > > + src->peer_macaddr.addr)) > > + goto found; > > + } > > +#endif > > + > > + ath10k_dbg(ar, ATH10K_DBG_WMI, > > + "Orphaned extended stats entry for station %pM.\n", > > + src->peer_macaddr.addr); > > + continue; > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > > +found: > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > - list_add_tail(&dst->list, &stats->peers_extd); > > } > > > > return 0; > > [shafi] Yes i am bit concerned about this change making 10.4 to update > over the existing peer_stats structure, the idea is to maintain uniformity > between the structures shared between ath10k and its corresponding to avoid > confusion/ bugs in the future. Kindly let me know your thoughts, feel free > to correct me if any of my analysis is incorrect. thank you ! Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure) that other functions can use, without caring about if the FW was 10.4 or 10.2.4? There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats conveys any different information than the rx_duration in wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make wmi_10_4_peer_extd_stats's rx_duration use the existing field in ath10k_fw_stats_peer? And if not: why exactly? Regards, Christian
Hi Christian, once again sorry for the delay > > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote: > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > > > The 10.4 firmware adds extended peer information to the > > > firmware's statistics payload. This additional info is > > > stored as a separate data field. During review of > > > "ath10k: add accounting for the extended peer statistics" [0] > > > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics > > > lists are of little use:"... there is not much use in appending > > > the extended peer stats (which gets periodically updated) to the > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > > > rid of the below (and the required cleanup as well) > > > > > > list_splice_tail_init(&stats.peers_extd, > > > &ar->debug.fw_stats.peers_extd); > > > > > > since rx_duration is getting updated periodically to the per sta > > > information." > > > > > > This patch replaces the extended peers list with a lookup and > > > puts the retrieved data (rx_duration) into the existing > > > ath10k_fw_stats_peer entry that was created earlier. > > > > [shafi] Its good to maintain the extended stats variable > > and retain the two different functions to update rx_duration. > > > > a) extended peer stats supported - mainly for 10.4 > > b) extender peer stats not supported - for 10.2 > Well, I have to ask why you want to retain the two different > functions to update the same arsta->rx_duration? I think a > little bit of code that helps to explain what's on your mind > (or how you would do it) would help immensely in this case. > Since I have the feeling that this is the problem here. > So please explain it in C(lang). [shafi] I see you prefer to stuff the 'rx_duration' from the extended stats to the existing global 'ath10k_peer_stats' The idea of extended stats is, ideally its not going to stop for 'rx_duration' . Extended stats is maintained as a seperate list / entry for addition of new fields as well > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > > > --- > > > drivers/net/wireless/ath/ath10k/core.h | 2 -- > > > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > > > 4 files changed, 28 insertions(+), 57 deletions(-) > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > > > index 09ff8b8a6441..3fffbbb18c25 100644 > > > --- a/drivers/net/wireless/ath/ath10k/core.h > > > +++ b/drivers/net/wireless/ath/ath10k/core.h > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > > > }; > > > > > > struct ath10k_fw_stats { > > > - bool extended; > > > struct list_head pdevs; > > > struct list_head vdevs; > > > struct list_head peers; > > > - struct list_head peers_extd; > > > }; > > > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > > > index 82a4c67f3672..89f7fde77cdf 100644 > > > --- a/drivers/net/wireless/ath/ath10k/debug.c > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > > > } > > > } > > > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > > > -{ > > > - struct ath10k_fw_extd_stats_peer *i, *tmp; > > > - > > > - list_for_each_entry_safe(i, tmp, head, list) { > > > - list_del(&i->list); > > > - kfree(i); > > > - } > > > -} > > > - > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > > > { > > > spin_lock_bh(&ar->data_lock); > > > ar->debug.fw_stats_done = false; > > > - ar->debug.fw_stats.extended = false; > > > > [shafi] this looks fine, but not removing the 'extended' variable > > from ath10k_fw_stats structure, I see the design for 'rx_duration' > > arguably some what convoluted ! > I removed extended because it is now a write-only variable. > So I figured, there's no point in keeping it around? I don't have > access to the firmware interface documentation, so I don't know > if there's a reason why it would be good to have it later. > So please tell me, what information do we gain from it? [shafi] while parsing the stats from 'wmi' we clearly mark there whether the extended stats is available (or) not and if its there parse it and deal with it seperately > > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' > > {certainly 'stats' object is for this particular update only, and freed > > up later) > > *Update immediately using 'ath10k_sta_update_rx_duration' > > > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation > > for 10.2 and 10.4 (the later supporting extended peer stats) > > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats() > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats > element which is basically a carbon-copy of wmi_10_2_4_peer_stats > (but with one extra __le32 for the rx_duration at the end.) > This rx_duration is then used to set the rx_duration field in the > generated ath10k_fw_stats_peer struct. > > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for > the communicating the rx_duration to the driver. > > Thing is, both function have the same signature. They produce the same > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So > why does 10.4 need to have it's peer_extd infrastructure, when it can use > the existing rx_duration field in the *universal* ath10k_fw_stats_peer? [shafi] agreed we need to fix that, it would have been easier if 10.2.4 and 10.4 had the same definitions. > > What's with the extra layers / HAL here? Because it looks like it's merged > back together in the same manner into the same arsta->rx_duration? > [ath10k_sta_update_stats_rx_duration() vs. > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too] > > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > > > index c893314a191f..c7ec7b9e9b55 100644 > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > > > return 0; > > > > > > - stats->extended = true; > > > - > > > for (i = 0; i < num_peer_stats; i++) { > > > const struct wmi_10_4_peer_extd_stats *src; > > > - struct ath10k_fw_extd_stats_peer *dst; > > > + struct ath10k_fw_stats_peer *dst; > > > > > > src = (void *)skb->data; > > > if (!skb_pull(skb, sizeof(*src))) > > > return -EPROTO; > > > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > > > - if (!dst) > > > - continue; > > > + /* Because the stat data may exceed htc-wmi buffer > > > + * limit the firmware might split the stats data > > > + * and delivers it in multiple update events. > > > + * if we can't find the entry in the current event > > > + * payload, we have to look in main list as well. > > > + */ [shafi] thanks ! sorry i might have missed this bug, did you happen to see this condition being hit ? > > > + list_for_each_entry(dst, &stats->peers, list) { > > > + if (ether_addr_equal(dst->peer_macaddr, > > > + src->peer_macaddr.addr)) > > > + goto found; > > > + } > > > + [shafi] Again, we can simply cache the macaddress and rx_duration and deal with it later, rather than doing the whole lookup here ? Will it be an overhead for large number of clients ? > > > +#ifdef CONFIG_ATH10K_DEBUGFS > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > > > + if (ether_addr_equal(dst->peer_macaddr, > > > + src->peer_macaddr.addr)) > > > + goto found; > > > + } > > > +#endif [shafi] again, this could be handled seperately. > > > + > > > + ath10k_dbg(ar, ATH10K_DBG_WMI, > > > + "Orphaned extended stats entry for station %pM.\n", > > > + src->peer_macaddr.addr); > > > + continue; > > > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > > > +found: > > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > > - list_add_tail(&dst->list, &stats->peers_extd); > > > } > > > > > > return 0; > > > > [shafi] Yes i am bit concerned about this change making 10.4 to update > > over the existing peer_stats structure, the idea is to maintain uniformity > > between the structures shared between ath10k and its corresponding to avoid > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free > > to correct me if any of my analysis is incorrect. thank you ! > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure) > that other functions can use, without caring about if the FW was 10.4 > or 10.2.4? > > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats > conveys any different information than the rx_duration in > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make > wmi_10_4_peer_extd_stats's rx_duration use the existing field in > ath10k_fw_stats_peer? And if not: why exactly? > [shafi] I will soon test your change and keep you posted. We will also get Kalle's take/view on this. regards, shafi
Hello Shafi, On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote: > > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote: > > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > > > > The 10.4 firmware adds extended peer information to the > > > > firmware's statistics payload. This additional info is > > > > stored as a separate data field. During review of > > > > "ath10k: add accounting for the extended peer statistics" [0] > > > > > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics > > > > lists are of little use:"... there is not much use in appending > > > > the extended peer stats (which gets periodically updated) to the > > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > > > > rid of the below (and the required cleanup as well) > > > > > > > > list_splice_tail_init(&stats.peers_extd, > > > > &ar->debug.fw_stats.peers_extd); > > > > > > > > since rx_duration is getting updated periodically to the per sta > > > > information." > > > > > > > > This patch replaces the extended peers list with a lookup and > > > > puts the retrieved data (rx_duration) into the existing > > > > ath10k_fw_stats_peer entry that was created earlier. > > > > > > [shafi] Its good to maintain the extended stats variable > > > and retain the two different functions to update rx_duration. > > > > > > a) extended peer stats supported - mainly for 10.4 > > > b) extender peer stats not supported - for 10.2 > > Well, I have to ask why you want to retain the two different > > functions to update the same arsta->rx_duration? I think a > > little bit of code that helps to explain what's on your mind > > (or how you would do it) would help immensely in this case. > > Since I have the feeling that this is the problem here. > > So please explain it in C(lang). > > [shafi] I see you prefer to stuff the 'rx_duration' from > the extended stats to the existing global 'ath10k_peer_stats' > The idea of extended stats is, ideally its not going to stop > for 'rx_duration' . Extended stats is maintained as a seperate > list / entry for addition of new fields as well I'm guessing you are trying to say here: replace: dst->rx_duration = __le32_to_cpu(src->rx_duration); with dst->rx_duration += __le32_to_cpu(src->rx_duration); in ath10k_wmi_10_4_op_pull_fw_stats()? Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats() is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer stats instead of looking up the number of extended peer items. Because this does imply that there are the "same" (and every peer stat has a matching extended peer stat)... (This will be important for the comment below - so ***). Of course, if this isn't true then this is a bug and should be fixed because otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost stats. > > > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > > > > --- > > > > drivers/net/wireless/ath/ath10k/core.h | 2 -- > > > > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > > > > 4 files changed, 28 insertions(+), 57 deletions(-) > > > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > > > > index 09ff8b8a6441..3fffbbb18c25 100644 > > > > --- a/drivers/net/wireless/ath/ath10k/core.h > > > > +++ b/drivers/net/wireless/ath/ath10k/core.h > > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > > > > }; > > > > > > > > struct ath10k_fw_stats { > > > > - bool extended; > > > > struct list_head pdevs; > > > > struct list_head vdevs; > > > > struct list_head peers; > > > > - struct list_head peers_extd; > > > > }; > > > > > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > > > > index 82a4c67f3672..89f7fde77cdf 100644 > > > > --- a/drivers/net/wireless/ath/ath10k/debug.c > > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c > > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > > > > } > > > > } > > > > > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > > > > -{ > > > > - struct ath10k_fw_extd_stats_peer *i, *tmp; > > > > - > > > > - list_for_each_entry_safe(i, tmp, head, list) { > > > > - list_del(&i->list); > > > > - kfree(i); > > > > - } > > > > -} > > > > - > > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > > > > { > > > > spin_lock_bh(&ar->data_lock); > > > > ar->debug.fw_stats_done = false; > > > > - ar->debug.fw_stats.extended = false; > > > > > > [shafi] this looks fine, but not removing the 'extended' variable > > > from ath10k_fw_stats structure, I see the design for 'rx_duration' > > > arguably some what convoluted ! > > I removed extended because it is now a write-only variable. > > So I figured, there's no point in keeping it around? I don't have > > access to the firmware interface documentation, so I don't know > > if there's a reason why it would be good to have it later. > > So please tell me, what information do we gain from it? > > [shafi] while parsing the stats from 'wmi' we clearly mark there whether > the extended stats is available (or) not and if its there parse it and > deal with it seperately No, there's no difference between stats or extended stats (10.2.4 vs 10.4): both ath10k_sta_update_extd_stats_rx_duration() [0] and ath10k_sta_update_stats_rx_duration() [1] are adding the ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration. With the merge of the peer stats and extended peer stats, this also removed the only usage of stats->extended. And hence it becomes a write-only variable. So there's no point in keeping it around ATM (as all other extended peer stats entries aren't used). > > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' > > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' > > > {certainly 'stats' object is for this particular update only, and freed > > > up later) > > > *Update immediately using 'ath10k_sta_update_rx_duration' > > > > > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation > > > for 10.2 and 10.4 (the later supporting extended peer stats) > > > > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats() > > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats > > element which is basically a carbon-copy of wmi_10_2_4_peer_stats > > (but with one extra __le32 for the rx_duration at the end.) > > This rx_duration is then used to set the rx_duration field in the > > generated ath10k_fw_stats_peer struct. > > > > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats > > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for > > the communicating the rx_duration to the driver. > > > > Thing is, both function have the same signature. They produce the same > > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So > > why does 10.4 need to have it's peer_extd infrastructure, when it can use > > the existing rx_duration field in the *universal* ath10k_fw_stats_peer? > > [shafi] agreed we need to fix that, it would have been easier if 10.2.4 > and 10.4 had the same definitions. Ok, I don't know the internals of the firmware to know what's the difference between 10.2.4 and 10.4's rx_duration (how both firmwares define them differently in this context) ? From what I can tell, it's just that the entry has moved from the peer stats to extended peer stats. Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations end up being added to arsta->rx_duration in the same way. There's no scaling or a comment that would indicate a difference. > > > > What's with the extra layers / HAL here? Because it looks like it's merged > > back together in the same manner into the same arsta->rx_duration? > > [ath10k_sta_update_stats_rx_duration() vs. > > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too] > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > > > > index c893314a191f..c7ec7b9e9b55 100644 > > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c > > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > > > > return 0; > > > > > > > > - stats->extended = true; > > > > - > > > > for (i = 0; i < num_peer_stats; i++) { > > > > const struct wmi_10_4_peer_extd_stats *src; > > > > - struct ath10k_fw_extd_stats_peer *dst; > > > > + struct ath10k_fw_stats_peer *dst; > > > > > > > > src = (void *)skb->data; > > > > if (!skb_pull(skb, sizeof(*src))) > > > > return -EPROTO; > > > > > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > > > > - if (!dst) > > > > - continue; > > > > + /* Because the stat data may exceed htc-wmi buffer > > > > + * limit the firmware might split the stats data > > > > + * and delivers it in multiple update events. > > > > + * if we can't find the entry in the current event > > > > + * payload, we have to look in main list as well. > > > > + */ > > [shafi] thanks ! sorry i might have missed this bug, did you happen > to see this condition being hit ? This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal Kazior to the discussion since his patch "ath10k: fix fw stats processing" added this in debug.c. I think he knows the firmware internals well enough to tell if this is a problem or not. As it stands now, it is and will be in the future. > > > > + list_for_each_entry(dst, &stats->peers, list) { > > > > + if (ether_addr_equal(dst->peer_macaddr, > > > > + src->peer_macaddr.addr)) > > > > + goto found; > > > > + } > > > > + > > [shafi] Again, we can simply cache the macaddress and rx_duration > and deal with it later, rather than doing the whole lookup here ? > Will it be an overhead for large number of clients ? Well, show me how you would do it more efficiently otherwise? I don't see how you can cache the macaddress and rx_duration since you are basically forced to process all the peer stats first and later all the extended peer stats. They don't mix. As for how expensive it is: From what I can tell, the 10.4 firmware sends the stat events every few seconds. So they are not part of any rx or tx hot-paths. The list_for_each within the for (i = 0; i < num_peers;i++) is actually in the O(1) class as far as both loops go. This might sound funny at first, but the number of extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16. And thanks to (***) the limit of num_peers is also 16. Furthermore we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it entirely if the stats are disabled. > > > > +#ifdef CONFIG_ATH10K_DEBUGFS > > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > > > > + if (ether_addr_equal(dst->peer_macaddr, > > > > + src->peer_macaddr.addr)) > > > > + goto found; > > > > + } > > > > +#endif > > [shafi] again, this could be handled seperately. This is more expensive. As fw_stats.peers can contain more entries than 16. However, this might be unnecessary if both peers and extended peers stats entries in the wmi event are always for the same stations. Note: There's an alternative way too. Instead of writing rx_duration into ath10k_fw_stats, we could skip the middle man write it directly into arsta->rx_duration. This would also mean that we can get rid of ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration() and ath10k_sta_update_rx_duration(). This has the benifit that we can remove even more. > > > > + > > > > + ath10k_dbg(ar, ATH10K_DBG_WMI, > > > > + "Orphaned extended stats entry for station %pM.\n", > > > > + src->peer_macaddr.addr); > > > > + continue; > > > > > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > > > > +found: > > > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > > > - list_add_tail(&dst->list, &stats->peers_extd); > > > > } > > > > > > > > return 0; > > > > > > [shafi] Yes i am bit concerned about this change making 10.4 to update > > > over the existing peer_stats structure, the idea is to maintain uniformity > > > between the structures shared between ath10k and its corresponding to avoid > > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free > > > to correct me if any of my analysis is incorrect. thank you ! > > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make > > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure) > > that other functions can use, without caring about if the FW was 10.4 > > or 10.2.4? > > > > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats > > conveys any different information than the rx_duration in > > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make > > wmi_10_4_peer_extd_stats's rx_duration use the existing field in > > ath10k_fw_stats_peer? And if not: why exactly? > > > [shafi] I will soon test your change and keep you posted. We will also > get Kalle's take/view on this. Ok. That said, I added him to the CC from the beginning and so far he silently agreed. Regards, Christian [0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21> [1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40> [2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360>
Hi Christian, On Thu, Dec 22, 2016 at 06:58:41PM +0100, Christian Lamparter wrote: > Hello Shafi, > > On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote: > > > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote: > > > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote: > > > > > The 10.4 firmware adds extended peer information to the > > > > > firmware's statistics payload. This additional info is > > > > > stored as a separate data field. During review of > > > > > "ath10k: add accounting for the extended peer statistics" [0] > > > > > > > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics > > > > > lists are of little use:"... there is not much use in appending > > > > > the extended peer stats (which gets periodically updated) to the > > > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > > > > > rid of the below (and the required cleanup as well) > > > > > > > > > > list_splice_tail_init(&stats.peers_extd, > > > > > &ar->debug.fw_stats.peers_extd); > > > > > > > > > > since rx_duration is getting updated periodically to the per sta > > > > > information." > > > > > > > > > > This patch replaces the extended peers list with a lookup and > > > > > puts the retrieved data (rx_duration) into the existing > > > > > ath10k_fw_stats_peer entry that was created earlier. > > > > > > > > [shafi] Its good to maintain the extended stats variable > > > > and retain the two different functions to update rx_duration. > > > > > > > > a) extended peer stats supported - mainly for 10.4 > > > > b) extender peer stats not supported - for 10.2 > > > Well, I have to ask why you want to retain the two different > > > functions to update the same arsta->rx_duration? I think a > > > little bit of code that helps to explain what's on your mind > > > (or how you would do it) would help immensely in this case. > > > Since I have the feeling that this is the problem here. > > > So please explain it in C(lang). > > > > [shafi] I see you prefer to stuff the 'rx_duration' from > > the extended stats to the existing global 'ath10k_peer_stats' > > The idea of extended stats is, ideally its not going to stop > > for 'rx_duration' . Extended stats is maintained as a seperate > > list / entry for addition of new fields as well > I'm guessing you are trying to say here: > > replace: > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > with > > dst->rx_duration += __le32_to_cpu(src->rx_duration); > > in ath10k_wmi_10_4_op_pull_fw_stats()? [shafi] oh no sorry, I am trying to say more members related to stats shall be added in extended stats structure . The extended stats structure is specifically introduced for adding more stats related variables. > > Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats() > is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer > stats instead of looking up the number of extended peer items. Because this does > imply that there are the "same" (and every peer stat has a matching extended > peer stat)... (This will be important for the comment below - so ***). > Of course, if this isn't true then this is a bug and should be fixed because > otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO > at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost > stats. [shafi] sorry i am not sure I got you, have you come across this error, please let me know ? > > > > > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > > > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > > > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > > > > > --- > > > > > drivers/net/wireless/ath/ath10k/core.h | 2 -- > > > > > drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- > > > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- > > > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- > > > > > 4 files changed, 28 insertions(+), 57 deletions(-) > > > > > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > > > > > index 09ff8b8a6441..3fffbbb18c25 100644 > > > > > --- a/drivers/net/wireless/ath/ath10k/core.h > > > > > +++ b/drivers/net/wireless/ath/ath10k/core.h > > > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { > > > > > }; > > > > > > > > > > struct ath10k_fw_stats { > > > > > - bool extended; > > > > > struct list_head pdevs; > > > > > struct list_head vdevs; > > > > > struct list_head peers; > > > > > - struct list_head peers_extd; > > > > > }; > > > > > > > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1 > > > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > > > > > index 82a4c67f3672..89f7fde77cdf 100644 > > > > > --- a/drivers/net/wireless/ath/ath10k/debug.c > > > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c > > > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) > > > > > } > > > > > } > > > > > > > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) > > > > > -{ > > > > > - struct ath10k_fw_extd_stats_peer *i, *tmp; > > > > > - > > > > > - list_for_each_entry_safe(i, tmp, head, list) { > > > > > - list_del(&i->list); > > > > > - kfree(i); > > > > > - } > > > > > -} > > > > > - > > > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar) > > > > > { > > > > > spin_lock_bh(&ar->data_lock); > > > > > ar->debug.fw_stats_done = false; > > > > > - ar->debug.fw_stats.extended = false; > > > > > > > > [shafi] this looks fine, but not removing the 'extended' variable > > > > from ath10k_fw_stats structure, I see the design for 'rx_duration' > > > > arguably some what convoluted ! > > > I removed extended because it is now a write-only variable. > > > So I figured, there's no point in keeping it around? I don't have > > > access to the firmware interface documentation, so I don't know > > > if there's a reason why it would be good to have it later. > > > So please tell me, what information do we gain from it? [shafi] sorry this is purely ath10k specific nothing to map with the firmware > > > > [shafi] while parsing the stats from 'wmi' we clearly mark there whether > > the extended stats is available (or) not and if its there parse it and > > deal with it seperately > No, there's no difference between stats or extended stats (10.2.4 vs 10.4): > both ath10k_sta_update_extd_stats_rx_duration() [0] > and ath10k_sta_update_stats_rx_duration() [1] are adding the > ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration. > > With the merge of the peer stats and extended peer stats, this also > removed the only usage of stats->extended. And hence it becomes a > write-only variable. So there's no point in keeping it around ATM (as > all other extended peer stats entries aren't used). [shafi] ok, I see the extended stats structure is introduced for 10.4 and i was thinking its good to have two seperate API's for updating the rx_duration .. > > > > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process' > > > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)' > > > > {certainly 'stats' object is for this particular update only, and freed > > > > up later) > > > > *Update immediately using 'ath10k_sta_update_rx_duration' > > > > > > > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation > > > > for 10.2 and 10.4 (the later supporting extended peer stats) > > > > > > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats() > > > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats > > > element which is basically a carbon-copy of wmi_10_2_4_peer_stats > > > (but with one extra __le32 for the rx_duration at the end.) > > > This rx_duration is then used to set the rx_duration field in the > > > generated ath10k_fw_stats_peer struct. [shafi] ok > > > > > > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats > > > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for > > > the communicating the rx_duration to the driver. [shafi] ok > > > > > > Thing is, both function have the same signature. They produce the same > > > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So > > > why does 10.4 need to have it's peer_extd infrastructure, when it can use > > > the existing rx_duration field in the *universal* ath10k_fw_stats_peer? > > > > [shafi] agreed we need to fix that, it would have been easier if 10.2.4 > > and 10.4 had the same definitions. > Ok, I don't know the internals of the firmware to know what's the difference > between 10.2.4 and 10.4's rx_duration (how both firmwares define them > differently in this context) ? From what I can tell, it's just that > the entry has moved from the peer stats to extended peer stats. > Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations > end up being added to arsta->rx_duration in the same way. There's no scaling > or a comment that would indicate a difference. [shafi] ok > > > > > > > What's with the extra layers / HAL here? Because it looks like it's merged > > > back together in the same manner into the same arsta->rx_duration? > > > [ath10k_sta_update_stats_rx_duration() vs. > > > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too] [shafi] my concern was for updating 'rx_duration' we just iterate over the list and update the same. > > > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > > > > > index c893314a191f..c7ec7b9e9b55 100644 > > > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c > > > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > > > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, > > > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) > > > > > return 0; > > > > > > > > > > - stats->extended = true; > > > > > - > > > > > for (i = 0; i < num_peer_stats; i++) { > > > > > const struct wmi_10_4_peer_extd_stats *src; > > > > > - struct ath10k_fw_extd_stats_peer *dst; > > > > > + struct ath10k_fw_stats_peer *dst; > > > > > > > > > > src = (void *)skb->data; > > > > > if (!skb_pull(skb, sizeof(*src))) > > > > > return -EPROTO; > > > > > > > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); > > > > > - if (!dst) > > > > > - continue; > > > > > + /* Because the stat data may exceed htc-wmi buffer > > > > > + * limit the firmware might split the stats data > > > > > + * and delivers it in multiple update events. > > > > > + * if we can't find the entry in the current event > > > > > + * payload, we have to look in main list as well. > > > > > + */ > > > > [shafi] thanks ! sorry i might have missed this bug, did you happen > > to see this condition being hit ? > This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal > Kazior to the discussion since his patch "ath10k: fix fw stats processing" > added this in debug.c. I think he knows the firmware internals well enough > to tell if this is a problem or not. As it stands now, it is and will be > in the future. [shafi] sure. > > > > > > + list_for_each_entry(dst, &stats->peers, list) { > > > > > + if (ether_addr_equal(dst->peer_macaddr, > > > > > + src->peer_macaddr.addr)) > > > > > + goto found; > > > > > + } > > > > > + > > > > [shafi] Again, we can simply cache the macaddress and rx_duration > > and deal with it later, rather than doing the whole lookup here ? > > Will it be an overhead for large number of clients ? > Well, show me how you would do it more efficiently otherwise? I don't > see how you can cache the macaddress and rx_duration since you are > basically forced to process all the peer stats first and later all > the extended peer stats. They don't mix. [shafi] hmmm, ok. > > As for how expensive it is: From what I can tell, the 10.4 firmware > sends the stat events every few seconds. So they are not part of any > rx or tx hot-paths. The list_for_each within the for > (i = 0; i < num_peers;i++) is actually in the O(1) class as far > as both loops go. This might sound funny at first, but the number of > extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16. > And thanks to (***) the limit of num_peers is also 16. Furthermore > we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it > entirely if the stats are disabled. [shafi] ok. > > > > > > > +#ifdef CONFIG_ATH10K_DEBUGFS > > > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { > > > > > + if (ether_addr_equal(dst->peer_macaddr, > > > > > + src->peer_macaddr.addr)) > > > > > + goto found; > > > > > + } > > > > > +#endif > > > > [shafi] again, this could be handled seperately. > This is more expensive. As fw_stats.peers can contain more entries than 16. > However, this might be unnecessary if both peers and extended peers stats > entries in the wmi event are always for the same stations. [shafi] ok > > Note: There's an alternative way too. Instead of writing rx_duration into > ath10k_fw_stats, we could skip the middle man write it directly into > arsta->rx_duration. This would also mean that we can get rid of > ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration() > and ath10k_sta_update_rx_duration(). This has the benifit that we can > remove even more. > > > > > > + > > > > > + ath10k_dbg(ar, ATH10K_DBG_WMI, > > > > > + "Orphaned extended stats entry for station %pM.\n", > > > > > + src->peer_macaddr.addr); > > > > > + continue; > > > > > > > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); > > > > > +found: > > > > > dst->rx_duration = __le32_to_cpu(src->rx_duration); > > > > > - list_add_tail(&dst->list, &stats->peers_extd); > > > > > } > > > > > > > > > > return 0; > > > > > > > > [shafi] Yes i am bit concerned about this change making 10.4 to update > > > > over the existing peer_stats structure, the idea is to maintain uniformity > > > > between the structures shared between ath10k and its corresponding to avoid > > > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free > > > > to correct me if any of my analysis is incorrect. thank you ! > > > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make > > > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure) > > > that other functions can use, without caring about if the FW was 10.4 > > > or 10.2.4? > > > > > > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats > > > conveys any different information than the rx_duration in > > > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make > > > wmi_10_4_peer_extd_stats's rx_duration use the existing field in > > > ath10k_fw_stats_peer? And if not: why exactly? > > > > > [shafi] I will soon test your change and keep you posted. We will also > > get Kalle's take/view on this. > Ok. That said, I added him to the CC from the beginning and so far > he silently agreed. [shafi] sure, I guess i quickly tested this change and I found things are fine, it would be good if you can share your test results as well please Yes we can discuss with Kalle and Michal ! If you guys think this is a more optimized version of updating the peer stats, I am fine with that as well. There is no doubt you guys know about open source better than me :-) > > Regards, > Christian > > [0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21> > [1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40> > [2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360> > >
Christian Lamparter <chunkeey@googlemail.com> wrote: > The 10.4 firmware adds extended peer information to the > firmware's statistics payload. This additional info is > stored as a separate data field. During review of > "ath10k: add accounting for the extended peer statistics" [0] > > Mohammed Shafi Shajakhan commented that the extended peer statistics > lists are of little use:"... there is not much use in appending > the extended peer stats (which gets periodically updated) to the > linked list '&ar->debug.fw_stats.peers_extd)' and should we get > rid of the below (and the required cleanup as well) > > list_splice_tail_init(&stats.peers_extd, > &ar->debug.fw_stats.peers_extd); > > since rx_duration is getting updated periodically to the per sta > information." > > This patch replaces the extended peers list with a lookup and > puts the retrieved data (rx_duration) into the existing > ath10k_fw_stats_peer entry that was created earlier. > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> The discussion with this bug was quite hard to follow, so what is the conclusion from this? The patch still applies using three way merge but there are compilation errors: drivers/net/wireless/ath/ath10k/debug.c: In function 'ath10k_debug_fw_stats_process': drivers/net/wireless/ath/ath10k/debug.c:388:4: error: implicit declaration of function 'ath10k_fw_extd_stats_peers_free' [-Werror=implicit-function-declaration] drivers/net/wireless/ath/ath10k/debug.c:388:55: error: 'struct ath10k_fw_stats' has no member named 'peers_extd' drivers/net/wireless/ath/ath10k/debug.c:400:32: error: 'struct ath10k_fw_stats' has no member named 'peers_extd' drivers/net/wireless/ath/ath10k/debug.c:401:31: error: 'struct ath10k_fw_stats' has no member named 'peers_extd' I admit that I didn't look this patch very carefully, but I'm not really seeing how this patch makes things better? wmi.c gets more complicated and a new ifdef is added. We have tried hard to keep all ar->debug access in debug.c, this way there's no need to sprinkle ifdefs in the code.
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 09ff8b8a6441..3fffbbb18c25 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev { }; struct ath10k_fw_stats { - bool extended; struct list_head pdevs; struct list_head vdevs; struct list_head peers; - struct list_head peers_extd; }; #define ATH10K_TPC_TABLE_TYPE_FLAG 1 diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c index 82a4c67f3672..89f7fde77cdf 100644 --- a/drivers/net/wireless/ath/ath10k/debug.c +++ b/drivers/net/wireless/ath/ath10k/debug.c @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head) } } -static void ath10k_fw_extd_stats_peers_free(struct list_head *head) -{ - struct ath10k_fw_extd_stats_peer *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} - static void ath10k_debug_fw_stats_reset(struct ath10k *ar) { spin_lock_bh(&ar->data_lock); ar->debug.fw_stats_done = false; - ar->debug.fw_stats.extended = false; ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers); - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd); spin_unlock_bh(&ar->data_lock); } @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) INIT_LIST_HEAD(&stats.pdevs); INIT_LIST_HEAD(&stats.vdevs); INIT_LIST_HEAD(&stats.peers); - INIT_LIST_HEAD(&stats.peers_extd); spin_lock_bh(&ar->data_lock); ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats); @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers); list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs); - list_splice_tail_init(&stats.peers_extd, - &ar->debug.fw_stats.peers_extd); } complete(&ar->debug.fw_stats_complete); @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) ath10k_fw_stats_pdevs_free(&stats.pdevs); ath10k_fw_stats_vdevs_free(&stats.vdevs); ath10k_fw_stats_peers_free(&stats.peers); - ath10k_fw_extd_stats_peers_free(&stats.peers_extd); spin_unlock_bh(&ar->data_lock); } @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar) INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); INIT_LIST_HEAD(&ar->debug.fw_stats.peers); - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd); return 0; } diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c index fce6f8137d33..bf2d49cbb3bb 100644 --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c @@ -18,27 +18,8 @@ #include "wmi-ops.h" #include "debug.h" -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar, - struct ath10k_fw_stats *stats) -{ - struct ath10k_fw_extd_stats_peer *peer; - struct ieee80211_sta *sta; - struct ath10k_sta *arsta; - - rcu_read_lock(); - list_for_each_entry(peer, &stats->peers_extd, list) { - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr, - NULL); - if (!sta) - continue; - arsta = (struct ath10k_sta *)sta->drv_priv; - arsta->rx_duration += (u64)peer->rx_duration; - } - rcu_read_unlock(); -} - -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, - struct ath10k_fw_stats *stats) +void ath10k_sta_update_rx_duration(struct ath10k *ar, + struct ath10k_fw_stats *stats) { struct ath10k_fw_stats_peer *peer; struct ieee80211_sta *sta; @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar, rcu_read_unlock(); } -void ath10k_sta_update_rx_duration(struct ath10k *ar, - struct ath10k_fw_stats *stats) -{ - if (stats->extended) - ath10k_sta_update_extd_stats_rx_duration(ar, stats); - else - ath10k_sta_update_stats_rx_duration(ar, stats); -} - void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_sta *sta, struct station_info *sinfo) diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index c893314a191f..c7ec7b9e9b55 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar, if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0) return 0; - stats->extended = true; - for (i = 0; i < num_peer_stats; i++) { const struct wmi_10_4_peer_extd_stats *src; - struct ath10k_fw_extd_stats_peer *dst; + struct ath10k_fw_stats_peer *dst; src = (void *)skb->data; if (!skb_pull(skb, sizeof(*src))) return -EPROTO; - dst = kzalloc(sizeof(*dst), GFP_ATOMIC); - if (!dst) - continue; + /* Because the stat data may exceed htc-wmi buffer + * limit the firmware might split the stats data + * and delivers it in multiple update events. + * if we can't find the entry in the current event + * payload, we have to look in main list as well. + */ + list_for_each_entry(dst, &stats->peers, list) { + if (ether_addr_equal(dst->peer_macaddr, + src->peer_macaddr.addr)) + goto found; + } + +#ifdef CONFIG_ATH10K_DEBUGFS + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) { + if (ether_addr_equal(dst->peer_macaddr, + src->peer_macaddr.addr)) + goto found; + } +#endif + + ath10k_dbg(ar, ATH10K_DBG_WMI, + "Orphaned extended stats entry for station %pM.\n", + src->peer_macaddr.addr); + continue; - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr); +found: dst->rx_duration = __le32_to_cpu(src->rx_duration); - list_add_tail(&dst->list, &stats->peers_extd); } return 0;
The 10.4 firmware adds extended peer information to the firmware's statistics payload. This additional info is stored as a separate data field. During review of "ath10k: add accounting for the extended peer statistics" [0] Mohammed Shafi Shajakhan commented that the extended peer statistics lists are of little use:"... there is not much use in appending the extended peer stats (which gets periodically updated) to the linked list '&ar->debug.fw_stats.peers_extd)' and should we get rid of the below (and the required cleanup as well) list_splice_tail_init(&stats.peers_extd, &ar->debug.fw_stats.peers_extd); since rx_duration is getting updated periodically to the per sta information." This patch replaces the extended peers list with a lookup and puts the retrieved data (rx_duration) into the existing ath10k_fw_stats_peer entry that was created earlier. [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com> Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> --- drivers/net/wireless/ath/ath10k/core.h | 2 -- drivers/net/wireless/ath/ath10k/debug.c | 17 -------------- drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++----------------------- drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++------- 4 files changed, 28 insertions(+), 57 deletions(-)