| Message ID | 1463663210-15610-2-git-send-email-jonathan@wizery.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, May 19, 2016 at 04:06:43PM +0300, Jonathan Afek wrote: > The regular hwsim tests use both unicast and broadcast frames to test > the connectivity between 2 interfaces. > For real hardware (remote hwsim tests) the broadcast frames will > sometimes not be seen by all connected stations since they can be in > low power mode during DTIM or because broadcast frames are not > acked. > This commit makes the connectivity test for real hardware use > only unicast frames instead of both unicast and broadcast. I don't think this is a good idea. Verifying that broadcast delivery works is an important part of connectivity testing especially with all test cases using encrypted connection.
On Sat, May 21, 2016 at 12:39 AM, Jouni Malinen <j@w1.fi> wrote: > I don't think this is a good idea. Verifying that broadcast delivery > works is an important part of connectivity testing especially with all > test cases using encrypted connection. You are right. What about doing 10 retries for real hardware and passing the broadcast connectivity test if one of them is received?
On Sun, May 22, 2016 at 04:27:45PM +0300, Jonathan Afek wrote: > On Sat, May 21, 2016 at 12:39 AM, Jouni Malinen <j@w1.fi> wrote: > > I don't think this is a good idea. Verifying that broadcast delivery > > works is an important part of connectivity testing especially with all > > test cases using encrypted connection. > You are right. What about doing 10 retries for real hardware and > passing the broadcast connectivity test if one of them is received? If that means doing up to 10 retries and stopping on the first success, then yes, that's probably fine. I'd rather not add significant extra latency here for the success case, but it is fine to make that take longer if the broadcast frames do not get through and the test case would fail anyway. Please also note that there are couple of cases where hwsim_utils.test_connectivity() is used for negative testing, i.e., there is not supposed to be connectivity and the test case fails if that function does not raise an exception. Doing 10 retries there should work, but it will add some latency. That said, I think it is justifiable to do that for the non-hwsim case without more complexity since there are only couple of such negative testing cases. Though, I'd be fine with adding a parameter to the function to indicate if it is used for the negative case so that the retries could be avoided.
On Mon, 2016-05-23 at 18:19 +0300, Jouni Malinen wrote: > Please also note that there are couple of cases where > hwsim_utils.test_connectivity() is used for negative testing, i.e., > there is not supposed to be connectivity and the test case fails if > that > function does not raise an exception. Doing 10 retries there should > work, but it will add some latency. That said, I think it is > justifiable > to do that for the non-hwsim case without more complexity since there > are only couple of such negative testing cases. Though, I'd be fine > with > adding a parameter to the function to indicate if it is used for the > negative case so that the retries could be avoided. In the OTA case, this might actually want to do 10 tries to not have spurious successes when the frame is lost? johannes
On Tue, May 24, 2016 at 9:56 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > In the OTA case, this might actually want to do 10 tries to not have > spurious successes when the frame is lost? In any case it looks like that in the negative cases we will never reach the broadcast test since the exception will be raised for the first unicast frame which comes before that. I will send another patch with 10 retries and no new parameter. If there will ever be a negative test which expect unicast to pass but broadcast to fail, this should work fine for such a case and all other regular cases.
diff --git a/tests/hwsim/hwsim_utils.py b/tests/hwsim/hwsim_utils.py index 85f54a2..9438372 100644 --- a/tests/hwsim/hwsim_utils.py +++ b/tests/hwsim/hwsim_utils.py @@ -61,19 +61,20 @@ def run_connectivity_test(dev1, dev2, tos, dev1group=False, dev2group=False, if "DATA-TEST-RX {} {}".format(addr2, addr1) not in ev: raise Exception("Unexpected dev1->dev2 unicast data result") - cmd = "DATA_TEST_TX ff:ff:ff:ff:ff:ff {} {}".format(addr1, tos) - if dev1group: - dev1.group_request(cmd) - else: - dev1.request(cmd) - if dev2group: - ev = dev2.wait_group_event(["DATA-TEST-RX"], timeout=timeout) - else: - ev = dev2.wait_event(["DATA-TEST-RX"], timeout=timeout) - if ev is None: - raise Exception("dev1->dev2 broadcast data delivery failed") - if "DATA-TEST-RX ff:ff:ff:ff:ff:ff {}".format(addr1) not in ev: - raise Exception("Unexpected dev1->dev2 broadcast data result") + if dev1.hostname is None and dev2.hostname is None: + cmd = "DATA_TEST_TX ff:ff:ff:ff:ff:ff {} {}".format(addr1, tos) + if dev1group: + dev1.group_request(cmd) + else: + dev1.request(cmd) + if dev2group: + ev = dev2.wait_group_event(["DATA-TEST-RX"], timeout=timeout) + else: + ev = dev2.wait_event(["DATA-TEST-RX"], timeout=timeout) + if ev is None: + raise Exception("dev1->dev2 broadcast data delivery failed") + if "DATA-TEST-RX ff:ff:ff:ff:ff:ff {}".format(addr1) not in ev: + raise Exception("Unexpected dev1->dev2 broadcast data result") cmd = "DATA_TEST_TX {} {} {}".format(addr1, addr2, tos) if dev2group: @@ -89,19 +90,20 @@ def run_connectivity_test(dev1, dev2, tos, dev1group=False, dev2group=False, if "DATA-TEST-RX {} {}".format(addr1, addr2) not in ev: raise Exception("Unexpected dev2->dev1 unicast data result") - cmd = "DATA_TEST_TX ff:ff:ff:ff:ff:ff {} {}".format(addr2, tos) - if dev2group: - dev2.group_request(cmd) - else: - dev2.request(cmd) - if dev1group: - ev = dev1.wait_group_event(["DATA-TEST-RX"], timeout=timeout) - else: - ev = dev1.wait_event(["DATA-TEST-RX"], timeout=timeout) - if ev is None: - raise Exception("dev2->dev1 broadcast data delivery failed") - if "DATA-TEST-RX ff:ff:ff:ff:ff:ff {}".format(addr2) not in ev: - raise Exception("Unexpected dev2->dev1 broadcast data result") + if dev1.hostname is None and dev2.hostname is None: + cmd = "DATA_TEST_TX ff:ff:ff:ff:ff:ff {} {}".format(addr2, tos) + if dev2group: + dev2.group_request(cmd) + else: + dev2.request(cmd) + if dev1group: + ev = dev1.wait_group_event(["DATA-TEST-RX"], timeout=timeout) + else: + ev = dev1.wait_event(["DATA-TEST-RX"], timeout=timeout) + if ev is None: + raise Exception("dev2->dev1 broadcast data delivery failed") + if "DATA-TEST-RX ff:ff:ff:ff:ff:ff {}".format(addr2) not in ev: + raise Exception("Unexpected dev2->dev1 broadcast data result") finally: if config: if dev1group:
The regular hwsim tests use both unicast and broadcast frames to test the connectivity between 2 interfaces. For real hardware (remote hwsim tests) the broadcast frames will sometimes not be seen by all connected stations since they can be in low power mode during DTIM or because broadcast frames are not acked. This commit makes the connectivity test for real hardware use only unicast frames instead of both unicast and broadcast. Signed-off-by: Jonathan Afek <jonathanx.afek@intel.com> --- tests/hwsim/hwsim_utils.py | 54 ++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 26 deletions(-)