| Message ID | 992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1e3330f22bc3c53e6a2c4282dd5a1dc6e6bcca1 |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
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 > >
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
Hello Christian, On Wed, Dec 14, 2016 at 05:38:02PM +0100, Christian Lamparter wrote: > On Wednesday, December 14, 2016 1:03:38 PM CET Mohammed Shafi Shajakhan wrote: > > > On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote: > > > > On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote: > > > > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > > > > goto free; > > > > > } > > > > > > > > > > + if (!list_empty(&stats.peers)) > > > > > > > > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking > > > > for normal 'peer stats' ? Is this the fix intended, i had started a build to > > > > check your change and we will keep you posted, does this fix displaying > > > > 'rx_duration' in ath10k fw_stats. > > > The idea is not to queue any "extended peer stats" when there where no "peer stats" to > > > begin with. Because otherwise, the function is still vulnerable to OOM since the > > > extended peers stats will be queued unchecked (not that this is currently a problem). > > > > [shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list > > and append if required ? please let me know if i am still missing something > Well, you can also count the entries in peers_extd and delete the old entries > if they start to overflow. If you want to do it differently, please go ahead. > [shafi] sorry for the delay (got stuck up with something else), I will add some prints explicitly and keep you posted ASAP. Since the extended peer stats gets updated periodically I would like to confirm the debug linked list associated to the extended peer stats does not overflows and potentially cause OOM. regards, shafi
Hi Christian, I am also thinking, as of now there is not much use in appending the extended peer stats (which gets periodically ) to the linked list '&ar->debug.fw_stats.peers_extd)' and should we get rid of the below (and the required cleanup as well) list_splice_tail_init(&stats.peers_extd, &ar->debug.fw_stats.peers_extd); since rx_duration is getting updated periodically to the per sta information. Kindly let me know your thoughts about this. regards, shafi On Thu, Dec 15, 2016 at 09:56:59PM +0530, Mohammed Shafi Shajakhan wrote: > Hello Christian, > > On Wed, Dec 14, 2016 at 05:38:02PM +0100, Christian Lamparter wrote: > > On Wednesday, December 14, 2016 1:03:38 PM CET Mohammed Shafi Shajakhan wrote: > > > > On Wednesday, December 7, 2016 11:58:24 AM CET Mohammed Shafi Shajakhan wrote: > > > > > On Mon, Dec 05, 2016 at 10:52:45PM +0100, Christian Lamparter wrote: > > > > > > @@ -409,10 +410,12 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb) > > > > > > goto free; > > > > > > } > > > > > > > > > > > > + if (!list_empty(&stats.peers)) > > > > > > > > > > [shafi] sorry please correct me if i am wrong, for 'extended peer stats' we are checking > > > > > for normal 'peer stats' ? Is this the fix intended, i had started a build to > > > > > check your change and we will keep you posted, does this fix displaying > > > > > 'rx_duration' in ath10k fw_stats. > > > > The idea is not to queue any "extended peer stats" when there where no "peer stats" to > > > > begin with. Because otherwise, the function is still vulnerable to OOM since the > > > > extended peers stats will be queued unchecked (not that this is currently a problem). > > > > > > [shafi] list_splice_tail_init should still check for non-empty 'peers_extd' list > > > and append if required ? please let me know if i am still missing something > > Well, you can also count the entries in peers_extd and delete the old entries > > if they start to overflow. If you want to do it differently, please go ahead. > > > [shafi] sorry for the delay (got stuck up with something else), I will add some prints explicitly > and keep you posted ASAP. Since the extended peer stats gets updated periodically I > would like to confirm the debug linked list associated to the extended peer > stats does not overflows and potentially cause OOM. > > regards, > shafi
Hello Shafi, On Thursday, December 15, 2016 10:13:39 PM CET Mohammed Shafi Shajakhan wrote: > I am also thinking, as of now there is not much use in appending > the extended peer stats (which gets periodically ) to the linked list > '&ar->debug.fw_stats.peers_extd)' and should we get rid of the below > (and the required cleanup as well) > > list_splice_tail_init(&stats.peers_extd, > &ar->debug.fw_stats.peers_extd); > > > since rx_duration is getting updated periodically to the per sta > information. Kindly let me know your thoughts about this. Yes, of course. I see what your are trying to do and I think it's much better to get rid of peers_extd and ath10k_fw_extd_stats_peers_free. Regards, Christian
Hi Christian, > On 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
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.
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 >
Christian Lamparter <chunkeey@googlemail.com> writes: > 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). 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/
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
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>
Christian Lamparter <chunkeey@googlemail.com> writes: > 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? 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: "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 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? 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.
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
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);
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(-)