Re: [PATCH] brcmfmac: add support for CQM RSSI notifications

Hi Arend,

Thanks for your comments - I'll prepare a v2 patch. Some 
comments/justification inline below...

On 1/14/21 2:39 PM, Arend van Spriel wrote:

On 12-01-2021 12:13, 'Alvin Šipraga' via BRCM80211-DEV-LIST,PDL wrote:

quoted

Add support for CQM RSSI measurement reporting and advertise the
NL80211_EXT_FEATURE_CQM_RSSI_LIST feature. This enables a userspace
supplicant such as iwd to be notified of changes in the RSSI for roaming
and signal monitoring purposes.

Needs a bit of rework. See my comments below...

quoted

Signed-off-by: Alvin Šipraga <redacted>
---
  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 82 +++++++++++++++++++
  .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  6 ++
  .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 28 +++++++
  3 files changed, 116 insertions(+)

diff --git 

a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 0ee421f30aa2..21b53bd27f7f 100644

--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c

@@ -5196,6 +5196,41 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, 

struct wireless_dev *wdev,
      return err;
  }
+static int brcmf_cfg80211_set_cqm_rssi_range_config(struct wiphy *wiphy,
+                            struct net_device *ndev,
+                            s32 rssi_low, s32 rssi_high)
+{
+    struct brcmf_cfg80211_vif *vif;
+    struct brcmf_if *ifp;
+    int err = 0;
+
+    brcmf_dbg(TRACE, "low=%d high=%d", rssi_low, rssi_high);
+
+    ifp = netdev_priv(ndev);
+    vif = ifp->vif;
+
+    if (rssi_low != vif->cqm_rssi_low || rssi_high != 
vif->cqm_rssi_high) {
+        struct brcmf_rssi_event_le config = {
+            .rate_limit_msec = cpu_to_le32(0),
+            .rssi_level_num = 2,
+            .rssi_levels = {
+                max_t(s32, rssi_low, S8_MIN),
+                min_t(s32, rssi_high, S8_MAX),

The type should be s8 iso s32.

The idea was to clamp out-of-bounds rssi_low/rssi_high (s32) values to 
S8_MIN/S8_MAX rather than casting an s32 to s8 and hoping for the best. 
But since max_t(s8, x, S8_MIN) will always equal (s8)x, I might as well 
just do:

     .rssi_levels = {
         rssi_low,
         min_t(s8, rssi_high, S8_MAX - 1),
         S8_MAX,
     },

I am inclined to keep it as it was, i.e.:

     .rssi_levels = {
         max_t(s32, rssi_low, S8_MIN),
         min_t(s32, rssi_high, S8_MAX - 1),
         S8_MAX,
     },

What do you think?

quoted

+            },
+        };

What is the expectation here? The firmware behavior for the above is 
that you will get an event when the rssi is lower or equal to the level 
and the previous rssi event was lower or equal to a different level. 

I think I see what you mean - you're concerned about not getting the 
"high" event because an RSSI greater than rssi_high will not be less 
than another level? If the behaviour of the firmware is as you describe 
then I will add an additional level like you suggested to cover this case.

There is another event RSSI_LQM that would be a better fit although that 
is not available in every firmware image ("rssi_mon" firmware feature).

Another option would be to add a level, ie.:

     .rssi_levels = {
         max_t(s8, rssi_low, S8_MIN),
         min_t(s8, rssi_high, S8_MAX - 1),
         S8_MAX
     }

quoted

+        err = brcmf_fil_iovar_data_set(ifp, "rssi_event", &config,
+                           sizeof(config));
+        if (err) {
+            err = -EINVAL;
+        } else {
+            vif->cqm_rssi_low = rssi_low;
+            vif->cqm_rssi_high = rssi_high;
+        }
+    }
+
+    return err;
+}
  static int
  brcmf_cfg80211_cancel_remain_on_channel(struct wiphy *wiphy,

@@ -5502,6 +5537,7 @@ static struct cfg80211_ops brcmf_cfg80211_ops = {

      .update_mgmt_frame_registrations =
          brcmf_cfg80211_update_mgmt_frame_registrations,
      .mgmt_tx = brcmf_cfg80211_mgmt_tx,
+    .set_cqm_rssi_range_config = 
brcmf_cfg80211_set_cqm_rssi_range_config,
      .remain_on_channel = brcmf_p2p_remain_on_channel,
      .cancel_remain_on_channel = 
brcmf_cfg80211_cancel_remain_on_channel,
      .get_channel = brcmf_cfg80211_get_channel,

@@ -6137,6 +6173,49 @@ brcmf_notify_mic_status(struct brcmf_if *ifp,

      return 0;
  }
+static s32 brcmf_notify_rssi(struct brcmf_if *ifp,
+                 const struct brcmf_event_msg *e, void *data)

align to the opening brace in the line above.

Is it not correct already? The 's' in struct is in the same column as 
the 'c' in const. I think it just looks wrong because of the '+' in the 
first column of the diff.

quoted

+{
+    struct brcmf_cfg80211_vif *vif = ifp->vif;
+    struct brcmf_rssi_be *info = data;
+    s32 rssi, snr, noise;
+    s32 low, high, last;
+
+    if (e->datalen < sizeof(*info)) {
+        brcmf_err("insufficient RSSI event data\n");
+        return 0;
+    }
+
+    rssi = be32_to_cpu(info->rssi);
+    snr = be32_to_cpu(info->snr);
+    noise = be32_to_cpu(info->noise);

Bit surprised to see this is BE, but it appears to be correct.

quoted

+    low = vif->cqm_rssi_low;
+    high = vif->cqm_rssi_high;
+    last = vif->cqm_rssi_last;
+
+    brcmf_dbg(TRACE, "rssi=%d snr=%d noise=%d low=%d high=%d last=%d\n",
+          rssi, snr, noise, low, high, last);
+
+    if (rssi != last) {

Given the firmware behavior I don't think you need this check.

OK.

quoted

+        vif->cqm_rssi_last = rssi;
+
+        if (rssi <= low || rssi == 0) {
+            brcmf_dbg(INFO, "LOW rssi=%d\n", rssi);
+            cfg80211_cqm_rssi_notify(ifp->ndev,
+                         NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW,
+                         rssi, GFP_KERNEL);
+        } else if (rssi > high) {
+            brcmf_dbg(INFO, "HIGH rssi=%d\n", rssi);
+            cfg80211_cqm_rssi_notify(ifp->ndev,
+                         NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH,
+                         rssi, GFP_KERNEL);
+        }
+    }
+
+    return 0;