Message-ID: <3a7de182-b0c7-352c-323b-3e3cebb9ffa3@bang-olufsen.dk>
Date:   Fri, 15 Jan 2021 14:57:50 +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>,
        Andrew Zaborowski <andrew.zaborowski@...el.com>
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>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] brcmfmac: add support for CQM RSSI notifications

Hi Arend,

On 1/15/21 3:10 PM, Arend Van Spriel wrote:
> + Johannes
> - netdevs
> 
> On 1/14/2021 5:36 PM, '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.
> 
> The more I am looking into this API the less I understand it or at least 
> it raises a couple of questions. Looking into nl80211_set_cqm_rssi() [1] 
> two behaviors are supported: 1) driver is provisioned with a threshold 
> and hysteresis, or 2) driver is provisioned with high and low threshold. >
> The second behavior is used when the driver advertises 
> NL80211_EXT_FEATURE_CQM_RSSI_LIST *and* user-space provides more than 
> one RSSI threshold. In both cases the same driver callback is being used 
> so I wonder what is expected from the driver. Seems to me the driver 
> would need to be able to distinguish between the two behavioral 
> scenarios. As there is no obvious way I assume the driver should behave 
> the same for both cases, but again it is unclear to me what that 
> expected/required behavior is.

It will only provision the driver according to behaviour (1) if 0 or 1 
thresholds are being set AND the driver implements 
set_cqm_rssi_config(). But it says in the documentation for the 
set_cqm_rssi_range_config() callback[1] that it supersedes 
set_cqm_rssi_config() (or at least that there is no point in 
implementing _config if range_config is implemented). In that case, and 
if just one threshold is supplied (with a hysteresis), then a suitable 
range is computed by cfg80211_cqm_rssi_update() and provided to 
set_cqm_rssi_range_config(). I guess the implication here is that the 
two behaviours are functionally equivalent. I'm not sure I can argue for 
or against that because I don't really know what the semantics of the 
original API were supposed to be, but it seems reasonable.

As a starting point - and since the firmware behaviour is very close 
already - I implemented only set_cqm_rssi_range(). I have been testing 
with iwd, which by default sets just a single threshold and hysteresis, 
and the driver was sending notifications as would be expected.

[1] 
https://elixir.bootlin.com/linux/v5.10.7/source/include/net/cfg80211.h#L3780

> 
> With behavior 2) some processing is done in cfg80211 itself by 
> cfg80211_cqm_rssi_update() which is called from nl80211_set_cqm_rssi() 
> upon NL80211_CMD_SET_CQM and cfg80211_cqm_rssi_notify() called by 
> driver. If I look at that it matches pretty close what our firmware is 
> doing. The difference is that our firmware avoids RSSI oscillation with 
> a time constraint between RSSI events whereas cfg80211 uses the hysteresis.

 From what I gathered, the set_cqm_rssi_range_config(low, high) API 
should configure the driver to send a LOW/HIGH event to cfg80211 
whenever the RSSI is outside of the range [low, high]. cfg80211 seems to 
take care of how to deal with multiple thresholds then by calling back 
into _range_config from cfg80211_cqm_rssi_notify() to readjust the 
range. I could be oversimplifying things though and I would be glad to 
get some clarification.

Kind regards,
Alvin

> 
> So before moving forward, I hope Johannes can chime in and clarify 
> things. Added the commit message introducing the extended feature below. 
> It mentions backward compatibility, but it only considers the extended 
> feature setting when user-space provides more than one threshold. 
> However, when the drivers set the extended feature is expects (low, 
> high) and (threshold, hysteresis) if not. So it seems the extended 
> feature should have precedence over the number of thresholds provided by 
> user-space.
>  > Regards,
> Arend
> 
> [1] 
> https://elixir.bootlin.com/linux/v5.10.7/source/net/wireless/nl80211.c#L11479 
> 
> 
> ---8<-----------------------------------------------------------------
> commit 4a4b8169501b18c3450ac735a7e277b24886a651
> Author: Andrew Zaborowski <andrew.zaborowski@...el.com>
> Date:   Fri Feb 10 10:02:31 2017 +0100
> 
>      cfg80211: Accept multiple RSSI thresholds for CQM
> 
>      Change the SET CQM command's RSSI threshold attribute to accept any
>      number of thresholds as a sorted array.  The API should be backwards
>      compatible so that if one s32 threshold value is passed, the old
>      mechanism is enabled.  The netlink event generated is the same in both
>      cases.
> 
>      cfg80211 handles an arbitrary number of RSSI thresholds but drivers 
> have
>      to provide a method (set_cqm_rssi_range_config) that configures a 
> range
>      set by a high and a low value.  Drivers have to call back when the 
> RSSI
>      goes out of that range and there's no additional event for each 
> time the
>      range is reconfigured as there was with the current one-threshold API.
> 
>      This method doesn't have a hysteresis parameter because there's no
>      benefit to the cfg80211 code from having the hysteresis be handled by
>      hardware/driver in terms of the number of wakeups.  At the same time
>      it would likely be less consistent between drivers if offloaded or
>      done in the drivers.
> 
>      Signed-off-by: Andrew Zaborowski <andrew.zaborowski@...el.com>
>      Signed-off-by: Johannes Berg <johannes.berg@...el.com>
> 

Powered by blists - more mailing lists