[PATCH] mac80211: Update last_ack status for all except probing frames

[PATCH] mac80211: Update last_ack status for all except probing frames

[Rajkumar Manoharan]

Update last_ack status for all except station probing frames
(i.e null data). Otherwise the station inactivity duration is
cleared whenever AP is checking presence of idle stations by sending
null data frame for every inactive threshold (ap_max_inactivity).
Though the station is idle for longer period, the inactive
time in station dump is restricted ap_max_inactivity threshold.

Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
 net/mac80211/status.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index da7427a41529..24ce416f74cd 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -185,10 +185,21 @@ static void ieee80211_check_pending_bar(struct sta_info *sta, u8 *addr, u8 tid)
 static void ieee80211_frame_acked(struct sta_info *sta, struct sk_buff *skb)
 {
 	struct ieee80211_mgmt *mgmt = (void *) skb->data;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_local *local = sta->local;
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
-	if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS))
+	/* Update last_ack status for all except station probing frames
+	 * (i.e null data). Otherwise the station inactivity duration is cleared
+	 * whenever AP is checking presence of idle stations by sending
+	 * null data frame for every inactive threshold (ap_max_inactivity).
+	 * Though the station is idle for longer period, the inactive time in
+	 * station dump is restricted ap_max_inactivity threshold.
+	 */
+	if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS) &&
+	    !((info->flags & IEEE80211_TX_INTFL_NL80211_FRAME_TX) &&
+	      (ieee80211_is_nullfunc(mgmt->frame_control) ||
+	       ieee80211_is_qos_nullfunc(mgmt->frame_control))))
 		sta->status_stats.last_ack = jiffies;
 
 	if (ieee80211_is_data_qos(mgmt->frame_control)) {
-- 
1.9.1

Re: [PATCH] mac80211: Update last_ack status for all except probing frames

[Igor Mitsyanko]

On 10/30/2017 05:29 PM, Rajkumar Manoharan wrote:
> 
> Update last_ack status for all except station probing frames
> (i.e null data). Otherwise the station inactivity duration is
> cleared whenever AP is checking presence of idle stations by sending
> null data frame for every inactive threshold (ap_max_inactivity).
> Though the station is idle for longer period, the inactive
> time in station dump is restricted ap_max_inactivity threshold.
> 
> Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
> ---

To clarify: wouldn't it break hostapd's "inactivity timeot kick-out" 
logic in hostapd? As hostapd generates NULL frames to idle stations to 
reset their inactivity time, otherwise it will disassociate a station, 
after an ap_max_inactivity period.

RE: [PATCH] mac80211: Update last_ack status for all except probing frames

[Rajkumar Manoharan]

PiBUbyBjbGFyaWZ5OiB3b3VsZG4ndCBpdCBicmVhayBob3N0YXBkJ3MgImluYWN0aXZpdHkgdGlt
ZW90IGtpY2stb3V0Ig0KPiBsb2dpYyBpbiBob3N0YXBkPyBBcyBob3N0YXBkIGdlbmVyYXRlcyBO
VUxMIGZyYW1lcyB0byBpZGxlIHN0YXRpb25zIHRvIHJlc2V0DQo+IHRoZWlyIGluYWN0aXZpdHkg
dGltZSwgb3RoZXJ3aXNlIGl0IHdpbGwgZGlzYXNzb2NpYXRlIGEgc3RhdGlvbiwgYWZ0ZXIgYW4N
Cj4gYXBfbWF4X2luYWN0aXZpdHkgcGVyaW9kLg0KDQpJZ29yLA0KDQpJIHZhbGlkYXRlZCBob3N0
cGFkIGluYWN0aXZpdHkgbW9uaXRvcmluZyB3aXRoIHRoaXMgY2hhbmdlIGFuZCBIb3N0YXBkIGlz
IHNlbmRpbmcNCk51bGxkYXRhIHBlcmlvZGljYWxseSBmb3IgaW5hY3RpdmUgc3RhdGlvbnMuIEhv
c3RhcGQgcmVzZXRzIHRpbWVyIGJhc2VkIG9uIHR4IGFjaw0Kc3RhdHVzIG9mIG51bGxkYXRhLiBT
byByZXRhaW5pbmcgYWN0dWFsIHN0YXRpb24gaW5hY3RpdmUgcGVyaW9kIHdpbGwgbm90IGJyZWFr
DQpob3N0YXBkIG1vbml0b3JpbmcuDQoNClBsZWFzZSByZXBvcnQgaWYgeW91IHNlZSBhbnkgaXNz
dWUgd2l0aCB0aGlzIGNoYW5nZS4gVGhhbmtzIGZvciB5b3VyIGZlZWRiYWNrLg0KDQotUmFqa3Vt
YXINCg0K

Re: [PATCH] mac80211: Update last_ack status for all except probing frames

[Johannes Berg]

On Mon, 2017-10-30 at 17:29 -0700, Rajkumar Manoharan wrote:
> Update last_ack status for all except station probing frames
> (i.e null data). Otherwise the station inactivity duration is
> cleared whenever AP is checking presence of idle stations by sending
> null data frame for every inactive threshold (ap_max_inactivity).

You make it sound like this is a problem, but don't explain why it
would be? I don't really see it anyway.

johannes

RE: [PATCH] mac80211: Update last_ack status for all except probing frames

[Rajkumar Manoharan]

PiBPbiBNb24sIDIwMTctMTAtMzAgYXQgMTc6MjkgLTA3MDAsIFJhamt1bWFyIE1hbm9oYXJhbiB3
cm90ZToNCj4gPiBVcGRhdGUgbGFzdF9hY2sgc3RhdHVzIGZvciBhbGwgZXhjZXB0IHN0YXRpb24g
cHJvYmluZyBmcmFtZXMgKGkuZSBudWxsDQo+ID4gZGF0YSkuIE90aGVyd2lzZSB0aGUgc3RhdGlv
biBpbmFjdGl2aXR5IGR1cmF0aW9uIGlzIGNsZWFyZWQgd2hlbmV2ZXINCj4gPiBBUCBpcyBjaGVj
a2luZyBwcmVzZW5jZSBvZiBpZGxlIHN0YXRpb25zIGJ5IHNlbmRpbmcgbnVsbCBkYXRhIGZyYW1l
DQo+ID4gZm9yIGV2ZXJ5IGluYWN0aXZlIHRocmVzaG9sZCAoYXBfbWF4X2luYWN0aXZpdHkpLg0K
PiANCj4gWW91IG1ha2UgaXQgc291bmQgbGlrZSB0aGlzIGlzIGEgcHJvYmxlbSwgYnV0IGRvbid0
IGV4cGxhaW4gd2h5IGl0IHdvdWxkIGJlPyBJDQo+IGRvbid0IHJlYWxseSBzZWUgaXQgYW55d2F5
Lg0KPiANCkZvciBzdGVlcmluZyBhbiBpZGxlIHN0YXRpb24gZnJvbSBvbmUgQlNTIHRvIGFub3Ro
ZXIsIHRoZSBzdGVlcmluZyBhcHBsaWNhdGlvbg0KaGFzIHRvIGtub3cgdGhlIGFjdHVhbCBzdGF0
aW9uIGlkbGUgcGVyaW9kLiBCdXQgaWYgdGhlIGlkbGUgcGVyaW9kIGlzIGNsZWFyZWQgYnkNCmFw
X21heF9pbmFjdGl2aXR5LCB0aGUgc3RlZXJpbmcgYXBwbGljYXRpb24gY2Fubm90IHN0ZWVyIHRo
ZSBzdGF0aW9uIGV2ZW4gdGhvdWdoDQp0aGUgc3RhdGlvbiBpcyBub3Qgc2VuZGluZyBvciByZWNl
aXZpbmcgYW55IGRhdGEgb3IgbWdtdC4gZnJhbWUuDQoNClRob3VnaCBrZWVwaW5nIGxvbmdlciBp
ZGxlIHRocmVzaG9sZCAoYXBfbWF4X2luYWN0aXZpdHkpIHRoYW4gc3RlZXJpbmcgYXBwbGljYXRp
b24NCmxpbWl0IG1heSBoZWxwIHRvIGlkZW50aWZ5IGlkbGUgc3RhdGlvbiwgaXQgd2lsbCBkaWZm
ZXIgcmVtb3ZpbmcgdGhlIHN0YXRpb25zIHRoYXQgbGVmdCBCU1MuDQoNCi1SYWprdW1hcg0K

Re: [PATCH] mac80211: Update last_ack status for all except probing frames

[Igor Mitsyanko]

On 11/06/2017 09:44 AM, Rajkumar Manoharan wrote:
>> On Mon, 2017-10-30 at 17:29 -0700, Rajkumar Manoharan wrote:
>>> Update last_ack status for all except station probing frames (i.e null
>>> data). Otherwise the station inactivity duration is cleared whenever
>>> AP is checking presence of idle stations by sending null data frame
>>> for every inactive threshold (ap_max_inactivity).
>>
>> You make it sound like this is a problem, but don't explain why it would be? I
>> don't really see it anyway.
>>
> For steering an idle station from one BSS to another, the steering application
> has to know the actual station idle period. But if the idle period is cleared by
> ap_max_inactivity, the steering application cannot steer the station even though
> the station is not sending or receiving any data or mgmt. frame.
> 
> Though keeping longer idle threshold (ap_max_inactivity) than steering application
> limit may help to identify idle station, it will differ removing the stations that left BSS.
> 
> -Rajkumar
> 

Hi Rajkumar, this will not help in all the cases as some drivers may 
choose to implement "inactivity" logic in firmware and firmware will 
send probing frames itself if required, resetting inactivity period. 
Besides, at this level we do not know who sent NULL frames and for which 
purpose, it doesn't look like a good idea to just ignore it.

I think that for what it was supposed to be, inactivity_ms works as 
expected and it can be used to identify "dead" stations. What you 
probably need is something different, called for example "rx_idle_time" 
that will keep track of how long STA itself did not send any frames 
(ignoring ACKs).

RE: [PATCH] mac80211: Update last_ack status for all except probing frames

[Rajkumar Manoharan]

PiA+Pj4gVXBkYXRlIGxhc3RfYWNrIHN0YXR1cyBmb3IgYWxsIGV4Y2VwdCBzdGF0aW9uIHByb2Jp
bmcgZnJhbWVzIChpLmUNCj4gPj4+IG51bGwgZGF0YSkuIE90aGVyd2lzZSB0aGUgc3RhdGlvbiBp
bmFjdGl2aXR5IGR1cmF0aW9uIGlzIGNsZWFyZWQNCj4gPj4+IHdoZW5ldmVyIEFQIGlzIGNoZWNr
aW5nIHByZXNlbmNlIG9mIGlkbGUgc3RhdGlvbnMgYnkgc2VuZGluZyBudWxsDQo+ID4+PiBkYXRh
IGZyYW1lIGZvciBldmVyeSBpbmFjdGl2ZSB0aHJlc2hvbGQgKGFwX21heF9pbmFjdGl2aXR5KS4N
Cj4gPj4NCj4gPj4gWW91IG1ha2UgaXQgc291bmQgbGlrZSB0aGlzIGlzIGEgcHJvYmxlbSwgYnV0
IGRvbid0IGV4cGxhaW4gd2h5IGl0DQo+ID4+IHdvdWxkIGJlPyBJIGRvbid0IHJlYWxseSBzZWUg
aXQgYW55d2F5Lg0KPiA+Pg0KPiA+IEZvciBzdGVlcmluZyBhbiBpZGxlIHN0YXRpb24gZnJvbSBv
bmUgQlNTIHRvIGFub3RoZXIsIHRoZSBzdGVlcmluZw0KPiA+IGFwcGxpY2F0aW9uIGhhcyB0byBr
bm93IHRoZSBhY3R1YWwgc3RhdGlvbiBpZGxlIHBlcmlvZC4gQnV0IGlmIHRoZQ0KPiA+IGlkbGUg
cGVyaW9kIGlzIGNsZWFyZWQgYnkgYXBfbWF4X2luYWN0aXZpdHksIHRoZSBzdGVlcmluZyBhcHBs
aWNhdGlvbg0KPiA+IGNhbm5vdCBzdGVlciB0aGUgc3RhdGlvbiBldmVuIHRob3VnaCB0aGUgc3Rh
dGlvbiBpcyBub3Qgc2VuZGluZyBvciByZWNlaXZpbmcNCj4gYW55IGRhdGEgb3IgbWdtdC4gZnJh
bWUuDQo+ID4NCj4gPiBUaG91Z2gga2VlcGluZyBsb25nZXIgaWRsZSB0aHJlc2hvbGQgKGFwX21h
eF9pbmFjdGl2aXR5KSB0aGFuIHN0ZWVyaW5nDQo+ID4gYXBwbGljYXRpb24gbGltaXQgbWF5IGhl
bHAgdG8gaWRlbnRpZnkgaWRsZSBzdGF0aW9uLCBpdCB3aWxsIGRpZmZlciByZW1vdmluZyB0aGUN
Cj4gc3RhdGlvbnMgdGhhdCBsZWZ0IEJTUy4NCj4gPg0KPiBIaSBSYWprdW1hciwgdGhpcyB3aWxs
IG5vdCBoZWxwIGluIGFsbCB0aGUgY2FzZXMgYXMgc29tZSBkcml2ZXJzIG1heSBjaG9vc2UgdG8N
Cj4gaW1wbGVtZW50ICJpbmFjdGl2aXR5IiBsb2dpYyBpbiBmaXJtd2FyZSBhbmQgZmlybXdhcmUg
d2lsbCBzZW5kIHByb2JpbmcgZnJhbWVzDQo+IGl0c2VsZiBpZiByZXF1aXJlZCwgcmVzZXR0aW5n
IGluYWN0aXZpdHkgcGVyaW9kLg0KPg0KSW4gb2ZmbG9hZCBhcHByb2FjaCwgbm8gbmVlZCB0byBy
ZXBvcnQgdHhfYWNrIGZvciBmaXJtd2FyZSBnZW5lcmF0ZWQgZnJhbWVzLiBubz8NCg0KPiBCZXNp
ZGVzLCBhdCB0aGlzIGxldmVsIHdlIGRvIG5vdCBrbm93IHdobyBzZW50IE5VTEwgZnJhbWVzIGFu
ZCBmb3Igd2hpY2gNCj4gcHVycG9zZSwgaXQgZG9lc24ndCBsb29rIGxpa2UgYSBnb29kIGlkZWEg
dG8ganVzdCBpZ25vcmUgaXQuDQo+DQpJIGhhZCB0aGUgc2FtZSBkb3VidC4gQnV0IEFGQUlLLCBw
cm9iZV9jbGllbnQgbmV0bGluayBjb21tYW5kIGlzIHVzZWQgYnkNCkhvc3RhcGQgb25seS4gVGhh
dOKAmXMgd2h5IEkgYWRkZWQgY29tbWVudCBzZWN0aW9uIGJlZm9yZSB0aGUgY2hhbmdlLg0KIA0K
PiBJIHRoaW5rIHRoYXQgZm9yIHdoYXQgaXQgd2FzIHN1cHBvc2VkIHRvIGJlLCBpbmFjdGl2aXR5
X21zIHdvcmtzIGFzIGV4cGVjdGVkIGFuZA0KPiBpdCBjYW4gYmUgdXNlZCB0byBpZGVudGlmeSAi
ZGVhZCIgc3RhdGlvbnMuIFdoYXQgeW91IHByb2JhYmx5IG5lZWQgaXMgc29tZXRoaW5nDQo+IGRp
ZmZlcmVudCwgY2FsbGVkIGZvciBleGFtcGxlICJyeF9pZGxlX3RpbWUiDQo+IHRoYXQgd2lsbCBr
ZWVwIHRyYWNrIG9mIGhvdyBsb25nIFNUQSBpdHNlbGYgZGlkIG5vdCBzZW5kIGFueSBmcmFtZXMg
KGlnbm9yaW5nDQo+IEFDS3MpLg0KPg0KSG1tLi4gInJ4X2lkbGVfdGltZSIgb25seSBub3QgZW5v
dWdoIHRvIGRldGVybWluZSBhY3RpdmUgc3RhdGUuIEJlY2F1c2UgYSBzdGF0aW9uDQpjYW4gcmVj
ZWl2ZSB1bmktZGlyZWN0aW9uIGRhdGEgdHJhZmZpYy4gRm9yIGV4YW1wbGUsIElmIHRoZSBzdGF0
aW9uIGlzIGNvbm5lY3RlZCBhbmQNCmlkbGUgZm9yIGhhbGYgYW4gaG91ciBidXQgd2l0aGluIEJT
UywgdGhlbiBib3RoICJpbmFjdGl2aXR5X21zIiBhbmQgImNvbm5lY3RlZF90aW1lIiANCnNob3Vs
ZCBiZSAzMCBtaW5zLiBJc24ndCBpdD8uDQoNCi1SYWprdW1hcg0K

Re: [PATCH] mac80211: Update last_ack status for all except probing frames

[Johannes Berg]

On Mon, 2017-11-06 at 20:20 +0000, Rajkumar Manoharan wrote:
> > > For steering an idle station from one BSS to another, the steering
> > > application has to know the actual station idle period. But if the
> > > idle period is cleared by ap_max_inactivity, the steering application
> > > cannot steer the station even though the station is not sending or receiving
> > 
> > any data or mgmt. frame.

I guess you want something else. Why even consider a station that's
sending mgmt frames as non-idle?

You can just use the data TX/RX counters to figure out if the station
has been idle.

You've not convinced me that changing the current logic here makes any
sense.

johannes

RE: [PATCH] mac80211: Update last_ack status for all except probing frames

[Rajkumar Manoharan]

PiBPbiBNb24sIDIwMTctMTEtMDYgYXQgMjA6MjAgKzAwMDAsIFJhamt1bWFyIE1hbm9oYXJhbiB3
cm90ZToNCj4gPiA+ID4gRm9yIHN0ZWVyaW5nIGFuIGlkbGUgc3RhdGlvbiBmcm9tIG9uZSBCU1Mg
dG8gYW5vdGhlciwgdGhlIHN0ZWVyaW5nDQo+ID4gPiA+IGFwcGxpY2F0aW9uIGhhcyB0byBrbm93
IHRoZSBhY3R1YWwgc3RhdGlvbiBpZGxlIHBlcmlvZC4gQnV0IGlmIHRoZQ0KPiA+ID4gPiBpZGxl
IHBlcmlvZCBpcyBjbGVhcmVkIGJ5IGFwX21heF9pbmFjdGl2aXR5LCB0aGUgc3RlZXJpbmcNCj4g
PiA+ID4gYXBwbGljYXRpb24gY2Fubm90IHN0ZWVyIHRoZSBzdGF0aW9uIGV2ZW4gdGhvdWdoIHRo
ZSBzdGF0aW9uIGlzDQo+ID4gPiA+IG5vdCBzZW5kaW5nIG9yIHJlY2VpdmluZw0KPiA+ID4NCj4g
PiA+IGFueSBkYXRhIG9yIG1nbXQuIGZyYW1lLg0KPiANCj4gSSBndWVzcyB5b3Ugd2FudCBzb21l
dGhpbmcgZWxzZS4gV2h5IGV2ZW4gY29uc2lkZXIgYSBzdGF0aW9uIHRoYXQncyBzZW5kaW5nDQo+
IG1nbXQgZnJhbWVzIGFzIG5vbi1pZGxlPw0KPiANCj4gWW91IGNhbiBqdXN0IHVzZSB0aGUgZGF0
YSBUWC9SWCBjb3VudGVycyB0byBmaWd1cmUgb3V0IGlmIHRoZSBzdGF0aW9uIGhhcyBiZWVuDQo+
IGlkbGUuDQo+IA0KSG1tLi4gQnV0IHBvbGxpbmcgVFgvUlggY291bnRlcnMgd2lsbCBub3QgZ2l2
ZSBleGFjdCBhY3Rpdml0eSB0aW1lLiBNb3Jlb3Zlcg0KSXQgd2lsbCBiZSB1c2VmdWwgaWYgU1RB
J3MgbGFzdCBkYXRhIGFjdGl2aXR5IHRzdGFtcCBpcyBsb2dnZWQgaW4gc3RhdGlvbiBkdW1wLiBO
bz8NCg0KPiBZb3UndmUgbm90IGNvbnZpbmNlZCBtZSB0aGF0IGNoYW5naW5nIHRoZSBjdXJyZW50
IGxvZ2ljIGhlcmUgbWFrZXMgYW55IHNlbnNlLg0KPiANCkkgdGhvdWdodCBhZGRpbmcgbmV3IGlu
YWN0aXZlIHRpbWUgYXR0cmlidXRlIG1heSBjb25mbGljdCB3aXRoIGV4aXN0aW5nIGxhc3RfYWN0
aXZlDQppbmZvLiAgQXMgSWdvciBtZW50aW9uZWQgZWFybGllciwgd2hhdCBpcyB5b3VyIHRob3Vn
aHQgb2YgYWRkaW5nIG5ldyBhdHRyaWJ1dGUgDQoic3RhX2xhc3RfZGF0YV9hY3RpdmUiIHRvIG1v
bml0b3IgZGF0YSBmcmFtZS4NCg0KQW55IHN1Z2dlc3Rpb25zIGFyZSB3ZWxjb21lLg0KDQotUmFq
a3VtYXINCg0K

Re: [PATCH] mac80211: Update last_ack status for all except probing frames

[Johannes Berg]

On Wed, 2017-11-15 at 19:21 +0000, Rajkumar Manoharan wrote:
> > 
> Hmm.. But polling TX/RX counters will not give exact activity time. 

Yeah but if you do it every minute, you'll know if it was idle for
roughly 5 minutes, or so... dunno, things could be done.

> Moreover
> It will be useful if STA's last data activity tstamp is logged in station dump. No?

I don't object to adding that, though we should be careful with adding
things and see if we _really_ need them, since they all have a cost.

johannes