Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210114163641.2427591-1-alsi@bang-olufsen.dk> <2adec5d6-fbc9-680c-01d6-25f83327bf21@broadcom.com>
In-Reply-To: <2adec5d6-fbc9-680c-01d6-25f83327bf21@broadcom.com>
From:   Andrew Zaborowski <andrew.zaborowski@intel.com>
Date:   Fri, 15 Jan 2021 15:51:23 +0100
Message-ID: <CAOq732KGRmQ0eq46cqkF-EW-A8W6QiOb02NZEq6H7K_28YSstA@mail.gmail.com>
Subject: Re: [PATCH v2] brcmfmac: add support for CQM RSSI notifications
To:     Arend Van Spriel <arend.vanspriel@broadcom.com>
Cc:     =?UTF-8?Q?Alvin_=C5=A0ipraga?= <ALSI@bang-olufsen.dk>,
        Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Chi-hsien Lin <chi-hsien.lin@infineon.com>,
        Wright Feng <wright.feng@infineon.com>,
        Chung-hsien Hsu <chung-hsien.hsu@infineon.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "brcm80211-dev-list.pdl@broadcom.com" 
        <brcm80211-dev-list.pdl@broadcom.com>,
        "SHA-cyfmac-dev-list@infineon.com" <SHA-cyfmac-dev-list@infineon.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, 15 Jan 2021 at 15:12, Arend Van Spriel
<arend.vanspriel@broadcom.com> 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.

Right.

>
> The second behavior is used when the driver advertises
> NL80211_EXT_FEATURE_CQM_RSSI_LIST *and* user-space provides more than
> one RSSI threshold.

Or, when the driver doesn't implement set_cqm_rssi_config (the old
method).  So the driver can stop supporting set_cqm_rssi_config when
it implements set_cqm_rssi_range_config.

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

When the driver wants to implement both methods it may be because the
hardware has a better method of handling the hysteresis than what is
implemented nl80211.c.  In that case, yes, it'd need to remember which
method was used and the parameters.  If set_cqm_rssi_range_config was
used the driver is expected to report when a momentary rssi
measurement goes out of the configured range.

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

That may be a good reason to keep supporting both methods.

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

Right, with one threshold the behaviour is unchanged.

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

I guess the answer is that if the driver implements both methods it is
expected to report threshold crossings according to the method that
was called last.

Best regards