lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f59f1133-10b2-6f84-b766-1f8b1606b17d@bang-olufsen.dk>
Date:   Thu, 14 Jan 2021 15:04:56 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Arend van Spriel <arend.vanspriel@...adcom.com>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Chi-hsien Lin <chi-hsien.lin@...ineon.com>,
        Wright Feng <wright.feng@...ineon.com>,
        Chung-hsien Hsu <chung-hsien.hsu@...ineon.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "brcm80211-dev-list.pdl@...adcom.com" 
        <brcm80211-dev-list.pdl@...adcom.com>,
        "SHA-cyfmac-dev-list@...ineon.com" <SHA-cyfmac-dev-list@...ineon.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 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:
>> 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@...g-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;
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ