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] [thread-next>] [day] [month] [year] [list]
Message-ID: <d97fcbc5-a0d1-40af-58d8-428f50282eed@broadcom.com>
Date:   Thu, 14 Jan 2021 14:39:29 +0100
From:   Arend van Spriel <arend.vanspriel@...adcom.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>,
        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

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.

> +			},
> +		};

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. 
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.

> +{
> +	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.

> +		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;

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4176 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ