Skip to contentSkip to conversation

ath11k: Fix no data captured in monitor co-exist mode

PATCH In review Jouni Malinen 3 participants 13 messages Jul 21, 2021 – Sep 24, 2021 Qualcomm Atheros ATH11K wireless driver

lore
Share & tools
lore

Jouni Malinen

author
#0 lore raw

cover ath11k: Fix no data captured in monitor co-exist mode

From: Seevalamuthu Mariappan <[email protected]>

In AP+monitor coexistance, monitor interface is capturing only local traffic. This patchset changes monitor mode approach to address monitor mode issues.

Seevalamuthu Mariappan (3):
ath11k: move static function ath11k_mac_vdev_setup_sync to top
ath11k: add separate APIs for monitor mode
ath11k: monitor mode clean up to use separate APIs

 drivers/net/wireless/ath/ath11k/core.h  |  10 +-
 drivers/net/wireless/ath/ath11k/dp_rx.c |   2 +-
 drivers/net/wireless/ath/ath11k/dp_tx.c |   9 +-
 drivers/net/wireless/ath/ath11k/mac.c   | 429 ++++++++++++++++++++++++++++----
 4 files changed, 387 insertions(+), 63 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/3 ath11k: move static function ath11k_mac_vdev_setup_sync to top

From: Seevalamuthu Mariappan <[email protected]>

This is to prepare for monitor mode clean up.
No functional changes are done.

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/mac.c | 28 +++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/net/wireless/ath/ath11k/mac.c +14 -14
index e8da4af82221..3fd9a79801cb 100644
@@ -731,6 +731,20 @@ static int ath11k_monitor_vdev_up(struct ath11k *ar, int vdev_id)
 	return 0;
 }
 
+static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar)
+{
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (test_bit(ATH11K_FLAG_CRASH_FLUSH, &ar->ab->dev_flags))
+		return -ESHUTDOWN;
+
+	if (!wait_for_completion_timeout(&ar->vdev_setup_done,
+					 ATH11K_VDEV_SETUP_TIMEOUT_HZ))
+		return -ETIMEDOUT;
+
+	return ar->last_wmi_vdev_start_status ? -EINVAL : 0;
+}
+
 static int ath11k_mac_op_config(struct ieee80211_hw *hw, u32 changed)
 {
 	/* mac80211 requires this op to be present and that's why

@@ -5165,20 +5179,6 @@ static void ath11k_mac_op_remove_chanctx(struct ieee80211_hw *hw,
 	mutex_unlock(&ar->conf_mutex);
 }
 
-static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar)
-{
-	lockdep_assert_held(&ar->conf_mutex);
-
-	if (test_bit(ATH11K_FLAG_CRASH_FLUSH, &ar->ab->dev_flags))
-		return -ESHUTDOWN;
-
-	if (!wait_for_completion_timeout(&ar->vdev_setup_done,
-					 ATH11K_VDEV_SETUP_TIMEOUT_HZ))
-		return -ETIMEDOUT;
-
-	return ar->last_wmi_vdev_start_status ? -EINVAL : 0;
-}
-
 static int
 ath11k_mac_vdev_start_restart(struct ath11k_vif *arvif,
 			      const struct cfg80211_chan_def *chandef,
Signature · 7 lines
2.25.1


-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
#2 lore raw

Re: [PATCH 1/3] ath11k: move static function ath11k_mac_vdev_setup_sync to top

Quoted 10 lines · Jouni Malinen

This is to prepare for monitor mode clean up.
No functional changes are done.

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

3 patches applied to ath-next branch of ath.git, thanks.

d37b4862312c ath11k: move static function ath11k_mac_vdev_setup_sync to top 64e06b78a927 ath11k: add separate APIs for monitor mode 689a5e6fff75 ath11k: monitor mode clean up to use separate APIs

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#3 lore raw

Re: [PATCH 1/3] ath11k: move static function ath11k_mac_vdev_setup_sync to top

In reply to Jouni Malinen
Quoted 10 lines · Jouni Malinen

This is to prepare for monitor mode clean up.
No functional changes are done.

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

3 patches applied to ath-next branch of ath.git, thanks.

d37b4862312c ath11k: move static function ath11k_mac_vdev_setup_sync to top 64e06b78a927 ath11k: add separate APIs for monitor mode 689a5e6fff75 ath11k: monitor mode clean up to use separate APIs

Signature · 9 lines
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
author
#4 lore raw

PATCH 2/3 ath11k: add separate APIs for monitor mode

In reply to Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

Add separate APIs for monitor_vdev_create/monitor_vdev_delete and monitor_vdev_start/monitor_vdev_stop.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.h |   5 +-
 drivers/net/wireless/ath/ath11k/mac.c  | 313 ++++++++++++++++++++++++-
 2 files changed, 312 insertions(+), 6 deletions(-)
drivers/net/wireless/ath/ath11k/core.h +4 -1
index 6a6cabdd3e30..3cddab695031 100644
@@ -488,7 +488,8 @@ struct ath11k {
 	u32 chan_tx_pwr;
 	u32 num_stations;
 	u32 max_num_stations;
-	bool monitor_present;
+	bool monitor_conf_enabled;
+	bool monitor_started;
 	/* To synchronize concurrent synchronous mac80211 callback operations,
 	 * concurrent debugfs configuration and concurrent FW statistics events.
 	 */

@@ -563,6 +564,7 @@ struct ath11k {
 	struct ath11k_per_peer_tx_stats cached_stats;
 	u32 last_ppdu_id;
 	u32 cached_ppdu_id;
+	int monitor_vdev_id;
 #ifdef CONFIG_ATH11K_DEBUGFS
 	struct ath11k_debug debug;
 #endif

@@ -571,6 +573,7 @@ struct ath11k {
 #endif
 	bool dfs_block_radar_events;
 	struct ath11k_thermal thermal;
+	bool monitor_vdev_created;
 };
 
 struct ath11k_band_cap {
drivers/net/wireless/ath/ath11k/mac.c +308 -5

328 lines not on this page · raw message

Signature · 1 line
2.25.1
#5 lore raw

Re: [PATCH 2/3] ath11k: add separate APIs for monitor mode

Quoted 130 lines · Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

Add separate APIs for monitor_vdev_create/monitor_vdev_delete and monitor_vdev_start/monitor_vdev_stop.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.h |   5 +-
 drivers/net/wireless/ath/ath11k/mac.c  | 313 ++++++++++++++++++++++++-
 2 files changed, 312 insertions(+), 6 deletions(-)
drivers/net/wireless/ath/ath11k/core.h +4 -1
index 6a6cabdd3e30..3cddab695031 100644
@@ -488,7 +488,8 @@ struct ath11k {
 	u32 chan_tx_pwr;
 	u32 num_stations;
 	u32 max_num_stations;
-	bool monitor_present;
+	bool monitor_conf_enabled;
+	bool monitor_started;
 	/* To synchronize concurrent synchronous mac80211 callback operations,
 	 * concurrent debugfs configuration and concurrent FW statistics events.
 	 */

@@ -563,6 +564,7 @@ struct ath11k {
 	struct ath11k_per_peer_tx_stats cached_stats;
 	u32 last_ppdu_id;
 	u32 cached_ppdu_id;
+	int monitor_vdev_id;
 #ifdef CONFIG_ATH11K_DEBUGFS
 	struct ath11k_debug debug;
 #endif

@@ -571,6 +573,7 @@ struct ath11k {
 #endif
 	bool dfs_block_radar_events;
 	struct ath11k_thermal thermal;
+	bool monitor_vdev_created;
 };
 
 struct ath11k_band_cap {
drivers/net/wireless/ath/ath11k/mac.c +66 -5
index 3fd9a79801cb..e446817ac8b0 100644
@@ -745,14 +745,314 @@ static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar)
 	return ar->last_wmi_vdev_start_status ? -EINVAL : 0;
 }
 
-static int ath11k_mac_op_config(struct ieee80211_hw *hw, u32 changed)
+static void
+ath11k_mac_get_any_chandef_iter(struct ieee80211_hw *hw,
+				struct ieee80211_chanctx_conf *conf,
+				void *data)
 {
-	/* mac80211 requires this op to be present and that's why
-	 * there's an empty function, this can be extended when
-	 * required.
-	 */
+	struct cfg80211_chan_def **def = data;
+
+	*def = &conf->def;
+}
+
+static int ath11k_mac_monitor_vdev_start(struct ath11k *ar, int vdev_id,
+					 struct cfg80211_chan_def *chandef)
+{
+	struct ieee80211_channel *channel;
+	struct wmi_vdev_start_req_arg arg = {};
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	channel = chandef->chan;
+
+	arg.vdev_id = vdev_id;
+	arg.channel.freq = channel->center_freq;
+	arg.channel.band_center_freq1 = chandef->center_freq1;
+	arg.channel.band_center_freq2 = chandef->center_freq2;
+
+	arg.channel.mode = ath11k_phymodes[chandef->chan->band][chandef->width];
+	arg.channel.chan_radar =
+			!!(channel->flags & IEEE80211_CHAN_RADAR);
+
+	arg.channel.min_power = 0;
+	arg.channel.max_power = channel->max_power * 2;
+	arg.channel.max_reg_power = channel->max_reg_power * 2;
+	arg.channel.max_antenna_gain = channel->max_antenna_gain * 2;
+
+	arg.pref_tx_streams = ar->num_tx_chains;
+	arg.pref_rx_streams = ar->num_rx_chains;
+
+	arg.channel.passive = !!(chandef->chan->flags & IEEE80211_CHAN_NO_IR);
+
+	reinit_completion(&ar->vdev_setup_done);
+	reinit_completion(&ar->vdev_delete_done);
 
+	ret = ath11k_wmi_vdev_start(ar, &arg, false);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to request monitor vdev %i start: %d\n",
+			    vdev_id, ret);
+		return ret;
+	}
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to synchronize setup for monitor vdev %i start: %d\n",
+			    vdev_id, ret);
+		return ret;
+	}
+	ret = ath11k_wmi_vdev_up(ar, vdev_id, 0, ar->mac_addr);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to put up monitor vdev %i: %d\n",
+			    vdev_id, ret);
+		goto vdev_stop;
+	}
+	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i started\n",
+		   vdev_id);
 	return 0;
+
+vdev_stop:
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, vdev_id);

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 826

Quoted 10 lines
drivers/net/wireless/ath/ath11k/mac.c +10 -0
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start failure: %d\n",
+			    vdev_id, ret);
+		return ret;
+	}
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop: %d\n",
+			    vdev_id, ret);

I added return ret here for consistency..

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 827

Quoted 1 line
drivers/net/wireless/ath/ath11k/mac.c +1 -0
+	return ret;

I don't thinks this is right, in an error path (vdev_stop label) we return 0? I changed this to -EIO.

Quoted 17 lines
drivers/net/wireless/ath/ath11k/mac.c +17 -0
+static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)
+{
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop: %d\n",
+			    ar->monitor_vdev_id, ret);
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop: %d\n",
+			    ar->monitor_vdev_id, ret);

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 856

Quoted 10 lines
drivers/net/wireless/ath/ath11k/mac.c +10 -0
+
+	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",
+			    ar->monitor_vdev_id, ret);
+
+	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",
+		   ar->monitor_vdev_id);
+	return ret;
+}

I was not sure what's the idea of error path handling here, we print warnings but still continue the normal execution. I changed this so that we bail out in the first error and if everything goes well we return 0.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#6 lore raw

Re: [PATCH 2/3] ath11k: add separate APIs for monitor mode

Quoted 46 lines · Kalle Valo
Quoted 4 lines
drivers/net/wireless/ath/ath11k/mac.c +4 -0
+vdev_stop:
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, vdev_id);

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 826

Quoted 10 lines
drivers/net/wireless/ath/ath11k/mac.c +10 -0
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start failure: %d\n",
+			    vdev_id, ret);
+		return ret;
+	}
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop: %d\n",
+			    vdev_id, ret);

I added return ret here for consistency..

Review

Quoted 1 line
(quoted diff) +1 -0
+	return ret;

I don't thinks this is right, in an error path (vdev_stop label) we return 0? I changed this to -EIO.

Quoted 24 lines
drivers/net/wireless/ath/ath11k/mac.c +16 -0
+static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)
+{
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop: %d\n",
+			    ar->monitor_vdev_id, ret);
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+ ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop:

%d\n",

drivers/net/wireless/ath/ath11k/mac.c +7 -0
+			    ar->monitor_vdev_id, ret);
+
+	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",
+			    ar->monitor_vdev_id, ret);
+
Quoted 8 lines

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 856

Quoted 4 lines
drivers/net/wireless/ath/ath11k/mac.c +4 -0
+	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",
+		   ar->monitor_vdev_id);
+	return ret;
+}

I was not sure what's the idea of error path handling here, we print warnings but still continue the normal execution. I changed this so that we bail out in the first error and if everything goes well we return 0.

I found quite a few missing error checks, too many to list here but fixed in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=8b2f8d11422e7909ff02db456cda41728f621de4

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5ff318be206b3d2a0bfdcfaf0ac52cc3b4ecdeae

Please double check, compile tested only.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#7 lore raw

Re: [PATCH 2/3] ath11k: add separate APIs for monitor mode

Quoted 60 lines · Kalle Valo
Quoted 49 lines · Kalle Valo
Quoted 15 lines
drivers/net/wireless/ath/ath11k/mac.c +6 -0
+vdev_stop:
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, vdev_id);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start

failure: %d\n",

drivers/net/wireless/ath/ath11k/mac.c +7 -0
+			    vdev_id, ret);
+		return ret;
+	}
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop:

%d\n",

Review

Quoted 1 line
(quoted diff) +1 -0
+			    vdev_id, ret);

I added return ret here for consistency..

Review

Quoted 1 line
(quoted diff) +1 -0
+	return ret;

I don't thinks this is right, in an error path (vdev_stop label) we return 0? I changed this to -EIO.

Quoted 25 lines
drivers/net/wireless/ath/ath11k/mac.c +11 -0
+static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)
+{
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop:

%d\n",

drivers/net/wireless/ath/ath11k/mac.c +5 -0
+			    ar->monitor_vdev_id, ret);
+
+	ret = ath11k_mac_vdev_setup_sync(ar);
+	if (ret)
+ ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop:

%d\n",

drivers/net/wireless/ath/ath11k/mac.c +7 -0
+			    ar->monitor_vdev_id, ret);
+
+	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",
+			    ar->monitor_vdev_id, ret);
+
Quoted 8 lines

drivers/net/wireless/ath/ath11k/mac.c static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar) · line 856

Quoted 4 lines
drivers/net/wireless/ath/ath11k/mac.c +4 -0
+	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",
+		   ar->monitor_vdev_id);
+	return ret;
+}

I was not sure what's the idea of error path handling here, we print warnings but still continue the normal execution. I changed this so that we bail out in the first error and if everything goes well we return 0.

Quoted 8 lines

I found quite a few missing error checks, too many to list here but fixed in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=8b2f8d11422e7909ff02db456cda41728f621de4

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5ff318be206b3d2a0bfdcfaf0ac52cc3b4ecdeae

Please double check, compile tested only.

Thanks for the fixes Kalle. It looks fine and tested the same.

Regards,
Seevalamuthu M

Signature · 3 lines
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
author
#8 lore raw

PATCH 3/3 ath11k: monitor mode clean up to use separate APIs

In reply to Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

If monitor interface is enabled in co-exist mode, only local traffic are captured. It's caused by missing monitor vdev in co-exist mode. So, monitor mode clean up is done with separate Monitor APIs. For this, introduce monitor_started and monitor_vdev_created boolean flags.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.h  |   5 --
 drivers/net/wireless/ath/ath11k/dp_rx.c |   2 +-
 drivers/net/wireless/ath/ath11k/dp_tx.c |   9 +-
 drivers/net/wireless/ath/ath11k/mac.c   | 112 ++++++++++++++----------
 4 files changed, 73 insertions(+), 55 deletions(-)
drivers/net/wireless/ath/ath11k/core.h +0 -5
index 3cddab695031..0ad5a935b52b 100644
@@ -192,10 +192,6 @@ enum ath11k_dev_flags {
 	ATH11K_FLAG_HTC_SUSPEND_COMPLETE,
 };
 
-enum ath11k_monitor_flags {
-	ATH11K_FLAG_MONITOR_ENABLED,
-};
-
 struct ath11k_vif {
 	u32 vdev_id;
 	enum wmi_vdev_type vdev_type;

@@ -478,7 +474,6 @@ struct ath11k {
 
 	unsigned long dev_flags;
 	unsigned int filter_flags;
-	unsigned long monitor_flags;
 	u32 min_tx_power;
 	u32 max_tx_power;
 	u32 txpower_limit_2g;
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -1
index 9a224817630a..6fde70914e1a 100644
@@ -5029,7 +5029,7 @@ int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id,
 	struct ath11k *ar = ath11k_ab_to_ar(ab, mac_id);
 	int ret = 0;
 
-	if (test_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags))
+	if (ar->monitor_started)
 		ret = ath11k_dp_mon_process_rx(ab, mac_id, napi, budget);
 	else
 		ret = ath11k_dp_rx_process_mon_status(ab, mac_id, napi, budget);
drivers/net/wireless/ath/ath11k/dp_tx.c +7 -2
index 3acdd4050d5b..253d0564f923 100644
@@ -1076,11 +1076,16 @@ int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset)
 
 	for (i = 0; i < ab->hw_params.num_rxmda_per_pdev; i++) {
 		ring_id = dp->rx_mon_status_refill_ring[i].refill_buf_ring.ring_id;
-		if (!reset)
+		if (!reset) {
 			tlv_filter.rx_filter =
 					HTT_RX_MON_FILTER_TLV_FLAGS_MON_STATUS_RING;
-		else
+		} else {
 			tlv_filter = ath11k_mac_mon_status_filter_default;
+#ifdef CONFIG_ATH11K_DEBUGFS
+			if (ar->debug.extd_rx_stats)
+				tlv_filter.rx_filter = ar->debug.rx_filter;
+#endif
+		}
 
 		ret = ath11k_dp_tx_htt_rx_filter_setup(ab, ring_id,
 						       dp->mac_id + i,
drivers/net/wireless/ath/ath11k/mac.c +65 -47

241 lines not on this page · raw message

Signature · 7 lines
2.25.1


-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
#9 lore raw

Re: [PATCH 3/3] ath11k: monitor mode clean up to use separate APIs

Quoted 8 lines · Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

If monitor interface is enabled in co-exist mode, only local traffic are captured. It's caused by missing monitor vdev in co-exist mode. So, monitor mode clean up is done with separate Monitor APIs. For this, introduce monitor_started and monitor_vdev_created boolean flags.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Seevalamuthu, in upstream IPQ8074 doesn't even support monitor mode:

static const struct ath11k_hw_params ath11k_hw_params[] = {
	{
		.hw_rev = ATH11K_HW_IPQ8074,
		.name = "ipq8074 hw2.0",
...
		.interface_modes = BIT(NL80211_IFTYPE_STATION) |
					BIT(NL80211_IFTYPE_AP) |
					BIT(NL80211_IFTYPE_MESH_POINT),

So I wonder how did you test this? Is this something which is only tested on ancient QSDK kernels and not with upstream kernels?

Signature · 8 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
#10 lore raw

Re: [PATCH 3/3] ath11k: monitor mode clean up to use separate APIs

Quoted 22 lines · Kalle Valo
Quoted 8 lines · Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

If monitor interface is enabled in co-exist mode, only local traffic are captured. It's caused by missing monitor vdev in co-exist mode. So, monitor mode clean up is done with separate Monitor APIs. For this, introduce monitor_started and monitor_vdev_created boolean flags.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Seevalamuthu, in upstream IPQ8074 doesn't even support monitor mode:

static const struct ath11k_hw_params ath11k_hw_params[] = {
	{
		.hw_rev = ATH11K_HW_IPQ8074,
		.name = "ipq8074 hw2.0",
...
		.interface_modes = BIT(NL80211_IFTYPE_STATION) |
					BIT(NL80211_IFTYPE_AP) |
					BIT(NL80211_IFTYPE_MESH_POINT),
Quoted 2 lines

So I wonder how did you test this? Is this something which is only tested on ancient QSDK kernels and not with upstream kernels?

Actually, ignore that. I forgot that there was a separate boolean for the monitor mode:

.supports_monitor = true,

Sorry for the noise.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#11 lore raw

Re: [PATCH 3/3] ath11k: monitor mode clean up to use separate APIs

In reply to Jouni Malinen
Quoted 50 lines · Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

If monitor interface is enabled in co-exist mode, only local traffic are captured. It's caused by missing monitor vdev in co-exist mode. So, monitor mode clean up is done with separate Monitor APIs. For this, introduce monitor_started and monitor_vdev_created boolean flags.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.h  |   5 --
 drivers/net/wireless/ath/ath11k/dp_rx.c |   2 +-
 drivers/net/wireless/ath/ath11k/dp_tx.c |   9 +-
 drivers/net/wireless/ath/ath11k/mac.c   | 112 ++++++++++++++----------
 4 files changed, 73 insertions(+), 55 deletions(-)
drivers/net/wireless/ath/ath11k/core.h +0 -5
index 3cddab695031..0ad5a935b52b 100644
@@ -192,10 +192,6 @@ enum ath11k_dev_flags {
 	ATH11K_FLAG_HTC_SUSPEND_COMPLETE,
 };
 
-enum ath11k_monitor_flags {
-	ATH11K_FLAG_MONITOR_ENABLED,
-};
-
 struct ath11k_vif {
 	u32 vdev_id;
 	enum wmi_vdev_type vdev_type;

@@ -478,7 +474,6 @@ struct ath11k {
 
 	unsigned long dev_flags;
 	unsigned int filter_flags;
-	unsigned long monitor_flags;
 	u32 min_tx_power;
 	u32 max_tx_power;
 	u32 txpower_limit_2g;
drivers/net/wireless/ath/ath11k/dp_rx.c +0 -0
index 9a224817630a..6fde70914e1a 100644

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id, · line 5035

Quoted 8 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -1
@@ -5029,7 +5029,7 @@ int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id,
 	struct ath11k *ar = ath11k_ab_to_ar(ab, mac_id);
 	int ret = 0;
 
-	if (test_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags))
+	if (ar->monitor_started)
 		ret = ath11k_dp_mon_process_rx(ab, mac_id, napi, budget);
 	else
 		ret = ath11k_dp_rx_process_mon_status(ab, mac_id, napi, budget);

Moving from test_bit() to a boolean looks racy to me, I don't see how monitor_started is serialised.

And why move away from monitor_flags and having separate booleans anyway? I would monitor_conf_enabled and monitor_started from patch 2 to use monitor_flags.

Quoted 5 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +0 -1
@@ -1076,11 +1076,16 @@ int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset)
 
 	for (i = 0; i < ab->hw_params.num_rxmda_per_pdev; i++) {
 		ring_id = dp->rx_mon_status_refill_ring[i].refill_buf_ring.ring_id;
-		if (!reset)

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset) · line 1087

Quoted 10 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +6 -1
+		if (!reset) {
 			tlv_filter.rx_filter =
 					HTT_RX_MON_FILTER_TLV_FLAGS_MON_STATUS_RING;
-		else
+		} else {
 			tlv_filter = ath11k_mac_mon_status_filter_default;
+#ifdef CONFIG_ATH11K_DEBUGFS
+			if (ar->debug.extd_rx_stats)
+				tlv_filter.rx_filter = ar->debug.rx_filter;
+#endif

This should use ath11k_debugfs_is_extd_rx_stats_enabled and ath11k_debugfs_rx_filter(), then the ifdef is not needed.

Signature · 8 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
#12 lore raw

Re: [PATCH 3/3] ath11k: monitor mode clean up to use separate APIs

Quoted 64 lines · Kalle Valo
Quoted 50 lines · Jouni Malinen

From: Seevalamuthu Mariappan <[email protected]>

If monitor interface is enabled in co-exist mode, only local traffic are captured. It's caused by missing monitor vdev in co-exist mode. So, monitor mode clean up is done with separate Monitor APIs. For this, introduce monitor_started and monitor_vdev_created boolean flags.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Co-developed-by Miles Hu <[email protected]>
Signed-off-by Miles Hu <[email protected]>
Co-developed-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by Seevalamuthu Mariappan <[email protected]>
Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.h  |   5 --
 drivers/net/wireless/ath/ath11k/dp_rx.c |   2 +-
 drivers/net/wireless/ath/ath11k/dp_tx.c |   9 +-
 drivers/net/wireless/ath/ath11k/mac.c   | 112 ++++++++++++++----------
 4 files changed, 73 insertions(+), 55 deletions(-)
drivers/net/wireless/ath/ath11k/core.h +0 -5
index 3cddab695031..0ad5a935b52b 100644
@@ -192,10 +192,6 @@ enum ath11k_dev_flags {
 	ATH11K_FLAG_HTC_SUSPEND_COMPLETE,
 };
 
-enum ath11k_monitor_flags {
-	ATH11K_FLAG_MONITOR_ENABLED,
-};
-
 struct ath11k_vif {
 	u32 vdev_id;
 	enum wmi_vdev_type vdev_type;

@@ -478,7 +474,6 @@ struct ath11k {
 
 	unsigned long dev_flags;
 	unsigned int filter_flags;
-	unsigned long monitor_flags;
 	u32 min_tx_power;
 	u32 max_tx_power;
 	u32 txpower_limit_2g;
drivers/net/wireless/ath/ath11k/dp_rx.c +0 -0
index 9a224817630a..6fde70914e1a 100644

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id, · line 5035

Quoted 8 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -1
@@ -5029,7 +5029,7 @@ int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id,
 	struct ath11k *ar = ath11k_ab_to_ar(ab, mac_id);
 	int ret = 0;
 
-	if (test_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags))
+	if (ar->monitor_started)
 		ret = ath11k_dp_mon_process_rx(ab, mac_id, napi, budget);
 	else
 		ret = ath11k_dp_rx_process_mon_status(ab, mac_id, napi, budget);

Moving from test_bit() to a boolean looks racy to me, I don't see how monitor_started is serialised.

Quoted 3 lines

And why move away from monitor_flags and having separate booleans anyway? I would monitor_conf_enabled and monitor_started from patch 2 to use monitor_flags.

In the pending branch I changed back to monitor_flags.

Quoted 18 lines
Quoted 5 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +0 -1
@@ -1076,11 +1076,16 @@ int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset)
 
 	for (i = 0; i < ab->hw_params.num_rxmda_per_pdev; i++) {
 		ring_id = dp->rx_mon_status_refill_ring[i].refill_buf_ring.ring_id;
-		if (!reset)

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset) · line 1087

Quoted 10 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +6 -1
+		if (!reset) {
 			tlv_filter.rx_filter =
 					HTT_RX_MON_FILTER_TLV_FLAGS_MON_STATUS_RING;
-		else
+		} else {
 			tlv_filter = ath11k_mac_mon_status_filter_default;
+#ifdef CONFIG_ATH11K_DEBUGFS
+			if (ar->debug.extd_rx_stats)
+				tlv_filter.rx_filter = ar->debug.rx_filter;
+#endif

This should use ath11k_debugfs_is_extd_rx_stats_enabled and ath11k_debugfs_rx_filter(), then the ifdef is not needed.

I also fixed this in the pending branch.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches