CQM RSSI notification in station_roamed()

CQM RSSI notification in station_roamed()

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

Hi,

I have been working on CQM RSSI notification support for the brcmfmac 
driver so that roaming can be properly offloaded to iwd. I want to ask 
about this particular behaviour in iwd's station code:

     static void station_roamed(struct station *station)
     {
         /*
          * New signal high/low notification should occur on the next
          * beacon from new AP.
          */
         station->signal_low = false;
         station->roam_min_time.tv_sec = 0;
         station->roam_no_orig_ap = false;
         station->roam_scan_full = false;

         ...
     }

When I look at the kernel API, this doesn't seem quite right to me. At 
least, there doesn't seem to be any onus on the driver to repeat itself. 
This led me to experience the following scenario during my testing:

     - station is connected to a BSS
     - driver notifies RSSI LOW
     - iwd roams to a new BSS
     - RSSI happens to remain LOW
     - driver doesn't notify RSSI LOW, because it never went HIGH

This leads to us getting stuck with a low RSSI, even though iwd should 
probably try roaming again because it was never informed that the RSSI 
went HIGH. In practice, it means that we might sooner disconnect 
entirely rather than roam to better BSS.

Would it not be more prudent to continue assuming that signal_low == 
true, and schedule another roam in station_roamed() to cover for this 
scenario? Something like what is already done in station_roam_failed():

     static void station_roam_failed(struct station *station)
     {
         ...

     delayed_retry:
         /*
          * If we're still connected to the old BSS, only clear 
preparing_roam
          * and reattempt in 60 seconds if signal level is still low at that
          * time.
          */
         station->preparing_roam = false;
         station->roam_scan_full = false;
         station->ap_directed_roaming = false;

         if (station->signal_low)
             station_roam_timeout_rearm(station, 60);
     }

If you like the suggestion then I can prepare a patch. But perhaps I 
have misunderstood the requirements of the CQM RSSI notification API in 
the kernel, and it should indeed always send a notification upon 
association to a new BSS, even if that happens to be redundant (e.g. LOW 
followed by LOW). In any case I would be happy to have some 
clarification on this.

Kind regards,
Alvin

Re: CQM RSSI notification in station_roamed()

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]

Hi,

On Tue, 19 Jan 2021 at 11:30, Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> I have been working on CQM RSSI notification support for the brcmfmac
> driver so that roaming can be properly offloaded to iwd. I want to ask
> about this particular behaviour in iwd's station code:
>
>      static void station_roamed(struct station *station)
>      {
>          /*
>           * New signal high/low notification should occur on the next
>           * beacon from new AP.
>           */
>          station->signal_low = false;
>          station->roam_min_time.tv_sec = 0;
>          station->roam_no_orig_ap = false;
>          station->roam_scan_full = false;
>
>          ...
>      }
>
> When I look at the kernel API, this doesn't seem quite right to me. At
> least, there doesn't seem to be any onus on the driver to repeat itself.

Right, apparently there isn't.  I imagine a lot of the things we and
wpa_supplicant rely on is not explicitly documented in
include/net/cfg80211.h but the drivers do them perhaps to match
mac80211.  I guess I assumed this was also the case here.  mac80211
sets the IEEE80211_STA_RESET_SIGNAL_AVE flag in
ieee80211_set_associated() which causes the beacon handler to always
emit an RSSI event on the next run.

> This led me to experience the following scenario during my testing:
>
>      - station is connected to a BSS
>      - driver notifies RSSI LOW
>      - iwd roams to a new BSS
>      - RSSI happens to remain LOW
>      - driver doesn't notify RSSI LOW, because it never went HIGH
>
> This leads to us getting stuck with a low RSSI, even though iwd should
> probably try roaming again because it was never informed that the RSSI
> went HIGH. In practice, it means that we might sooner disconnect
> entirely rather than roam to better BSS.
>
> Would it not be more prudent to continue assuming that signal_low ==
> true, and schedule another roam in station_roamed() to cover for this
> scenario?

Yes, this is probably the right fix.

We do want some delay before the next roam attempt because if we've
just scanned and roamed to the best BSS in the scan results there's
probably no point scanning again immediately.

> Something like what is already done in station_roam_failed():>
>      static void station_roam_failed(struct station *station)
>      {
>          ...
>
>      delayed_retry:
>          /*
>           * If we're still connected to the old BSS, only clear
> preparing_roam
>           * and reattempt in 60 seconds if signal level is still low at that
>           * time.
>           */
>          station->preparing_roam = false;
>          station->roam_scan_full = false;
>          station->ap_directed_roaming = false;
>
>          if (station->signal_low)
>              station_roam_timeout_rearm(station, 60);
>      }
>
> If you like the suggestion then I can prepare a patch. But perhaps I
> have misunderstood the requirements of the CQM RSSI notification API in
> the kernel, and it should indeed always send a notification upon
> association to a new BSS, even if that happens to be redundant (e.g. LOW
> followed by LOW). In any case I would be happy to have some
> clarification on this.

It doesn't seem to be documented so probably no one knows, it's also a
question of how many drivers do emit the event and how many don't.

Best regards

Re: CQM RSSI notification in station_roamed()

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

Hi,

On 1/21/21 12:46 AM, Andrew Zaborowski wrote:
> Hi,
> 
> On Tue, 19 Jan 2021 at 11:30, Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
>> I have been working on CQM RSSI notification support for the brcmfmac
>> driver so that roaming can be properly offloaded to iwd. I want to ask
>> about this particular behaviour in iwd's station code:
>>
>>       static void station_roamed(struct station *station)
>>       {
>>           /*
>>            * New signal high/low notification should occur on the next
>>            * beacon from new AP.
>>            */
>>           station->signal_low = false;
>>           station->roam_min_time.tv_sec = 0;
>>           station->roam_no_orig_ap = false;
>>           station->roam_scan_full = false;
>>
>>           ...
>>       }
>>
>> When I look at the kernel API, this doesn't seem quite right to me. At
>> least, there doesn't seem to be any onus on the driver to repeat itself.
> 
> Right, apparently there isn't.  I imagine a lot of the things we and
> wpa_supplicant rely on is not explicitly documented in
> include/net/cfg80211.h but the drivers do them perhaps to match
> mac80211.  I guess I assumed this was also the case here.  mac80211
> sets the IEEE80211_STA_RESET_SIGNAL_AVE flag in
> ieee80211_set_associated() which causes the beacon handler to always
> emit an RSSI event on the next run.
> 
>> This led me to experience the following scenario during my testing:
>>
>>       - station is connected to a BSS
>>       - driver notifies RSSI LOW
>>       - iwd roams to a new BSS
>>       - RSSI happens to remain LOW
>>       - driver doesn't notify RSSI LOW, because it never went HIGH
>>
>> This leads to us getting stuck with a low RSSI, even though iwd should
>> probably try roaming again because it was never informed that the RSSI
>> went HIGH. In practice, it means that we might sooner disconnect
>> entirely rather than roam to better BSS.
>>
>> Would it not be more prudent to continue assuming that signal_low ==
>> true, and schedule another roam in station_roamed() to cover for this
>> scenario?
> 
> Yes, this is probably the right fix.
> 
> We do want some delay before the next roam attempt because if we've
> just scanned and roamed to the best BSS in the scan results there's
> probably no point scanning again immediately.

OK, thanks for the clarification. I will prepare a patch and send it to 
the mailing list - probably tomorrow.

Kind regards,
Alvin