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

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

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