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.
