[PATCH V3 1/2] ath10k: Move ath10k_vdev_stop() up before ath10k_vdev_start_restart()

[PATCH V3 1/2] ath10k: Move ath10k_vdev_stop() up before ath10k_vdev_start_restart()

[Vasanthakumar Thiagarajan]

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(-)

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)
 {
-- 
1.7.9.5

[PATCH V3 1/2] ath10k: Move ath10k_vdev_stop() up before ath10k_vdev_start_restart()

[Vasanthakumar Thiagarajan]

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(-)

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)
 {
-- 
1.7.9.5


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

Promiscuous mode is enabled when wlan interface is added to
bridge. ath10k creates a monitor mode when promiscuous mode
is enabled. When monitor vdev is runing along with other
vdev(s) there is a huge number of interrupts generated
especially in noisy condition. Fix this by not enabling
promiscuous(monitor) mode when already a vdev is running.
As disabling promiscuous mode may have issues with 4-address
bridging in STA mode, the change is done specific to non-sta/ibss
mode types. This does not change the support of virtual interface of
type monitor along with other vdevs of any type.

This could fix manangement frame drop in fw due to unavailable
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)
+{
+	struct ath10k_vif *arvif;
+
+	if (!ar->num_started_vdevs)
+		return false;
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		/* Disabling promiscuous mode when STA/IBSS is running */
+		if (arvif->vdev_type == WMI_VDEV_TYPE_STA ||
+		    arvif->vdev_type == WMI_VDEV_TYPE_IBSS)
+			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");
+	}
+
 	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);
+
 	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;
+		}
+	}
 	ar->filter_flags = *total_flags;
 
 	ret = ath10k_monitor_recalc(ar);
-- 
1.7.9.5

[PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

Promiscuous mode is enabled when wlan interface is added to
bridge. ath10k creates a monitor mode when promiscuous mode
is enabled. When monitor vdev is runing along with other
vdev(s) there is a huge number of interrupts generated
especially in noisy condition. Fix this by not enabling
promiscuous(monitor) mode when already a vdev is running.
As disabling promiscuous mode may have issues with 4-address
bridging in STA mode, the change is done specific to non-sta/ibss
mode types. This does not change the support of virtual interface of
type monitor along with other vdevs of any type.

This could fix manangement frame drop in fw due to unavailable
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)
+{
+	struct ath10k_vif *arvif;
+
+	if (!ar->num_started_vdevs)
+		return false;
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		/* Disabling promiscuous mode when STA/IBSS is running */
+		if (arvif->vdev_type == WMI_VDEV_TYPE_STA ||
+		    arvif->vdev_type == WMI_VDEV_TYPE_IBSS)
+			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");
+	}
+
 	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);
+
 	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;
+		}
+	}
 	ar->filter_flags = *total_flags;
 
 	ret = ath10k_monitor_recalc(ar);
-- 
1.7.9.5


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Michal Kazior]

On 27 February 2015 at 10:50, Vasanthakumar Thiagarajan
<vthiagar@qti.qualcomm.com> wrote:
> Promiscuous mode is enabled when wlan interface is added to
> bridge. ath10k creates a monitor mode when promiscuous mode
> is enabled. When monitor vdev is runing along with other

s/runing/running/


> vdev(s) there is a huge number of interrupts generated
> especially in noisy condition. Fix this by not enabling
> promiscuous(monitor) mode when already a vdev is running.
> As disabling promiscuous mode may have issues with 4-address
> bridging in STA mode, the change is done specific to non-sta/ibss
> mode types. This does not change the support of virtual interface of
> type monitor along with other vdevs of any type.
>
> This could fix manangement frame drop in fw due to unavailable

s/manangement/management/


> 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).


> +{
> +       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).


> +               /* 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?


> +                       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().


>         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.


> +
>         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.


Michał

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Michal Kazior]

On 27 February 2015 at 10:50, Vasanthakumar Thiagarajan
<vthiagar@qti.qualcomm.com> wrote:
> Promiscuous mode is enabled when wlan interface is added to
> bridge. ath10k creates a monitor mode when promiscuous mode
> is enabled. When monitor vdev is runing along with other

s/runing/running/


> vdev(s) there is a huge number of interrupts generated
> especially in noisy condition. Fix this by not enabling
> promiscuous(monitor) mode when already a vdev is running.
> As disabling promiscuous mode may have issues with 4-address
> bridging in STA mode, the change is done specific to non-sta/ibss
> mode types. This does not change the support of virtual interface of
> type monitor along with other vdevs of any type.
>
> This could fix manangement frame drop in fw due to unavailable

s/manangement/management/


> 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).


> +{
> +       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).


> +               /* 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?


> +                       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().


>         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.


> +
>         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.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

>
>
>> 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

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

>
>
>> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Michal Kazior]

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ł

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Michal Kazior]

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ł

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

>
>
>>>> @@ -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

Re: [PATCH V3 2/2] ath10k: Fix interrupt storm

[Vasanthakumar Thiagarajan]

>
>
>>>> @@ -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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k