Re: [PATCH v2] brcmfmac: add support for CQM RSSI notifications

Hi,

On 1/19/21 9:30 AM, Arend Van Spriel wrote:

On 1/15/2021 3:57 PM, Alvin Šipraga wrote:

quoted

Hi Arend,

On 1/15/21 3:10 PM, Arend Van Spriel wrote:

quoted

+ Johannes
- netdevs

On 1/14/2021 5:36 PM, 'Alvin Šipraga' via BRCM80211-DEV-LIST,PDL wrote:

quoted

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