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

[Alvin Šipraga]

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
>