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