Message ID | 20200913125934.2808-1-andrei.otcheretianski@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2] driver_nl80211: report invalid signal and noise when info is unavailable | expand |
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
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; }