12:24 a.m.
Fix an issue with the recent changes to signal monitoring from commit
f456501b ("station: retry roaming unless notified of a high RSSI"):
1. driver sends NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW
2. netdev->cur_rssi_low changes from FALSE to TRUE
3. netdev sends NETDEV_EVENT_RSSI_THRESHOLD_LOW to station
4. on roam reassociation, cur_rssi_low is reset to FALSE
5. station still assumes RSSI is low, periodically roams
until netdev sends NETDEV_EVENT_RSSI_THRESHOLD_HIGH
6. driver sends NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH
7. netdev->cur_rssi_low doesn't change (still FALSE)
8. netdev never sends NETDEV_EVENT_RSSI_THRESHOLD_HIGH
9. station remains stuck in an infinite roaming loop
The commit in question introduced the logic in (5). Previously the
assumption in station was - like in netdev - that if the signal was
still low, the driver would send a duplicate LOW event after
reassociation. This change makes netdev follow the same new logic as
station, i.e. assume the same signal state (LOW/HIGH) until told
otherwise by the driver.
---
v1 -> v2:
- No changes to this patch, but added an autotest to the patch series to
validate the fix
src/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/netdev.c b/src/netdev.c
index a19270c0..630f61f5 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -659,6 +659,7 @@ static void netdev_connect_free(struct netdev *netdev)
netdev->in_ft = false;
netdev->ignore_connect_event = false;
netdev->expect_connect_failure = false;
+ netdev->cur_rssi_low = false;
if (netdev->connect_cmd) {
l_genl_msg_unref(netdev->connect_cmd);
@@ -2810,7 +2811,6 @@ static int netdev_connect_common(struct netdev *netdev,
netdev->handshake = hs;
netdev->sm = sm;
netdev->frequency = bss->frequency;
- netdev->cur_rssi_low = false; /* Gets updated on the 1st CQM event */
netdev->cur_rssi = bss->signal_strength / 100;
netdev_rssi_level_init(netdev);
--
2.29.2
12:24 a.m.
New subject: [PATCH v2 2/2] auto-t: add test for stopping periodic roam retries
---
autotests/testRoamRetry/stop_retry_test.py | 134 +++++++++++++++++++++
1 file changed, 134 insertions(+)
create mode 100644 autotests/testRoamRetry/stop_retry_test.py
diff --git a/autotests/testRoamRetry/stop_retry_test.py
b/autotests/testRoamRetry/stop_retry_test.py
new file mode 100644
index 00000000..bbe6e2a1
--- /dev/null
+++ b/autotests/testRoamRetry/stop_retry_test.py
@@ -0,0 +1,134 @@
+#!/usr/bin/python3
+
+import unittest
+import sys
+
+sys.path.append('../util')
+import iwd
+from iwd import IWD
+from iwd import PSKAgent
+from iwd import NetworkType
+
+import testutil
+
+from hostapd import HostapdCLI
+from hwsim import Hwsim
+
+class Test(unittest.TestCase):
+ # Test that we do not periodically retry roaming if the transision looks
+ # like this: LOW [roam] [new bss] HIGH.
+ def test_stop_retry(self):
+ hwsim = Hwsim()
+
+ bss_hostapd = [ HostapdCLI(config='ssid1.conf'),
+ HostapdCLI(config='ssid2.conf') ]
+ bss_radio = [ hwsim.get_radio('rad0'),
+ hwsim.get_radio('rad1') ]
+
+ rule0 = hwsim.rules.create()
+ rule0.source = bss_radio[0].addresses[0]
+ rule0.bidirectional = True
+
+ rule1 = hwsim.rules.create()
+ rule1.source = bss_radio[1].addresses[0]
+ rule1.bidirectional = True
+
+ # Fill in the neighbor AP tables in both BSSes. By default each
+ # instance knows only about current BSS, even inside one hostapd
+ # process.
+ # Roaming still works without the neighbor AP table but neighbor
+ # reports have to be disabled in the .conf files
+ bss0_nr = ''.join(bss_radio[0].addresses[0].split(':')) + \
+ '8f0000005101060603000000'
+ bss1_nr = ''.join(bss_radio[1].addresses[0].split(':')) + \
+ '8f0000005102060603000000'
+
+ bss_hostapd[0].set_neighbor(bss_radio[1].addresses[0], 'TestRoamRetry',
+ bss1_nr)
+ bss_hostapd[1].set_neighbor(bss_radio[0].addresses[0], 'TestRoamRetry',
+ bss0_nr)
+
+ # Start in the vicinity of BSS 0, check that iwd connects to BSS 0
+ rule0.signal = -2000
+ rule1.signal = -5000
+
+ wd = IWD()
+
+ psk_agent = PSKAgent("secret123")
+ wd.register_psk_agent(psk_agent)
+
+ devices = wd.list_devices(1)
+ device = devices[0]
+
+ condition = 'not obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ device.scan()
+
+ condition = 'not obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ ordered_network = device.get_ordered_network('TestRoamRetry')
+
+ self.assertEqual(ordered_network.type, NetworkType.psk)
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+ ordered_network.network_object.connect()
+
+ condition = 'obj.state == DeviceState.connected'
+ wd.wait_for_object_condition(device, condition)
+
+ self.assertTrue(bss_hostapd[0].list_sta())
+ self.assertFalse(bss_hostapd[1].list_sta())
+
+ wd.wait(5)
+
+ # Now push the signal below the RSSI threshold and check that iwd
+ # connects to BSS 1
+ rule0.signal = -8000
+
+ condition = 'obj.state == DeviceState.roaming'
+ wd.wait_for_object_condition(device, condition, max_wait=10)
+
+ # Check that iwd is on BSS 1 once out of roaming state and doesn't
+ # go through 'disconnected', 'autoconnect', 'connecting'
in between
+ condition = 'obj.state != DeviceState.roaming'
+ wd.wait_for_object_condition(device, condition, max_wait=5)
+
+ self.assertEqual(device.state, iwd.DeviceState.connected)
+ self.assertTrue(bss_hostapd[1].list_sta())
+
+ wd.wait(5)
+
+ # Now make sure that we don't roam anymore. In order to catch this via
+ # DeviceState, a suitable roaming target needs to be available. So jack
+ # up the RSSI of BSS 0 again. The retry interval is 60 seconds, so we
+ # should have roamed within that timeframe.
+ rule0.signal = -2000
+
+ condition = 'obj.state == DeviceState.roaming'
+ self.assertRaises(TimeoutError, wd.wait_for_object_condition, device,
+ condition, max_wait=60)
+
+ device.disconnect()
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+ wd.unregister_psk_agent(psk_agent)
+
+ rule0.remove()
+ rule1.remove()
+
+ @classmethod
+ def setUpClass(cls):
+ pass
+
+ @classmethod
+ def tearDownClass(cls):
+ IWD.clear_storage()
+
+if __name__ == '__main__':
+ unittest.main(exit=True)
--
2.29.2
7:05 a.m.
New subject: [PATCH v2 2/2] auto-t: add test for stopping periodic roam
retries
Hi Alvin,
On Thu, 2021-01-28 at 13:24 +0000, Alvin Šipraga wrote:
---
autotests/testRoamRetry/stop_retry_test.py | 134
+++++++++++++++++++++
1 file changed, 134 insertions(+)
create mode 100644 autotests/testRoamRetry/stop_retry_test.py
diff --git a/autotests/testRoamRetry/stop_retry_test.py
b/autotests/testRoamRetry/stop_retry_test.py
new file mode 100644
index 00000000..bbe6e2a1
--- /dev/null
+++ b/autotests/testRoamRetry/stop_retry_test.py
@@ -0,0 +1,134 @@
+#!/usr/bin/python3
+
+import unittest
+import sys
+
+sys.path.append('../util')
+import iwd
+from iwd import IWD
+from iwd import PSKAgent
+from iwd import NetworkType
+
+import testutil
+
+from hostapd import HostapdCLI
+from hwsim import Hwsim
+
+class Test(unittest.TestCase):
+ # Test that we do not periodically retry roaming if the
transision looks
+ # like this: LOW [roam] [new bss] HIGH.
+ def test_stop_retry(self):
+ hwsim = Hwsim()
+
+ bss_hostapd = [ HostapdCLI(config='ssid1.conf'),
+ HostapdCLI(config='ssid2.conf') ]
+ bss_radio = [ hwsim.get_radio('rad0'),
+ hwsim.get_radio('rad1') ]
+
+ rule0 = hwsim.rules.create()
+ rule0.source = bss_radio[0].addresses[0]
+ rule0.bidirectional = True
+
+ rule1 = hwsim.rules.create()
+ rule1.source = bss_radio[1].addresses[0]
+ rule1.bidirectional = True
+
+ # Fill in the neighbor AP tables in both BSSes. By default
each
+ # instance knows only about current BSS, even inside one
hostapd
+ # process.
+ # Roaming still works without the neighbor AP table but
neighbor
+ # reports have to be disabled in the .conf files
+ bss0_nr = ''.join(bss_radio[0].addresses[0].split(':')) + \
+ '8f0000005101060603000000'
+ bss1_nr = ''.join(bss_radio[1].addresses[0].split(':')) + \
+ '8f0000005102060603000000'
+
+ bss_hostapd[0].set_neighbor(bss_radio[1].addresses[0],
'TestRoamRetry',
+ bss1_nr)
+ bss_hostapd[1].set_neighbor(bss_radio[0].addresses[0],
'TestRoamRetry',
+ bss0_nr)
+
+ # Start in the vicinity of BSS 0, check that iwd connects to
BSS 0
+ rule0.signal = -2000
+ rule1.signal = -5000
+
+ wd = IWD()
+
+ psk_agent = PSKAgent("secret123")
+ wd.register_psk_agent(psk_agent)
+
+ devices = wd.list_devices(1)
+ device = devices[0]
+
+ condition = 'not obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ device.scan()
+
+ condition = 'not obj.scanning'
+ wd.wait_for_object_condition(device, condition)
+
+ ordered_network =
device.get_ordered_network('TestRoamRetry')
+
+ self.assertEqual(ordered_network.type, NetworkType.psk)
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object,
condition)
+
+ ordered_network.network_object.connect()
+
+ condition = 'obj.state == DeviceState.connected'
+ wd.wait_for_object_condition(device, condition)
+
+ self.assertTrue(bss_hostapd[0].list_sta())
+ self.assertFalse(bss_hostapd[1].list_sta())
+
+ wd.wait(5)
Generally we try and avoid waits/sleeps at all costs because its very
unreliable. For example test-runner is used by some people inside a VM
(which then nests another VM) and this can be extremely slow. This
causes all kinds of weird behavior when a test relies on a sleep or
wait. Sometimes waits are necissary but in this case I don't think it
is.
Instead it looks like this wait could be removed completely, then also
remove max_wait=10 below. This is, again, not something we generally
set explicitly for reasons mentioned above. Your fast system may start
roaming in 10 seconds, but a slow VM may take longer.
+
+ # Now push the signal below the RSSI threshold and check
that iwd
+ # connects to BSS 1
+ rule0.signal = -8000
+
+ condition = 'obj.state == DeviceState.roaming'
+ wd.wait_for_object_condition(device, condition, max_wait=10)
I would just remove max_wait here.
+
+ # Check that iwd is on BSS 1 once out of roaming state and
doesn't
+ # go through 'disconnected', 'autoconnect', 'connecting'
in
between
+ condition = 'obj.state != DeviceState.roaming'
+ wd.wait_for_object_condition(device, condition, max_wait=5)
And here.
+
+ self.assertEqual(device.state, iwd.DeviceState.connected)
+ self.assertTrue(bss_hostapd[1].list_sta())
+
+ wd.wait(5)
I also would remove this wait.
+
+ # Now make sure that we don't roam anymore. In order to
catch this via
+ # DeviceState, a suitable roaming target needs to be
available. So jack
+ # up the RSSI of BSS 0 again. The retry interval is 60
seconds, so we
+ # should have roamed within that timeframe.
+ rule0.signal = -2000
+
+ condition = 'obj.state == DeviceState.roaming'
+ self.assertRaises(TimeoutError,
wd.wait_for_object_condition, device,
+ condition, max_wait=60)
+
+ device.disconnect()
+
+ condition = 'not obj.connected'
+ wd.wait_for_object_condition(ordered_network.network_object,
condition)
+
+ wd.unregister_psk_agent(psk_agent)
+
+ rule0.remove()
+ rule1.remove()
+
+ @classmethod
+ def setUpClass(cls):
+ pass
+
+ @classmethod
+ def tearDownClass(cls):
+ IWD.clear_storage()
+
+if __name__ == '__main__':
+ unittest.main(exit=True)
Thanks again for your patches! Sorry if its frustrating that our test
framework has what seem like 'rules' which are undocumented and not
intuative. These type of things have been added over time for certain
cases and not much care was taken to prevent them from being used
generally. Besides these minor things the tests look great.
Thanks,
James
11:09 p.m.
New subject: [PATCH v2 2/2] auto-t: add test for stopping periodic roam
retries
Hi James,
On 1/28/21 9:05 PM, James Prestwood wrote:
Hi Alvin,
On Thu, 2021-01-28 at 13:24 +0000, Alvin Šipraga wrote:
> ---
> autotests/testRoamRetry/stop_retry_test.py | 134
> +++++++++++++++++++++
> 1 file changed, 134 insertions(+)
> create mode 100644 autotests/testRoamRetry/stop_retry_test.py
>
> diff --git a/autotests/testRoamRetry/stop_retry_test.py
> b/autotests/testRoamRetry/stop_retry_test.py
> new file mode 100644
> index 00000000..bbe6e2a1
> --- /dev/null
> +++ b/autotests/testRoamRetry/stop_retry_test.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/python3
> +
> +import unittest
> +import sys
> +
> +sys.path.append('../util')
> +import iwd
> +from iwd import IWD
> +from iwd import PSKAgent
> +from iwd import NetworkType
> +
> +import testutil
> +
> +from hostapd import HostapdCLI
> +from hwsim import Hwsim
> +
> +class Test(unittest.TestCase):
> + # Test that we do not periodically retry roaming if the
> transision looks
> + # like this: LOW [roam] [new bss] HIGH.
> + def test_stop_retry(self):
> + hwsim = Hwsim()
> +
> + bss_hostapd = [ HostapdCLI(config='ssid1.conf'),
> + HostapdCLI(config='ssid2.conf') ]
> + bss_radio = [ hwsim.get_radio('rad0'),
> + hwsim.get_radio('rad1') ]
> +
> + rule0 = hwsim.rules.create()
> + rule0.source = bss_radio[0].addresses[0]
> + rule0.bidirectional = True
> +
> + rule1 = hwsim.rules.create()
> + rule1.source = bss_radio[1].addresses[0]
> + rule1.bidirectional = True
> +
> + # Fill in the neighbor AP tables in both BSSes. By default
> each
> + # instance knows only about current BSS, even inside one
> hostapd
> + # process.
> + # Roaming still works without the neighbor AP table but
> neighbor
> + # reports have to be disabled in the .conf files
> + bss0_nr = ''.join(bss_radio[0].addresses[0].split(':')) +
\
> + '8f0000005101060603000000'
> + bss1_nr = ''.join(bss_radio[1].addresses[0].split(':')) +
\
> + '8f0000005102060603000000'
> +
> + bss_hostapd[0].set_neighbor(bss_radio[1].addresses[0],
> 'TestRoamRetry',
> + bss1_nr)
> + bss_hostapd[1].set_neighbor(bss_radio[0].addresses[0],
> 'TestRoamRetry',
> + bss0_nr)
> +
> + # Start in the vicinity of BSS 0, check that iwd connects to
> BSS 0
> + rule0.signal = -2000
> + rule1.signal = -5000
> +
> + wd = IWD()
> +
> + psk_agent = PSKAgent("secret123")
> + wd.register_psk_agent(psk_agent)
> +
> + devices = wd.list_devices(1)
> + device = devices[0]
> +
> + condition = 'not obj.scanning'
> + wd.wait_for_object_condition(device, condition)
> +
> + device.scan()
> +
> + condition = 'not obj.scanning'
> + wd.wait_for_object_condition(device, condition)
> +
> + ordered_network =
> device.get_ordered_network('TestRoamRetry')
> +
> + self.assertEqual(ordered_network.type, NetworkType.psk)
> +
> + condition = 'not obj.connected'
> + wd.wait_for_object_condition(ordered_network.network_object,
> condition)
> +
> + ordered_network.network_object.connect()
> +
> + condition = 'obj.state == DeviceState.connected'
> + wd.wait_for_object_condition(device, condition)
> +
> + self.assertTrue(bss_hostapd[0].list_sta())
> + self.assertFalse(bss_hostapd[1].list_sta())
> +
> + wd.wait(5)
Generally we try and avoid waits/sleeps at all costs because its very
unreliable. For example test-runner is used by some people inside a VM
(which then nests another VM) and this can be extremely slow. This
causes all kinds of weird behavior when a test relies on a sleep or
wait. Sometimes waits are necissary but in this case I don't think it
is.
Instead it looks like this wait could be removed completely, then also
remove max_wait=10 below. This is, again, not something we generally
set explicitly for reasons mentioned above. Your fast system may start
roaming in 10 seconds, but a slow VM may take longer.
> +
> + # Now push the signal below the RSSI threshold and check
> that iwd
> + # connects to BSS 1
> + rule0.signal = -8000
> +
> + condition = 'obj.state == DeviceState.roaming'
> + wd.wait_for_object_condition(device, condition, max_wait=10)
I would just remove max_wait here.
> +
> + # Check that iwd is on BSS 1 once out of roaming state and
> doesn't
> + # go through 'disconnected', 'autoconnect',
'connecting' in
> between
> + condition = 'obj.state != DeviceState.roaming'
> + wd.wait_for_object_condition(device, condition, max_wait=5)
And here.
> +
> + self.assertEqual(device.state, iwd.DeviceState.connected)
> + self.assertTrue(bss_hostapd[1].list_sta())
> +
> + wd.wait(5)
I also would remove this wait.
> +
> + # Now make sure that we don't roam anymore. In order to
> catch this via
> + # DeviceState, a suitable roaming target needs to be
> available. So jack
> + # up the RSSI of BSS 0 again. The retry interval is 60
> seconds, so we
> + # should have roamed within that timeframe.
> + rule0.signal = -2000
> +
> + condition = 'obj.state == DeviceState.roaming'
> + self.assertRaises(TimeoutError,
> wd.wait_for_object_condition, device,
> + condition, max_wait=60)
> +
> + device.disconnect()
> +
> + condition = 'not obj.connected'
> + wd.wait_for_object_condition(ordered_network.network_object,
> condition)
> +
> + wd.unregister_psk_agent(psk_agent)
> +
> + rule0.remove()
> + rule1.remove()
> +
> + @classmethod
> + def setUpClass(cls):
> + pass
> +
> + @classmethod
> + def tearDownClass(cls):
> + IWD.clear_storage()
> +
> +if __name__ == '__main__':
> + unittest.main(exit=True)
Thanks again for your patches! Sorry if its frustrating that our test
framework has what seem like 'rules' which are undocumented and not
intuative. These type of things have been added over time for certain
cases and not much care was taken to prevent them from being used
generally. Besides these minor things the tests look great.
No problem at all. I reworked the test and removed all but the last
max_wait condition, so please take a look at v3. Note that I increased
the max_wait for the final check from 60 to 70 seconds on account of the
slowness of the test environment. Hope that's OK!
Kind regards,
Alvin
6:40 a.m.
Hi Alvin,
On 1/28/21 7:24 AM, Alvin Šipraga wrote:
Fix an issue with the recent changes to signal monitoring from
commit
f456501b ("station: retry roaming unless notified of a high RSSI"):
1. driver sends NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW
2. netdev->cur_rssi_low changes from FALSE to TRUE
3. netdev sends NETDEV_EVENT_RSSI_THRESHOLD_LOW to station
4. on roam reassociation, cur_rssi_low is reset to FALSE
5. station still assumes RSSI is low, periodically roams
until netdev sends NETDEV_EVENT_RSSI_THRESHOLD_HIGH
6. driver sends NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH
7. netdev->cur_rssi_low doesn't change (still FALSE)
8. netdev never sends NETDEV_EVENT_RSSI_THRESHOLD_HIGH
9. station remains stuck in an infinite roaming loop
The commit in question introduced the logic in (5). Previously the
assumption in station was - like in netdev - that if the signal was
still low, the driver would send a duplicate LOW event after
reassociation. This change makes netdev follow the same new logic as
station, i.e. assume the same signal state (LOW/HIGH) until told
otherwise by the driver.
---
v1 -> v2:
- No changes to this patch, but added an autotest to the patch series to
validate the fix
src/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
Regards,
-Denis
597
days inactive
598
days old
4 comments
3 participants
participants (3)
-
Alvin Šipraga -
Denis Kenzior -
James Prestwood