| Message ID | 1539626250-769-4-git-send-email-tamizhr@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211/mac80211: Add support to configure and monitor station's rssi threshold | expand |
> +/* > + * How many frames need to have been used in average station's > + * signal strength before checking against the threshold > + */ > +#define IEEE80211_STA_SIGNAL_AVE_MIN_COUNT 4 > + > + > /* there are 40 bytes if you don't need the rateset to be kept */ > #define IEEE80211_TX_INFO_DRIVER_DATA_SIZE 40 ... > +static void ieee80211_sta_rx_signal_thold_check(struct ieee80211_rx_data *rx) > +{ > + struct sta_info *sta = rx->sta; > + struct ieee80211_bss_conf *bss_conf = &rx->sdata->vif.bss_conf; > + bool rssi_cross = false; > + > + if (!wiphy_ext_feature_isset(rx->local->hw.wiphy, > + NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG)) > + return; > + > + sta->count_rx_signal++; > + if (sta->count_rx_signal < IEEE80211_STA_SIGNAL_AVE_MIN_COUNT) > + return; Could you please clarify count_rx_signal processing and averaging approach ? I couldn't find where this counter is reset or modified in any other way. Regards, Sergey
Oh, umm, that patch is still here ... I guess we can combine 2 and 3 too. > + if (sta->rssi_low && bss_conf->enable_beacon) { > + int last_event = > + sta->last_rssi_event_value; > + int sig = -ewma_signal_read(&sta->rx_stats_avg.signal); > + int low = sta->rssi_low; > + int high = sta->rssi_high; This doesn't really support a list, like in patch 2 where you store sta_info::rssi_tholds? > + if (sig < low && > + (last_event == 0 || last_event >= low)) { > + sta->last_rssi_event_value = sig; > + cfg80211_sta_mon_rssi_notify( > + rx->sdata->dev, sta->addr, > + NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW, > + sig, GFP_ATOMIC); > + rssi_cross = true; > + } else if (sig > high && > + (last_event == 0 || last_event <= high)) { > + sta->last_rssi_event_value = sig; > + cfg80211_sta_mon_rssi_notify( > + rx->sdata->dev, sta->addr, > + NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH, > + sig, GFP_ATOMIC); > + rssi_cross = true; > + } > + } > + > + if (rssi_cross) { > + ieee80211_update_rssi_config(sta); > + rssi_cross = false; > + } > +} Ah, but it does, just hard to understand. However, this does mean what I suspected on the other patch is true - you're calling ieee80211_update_rssi_config() here, and that uses the sta->rssi_tholds array without any protection, while a concurrent change of configuration can free the data. You need to sort that out. I would suggest to stick all the sta->rssi_* fields you add into a new struct (you even had an empty one), and protect that struct using RCU. That also saves the memory in case it's not assigned at all. Something like struct sta_mon_rssi_config { struct rcu_head rcu_head; int n_thresholds; s32 low, high; u32 hyst; s32 last_value; s32 thresholds[]; }; then you can kfree_rcu() it, and just do a single allocation using struct_size() for however many entries you need. > + * @count_rx_signal: Number of data frames used in averaging station signal. > + * This can be used to avoid generating less reliable station rssi cross > + * events that would be based only on couple of received frames. > */ > struct sta_info { > /* General information, mostly static */ > @@ -600,6 +603,7 @@ struct sta_info { > s32 rssi_high; > u32 rssi_hyst; > s32 last_rssi_event_value; > + unsigned int count_rx_signal; I guess that would also move into the new struct. johannes
On 2018-11-09 17:25, Johannes Berg wrote: > Oh, umm, that patch is still here ... > > I guess we can combine 2 and 3 too. > > Sure. >> + if (sta->rssi_low && bss_conf->enable_beacon) { >> + int last_event = >> + sta->last_rssi_event_value; >> + int sig = -ewma_signal_read(&sta->rx_stats_avg.signal); >> + int low = sta->rssi_low; >> + int high = sta->rssi_high; > > This doesn't really support a list, like in patch 2 where you store > sta_info::rssi_tholds? > rssi_low and rssi_high will have a configured value or zero know ? Configured value has been stored in the previous patch. >> + if (sig < low && >> + (last_event == 0 || last_event >= low)) { >> + sta->last_rssi_event_value = sig; >> + cfg80211_sta_mon_rssi_notify( >> + rx->sdata->dev, sta->addr, >> + NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW, >> + sig, GFP_ATOMIC); >> + rssi_cross = true; >> + } else if (sig > high && >> + (last_event == 0 || last_event <= high)) { >> + sta->last_rssi_event_value = sig; >> + cfg80211_sta_mon_rssi_notify( >> + rx->sdata->dev, sta->addr, >> + NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH, >> + sig, GFP_ATOMIC); >> + rssi_cross = true; >> + } >> + } >> + >> + if (rssi_cross) { >> + ieee80211_update_rssi_config(sta); >> + rssi_cross = false; >> + } >> +} > > Ah, but it does, just hard to understand. > > However, this does mean what I suspected on the other patch is true - > you're calling ieee80211_update_rssi_config() here, and that uses the > sta->rssi_tholds array without any protection, while a concurrent > change > of configuration can free the data. > yes, I'll add a protection mechanism. > You need to sort that out. I would suggest to stick all the sta->rssi_* > fields you add into a new struct (you even had an empty one), and > protect that struct using RCU. That also saves the memory in case it's > not assigned at all. Something like > > struct sta_mon_rssi_config { > struct rcu_head rcu_head; > int n_thresholds; > s32 low, high; > u32 hyst; > s32 last_value; > s32 thresholds[]; > }; > > then you can kfree_rcu() it, and just do a single allocation using > struct_size() for however many entries you need. > Yes correct. I'll change it. >> + * @count_rx_signal: Number of data frames used in averaging station >> signal. >> + * This can be used to avoid generating less reliable station rssi >> cross >> + * events that would be based only on couple of received frames. >> */ >> struct sta_info { >> /* General information, mostly static */ >> @@ -600,6 +603,7 @@ struct sta_info { >> s32 rssi_high; >> u32 rssi_hyst; >> s32 last_rssi_event_value; >> + unsigned int count_rx_signal; > > I guess that would also move into the new struct. > Okay. Thanks, Tamizh.
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 71985e9..355e522 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -849,6 +849,13 @@ enum mac80211_rate_control_flags { }; +/* + * How many frames need to have been used in average station's + * signal strength before checking against the threshold + */ +#define IEEE80211_STA_SIGNAL_AVE_MIN_COUNT 4 + + /* there are 40 bytes if you don't need the rateset to be kept */ #define IEEE80211_TX_INFO_DRIVER_DATA_SIZE 40 diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 72d2b07..be386ff 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -3856,7 +3856,7 @@ void sta_mon_rssi_config_free(struct sta_info *sta) sta->n_rssi_tholds = 0; } -static void ieee80211_update_rssi_config(struct sta_info *sta) +void ieee80211_update_rssi_config(struct sta_info *sta) { s32 last; u32 hyst; diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 3bd3b57..1afef40 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -1681,6 +1681,52 @@ void ieee80211_sta_uapsd_trigger(struct ieee80211_sta *pubsta, u8 tid) return RX_CONTINUE; } +static void ieee80211_sta_rx_signal_thold_check(struct ieee80211_rx_data *rx) +{ + struct sta_info *sta = rx->sta; + struct ieee80211_bss_conf *bss_conf = &rx->sdata->vif.bss_conf; + bool rssi_cross = false; + + if (!wiphy_ext_feature_isset(rx->local->hw.wiphy, + NL80211_EXT_FEATURE_STA_MON_RSSI_CONFIG)) + return; + + sta->count_rx_signal++; + if (sta->count_rx_signal < IEEE80211_STA_SIGNAL_AVE_MIN_COUNT) + return; + + if (sta->rssi_low && bss_conf->enable_beacon) { + int last_event = + sta->last_rssi_event_value; + int sig = -ewma_signal_read(&sta->rx_stats_avg.signal); + int low = sta->rssi_low; + int high = sta->rssi_high; + + if (sig < low && + (last_event == 0 || last_event >= low)) { + sta->last_rssi_event_value = sig; + cfg80211_sta_mon_rssi_notify( + rx->sdata->dev, sta->addr, + NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW, + sig, GFP_ATOMIC); + rssi_cross = true; + } else if (sig > high && + (last_event == 0 || last_event <= high)) { + sta->last_rssi_event_value = sig; + cfg80211_sta_mon_rssi_notify( + rx->sdata->dev, sta->addr, + NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH, + sig, GFP_ATOMIC); + rssi_cross = true; + } + } + + if (rssi_cross) { + ieee80211_update_rssi_config(sta); + rssi_cross = false; + } +} + static ieee80211_rx_result debug_noinline ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx) { @@ -1736,6 +1782,7 @@ void ieee80211_sta_uapsd_trigger(struct ieee80211_sta *pubsta, u8 tid) if (!(status->flag & RX_FLAG_NO_SIGNAL_VAL)) { sta->rx_stats.last_signal = status->signal; ewma_signal_add(&sta->rx_stats_avg.signal, -status->signal); + ieee80211_sta_rx_signal_thold_check(rx); } if (status->chains) { @@ -4177,9 +4224,11 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx, /* statistics part of ieee80211_rx_h_sta_process() */ if (!(status->flag & RX_FLAG_NO_SIGNAL_VAL)) { stats->last_signal = status->signal; - if (!fast_rx->uses_rss) + if (!fast_rx->uses_rss) { ewma_signal_add(&sta->rx_stats_avg.signal, -status->signal); + ieee80211_sta_rx_signal_thold_check(rx); + } } if (status->chains) { diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index acbad98..ebf5cb0 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -493,6 +493,9 @@ struct sta_mon_rssi_config { * @rssi_hyst: RSSI hysteresis for this station * @last_rssi_event_value: Last RSSI value for this station triggered the * RSSI cross event. + * @count_rx_signal: Number of data frames used in averaging station signal. + * This can be used to avoid generating less reliable station rssi cross + * events that would be based only on couple of received frames. */ struct sta_info { /* General information, mostly static */ @@ -600,6 +603,7 @@ struct sta_info { s32 rssi_high; u32 rssi_hyst; s32 last_rssi_event_value; + unsigned int count_rx_signal; /* keep last! */ struct ieee80211_sta sta; @@ -771,6 +775,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata, unsigned long exp_time); u8 sta_info_tx_streams(struct sta_info *sta); +void ieee80211_update_rssi_config(struct sta_info *sta); void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta); void ieee80211_sta_ps_deliver_poll_response(struct sta_info *sta);
Triggers cfg80211_sta_mon_rssi_notify with the corresponding event when station signal goes out of configured threshold. It uses rx data signal to check against rssi threshold configured by the user. And update the lower and upper RSSI threshold for the station if it is not configured as a fixed threshold. This event will be useful for the application like steering to take decision on any station depends on its current link quality. Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org> --- include/net/mac80211.h | 7 +++++++ net/mac80211/cfg.c | 2 +- net/mac80211/rx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++- net/mac80211/sta_info.h | 5 +++++ 4 files changed, 63 insertions(+), 2 deletions(-)