The 10.4 firmware adds extended peer information to the
firmware's statistics payload. This additional info is
stored as a separate data field and the elements are
stored in their own "peers_extd" list.
These elements can pile up in the same way as the peer
information elements. This is because the
ath10k_wmi_10_4_op_pull_fw_stats() function tries to
pull the same amount (num_peer_stats) for every statistic
data unit.
Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/net/wireless/ath/ath10k/debug.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 82a4c67f3672..4acd9eb65910 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -399,6 +399,7 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
* prevent firmware from DoS-ing the host.
*/
ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
+ ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
ath10k_warn(ar, "dropping fw peer stats\n");
goto free;
}
@@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
goto free;
}
+ if (!list_empty(&stats.peers))
+ list_splice_tail_init(&stats.peers_extd,
+ &ar->debug.fw_stats.peers_extd);
+
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);
--
2.11.0
On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo <[email protected]> wrote:
> Christian Lamparter <[email protected]> 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 and the elements are
>> stored in their own "peers_extd" list.
>>
>> These elements can pile up in the same way as the peer
>> information elements. This is because the
>> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
>> pull the same amount (num_peer_stats) for every statistic
>> data unit.
>>
>> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
>> Signed-off-by: Christian Lamparter <[email protected]>
>
> My understanding is that I should skip this patch 1. Please let me know if
> I misunderstood. But I'm still plannning to apply patch 2.
I added Mohammed (I hope, he can reply to the open question when he
returns), Since I'm not sure what he wants or not.
As far as I'm aware, the "extended" boolean serves no purpose
because it was only used in once place in debugfs_sta which was
removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
> Patch set to Changes Requested.
Isn't there a: "Waiting for Maintainer" state as well?
Otherwise, if nobody has any complains or question:
can you please queue it for the next merge window?
Regards,
Christian
> --
> https://patchwork.kernel.org/patch/9461631/
>
> Documentation about submitting wireless patches and checking status
> from patchwork:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>
On Wednesday, December 14, 2016 1:03:38 PM CET Mohammed Shafi Shajakhan wrote:
> > On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> > > On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote:
> > > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > > > goto free;
> > > > }
> > > >
> > > > + if (!list_empty(&stats.peers))
> > >
> > > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> > > for normal 'peer stats' ? Is this the fix intended, i had started a build to
> > > check your change and we will keep you posted, does this fix displaying
> > > 'rx_duration' in ath10k fw_stats.
> > The idea is not to queue any "extended peer stats" when there where no "peer stats" to
> > begin with. Because otherwise, the function is still vulnerable to OOM since the
> > extended peers stats will be queued unchecked (not that this is currently a problem).
>
> [shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list
> and append if required ? please let me know if i am still missing something
Well, you can also count the entries in peers_extd and delete the old entries
if they start to overflow. If you want to do it differently, please go ahead.
Regards,
Christian
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 <[email protected]>
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> 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
Christian Lamparter <[email protected]> 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 and the elements are
> stored in their own "peers_extd" list.
>
> These elements can pile up in the same way as the peer
> information elements. This is because the
> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> pull the same amount (num_peer_stats) for every statistic
> data unit.
>
> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> Signed-off-by: Christian Lamparter <[email protected]>
My understanding is that I should skip this patch 1. Please let me know if
I misunderstood. But I'm still plannning to apply patch 2.
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/9461631/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Christian Lamparter <[email protected]> writes:
> On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo <[email protected]> wrot=
e:
>> Christian Lamparter <[email protected]> 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 and the elements are
>>> stored in their own "peers_extd" list.
>>>
>>> These elements can pile up in the same way as the peer
>>> information elements. This is because the
>>> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
>>> pull the same amount (num_peer_stats) for every statistic
>>> data unit.
>>>
>>> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
>>> Signed-off-by: Christian Lamparter <[email protected]>
>>
>> My understanding is that I should skip this patch 1. Please let me know =
if
>> I misunderstood. But I'm still plannning to apply patch 2.
>
> I added Mohammed (I hope, he can reply to the open question when he
> returns), Since I'm not sure what he wants or not.
>
> As far as I'm aware, the "extended" boolean serves no purpose
> because it was only used in once place in debugfs_sta which was
> removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
> and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
I had problems following the discussion so the conclusion was not clear
for me.
>> Patch set to Changes Requested.
>
> Isn't there a: "Waiting for Maintainer" state as well?
> Otherwise, if nobody has any complains or question:
> can you please queue it for the next merge window?
There is a state called "Deferred" which I use whenever I need to
revisit a patch later time. I moved this patch to that state now:
https://patchwork.kernel.org/patch/9461631/
--=20
Kalle Valo=
Hi Christian,
> On Thursday, December 15, 2016 10:13:39 PM CET Mohammed Shafi Shajakhan wrote:
> > I am also thinking, as of now there is not much use in appending
> > the extended peer stats (which gets periodically ) 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. Kindly let me know your thoughts about this.
>
> Yes, of course. I see what your are trying to do and I think it's much better
> to get rid of peers_extd and ath10k_fw_extd_stats_peers_free.
>
[shafi] Feel free to post the change and I can test the same for you(next week) !
Let me know if you are busy on something else, I can take this up.
As discussed, the fix to free 'extd stats' when number of peers exceeds the range
is definitely needed. Thank you for looking into this.
thanks,
shafi
Hello,
It looks like google put your mail into the spam-can.
I'm sorry for not answering sooner.
On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> On Mon, Dec 05, 2016 at 10:52:45PM +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 and the elements are
> > stored in their own "peers_extd" list.
> >
> > These elements can pile up in the same way as the peer
> > information elements. This is because the
> > ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> > pull the same amount (num_peer_stats) for every statistic
> > data unit.
> >
> > Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > drivers/net/wireless/ath/ath10k/debug.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..4acd9eb65910 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -399,6 +399,7 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > * prevent firmware from DoS-ing the host.
> > */
> > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> > + ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
>
> [shafi] thanks for fixing this !
>
> > ath10k_warn(ar, "dropping fw peer stats\n");
> > goto free;
> > }
> > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > goto free;
> > }
> >
> > + if (!list_empty(&stats.peers))
>
> [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> for normal 'peer stats' ? Is this the fix intended, i had started a build to
> check your change and we will keep you posted, does this fix displaying
> 'rx_duration' in ath10k fw_stats.
The idea is not to queue any "extended peer stats" when there where no "peer stats" to
begin with. Because otherwise, the function is still vulnerable to OOM since the
extended peers stats will be queued unchecked (not that this is currently a problem).
> > + list_splice_tail_init(&stats.peers_extd,
> > + &ar->debug.fw_stats.peers_extd);
> > +
> > 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);
Regards,
Christian
Hello Shafi,
On Thursday, December 15, 2016 10:13:39 PM CET Mohammed Shafi Shajakhan wrote:
> I am also thinking, as of now there is not much use in appending
> the extended peer stats (which gets periodically ) 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. Kindly let me know your thoughts about this.
Yes, of course. I see what your are trying to do and I think it's much better
to get rid of peers_extd and ath10k_fw_extd_stats_peers_free.
Regards,
Christian
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 <[email protected]>
> > > > > Signed-off-by: Christian Lamparter <[email protected]>
> > > > > ---
> > > > > 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>
>
>
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 <[email protected]>
> > > Signed-off-by: Christian Lamparter <[email protected]>
> > > ---
> > > 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
ath10k_wmi_tlv_op_pull_fw_stats() uses tb = ath10k_wmi_tlv_parse_alloc(...)
function, which allocates memory. If any of the three error-paths are
taken, this tb needs to be freed.
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index f304f6632c4f..1f6bb9e8bb01 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -1105,8 +1105,10 @@ static int ath10k_wmi_tlv_op_pull_fw_stats(struct ath10k *ar,
struct ath10k_fw_stats_pdev *dst;
src = data;
- if (data_len < sizeof(*src))
+ if (data_len < sizeof(*src)) {
+ kfree(tb);
return -EPROTO;
+ }
data += sizeof(*src);
data_len -= sizeof(*src);
@@ -1126,8 +1128,10 @@ static int ath10k_wmi_tlv_op_pull_fw_stats(struct ath10k *ar,
struct ath10k_fw_stats_vdev *dst;
src = data;
- if (data_len < sizeof(*src))
+ if (data_len < sizeof(*src)) {
+ kfree(tb);
return -EPROTO;
+ }
data += sizeof(*src);
data_len -= sizeof(*src);
@@ -1145,8 +1149,10 @@ static int ath10k_wmi_tlv_op_pull_fw_stats(struct ath10k *ar,
struct ath10k_fw_stats_peer *dst;
src = data;
- if (data_len < sizeof(*src))
+ if (data_len < sizeof(*src)) {
+ kfree(tb);
return -EPROTO;
+ }
data += sizeof(*src);
data_len -= sizeof(*src);
--
2.11.0
Christian Lamparter <[email protected]> wrote:
> ath10k_wmi_tlv_op_pull_fw_stats() uses tb = ath10k_wmi_tlv_parse_alloc(...)
> function, which allocates memory. If any of the three error-paths are
> taken, this tb needs to be freed.
>
> Signed-off-by: Christian Lamparter <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
097e46d2ae90 ath10k: fix potential memory leak in ath10k_wmi_tlv_op_pull_fw_stats()
--
https://patchwork.kernel.org/patch/9461627/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Hello Christian,
On Wed, Dec 14, 2016 at 05:38:02PM +0100, Christian Lamparter wrote:
> On Wednesday, December 14, 2016 1:03:38 PM CET Mohammed Shafi Shajakhan wrote:
> > > On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> > > > On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote:
> > > > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > > > > goto free;
> > > > > }
> > > > >
> > > > > + if (!list_empty(&stats.peers))
> > > >
> > > > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> > > > for normal 'peer stats' ? Is this the fix intended, i had started a build to
> > > > check your change and we will keep you posted, does this fix displaying
> > > > 'rx_duration' in ath10k fw_stats.
> > > The idea is not to queue any "extended peer stats" when there where no "peer stats" to
> > > begin with. Because otherwise, the function is still vulnerable to OOM since the
> > > extended peers stats will be queued unchecked (not that this is currently a problem).
> >
> > [shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list
> > and append if required ? please let me know if i am still missing something
> Well, you can also count the entries in peers_extd and delete the old entries
> if they start to overflow. If you want to do it differently, please go ahead.
>
[shafi] sorry for the delay (got stuck up with something else), I will add some prints explicitly
and keep you posted ASAP. Since the extended peer stats gets updated periodically I
would like to confirm the debug linked list associated to the extended peer
stats does not overflows and potentially cause OOM.
regards,
shafi
Hi Christian,
I am also thinking, as of now there is not much use in appending
the extended peer stats (which gets periodically ) 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. Kindly let me know your thoughts about this.
regards,
shafi
On Thu, Dec 15, 2016 at 09:56:59PM +0530, Mohammed Shafi Shajakhan wrote:
> Hello Christian,
>
> On Wed, Dec 14, 2016 at 05:38:02PM +0100, Christian Lamparter wrote:
> > On Wednesday, December 14, 2016 1:03:38 PM CET Mohammed Shafi Shajakhan wrote:
> > > > On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> > > > > On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote:
> > > > > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > > > > > goto free;
> > > > > > }
> > > > > >
> > > > > > + if (!list_empty(&stats.peers))
> > > > >
> > > > > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> > > > > for normal 'peer stats' ? Is this the fix intended, i had started a build to
> > > > > check your change and we will keep you posted, does this fix displaying
> > > > > 'rx_duration' in ath10k fw_stats.
> > > > The idea is not to queue any "extended peer stats" when there where no "peer stats" to
> > > > begin with. Because otherwise, the function is still vulnerable to OOM since the
> > > > extended peers stats will be queued unchecked (not that this is currently a problem).
> > >
> > > [shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list
> > > and append if required ? please let me know if i am still missing something
> > Well, you can also count the entries in peers_extd and delete the old entries
> > if they start to overflow. If you want to do it differently, please go ahead.
> >
> [shafi] sorry for the delay (got stuck up with something else), I will add some prints explicitly
> and keep you posted ASAP. Since the extended peer stats gets updated periodically I
> would like to confirm the debug linked list associated to the extended peer
> stats does not overflows and potentially cause OOM.
>
> regards,
> shafi
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 <[email protected]>
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > 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
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 <[email protected]>
Signed-off-by: Christian Lamparter <[email protected]>
---
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;
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;
--
2.11.0
On Tue, Dec 13, 2016 at 01:41:33PM +0100, Christian Lamparter wrote:
> Hello,
>
> It looks like google put your mail into the spam-can.
> I'm sorry for not answering sooner.
[shafi] np, thanks for your reply !
>
> On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote:
> > On Mon, Dec 05, 2016 at 10:52:45PM +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 and the elements are
> > > stored in their own "peers_extd" list.
> > >
> > > These elements can pile up in the same way as the peer
> > > information elements. This is because the
> > > ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> > > pull the same amount (num_peer_stats) for every statistic
> > > data unit.
> > >
> > > Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> > > Signed-off-by: Christian Lamparter <[email protected]>
> > > ---
> > > drivers/net/wireless/ath/ath10k/debug.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > index 82a4c67f3672..4acd9eb65910 100644
> > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > @@ -399,6 +399,7 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > > * prevent firmware from DoS-ing the host.
> > > */
> > > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> > > + ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
> >
> > [shafi] thanks for fixing this !
> >
> > > ath10k_warn(ar, "dropping fw peer stats\n");
> > > goto free;
> > > }
> > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > > goto free;
> > > }
> > >
> > > + if (!list_empty(&stats.peers))
> >
> > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
> > for normal 'peer stats' ? Is this the fix intended, i had started a build to
> > check your change and we will keep you posted, does this fix displaying
> > 'rx_duration' in ath10k fw_stats.
> The idea is not to queue any "extended peer stats" when there where no "peer stats" to
> begin with. Because otherwise, the function is still vulnerable to OOM since the
> extended peers stats will be queued unchecked (not that this is currently a problem).
[shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list
and append if required ? please let me know if i am still missing something
>
> > > + list_splice_tail_init(&stats.peers_extd,
> > > + &ar->debug.fw_stats.peers_extd);
> > > +
> > > 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);
>
> Regards,
> Christian
>
>
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 <[email protected]>
> > > > Signed-off-by: Christian Lamparter <[email protected]>
> > > > ---
> > > > 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>
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 <[email protected]>
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k
Hi Christian,
On Mon, Dec 05, 2016 at 10:52:45PM +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 and the elements are
> stored in their own "peers_extd" list.
>
> These elements can pile up in the same way as the peer
> information elements. This is because the
> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> pull the same amount (num_peer_stats) for every statistic
> data unit.
>
> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/debug.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 82a4c67f3672..4acd9eb65910 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -399,6 +399,7 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> * prevent firmware from DoS-ing the host.
> */
> ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> + ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
[shafi] thanks for fixing this !
> ath10k_warn(ar, "dropping fw peer stats\n");
> goto free;
> }
> @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> goto free;
> }
>
> + if (!list_empty(&stats.peers))
[shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking
for normal 'peer stats' ? Is this the fix intended, i had started a build to
check your change and we will keep you posted, does this fix displaying
'rx_duration' in ath10k fw_stats.
> + list_splice_tail_init(&stats.peers_extd,
> + &ar->debug.fw_stats.peers_extd);
> +
> 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);
thanks,
shafi
Hi Christian / Kalle,
On Fri, Dec 30, 2016 at 03:35:10PM +0100, Christian Lamparter wrote:
> On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo <[email protected]> wrote:
> > Christian Lamparter <[email protected]> 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 and the elements are
> >> stored in their own "peers_extd" list.
> >>
> >> These elements can pile up in the same way as the peer
> >> information elements. This is because the
> >> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> >> pull the same amount (num_peer_stats) for every statistic
> >> data unit.
> >>
> >> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> >> Signed-off-by: Christian Lamparter <[email protected]>
> >
> > My understanding is that I should skip this patch 1. Please let me know if
> > I misunderstood. But I'm still plannning to apply patch 2.
>
> I added Mohammed (I hope, he can reply to the open question when he
> returns), Since I'm not sure what he wants or not.
>
> As far as I'm aware, the "extended" boolean serves no purpose
> because it was only used in once place in debugfs_sta which was
> removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
> and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
[shafi] We will wait for Kalle to review from the de-ferred stage
and get his opinion as well(regarding the design change).
I have no concerns as long this does not changes the existing behaviour.
thank you !
regards,
shafi
Christian Lamparter <[email protected]> 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 and the elements are
> stored in their own "peers_extd" list.
>
> These elements can pile up in the same way as the peer
> information elements. This is because the
> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> pull the same amount (num_peer_stats) for every statistic
> data unit.
>
> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> Signed-off-by: Christian Lamparter <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
c1e3330f22bc ath10k: add accounting for the extended peer statistics
--
https://patchwork.kernel.org/patch/9461631/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Christian Lamparter <[email protected]> writes:
> Hello Shafi and Kalle,
>
> On Tuesday, January 3, 2017 10:58:27 AM CET Mohammed Shafi Shajakhan wrot=
e:
>> On Fri, Dec 30, 2016 at 03:35:10PM +0100, Christian Lamparter wrote:
>> > On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo <[email protected]> w=
rote:
>> > > Christian Lamparter <[email protected]> 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 and the elements are
>> > >> stored in their own "peers_extd" list.
>> > >>
>> > >> These elements can pile up in the same way as the peer
>> > >> information elements. This is because the
>> > >> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
>> > >> pull the same amount (num_peer_stats) for every statistic
>> > >> data unit.
>> > >>
>> > >> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats updat=
e")
>> > >> Signed-off-by: Christian Lamparter <[email protected]>
>> > >
>> > > My understanding is that I should skip this patch 1. Please let me k=
now if
>> > > I misunderstood. But I'm still plannning to apply patch 2.
>> >=20
>> > I added Mohammed (I hope, he can reply to the open question when he
>> > returns), Since I'm not sure what he wants or not.
>> >=20
>> > As far as I'm aware, the "extended" boolean serves no purpose
>> > because it was only used in once place in debugfs_sta which was
>> > removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
>> > and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
>>=20
>> [shafi] We will wait for Kalle to review from the de-ferred stage
>> and get his opinion as well(regarding the design change).
>> I have no concerns as long this does not changes the existing behaviour.
>> thank you !
>
> Thank you Shafi for sticking around. I just fished your response to=20
> "Re: [PATCH] ath10k: merge extended peer info data with existing peers in=
fo" [0].
> out of my spam-bucket. Kalle, please look if your copy of the mail got=20
> flagged/deleted as well. Judging from the reply in this thread, I think y=
ou
> overlooked it as well?=20
I think I just read the discussion to hastily as it was rather long,
sorry about that.
After really long or confusin discussions, just to help the maintainers
and also avoid miscommunication between participants, it's usually a
good idea to summarise the conclusion. If us maintainers need to figure
out the conclusion ourselves from a long discussion we are bound to make
mistakes, just like I did here.
So something like this would help me a lot:
"Kalle, please drop these patches. I need to work on these a bit more."
Or:=20
"Kalle, me and John came to agreement about foo. So these should be good
to apply."
> After reading it, I think the previous post and the request to put the pa=
tch
> on wait was unnecessary. As of now, it seems to me that the open question=
s
> between us have been settled amically (so to speak). Kalle, do you have a=
ny
> concerns or can you put this in the next round then?
If you both are happy with the patch, I'm happy to take it :)
I actived the patch again in my queue by moving the state from Deferred
to New:
https://patchwork.kernel.org/patch/9461631/
If all goes well I'm expecting to apply it later this week.
--=20
Kalle Valo=
Hello Shafi and Kalle,
On Tuesday, January 3, 2017 10:58:27 AM CET Mohammed Shafi Shajakhan wrote:
> On Fri, Dec 30, 2016 at 03:35:10PM +0100, Christian Lamparter wrote:
> > On Thu, Dec 29, 2016 at 3:11 PM, Kalle Valo <[email protected]> wrote:
> > > Christian Lamparter <[email protected]> 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 and the elements are
> > >> stored in their own "peers_extd" list.
> > >>
> > >> These elements can pile up in the same way as the peer
> > >> information elements. This is because the
> > >> ath10k_wmi_10_4_op_pull_fw_stats() function tries to
> > >> pull the same amount (num_peer_stats) for every statistic
> > >> data unit.
> > >>
> > >> Fixes: 4a49ae94a448faa ("ath10k: fix 10.4 extended peer stats update")
> > >> Signed-off-by: Christian Lamparter <[email protected]>
> > >
> > > My understanding is that I should skip this patch 1. Please let me know if
> > > I misunderstood. But I'm still plannning to apply patch 2.
> >
> > I added Mohammed (I hope, he can reply to the open question when he
> > returns), Since I'm not sure what he wants or not.
> >
> > As far as I'm aware, the "extended" boolean serves no purpose
> > because it was only used in once place in debugfs_sta which was
> > removed in the patch. ( "ath10k_sta_update_stats_rx_duration"
> > and "ath10k_sta_update_extd_stats_rx_duration" have been unified).
>
> [shafi] We will wait for Kalle to review from the de-ferred stage
> and get his opinion as well(regarding the design change).
> I have no concerns as long this does not changes the existing behaviour.
> thank you !
Thank you Shafi for sticking around. I just fished your response to
"Re: [PATCH] ath10k: merge extended peer info data with existing peers info" [0].
out of my spam-bucket. Kalle, please look if your copy of the mail got
flagged/deleted as well. Judging from the reply in this thread, I think you
overlooked it as well?
After reading it, I think the previous post and the request to put the patch
on wait was unnecessary. As of now, it seems to me that the open questions
between us have been settled amically (so to speak). Kalle, do you have any
concerns or can you put this in the next round then?
Regards,
Christian
[0] <https://www.mail-archive.com/[email protected]/msg06066.html>
Christian Lamparter <[email protected]> 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 <[email protected]>
> Signed-off-by: Christian Lamparter <[email protected]>
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.
--
https://patchwork.kernel.org/patch/9479079/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches