| Message ID | 1425030606-21933-1-git-send-email-vthiagar@qti.qualcomm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
> > >> 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@qti.qualcomm.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 March 2015 at 11:17, Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com> wrote: [...] >>> +{ >>> + 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. No need. With the way ath10k_monitor_recalc() is called it makes sense to rely on lockdep_assert_held(ar->conf_mutex) only. The data_lock was just FYI. >>> @@ -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?. See the "DOC: Frame filtering" in include/net/mac80211.h. ath10k either always delivers frames IN_BSS (AP operation) or can be forced to (via monitor vdev for STA/IBSS operation) so FIF_PROMISC_IN_BSS should never be cleared. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >>>> @@ -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?. > > See the "DOC: Frame filtering" in include/net/mac80211.h. ath10k > either always delivers frames IN_BSS (AP operation) or can be forced > to (via monitor vdev for STA/IBSS operation) so FIF_PROMISC_IN_BSS > should never be cleared. Your are right. We should not be clearing that flag as long as we support that functionality in one or other way. Thanks. Vasanth -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 0f39af7..3b5aaa3 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -872,6 +872,39 @@ static void ath10k_recalc_radar_detection(struct ath10k *ar) } } +static int ath10k_vdev_stop(struct ath10k_vif *arvif) +{ + struct ath10k *ar = arvif->ar; + int ret; + + lockdep_assert_held(&ar->conf_mutex); + + reinit_completion(&ar->vdev_setup_done); + + ret = ath10k_wmi_vdev_stop(ar, arvif->vdev_id); + if (ret) { + ath10k_warn(ar, "failed to stop WMI vdev %i: %d\n", + arvif->vdev_id, ret); + return ret; + } + + ret = ath10k_vdev_setup_sync(ar); + if (ret) { + ath10k_warn(ar, "failed to syncronise setup for vdev %i: %d\n", + arvif->vdev_id, ret); + return ret; + } + + WARN_ON(ar->num_started_vdevs == 0); + + if (ar->num_started_vdevs != 0) { + ar->num_started_vdevs--; + ath10k_recalc_radar_detection(ar); + } + + return ret; +} + static int ath10k_vdev_start_restart(struct ath10k_vif *arvif, bool restart) { struct ath10k *ar = arvif->ar; @@ -949,39 +982,6 @@ static int ath10k_vdev_restart(struct ath10k_vif *arvif) return ath10k_vdev_start_restart(arvif, true); } -static int ath10k_vdev_stop(struct ath10k_vif *arvif) -{ - struct ath10k *ar = arvif->ar; - int ret; - - lockdep_assert_held(&ar->conf_mutex); - - reinit_completion(&ar->vdev_setup_done); - - ret = ath10k_wmi_vdev_stop(ar, arvif->vdev_id); - if (ret) { - ath10k_warn(ar, "failed to stop WMI vdev %i: %d\n", - arvif->vdev_id, ret); - return ret; - } - - ret = ath10k_vdev_setup_sync(ar); - if (ret) { - ath10k_warn(ar, "failed to synchronize setup for vdev %i stop: %d\n", - arvif->vdev_id, ret); - return ret; - } - - WARN_ON(ar->num_started_vdevs == 0); - - if (ar->num_started_vdevs != 0) { - ar->num_started_vdevs--; - ath10k_recalc_radar_detection(ar); - } - - return ret; -} - static int ath10k_mac_setup_bcn_p2p_ie(struct ath10k_vif *arvif, struct sk_buff *bcn) {
This patches does not modify any functionality. Just a code move so that ath10k_vdev_stop() can be used in ath10k_vdev_start_restart() for any failure cases which involves vdev_stop(). Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com> --- V2: - Call ath10k_vdev_stop() when ath10k_monitor_recalc() failed. This results in code move done in patch 1. V3: - Make change specific to AP mode and update the commit log. drivers/net/wireless/ath/ath10k/mac.c | 66 ++++++++++++++++----------------- 1 file changed, 33 insertions(+), 33 deletions(-)