From: Ahmed Zaki <[email protected]>
[ Upstream commit 285531f9e6774e3be71da6673d475ff1a088d675 ]
In the first 5 minutes after boot (time of INITIAL_JIFFIES),
ieee80211_sta_last_active() returns zero if last_ack is zero. This
leads to "inactive time" showing jiffies_to_msecs(jiffies).
# iw wlan0 station get fc:ec:da:64:a6:dd
Station fc:ec:da:64:a6:dd (on wlan0)
inactive time: 4294894049 ms
.
.
connected time: 70 seconds
Fix by returning last_rx if last_ack == 0.
Signed-off-by: Ahmed Zaki <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Johannes Berg <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
net/mac80211/sta_info.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index f34202242d24d..507409e3fd39c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2356,7 +2356,8 @@ unsigned long ieee80211_sta_last_active(struct sta_info *sta)
{
struct ieee80211_sta_rx_stats *stats = sta_get_last_rx_stats(sta);
- if (time_after(stats->last_rx, sta->status_stats.last_ack))
+ if (!sta->status_stats.last_ack ||
+ time_after(stats->last_rx, sta->status_stats.last_ack))
return stats->last_rx;
return sta->status_stats.last_ack;
}
--
2.20.1
On Tue 2019-12-03 23:31:50, Greg Kroah-Hartman wrote:
> From: Ahmed Zaki <[email protected]>
>
> [ Upstream commit 285531f9e6774e3be71da6673d475ff1a088d675 ]
>
> In the first 5 minutes after boot (time of INITIAL_JIFFIES),
> ieee80211_sta_last_active() returns zero if last_ack is zero. This
> leads to "inactive time" showing jiffies_to_msecs(jiffies).
>
> # iw wlan0 station get fc:ec:da:64:a6:dd
> Station fc:ec:da:64:a6:dd (on wlan0)
> inactive time: 4294894049 ms
> .
> .
> connected time: 70 seconds
>
> Fix by returning last_rx if last_ack == 0.
I guess it fixes the message, but is it right fix?
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index f34202242d24d..507409e3fd39c 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -2356,7 +2356,8 @@ unsigned long ieee80211_sta_last_active(struct sta_info *sta)
> {
> struct ieee80211_sta_rx_stats *stats = sta_get_last_rx_stats(sta);
>
> - if (time_after(stats->last_rx, sta->status_stats.last_ack))
> + if (!sta->status_stats.last_ack ||
> + time_after(stats->last_rx, sta->status_stats.last_ack))
> return stats->last_rx;
> return sta->status_stats.last_ack;
> }
I mean, jiffies do wrapraound periodically, so eventually we'll have
sta->status_stats.last_ack == 0 even through it is not short after
boot, no?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 2019-12-04 at 11:56 +0000, Pavel Machek wrote:
> > - if (time_after(stats->last_rx, sta->status_stats.last_ack))
> > + if (!sta->status_stats.last_ack ||
> > + time_after(stats->last_rx, sta->status_stats.last_ack))
> > return stats->last_rx;
> > return sta->status_stats.last_ack;
> > }
>
> I mean, jiffies do wrapraound periodically, so eventually we'll have
> sta->status_stats.last_ack == 0 even through it is not short after
> boot, no?
Yeah. I contemplated that when I applied the original patch - it's a bit
complicated otherwise, you have to track "is this valid" etc.
Since this is updated on pretty much every frame, it's highly unlikely
you'll go without the value for long, so I figured this was good enough.
johannes
On Wed, 4 Dec 2019 at 14:05, Johannes Berg <[email protected]> wrote:
>
> On Wed, 2019-12-04 at 11:56 +0000, Pavel Machek wrote:
>
> > > - if (time_after(stats->last_rx, sta->status_stats.last_ack))
> > > + if (!sta->status_stats.last_ack ||
> > > + time_after(stats->last_rx, sta->status_stats.last_ack))
> > > return stats->last_rx;
> > > return sta->status_stats.last_ack;
> > > }
> >
> > I mean, jiffies do wrapraound periodically, so eventually we'll have
> > sta->status_stats.last_ack == 0 even through it is not short after
> > boot, no?
>
> Yeah. I contemplated that when I applied the original patch - it's a bit
> complicated otherwise, you have to track "is this valid" etc.
>
> Since this is updated on pretty much every frame, it's highly unlikely
> you'll go without the value for long, so I figured this was good enough.
>
> johannes
>
That happened when I was testing an IBSS network, so a unit would have
last_ack = 0
for some neighbours for extended period of time, but last_rx was
always valid (from
broadcasts and beacons I guess). In this case, it makes sense to use
last_rx, no matter
jiffies had wrapped around or not.
For STA/AP situations, last_ack should pretty much be valid and non-zero.
Ahmed