[DCS Patch 1/5] port dynamic channel selection from rdkb bwl #637

itayx · 2020-01-05T17:48:49Z

The Dynamic Channel Selection as a feature can provide information on the supported channels which can be utilized by the management system to provide better availability and user experience.
#562

The Dynamic Channel Selection feature is divided into several pull request, this is the first out of 5

Low Level Support (DWPAL): Full NL support in BWL:

Changes in this PR include the following:

bwl: Add NL DWPAL monitor implementation
bwl: Add NL DWPAL base implementation
bwl: Add NL support (with stubs)
bwl: cmake: add nl libs for dwpal

tomereli

Second, third and fourth commits don't have a description and Signed off by.

common/beerocks/bwl/common/base_wlan_hal.cpp

common/beerocks/bwl/dummy/base_wlan_hal_dummy.cpp

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

common/beerocks/bwl/include/bwl/base_wlan_hal.h

common/beerocks/bwl/nl80211/base_wlan_hal_nl80211.cpp

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal_types.h

vitalybu · 2020-01-09T12:32:53Z

common/beerocks/bwl/CMakeLists.txt

@@ -59,7 +59,7 @@ if(BWL_TYPE STREQUAL "DWPAL")
    )

    find_package(dwpal REQUIRED)
-    list(APPEND BWL_LIBS dwpal)
+    list(APPEND BWL_LIBS dwpal nl-3 nl-genl-3)


Perhaps it's better to create a FindNL cmake file and use the standard cmake find_package().
Here's an example: https://github.com/nasa/channel-emulator/blob/master/cmake/Modules/FindLibNL.cmake

Here's an example: https://github.com/nasa/channel-emulator/blob/master/cmake/Modules/FindLibNL.cmake

Careful with that! The project is GPL-2.0 so presumably that file is GPL-2.0 as well. As such it's OK if we include GPL-2.0 files that are used during the build (it just means that if you distribute the source code, you need to give the recipient the right to redistribute that particular file further). But it's making our license more complicated. If you do take over that file, it should get a copyright header including authorship information of the original (which is difficult to retrieve since they probably also copied it from somewhere else).

That was just an example. We should write our own cmake find file for libnl..

As agreed, and since its already exists for other libraries, lets do this in a separate PR as part #665.
Keeping the comment open for that.

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

vitalybu · 2020-01-09T12:45:43Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

                // Success
                LOG(DEBUG)
                    << "Open and attach an event interface to wpa_supplicant/hostapd - SUCCESS!";
+
+                // Get the nl event interface file descriptor
+                if (!m_dwpal_nl_ctx) {


The dwpal netlink context is created in the Init state. On failure, we transition the FSM to the Detach state.
In what scenario do we get to the Attach state without a valid context?
I think this if statement is redundant.

not sure it this test is mandatory.
I added it since we do the same with the VAPs .

Still, this should be removed as part of #665

here is a live example why it should stay.

netgear-rax40-1_logs:
DEBUG 17:21:12:420 <1991208208>
base_wlan_hal_dwpal.cpp[148] --> dwpal_driver_nl_attach() success for radio =wlan0
...
DEBUG 17:21:13:264 <1991208208>
base_wlan_hal_dwpal.cpp[394] --> DWPAL FSM wlan0 Attach State: Attach
INFO 17:21:13:264 <1991208208>
ap_manager_thread.cpp[343] --> waiting to attach to wlan0
DEBUG 17:21:13:665 <1991208208>
base_wlan_hal_dwpal.cpp[306] --> Open and attach an event interface to wpa_supplicant/hostapd - SUCCESS!
ERROR 17:21:13:665 <1991208208>
base_wlan_hal_dwpal.cpp[322] --> Can't get event fd since dwpal ctx is NULL!
INFO 17:21:13:666 <1991208208>
beerocks_state_machine.h[53] --> 'Attach': Overriding destination for 'Attach' from 'Operational' to 'Detach'
DEBUG 17:21:13:666 <1991208208>
base_wlan_hal_dwpal.cpp[348] --> dwpal_fsm_state::Detach

Exactly. Remember that RAX40 has a much older driver version at the moment so the attach will fail
We mustn't fail the FSM on that! Just log an warning and proceed without NL. Similar to what we do when we lack permissions to open raw socket - we simply don't open it.

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

vitalybu · 2020-01-09T13:18:07Z

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

+bool mon_wlan_hal_dwpal::channel_scan_trigger(int dwell_time_msec,
+                                              const std::vector<unsigned int> &channel_pool)
+{
+    LOG(DEBUG) << __func__ << " received on interface=" << m_radio_info.iface_name;


since its the only print for triggering scan i prefer to leave it.

Ok, so just remove the __func__ and improve the log message.
For example: "Channel scan trigger received on interface: ..."

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

vitalybu · 2020-01-13T08:54:56Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+bool base_wlan_hal_dwpal::dwpal_nl_cmd_set(const std::string &ifname, unsigned int nl_cmd,
+                                           unsigned char *vendor_data, size_t vendor_data_size)
+{
+    if (vendor_data == nullptr) {


if (!vendor_data) { ... }

vitalybu · 2020-01-13T08:55:25Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+                                           unsigned char *vendor_data, size_t vendor_data_size)
+{
+    if (vendor_data == nullptr) {
+        LOG(ERROR) << __func__ << "vendor_data is NULL ==> Abort!";


Remove __func__ and __FUNCTION__ from all log messages

vitalybu · 2020-01-13T08:55:36Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+                                             unsigned char *out_buffer,
+                                             const size_t max_buffer_size)
+{
+


Remove extra newline

Why wasn't this removed? Please remove before merging.

vitalybu · 2020-01-13T08:56:08Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+
+    size_t data_size = 0;
+
+    if (out_buffer == nullptr) {


if (!out_buffer) { ... }

vitalybu · 2020-01-13T08:56:49Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+    return true;
+}
+
+size_t base_wlan_hal_dwpal::dwpal_nl_cmd_get(const std::string &ifname, unsigned int nl_cmd,


Function should return ssize_t (signed size_t).
Error value should be <= 0 and any positive value is the length of the received message.

vitalybu · 2020-01-13T08:57:40Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+            m_dwpal_nl_ctx, DWPAL_NL_SOLICITED_EVENT, (char *)ifname.c_str(), NL80211_CMD_VENDOR,
+            DWPAL_NETDEV_ID, (enum ltq_nl80211_vendor_subcmds)nl_cmd, NULL, 0) == DWPAL_FAILURE) {
+        LOG(ERROR) << __FUNCTION__ << "ERROR for cmd = " << nl_cmd;
+        return data_size;


Either initialize data_size to -1, or simply return -1 here to indicate a failure.
Same goes to all failures...

vitalybu · 2020-01-13T08:59:54Z

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

+bool mon_wlan_hal_dwpal::channel_scan_trigger(int dwell_time_msec,
+                                              const std::vector<unsigned int> &channel_pool)
+{
+    LOG(DEBUG) << __func__ << " received on interface=" << m_radio_info.iface_name;


Ok, so just remove the __func__ and improve the log message.
For example: "Channel scan trigger received on interface: ..."

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.h

tomereli

Seccond commit message description is missing - saying "Preperative commit" without context is not enough.
Third commit description is also missing - you need to explain what netlink is basically, why it is needed for DCS, and that you add it for now with stubs to make the review simpler.
954a758 is problematic since it assumes slibc is available where AFAIK it is only available in RDKB.

tomereli · 2020-01-13T12:35:16Z

common/beerocks/bwl/CMakeLists.txt

@@ -59,7 +59,7 @@ if(BWL_TYPE STREQUAL "DWPAL")
    )

    find_package(dwpal REQUIRED)
-    list(APPEND BWL_LIBS dwpal)
+    list(APPEND BWL_LIBS dwpal nl-3 nl-genl-3)


As agreed, and since its already exists for other libraries, lets do this in a separate PR as part #665.
Keeping the comment open for that.

tomereli · 2020-01-13T12:37:10Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.h

@@ -20,6 +20,15 @@ extern "C" {
 #include <chrono>
 #include <memory>

+#ifdef USE_LIBSAFEC


Since you moved it to a header file, how about deleting this from the implementation files?
#665

tomereli · 2020-01-13T12:39:56Z

common/beerocks/bwl/dummy/base_wlan_hal_dummy.cpp

@@ -279,6 +279,12 @@ bool base_wlan_hal_dummy::dummy_send_cmd(const std::string &cmd) { return false;

 bool base_wlan_hal_dummy::dummy_send_cmd(const std::string &cmd, char **reply) { return false; }

+bool base_wlan_hal_dummy::process_nl_events()
+{
+    LOG(ERROR) << __func__ << "not implemented";


Didn't we agree to remove the __func__ from all logs?
#665

first time i hear about it. why?

It is redundant - see @vitalybu 's comments

tomereli · 2020-01-13T12:41:57Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

                // Success
                LOG(DEBUG)
                    << "Open and attach an event interface to wpa_supplicant/hostapd - SUCCESS!";
+
+                // Get the nl event interface file descriptor
+                if (!m_dwpal_nl_ctx) {


Still, this should be removed as part of #665

tomereli · 2020-01-13T12:42:20Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+bool base_wlan_hal_dwpal::dwpal_nl_cmd_set(const std::string &ifname, unsigned int nl_cmd,
+                                           unsigned char *vendor_data, size_t vendor_data_size)
+{
+    if (vendor_data == nullptr) {


tomereli · 2020-01-13T12:43:31Z

common/beerocks/bwl/dwpal/base_wlan_hal_dwpal.cpp

+
+bool base_wlan_hal_dwpal::dwpal_nl_cmd_scan_dump()
+{
+    // Passing a lambda with capture is not supported for standard C function


common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

tomereli · 2020-01-13T12:46:13Z

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

+{
+    switch (cmd) {
+    case NL80211_CMD_TRIGGER_SCAN:
+        return mon_wlan_hal::Event::Channel_Scan_Triggered;


tomereli · 2020-01-13T12:46:26Z

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.cpp

+bool mon_wlan_hal_dwpal::channel_scan_trigger(int dwell_time_msec,
+                                              const std::vector<unsigned int> &channel_pool)
+{
+    LOG(DEBUG) << __func__ << " received on interface=" << m_radio_info.iface_name;


common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.h

tomereli

Approving since most comments are fixed, the rest will be fixed as part of #665

arnout · 2020-01-13T20:20:44Z

common/beerocks/bwl/dwpal/mon_wlan_hal_dwpal.h

+            return false;
+        }
+
+        if (memcpy_s(&params, sizeof(params), &m_nl_buffer[NL_ATTR_HDR], sizeof(params)) != 0) {


This triggers a build failure: https://gitlab.com/prpl-foundation/prplMesh/-/jobs/401484296

mmemcpy_s was not declared in this scope

If you're going to use safelibc functions in the header file, you have to include the header (which still requires the ifdeffery to support both rdkb and ugw/openwrt).

However, I'd suggest moving it to the .cpp file.

And while you're at it, use std::copy maybe?

However, there's definitely something wrong here. You check that the return value of dwpal_nl_cmd_get (which is the length of the returned buffer) is equal to sizeof(params), and then you copy out sizeof(params) with an offset of 4 (NL_ATTR_HEADER). So you're guaranteeing an out of bounds access. So either you need to add NL_ATTR_HDR to the check above, or you have to copy without offset.

If the offset is needed, it is going to be needed for all calls of dwpal_nl_cmd_get. So you'd better skip the header inside the dwpal_nl_cmd_get function then. But then, the copy is again redundant, you can just pass params and sizeof(params) as arguments to dwpal_nl_cmd_get.

In conclusion, I don't think this copy will be needed.

BTW I have the feeling I've already made this comment in the original #619.

Good thing we have gitlab CI!

Just one feedback about this - CI tee's the build log into 'build.log' and saves it in the artifacts zip file. It took me some time to figure this out without knowing what the error is. Why can't we log the build both to file and stdout? It will make our lives much easier analyzing build failures.
@arnout @rmerlot

Currently building the image and running it is done in a single step. Building the image is really verbose (and we want to keep this verbosity to debug failing builds) and would flood the CI logs completely.
Once #621 is done, we can probably output everything when running the image only (i.e. when building prplmesh itself).

This triggers a build failure: https://gitlab.com/prpl-foundation/prplMesh/-/jobs/401484296

mmemcpy_s was not declared in this scope

If you're going to use safelibc functions in the header file, you have to include the header (which still requires the ifdeffery to support both rdkb and ugw/openwrt).

However, I'd suggest moving it to the .cpp file.

And while you're at it, use std::copy maybe?

However, there's definitely something wrong here. You check that the return value of dwpal_nl_cmd_get (which is the length of the returned buffer) is equal to sizeof(params), and then you copy out sizeof(params) with an offset of 4 (NL_ATTR_HEADER). So you're guaranteeing an out of bounds access. So either you need to add NL_ATTR_HDR to the check above, or you have to copy without offset.

If the offset is needed, it is going to be needed for all calls of dwpal_nl_cmd_get. So you'd better skip the header inside the dwpal_nl_cmd_get function then. But then, the copy is again redundant, you can just pass params and sizeof(params) as arguments to dwpal_nl_cmd_get.

In conclusion, I don't think this copy will be needed.

BTW I have the feeling I've already made this comment in the original #619.

This triggers a build failure: https://gitlab.com/prpl-foundation/prplMesh/-/jobs/401484296

mmemcpy_s was not declared in this scope

If you're going to use safelibc functions in the header file, you have to include the header (which still requires the ifdeffery to support both rdkb and ugw/openwrt).

However, I'd suggest moving it to the .cpp file.

And while you're at it, use std::copy maybe?

However, there's definitely something wrong here. You check that the return value of dwpal_nl_cmd_get (which is the length of the returned buffer) is equal to sizeof(params), and then you copy out sizeof(params) with an offset of 4 (NL_ATTR_HEADER). So you're guaranteeing an out of bounds access. So either you need to add NL_ATTR_HDR to the check above, or you have to copy without offset.

If the offset is needed, it is going to be needed for all calls of dwpal_nl_cmd_get. So you'd better skip the header inside the dwpal_nl_cmd_get function then. But then, the copy is again redundant, you can just pass params and sizeof(params) as arguments to dwpal_nl_cmd_get.

In conclusion, I don't think this copy will be needed.

BTW I have the feeling I've already made this comment in the original #619.

changed to std::copy and removed the safelibc commit (no need for it now).
fixed the offset issue.

In general i prefer to leave dwpal_nl_cmd_get raw as possible and not to remove the offset there just so i could pass the params instead of the buffer.

prperative commit. In order to use nl libraries in dwpal linker needs to know their location. Signed-off-by: alex kanter <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

Add netlink common support for bwl libs. Added with stubs to make the review simpler. Netlink (NL) is direct communication with kernel. We use NL for channel scan triggering and results. Signed-off-by: alex kanter <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

Open nl socket at dwpal init state for each interface. proccess nl events after select. Add stubs and structures for monitor hal. Signed-off-by: alex kanter <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

Implement channel scan events processing. Implement scan triggering and dump results requests. Signed-off-by: alex kanter <[email protected]> Signed-off-by: itay elenzweig <[email protected]>

kantera800 · 2020-01-16T15:10:15Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:16Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:16Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:17Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:18Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:18Z

[DCS Patch 1/5]Port dynamic channel selection

kantera800 · 2020-01-16T15:10:19Z

[DCS Patch 1/5]Port dynamic channel selection

itayx requested review from arnout, morantr and tomereli as code owners January 5, 2020 17:48

kantera800 mentioned this pull request Jan 6, 2020

[TASK] port Dynamic channel selection (DCS) feature from RDKB #562

Closed

8 tasks

itayx mentioned this pull request Jan 6, 2020

[DCS Patch 2/5] port dynamic channel selection from rdkb infrastructure #643

Merged

tomereli changed the title ~~Feature/port dynamic channel selection from rdkb bwl~~ [DCS Patch 1/5] port dynamic channel selection from rdkb bwl Jan 7, 2020

tomereli suggested changes Jan 7, 2020

View reviewed changes

itayx mentioned this pull request Jan 8, 2020

Feature/port dynamic channel selection from rdkb #619

Closed

itayx force-pushed the feature/port-dynamic-channel-selection-from-rdkb-bwl branch from 20089eb to a6bcd85 Compare January 8, 2020 14:17