| Message ID | 20210112111253.4176340-1-alsi@bang-olufsen.dk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | brcmfmac: add support for CQM RSSI notifications | expand |
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: >> 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... > >> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> >> --- >> .../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? > >> + }, >> + }; > > 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 > } > >> + 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. > >> +{ >> + 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. > >> + 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. > >> + 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; >
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), + }, + }; + + 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) +{ + 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); + + 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) { + 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; +} + static s32 brcmf_notify_vif_event(struct brcmf_if *ifp, const struct brcmf_event_msg *e, void *data) { @@ -6235,6 +6314,7 @@ static void brcmf_register_event_handlers(struct brcmf_cfg80211_info *cfg) brcmf_p2p_notify_action_tx_complete); brcmf_fweh_register(cfg->pub, BRCMF_E_PSK_SUP, brcmf_notify_connect_status); + brcmf_fweh_register(cfg->pub, BRCMF_E_RSSI, brcmf_notify_rssi); } static void brcmf_deinit_priv_mem(struct brcmf_cfg80211_info *cfg) @@ -7169,6 +7249,8 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp) wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_DFS_OFFLOAD); + wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST); + wiphy_read_of_freq_limits(wiphy); return 0; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h index 17817cdb5de2..e90a30808c22 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h @@ -213,6 +213,9 @@ struct vif_saved_ie { * @list: linked list. * @mgmt_rx_reg: registered rx mgmt frame types. * @mbss: Multiple BSS type, set if not first AP (not relevant for P2P). + * @cqm_rssi_low: Lower RSSI limit for CQM monitoring + * @cqm_rssi_high: Upper RSSI limit for CQM monitoring + * @cqm_rssi_last: Last RSSI reading for CQM monitoring */ struct brcmf_cfg80211_vif { struct brcmf_if *ifp; @@ -224,6 +227,9 @@ struct brcmf_cfg80211_vif { u16 mgmt_rx_reg; bool mbss; int is_11d; + s32 cqm_rssi_low; + s32 cqm_rssi_high; + s32 cqm_rssi_last; }; /* association inform */ diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h index 2e31cc10c195..ff2ef557f0ea 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h @@ -752,6 +752,34 @@ struct brcmf_assoclist_le { u8 mac[BRCMF_MAX_ASSOCLIST][ETH_ALEN]; }; +/** + * struct brcmf_rssi_be - RSSI threshold event format + * + * @rssi: receive signal strength (in dBm) + * @snr: signal-noise ratio + * @noise: noise (in dBm) + */ +struct brcmf_rssi_be { + __be32 rssi; + __be32 snr; + __be32 noise; +}; + +#define BRCMF_MAX_RSSI_LEVELS 8 + +/** + * struct brcm_rssi_event_le - rssi_event IOVAR format + * + * @rate_limit_msec: RSSI event rate limit + * @rssi_level_num: number of supplied RSSI levels + * @rssi_levels: RSSI levels in ascending order + */ +struct brcmf_rssi_event_le { + __le32 rate_limit_msec; + s8 rssi_level_num; + s8 rssi_levels[BRCMF_MAX_RSSI_LEVELS]; +}; + /** * struct brcmf_wowl_wakeind_le - Wakeup indicators * Note: note both fields contain same information.
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. Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> --- .../broadcom/brcm80211/brcmfmac/cfg80211.c | 82 +++++++++++++++++++ .../broadcom/brcm80211/brcmfmac/cfg80211.h | 6 ++ .../broadcom/brcm80211/brcmfmac/fwil_types.h | 28 +++++++ 3 files changed, 116 insertions(+) -- 2.29.2