[v2] driver_nl80211: report invalid signal and noise when info is unavailable

From: Avraham Stern <avraham.stern@intel.com>

When the driver sends a CQM RSSI threshold event, wpa_supplicant
queries the driver for the signal and noise values. However, it
is possible that by that time the station has already disconnected
from the AP, so these values are no longer valid. In this case,
indicate that these values are invalid by setting them to
WPA_INVALID_NOISE.
Previously a value of 0 would be reported, which may be confusing as
this is a valid value.
Since nl80211_get_link_signal() and nl80211_get_link_noise() already
set invalid values for a case of failure, just use the value set by
these functions even if they fail.

Signed-off-by: Avraham Stern <avraham.stern@intel.com>
Signed-off-by: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
---
 src/drivers/driver_nl80211_event.c | 26 ++++++++++++++++----------
 wpa_supplicant/bss.h               |  2 +-
 2 files changed, 17 insertions(+), 11 deletions(-)

In wpa_bss_update_level, we should probably continue to check for
new_level < 0 as well as new_level > -WPA_INVALID_NOISE.

On Sun, Sep 13, 2020 at 6:00 AM Andrei Otcheretianski
<andrei.otcheretianski@intel.com> wrote:
>
> From: Avraham Stern <avraham.stern@intel.com>
>
> When the driver sends a CQM RSSI threshold event, wpa_supplicant
> queries the driver for the signal and noise values. However, it
> is possible that by that time the station has already disconnected
> from the AP, so these values are no longer valid. In this case,
> indicate that these values are invalid by setting them to
> WPA_INVALID_NOISE.
> Previously a value of 0 would be reported, which may be confusing as
> this is a valid value.
> Since nl80211_get_link_signal() and nl80211_get_link_noise() already
> set invalid values for a case of failure, just use the value set by
> these functions even if they fail.
>
> Signed-off-by: Avraham Stern <avraham.stern@intel.com>
> Signed-off-by: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
> ---
>  src/drivers/driver_nl80211_event.c | 26 ++++++++++++++++----------
>  wpa_supplicant/bss.h               |  2 +-
>  2 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/src/drivers/driver_nl80211_event.c b/src/drivers/driver_nl80211_event.c
> index ce95e9cd39..31b2799535 100644
> --- a/src/drivers/driver_nl80211_event.c
> +++ b/src/drivers/driver_nl80211_event.c
> @@ -1277,7 +1277,6 @@ static void nl80211_cqm_event(struct wpa_driver_nl80211_data *drv,
>         struct nlattr *cqm[NL80211_ATTR_CQM_MAX + 1];
>         enum nl80211_cqm_rssi_threshold_event event;
>         union wpa_event_data ed;
> -       struct wpa_signal_info sig;
>         int res;
>
>         if (tb[NL80211_ATTR_CQM] == NULL ||
> @@ -1344,20 +1343,27 @@ static void nl80211_cqm_event(struct wpa_driver_nl80211_data *drv,
>                 return;
>         }
>
> -       res = nl80211_get_link_signal(drv, &sig);
> +       /*
> +        * nl80211_get_link_signal() and nl80211_get_link_noise() set default
> +        * values in case querying the driver fails.
> +        */
> +       res = nl80211_get_link_signal(drv, &ed.signal_change);
>         if (res == 0) {
> -               ed.signal_change.current_signal = sig.current_signal;
> -               ed.signal_change.current_txrate = sig.current_txrate;
>                 wpa_printf(MSG_DEBUG, "nl80211: Signal: %d dBm  txrate: %d",
> -                          sig.current_signal, sig.current_txrate);
> +                          ed.signal_change.current_signal,
> +                          ed.signal_change.current_txrate);
> +       } else {
> +               wpa_printf(MSG_DEBUG,
> +                          "nl80211: querying the driver for signal info failed");
>         }
>
> -       res = nl80211_get_link_noise(drv, &sig);
> -       if (res == 0) {
> -               ed.signal_change.current_noise = sig.current_noise;
> +       res = nl80211_get_link_noise(drv, &ed.signal_change);
> +       if (res == 0)
>                 wpa_printf(MSG_DEBUG, "nl80211: Noise: %d dBm",
> -                          sig.current_noise);
> -       }
> +                          ed.signal_change.current_noise);
> +       else
> +               wpa_printf(MSG_DEBUG,
> +                          "nl80211: querying the driver for noise info failed");
>
>         wpa_supplicant_event(drv->ctx, EVENT_SIGNAL_CHANGE, &ed);
>  }
> diff --git a/wpa_supplicant/bss.h b/wpa_supplicant/bss.h
> index 0716761749..750cdcbccc 100644
> --- a/wpa_supplicant/bss.h
> +++ b/wpa_supplicant/bss.h
> @@ -169,7 +169,7 @@ static inline int bss_is_pbss(struct wpa_bss *bss)
>
>  static inline void wpa_bss_update_level(struct wpa_bss *bss, int new_level)
>  {
> -       if (bss != NULL && new_level < 0)
> +       if (bss != NULL && new_level > -WPA_INVALID_NOISE)
>                 bss->level = new_level;
>  }
>
> --
> 2.28.0
>
>
> _______________________________________________
> Hostap mailing list
> Hostap@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap

> From: Matthew Wang <matthewmwang@chromium.org>
> Sent: Tuesday, September 29, 2020 06:52
> To: Otcheretianski, Andrei <andrei.otcheretianski@intel.com>
> Cc: hostap@lists.infradead.org; Stern, Avraham <avraham.stern@intel.com>
> Subject: Re: [PATCH v2] driver_nl80211: report invalid signal and noise when
> info is unavailable
> 
> In wpa_bss_update_level, we should probably continue to check for
> new_level < 0 as well as new_level > -WPA_INVALID_NOISE.

Why? RSSI is in dbm's and 0 (and positive numbers) is a valid though unlikely value.
We even actually may get it in some conductive environments.

Andrei

Sorry for the late reply, this got lost in my inbox. In e.g.
wpa_supplicant_need_to_roam_within_ess, we consider RSSI >= 0 to be
"unspecified units (not in dBm)", and it's best to keep these all
consistent IMO. If we should actually be treating >= 0 RSSI values as
valid dBm values, then there should be a separate patch making all the
relevant changes at once. For the purpose of this patch, that seems
like an unnecessary (and potentially non-trivial) change in
functionality that isn't mentioned in the commit message anyway.

On Wed, Oct 14, 2020 at 1:31 AM Otcheretianski, Andrei
<andrei.otcheretianski@intel.com> wrote:
>
> > From: Matthew Wang <matthewmwang@chromium.org>
> > Sent: Tuesday, September 29, 2020 06:52
> > To: Otcheretianski, Andrei <andrei.otcheretianski@intel.com>
> > Cc: hostap@lists.infradead.org; Stern, Avraham <avraham.stern@intel.com>
> > Subject: Re: [PATCH v2] driver_nl80211: report invalid signal and noise when
> > info is unavailable
> >
> > In wpa_bss_update_level, we should probably continue to check for
> > new_level < 0 as well as new_level > -WPA_INVALID_NOISE.
>
> Why? RSSI is in dbm's and 0 (and positive numbers) is a valid though unlikely value.
> We even actually may get it in some conductive environments.
>
> Andrei
>

On Tue, Oct 27, 2020 at 4:00 PM Matthew Wang <matthewmwang@chromium.org> wrote:
>
> Sorry for the late reply, this got lost in my inbox. In e.g.
> wpa_supplicant_need_to_roam_within_ess, we consider RSSI >= 0 to be
> "unspecified units (not in dBm)", and it's best to keep these all

To add to this: "unspecified" units come from something like
NL80211_BSS_SIGNAL_MBM vs. NL80211_BSS_SIGNAL_UNSPEC, which apply to
scan results. I don't think there's an equivalent UNSPEC possibility
for the CQM / EVENT_SIGNAL_CHANGE events where wpa_bss_update_level()
is used -- at least in this case, the signal values are coming from
NL80211_STA_INFO_SIGNAL, which has documented units (dBm).

But the below seems more compelling:

> consistent IMO. If we should actually be treating >= 0 RSSI values as
> valid dBm values, then there should be a separate patch making all the
> relevant changes at once. For the purpose of this patch, that seems
> like an unnecessary (and potentially non-trivial) change in
> functionality that isn't mentioned in the commit message anyway.

+1, it doesn't seem like a necessary change to accomplish the goal of
this patch.

Brian

>
> But the below seems more compelling:
> 
> > consistent IMO. If we should actually be treating >= 0 RSSI values as
> > valid dBm values, then there should be a separate patch making all the
> > relevant changes at once. For the purpose of this patch, that seems
> > like an unnecessary (and potentially non-trivial) change in
> > functionality that isn't mentioned in the commit message anyway.
> 
> +1, it doesn't seem like a necessary change to accomplish the goal of
> this patch.

Ok, that makes sense. I'll send a v2 of this patch

Andrei

> 
> Brian