buffers because in monitor mode device receives everything seen
on the air. In noisy condition, disabling monitor mode helps assoc
go through without any issue.
Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@xxxxxxxxxxxxxxxx>
---
drivers/net/wireless/ath/ath10k/mac.c | 35 +++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3b5aaa3..885e984 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -766,12 +766,36 @@ static int ath10k_monitor_stop(struct ath10k *ar)
return 0;
}
+static bool ath10k_disable_promisc_mode(struct ath10k *ar)
The function name implies something that has a side-effect.
If anything, this should be named more like:
ath10k_mac_should_disable_promisc() or ath10k_mac_is_promisc() (with
the logic inverted).
Ok.
+{
+ struct ath10k_vif *arvif;
+
+ if (!ar->num_started_vdevs)
+ return false;
+
+ list_for_each_entry(arvif, &ar->arvifs, list) {
This means function must be called while holding conf_mutex (my MCC
patch adds data_lock as an option, but current upstream tree uses
conf_mutex only).
Ok. Sure, we can fix it when we have MCC change into the tree.
+ /* Disabling promiscuous mode when STA/IBSS is running */
+ if (arvif->vdev_type == WMI_VDEV_TYPE_STA ||
+ arvif->vdev_type == WMI_VDEV_TYPE_IBSS)
Wouldn't `if (arvif->vdev_type != WMI_VDEV_TYPE_AP) return false` be
safer? We only know this is safe for AP, right?
Sure.
+ return false;
+ }
+
+ return true;
+}
+
static int ath10k_monitor_recalc(struct ath10k *ar)
{
bool should_start;
lockdep_assert_held(&ar->conf_mutex);
+ if ((ar->filter_flags & FIF_PROMISC_IN_BSS) &&
+ ath10k_disable_promisc_mode(ar)) {
+ ar->filter_flags &= ~FIF_PROMISC_IN_BSS;
+ ath10k_dbg(ar, ATH10K_DBG_MAC,
+ "mac disabling promiscuous mode because vdev is started\n");
+ }
+
I don't like this. You modify filter_flags. This shouldn't be
happening in the recalc function. The recalc function should have only
a side-effect of starting/stopping monitor vdev.
Instead:
should_start = ar->monitor || ath10k_mac_is_promisc() ||
test_bit(ATH10K_CAC_RUNNING);
And put the promisc skipping logic in ath10k_mac_is_promisc().
Right, this is much better.
should_start = ar->monitor ||
ar->filter_flags & FIF_PROMISC_IN_BSS ||
test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
@@ -969,6 +993,10 @@ static int ath10k_vdev_start_restart(struct ath10k_vif *arvif, bool restart)
ar->num_started_vdevs++;
ath10k_recalc_radar_detection(ar);
+ ret = ath10k_monitor_recalc(ar);
+ if (ret)
+ ath10k_vdev_stop(arvif);
You should warn here "failed to recalc monitor: %d\n". Also it'd be
nice if vdev_stop() was checked for error as well (but not with "ret"
as to not lose the original failure reason code; `ret2` is okay). A
warning for that is would also be desired.
Sure.
+
return ret;
}
@@ -3476,6 +3504,13 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
changed_flags &= SUPPORTED_FILTERS;
*total_flags &= SUPPORTED_FILTERS;
+ if (*total_flags & FIF_PROMISC_IN_BSS) {
+ if (ar->num_started_vdevs) {
+ ath10k_dbg(ar, ATH10K_DBG_MAC,
+ "mac does not enable promiscuous mode when already a vdev is running\n");
+ *total_flags &= ~FIF_PROMISC_IN_BSS;
+ }
+ }
There's no need for that, is there? The monitor_recalc() is supposed
to deal with this.
Right, but we may not want to create any inconsistencies between *total_flags and actual
filters enabled in the driver?.
Thanks for the review.
Vasanth
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
