Re: [PATCH] brcmfmac: add support for CQM RSSI notifications
From: Alvin Šipraga
Date: Thu Jan 14 2021 - 10:05:53 EST
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@xxxxxxxxxxxxxxx>
>> ---
>> .../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;
>