diff mbox series

[v2] brcmfmac: add support for CQM RSSI notifications

Message ID 20210114163641.2427591-1-alsi@bang-olufsen.dk
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series [v2] brcmfmac: add support for CQM RSSI notifications | expand

Commit Message

Alvin Šipraga Jan. 14, 2021, 4:36 p.m. UTC
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.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
v1 -> v2:
- clarify firmware behaviour in a comment
- fix detection of upper bound RSSI transition
- improve clamping of min/max RSSI values
- remove unnecessary check on last RSSI value
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 87 +++++++++++++++++++
 .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  6 ++
 .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 28 ++++++
 3 files changed, 121 insertions(+)

Comments

Arend Van Spriel Jan. 15, 2021, 2:10 p.m. UTC | #1
+ 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.

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.

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@intel.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@intel.com>
     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Andrew Zaborowski Jan. 15, 2021, 2:51 p.m. UTC | #2
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
Alvin Šipraga Jan. 15, 2021, 2:57 p.m. UTC | #3
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@intel.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@intel.com>
>      Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>
Arend Van Spriel Jan. 15, 2021, 3:08 p.m. UTC | #4
On 1/15/2021 3:51 PM, Andrew Zaborowski wrote:
> 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.

Argh. Totally overlooked these were two different callbacks. In that 
case it is easy to determine what is being requested.

Thanks,
Arend
Arend Van Spriel Jan. 19, 2021, 8:30 a.m. UTC | #5
On 1/15/2021 3:57 PM, Alvin Šipraga wrote:
> 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.

OK. I overlooked that there were two different callbacks involved. So I 
will review the patch with that knowledge. What wifi chip did you test 
this with and more importantly which firmware version?

Regards,
Arend
Alvin Šipraga Jan. 19, 2021, 10:03 a.m. UTC | #6
Hi,

On 1/19/21 9:30 AM, Arend Van Spriel wrote:
> On 1/15/2021 3:57 PM, Alvin Šipraga wrote:
>> 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.
> 
> OK. I overlooked that there were two different callbacks involved. So I 
> will review the patch with that knowledge. What wifi chip did you test 
> this with and more importantly which firmware version?

All testing was done with a PCIe Cypress CYW88359 (Murata Type 1VA).

I tested with two firmwares:

1. A custom firmware from Cypress with some vendor-specific features:
    version 9.40.98.19 (r727154 CY) FWID 01-1ff1c30

2. The latest public firmware release from Murata[1] for this chip:
    version 9.40.130 (r724855 CY) FWID 01-9ae2cd6d

Thanks for the review.

[1] https://github.com/murata-wireless/cyw-fmac-fw

Kind regards,
Alvin
Alvin Šipraga Feb. 8, 2021, 10:37 a.m. UTC | #7
Hi Arend,

On 1/19/21 11:03 AM, Alvin Šipraga wrote:
> Hi,
> 
> On 1/19/21 9:30 AM, Arend Van Spriel wrote:
>> On 1/15/2021 3:57 PM, Alvin Šipraga wrote:
>>> 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.
>>
>> OK. I overlooked that there were two different callbacks involved. So I
>> will review the patch with that knowledge. What wifi chip did you test
>> this with and more importantly which firmware version?

Did you have a chance to review? Do you require any more information 
from my end? We have been using this patch for some time now and it 
continues to function as expected.

Thanks in advance.

Kind regards,
Alvin

> 
> All testing was done with a PCIe Cypress CYW88359 (Murata Type 1VA).
> 
> I tested with two firmwares:
> 
> 1. A custom firmware from Cypress with some vendor-specific features:
>      version 9.40.98.19 (r727154 CY) FWID 01-1ff1c30
> 
> 2. The latest public firmware release from Murata[1] for this chip:
>      version 9.40.130 (r724855 CY) FWID 01-9ae2cd6d
> 
> Thanks for the review.
> 
> [1] https://github.com/murata-wireless/cyw-fmac-fw
> 
> Kind regards,
> Alvin
>
Kalle Valo Feb. 8, 2021, 10:55 a.m. UTC | #8
Alvin Šipraga <ALSI@bang-olufsen.dk> 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.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

I'll drop this from my queue, please resend once the review is done.

Patch set to Changes Requested.
Arend Van Spriel Feb. 8, 2021, 11:53 a.m. UTC | #9
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.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
> v1 -> v2:
> - clarify firmware behaviour in a comment
> - fix detection of upper bound RSSI transition
> - improve clamping of min/max RSSI values
> - remove unnecessary check on last RSSI value
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 87 +++++++++++++++++++
>   .../broadcom/brcm80211/brcmfmac/cfg80211.h    |  6 ++
>   .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 28 ++++++
>   3 files changed, 121 insertions(+)
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 0ee421f30aa2..605aa3f4ddc7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5196,6 +5196,48 @@  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) {
+		/* The firmware will send an event when the RSSI is less than or
+		 * equal to a configured level and the previous RSSI event was
+		 * less than or equal to a different level. Set a third level
+		 * so that we also detect the transition from rssi <= rssi_high
+		 * to rssi > rssi_high.
+		 */
+		struct brcmf_rssi_event_le config = {
+			.rate_limit_msec = cpu_to_le32(0),
+			.rssi_level_num = 3,
+			.rssi_levels = {
+				clamp_val(rssi_low, S8_MIN, S8_MAX - 2),
+				clamp_val(rssi_high, S8_MIN + 1, 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 +5544,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 +6180,47 @@  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)
+{
+	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);
+
+	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);
+
+	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;
+}
+
 static s32 brcmf_notify_vif_event(struct brcmf_if *ifp,
 				  const struct brcmf_event_msg *e, void *data)
 {
@@ -6235,6 +6319,7 @@  static void brcmf_register_event_handlers(struct brcmf_cfg80211_info *cfg)
 			    brcmf_p2p_notify_action_tx_complete);
 	brcmf_fweh_register(cfg->pub, BRCMF_E_PSK_SUP,
 			    brcmf_notify_connect_status);
+	brcmf_fweh_register(cfg->pub, BRCMF_E_RSSI, brcmf_notify_rssi);
 }
 
 static void brcmf_deinit_priv_mem(struct brcmf_cfg80211_info *cfg)
@@ -7169,6 +7254,8 @@  static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 		wiphy_ext_feature_set(wiphy,
 				      NL80211_EXT_FEATURE_DFS_OFFLOAD);
 
+	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
+
 	wiphy_read_of_freq_limits(wiphy);
 
 	return 0;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
index 17817cdb5de2..e90a30808c22 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
@@ -213,6 +213,9 @@  struct vif_saved_ie {
  * @list: linked list.
  * @mgmt_rx_reg: registered rx mgmt frame types.
  * @mbss: Multiple BSS type, set if not first AP (not relevant for P2P).
+ * @cqm_rssi_low: Lower RSSI limit for CQM monitoring
+ * @cqm_rssi_high: Upper RSSI limit for CQM monitoring
+ * @cqm_rssi_last: Last RSSI reading for CQM monitoring
  */
 struct brcmf_cfg80211_vif {
 	struct brcmf_if *ifp;
@@ -224,6 +227,9 @@  struct brcmf_cfg80211_vif {
 	u16 mgmt_rx_reg;
 	bool mbss;
 	int is_11d;
+	s32 cqm_rssi_low;
+	s32 cqm_rssi_high;
+	s32 cqm_rssi_last;
 };
 
 /* association inform */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index 2e31cc10c195..ff2ef557f0ea 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -752,6 +752,34 @@  struct brcmf_assoclist_le {
 	u8 mac[BRCMF_MAX_ASSOCLIST][ETH_ALEN];
 };
 
+/**
+ * struct brcmf_rssi_be - RSSI threshold event format
+ *
+ * @rssi: receive signal strength (in dBm)
+ * @snr: signal-noise ratio
+ * @noise: noise (in dBm)
+ */
+struct brcmf_rssi_be {
+	__be32 rssi;
+	__be32 snr;
+	__be32 noise;
+};
+
+#define BRCMF_MAX_RSSI_LEVELS 8
+
+/**
+ * struct brcm_rssi_event_le - rssi_event IOVAR format
+ *
+ * @rate_limit_msec: RSSI event rate limit
+ * @rssi_level_num: number of supplied RSSI levels
+ * @rssi_levels: RSSI levels in ascending order
+ */
+struct brcmf_rssi_event_le {
+	__le32 rate_limit_msec;
+	s8 rssi_level_num;
+	s8 rssi_levels[BRCMF_MAX_RSSI_LEVELS];
+};
+
 /**
  * struct brcmf_wowl_wakeind_le - Wakeup indicators
  *	Note: note both fields contain same information.