[PATCH 0/2] wifi: mac80211/mac80211_hwsim: support MLO CSA test case

[PATCH 0/2] wifi: mac80211/mac80211_hwsim: support MLO CSA test case

[Aditya Kumar Singh]

Recently MLO CSA is enabled on the stack. Add test infra support in
mac80211_hwsim in order to support basic MLO CSA test case.

Aditya Kumar Singh (2):
  wifi: mac80211: check beacon countdown is complete on per link basis
  wifi: mac80211_hwsim: add support for switch_vif_chanctx callback
---
 drivers/net/wireless/ath/ath10k/mac.c         |  2 +-
 drivers/net/wireless/ath/ath10k/wmi.c         |  2 +-
 drivers/net/wireless/ath/ath11k/mac.c         |  2 +-
 drivers/net/wireless/ath/ath9k/beacon.c       |  2 +-
 .../net/wireless/ath/ath9k/htc_drv_beacon.c   |  2 +-
 .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c |  2 +-
 .../wireless/intel/iwlwifi/mvm/time-event.c   |  2 +-
 drivers/net/wireless/mediatek/mt76/mac80211.c |  4 +--
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  2 +-
 drivers/net/wireless/virtual/mac80211_hwsim.c | 35 +++++++++++++++++--
 include/net/mac80211.h                        |  4 ++-
 net/mac80211/tx.c                             | 14 ++++++--
 12 files changed, 58 insertions(+), 15 deletions(-)


base-commit: 42ffccd0a36e099dea3d3272c5d62a0454ded1f0
-- 
2.25.1

[PATCH 1/2] wifi: mac80211: check beacon countdown is complete on per link basis

[Aditya Kumar Singh]

Currently, function to check if beacon countdown is complete uses deflink
to fetch the beacon and check the counter. However, with MLO, there is
a need to check the counter for the beacon in a particular link.

Add support to use link_id in order to fetch the beacon from a particular
link data.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/mac.c              |  2 +-
 drivers/net/wireless/ath/ath10k/wmi.c              |  2 +-
 drivers/net/wireless/ath/ath11k/mac.c              |  2 +-
 drivers/net/wireless/ath/ath9k/beacon.c            |  2 +-
 drivers/net/wireless/ath/ath9k/htc_drv_beacon.c    |  2 +-
 drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c  |  2 +-
 .../net/wireless/intel/iwlwifi/mvm/time-event.c    |  2 +-
 drivers/net/wireless/mediatek/mt76/mac80211.c      |  4 ++--
 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  |  2 +-
 drivers/net/wireless/virtual/mac80211_hwsim.c      |  2 +-
 include/net/mac80211.h                             |  4 +++-
 net/mac80211/tx.c                                  | 14 ++++++++++++--
 12 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 41053219ee95..e322b528baaf 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2034,7 +2034,7 @@ static void ath10k_mac_vif_ap_csa_count_down(struct ath10k_vif *arvif)
 	if (!arvif->is_up)
 		return;
 
-	if (!ieee80211_beacon_cntdwn_is_complete(vif)) {
+	if (!ieee80211_beacon_cntdwn_is_complete(vif, 0)) {
 		ieee80211_beacon_update_cntdwn(vif, 0);
 
 		ret = ath10k_mac_setup_bcn_tmpl(arvif);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index ddf15717d504..2e9661f4bea8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -3884,7 +3884,7 @@ void ath10k_wmi_event_host_swba(struct ath10k *ar, struct sk_buff *skb)
 		 * actual channel switch is done
 		 */
 		if (arvif->vif->bss_conf.csa_active &&
-		    ieee80211_beacon_cntdwn_is_complete(arvif->vif)) {
+		    ieee80211_beacon_cntdwn_is_complete(arvif->vif, 0)) {
 			ieee80211_csa_finish(arvif->vif, 0);
 			continue;
 		}
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index f7cab50bdfd1..a1e2729582e8 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -1577,7 +1577,7 @@ void ath11k_mac_bcn_tx_event(struct ath11k_vif *arvif)
 		return;
 
 	if (vif->bss_conf.color_change_active &&
-	    ieee80211_beacon_cntdwn_is_complete(vif)) {
+	    ieee80211_beacon_cntdwn_is_complete(vif, 0)) {
 		arvif->bcca_zero_sent = true;
 		ieee80211_color_change_finish(vif);
 		return;
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index 4e48407138b2..b399a7926ef5 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -365,7 +365,7 @@ bool ath9k_csa_is_finished(struct ath_softc *sc, struct ieee80211_vif *vif)
 	if (!vif || !vif->bss_conf.csa_active)
 		return false;
 
-	if (!ieee80211_beacon_cntdwn_is_complete(vif))
+	if (!ieee80211_beacon_cntdwn_is_complete(vif, 0))
 		return false;
 
 	ieee80211_csa_finish(vif, 0);
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c b/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
index 8179d35dc310..547634f82183 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
@@ -514,7 +514,7 @@ bool ath9k_htc_csa_is_finished(struct ath9k_htc_priv *priv)
 	if (!vif || !vif->bss_conf.csa_active)
 		return false;
 
-	if (!ieee80211_beacon_cntdwn_is_complete(vif))
+	if (!ieee80211_beacon_cntdwn_is_complete(vif, 0))
 		return false;
 
 	ieee80211_csa_finish(vif, 0);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
index cc592e288188..123fe9bba982 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
@@ -1466,7 +1466,7 @@ static void iwl_mvm_csa_count_down(struct iwl_mvm *mvm,
 
 	mvmvif->csa_countdown = true;
 
-	if (!ieee80211_beacon_cntdwn_is_complete(csa_vif)) {
+	if (!ieee80211_beacon_cntdwn_is_complete(csa_vif, 0)) {
 		int c = ieee80211_beacon_update_cntdwn(csa_vif, 0);
 
 		iwl_mvm_mac_ctxt_beacon_changed(mvm, csa_vif,
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c b/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c
index 89b1c7a87660..a59d264a11c5 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c
@@ -156,7 +156,7 @@ static void iwl_mvm_csa_noa_start(struct iwl_mvm *mvm)
 	 * So we just do nothing here and the switch
 	 * will be performed on the last TBTT.
 	 */
-	if (!ieee80211_beacon_cntdwn_is_complete(csa_vif)) {
+	if (!ieee80211_beacon_cntdwn_is_complete(csa_vif, 0)) {
 		IWL_WARN(mvm, "CSA NOA started too early\n");
 		goto out_unlock;
 	}
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 8bf82755ca4c..758e380fdf1d 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -1613,7 +1613,7 @@ EXPORT_SYMBOL_GPL(mt76_get_sar_power);
 static void
 __mt76_csa_finish(void *priv, u8 *mac, struct ieee80211_vif *vif)
 {
-	if (vif->bss_conf.csa_active && ieee80211_beacon_cntdwn_is_complete(vif))
+	if (vif->bss_conf.csa_active && ieee80211_beacon_cntdwn_is_complete(vif, 0))
 		ieee80211_csa_finish(vif, 0);
 }
 
@@ -1638,7 +1638,7 @@ __mt76_csa_check(void *priv, u8 *mac, struct ieee80211_vif *vif)
 	if (!vif->bss_conf.csa_active)
 		return;
 
-	dev->csa_complete |= ieee80211_beacon_cntdwn_is_complete(vif);
+	dev->csa_complete |= ieee80211_beacon_cntdwn_is_complete(vif, 0);
 }
 
 void mt76_csa_check(struct mt76_dev *dev)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 66bf92c164c3..db5041d23938 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -5739,7 +5739,7 @@ static void rtl8xxxu_update_beacon_work_callback(struct work_struct *work)
 	}
 
 	if (vif->bss_conf.csa_active) {
-		if (ieee80211_beacon_cntdwn_is_complete(vif)) {
+		if (ieee80211_beacon_cntdwn_is_complete(vif, 0)) {
 			ieee80211_csa_finish(vif, 0);
 			return;
 		}
diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index 0554946ed30e..2ea11a86d920 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -2305,7 +2305,7 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
 			rcu_dereference(link_conf->chanctx_conf)->def.chan);
 	}
 
-	if (link_conf->csa_active && ieee80211_beacon_cntdwn_is_complete(vif))
+	if (link_conf->csa_active && ieee80211_beacon_cntdwn_is_complete(vif, link_id))
 		ieee80211_csa_finish(vif, link_id);
 }
 
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index fc223761e3af..25c892ea9eb3 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5566,10 +5566,12 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif, unsigned int link_id);
 /**
  * ieee80211_beacon_cntdwn_is_complete - find out if countdown reached 1
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @link_id: valid link_id during MLO or 0 for non-MLO
  *
  * This function returns whether the countdown reached zero.
  */
-bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif);
+bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
+					 unsigned int link_id);
 
 /**
  * ieee80211_color_change_finish - notify mac80211 about color change
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f4be4af568be..6bf223e6cd1a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5095,9 +5095,11 @@ void ieee80211_beacon_set_cntdwn(struct ieee80211_vif *vif, u8 counter)
 }
 EXPORT_SYMBOL(ieee80211_beacon_set_cntdwn);
 
-bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
+bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
+					 unsigned int link_id)
 {
 	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct ieee80211_link_data *link;
 	struct beacon_data *beacon = NULL;
 	u8 *beacon_data;
 	size_t beacon_data_len;
@@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
 	if (!ieee80211_sdata_running(sdata))
 		return false;
 
+	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
+		return 0;
+
 	rcu_read_lock();
+
+	link = rcu_dereference(sdata->link[link_id]);
+	if (!link)
+		goto out;
+
 	if (vif->type == NL80211_IFTYPE_AP) {
-		beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
+		beacon = rcu_dereference(link->u.ap.beacon);
 		if (WARN_ON(!beacon || !beacon->tail))
 			goto out;
 		beacon_data = beacon->tail;
-- 
2.25.1

[PATCH 2/2] wifi: mac80211_hwsim: add support for switch_vif_chanctx callback

[Aditya Kumar Singh]

Currently switch_vif_chanctx mac80211 callback is not supported for
MLO. Add it.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 drivers/net/wireless/virtual/mac80211_hwsim.c | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index 2ea11a86d920..ccc321898d6f 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -3215,6 +3215,36 @@ static void mac80211_hwsim_unassign_vif_chanctx(struct ieee80211_hw *hw,
 	}
 }
 
+static int mac80211_hwsim_switch_vif_chanctx(struct ieee80211_hw *hw,
+					     struct ieee80211_vif_chanctx_switch *vifs,
+					     int n_vifs,
+					     enum ieee80211_chanctx_switch_mode mode)
+{
+	int i;
+
+	if (n_vifs <= 0)
+		return -EINVAL;
+
+	wiphy_dbg(hw->wiphy,
+		  "switch vif channel context mode: %u\n", mode);
+
+	for (i = 0; i < n_vifs; i++) {
+		hwsim_check_chanctx_magic(vifs[i].old_ctx);
+		wiphy_dbg(hw->wiphy,
+			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
+			  vifs[i].old_ctx->def.chan->center_freq,
+			  vifs[i].old_ctx->def.width,
+			  vifs[i].old_ctx->def.center_freq1,
+			  vifs[i].old_ctx->def.center_freq2,
+			  vifs[i].new_ctx->def.chan->center_freq,
+			  vifs[i].new_ctx->def.width,
+			  vifs[i].new_ctx->def.center_freq1,
+			  vifs[i].new_ctx->def.center_freq2);
+		hwsim_set_chanctx_magic(vifs[i].new_ctx);
+	}
+	return 0;
+}
+
 static const char mac80211_hwsim_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"tx_pkts_nic",
 	"tx_bytes_nic",
@@ -3940,7 +3970,8 @@ static const struct ieee80211_ops mac80211_hwsim_ops = {
 	.remove_chanctx = mac80211_hwsim_remove_chanctx,	\
 	.change_chanctx = mac80211_hwsim_change_chanctx,	\
 	.assign_vif_chanctx = mac80211_hwsim_assign_vif_chanctx,\
-	.unassign_vif_chanctx = mac80211_hwsim_unassign_vif_chanctx,
+	.unassign_vif_chanctx = mac80211_hwsim_unassign_vif_chanctx, \
+	.switch_vif_chanctx = mac80211_hwsim_switch_vif_chanctx,
 
 static const struct ieee80211_ops mac80211_hwsim_mchan_ops = {
 	HWSIM_COMMON_OPS
-- 
2.25.1

Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete on per link basis

[Johannes Berg]

> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
> +					 unsigned int link_id)
>  {
>  	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> +	struct ieee80211_link_data *link;
>  	struct beacon_data *beacon = NULL;
>  	u8 *beacon_data;
>  	size_t beacon_data_len;
> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>  	if (!ieee80211_sdata_running(sdata))
>  		return false;
>  
> +	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
> +		return 0;
> +
>  	rcu_read_lock();
> +
> +	link = rcu_dereference(sdata->link[link_id]);
> +	if (!link)
> +		goto out;
> 

Maybe that should be a warning too? Not sure I see any case where the
driver can/should call it with a link that's not even there?

Oh ... and maybe it should check if the link is active? We had the
sdata_running() check before, but that doesn't mean much for MLO?

Though then again we have the check below anyway:

>  	if (vif->type == NL80211_IFTYPE_AP) {
> -		beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
> +		beacon = rcu_dereference(link->u.ap.beacon);
>  		if (WARN_ON(!beacon || !beacon->tail))
>  			goto out;
> 

So that will just be NULL if it's not active... so I guess that's fine.

johannes

Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for switch_vif_chanctx callback

[Johannes Berg]

On Thu, 2024-02-15 at 21:58 +0530, Aditya Kumar Singh wrote: 
> +static int mac80211_hwsim_switch_vif_chanctx(struct ieee80211_hw *hw,
> +					     struct ieee80211_vif_chanctx_switch *vifs,
> +					     int n_vifs,
> +					     enum ieee80211_chanctx_switch_mode mode)
> +{
> +	int i;
> +
> +	if (n_vifs <= 0)
> +		return -EINVAL;
> +
> +	wiphy_dbg(hw->wiphy,
> +		  "switch vif channel context mode: %u\n", mode);
> +
> +	for (i = 0; i < n_vifs; i++) {
> +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
> +		wiphy_dbg(hw->wiphy,
> +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
> +			  vifs[i].old_ctx->def.chan->center_freq,
> +			  vifs[i].old_ctx->def.width,
> +			  vifs[i].old_ctx->def.center_freq1,
> +			  vifs[i].old_ctx->def.center_freq2,
> +			  vifs[i].new_ctx->def.chan->center_freq,
> +			  vifs[i].new_ctx->def.width,
> +			  vifs[i].new_ctx->def.center_freq1,
> +			  vifs[i].new_ctx->def.center_freq2);
> +		hwsim_set_chanctx_magic(vifs[i].new_ctx);


hwsim_set_chanctx_magic() is only partially correct, I think, this
depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
already exist as well, so should also be hwsim_check_chanctx_magic() in
that case?

johannes

Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete on per link basis

[Aditya Kumar Singh]

On 2/16/24 01:18, Johannes Berg wrote:
> 
>> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
>> +					 unsigned int link_id)
>>   {
>>   	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> +	struct ieee80211_link_data *link;
>>   	struct beacon_data *beacon = NULL;
>>   	u8 *beacon_data;
>>   	size_t beacon_data_len;
>> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>>   	if (!ieee80211_sdata_running(sdata))
>>   		return false;
>>   
>> +	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>> +		return 0;
>> +
>>   	rcu_read_lock();
>> +
>> +	link = rcu_dereference(sdata->link[link_id]);
>> +	if (!link)
>> +		goto out;
>>
> 
> Maybe that should be a warning too? Not sure I see any case where the
> driver can/should call it with a link that's not even there?
> 

Yes even I don't see any case like that.

> Oh ... and maybe it should check if the link is active? We had the
> sdata_running() check before, but that doesn't mean much for MLO?
> 

Correct. But at least in this function I don't see much use of checking 
if link is active.

> Though then again we have the check below anyway:
> 
>>   	if (vif->type == NL80211_IFTYPE_AP) {
>> -		beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
>> +		beacon = rcu_dereference(link->u.ap.beacon);
>>   		if (WARN_ON(!beacon || !beacon->tail))
>>   			goto out;
>>
> 
> So that will just be NULL if it's not active... so I guess that's fine.
> 
Yep. That's the intention here. Ultimately beacon would be NULL if link 
is not active so eventually a WARN_ON() will trigger.

Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for switch_vif_chanctx callback

[Aditya Kumar Singh]

On 2/16/24 01:20, Johannes Berg wrote:
> On Thu, 2024-02-15 at 21:58 +0530, Aditya Kumar Singh wrote:
>> +static int mac80211_hwsim_switch_vif_chanctx(struct ieee80211_hw *hw,
>> +					     struct ieee80211_vif_chanctx_switch *vifs,
>> +					     int n_vifs,
>> +					     enum ieee80211_chanctx_switch_mode mode)
>> +{
>> +	int i;
>> +
>> +	if (n_vifs <= 0)
>> +		return -EINVAL;
>> +
>> +	wiphy_dbg(hw->wiphy,
>> +		  "switch vif channel context mode: %u\n", mode);
>> +
>> +	for (i = 0; i < n_vifs; i++) {
>> +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
>> +		wiphy_dbg(hw->wiphy,
>> +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
>> +			  vifs[i].old_ctx->def.chan->center_freq,
>> +			  vifs[i].old_ctx->def.width,
>> +			  vifs[i].old_ctx->def.center_freq1,
>> +			  vifs[i].old_ctx->def.center_freq2,
>> +			  vifs[i].new_ctx->def.chan->center_freq,
>> +			  vifs[i].new_ctx->def.width,
>> +			  vifs[i].new_ctx->def.center_freq1,
>> +			  vifs[i].new_ctx->def.center_freq2);
>> +		hwsim_set_chanctx_magic(vifs[i].new_ctx);
> 
> 
> hwsim_set_chanctx_magic() is only partially correct, I think, this
> depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
> already exist as well, so should also be hwsim_check_chanctx_magic() in
> that case?
> 

Oh yeah missed the mode dependency here. Thanks for pointing it out. So 
it should be something like -

...

/* old already exist so check magic */
hwsim_check_chanctx_magic(vifs[i].old_ctx);

if (mode == CHANCTX_SWMODE_REASSIGN_VIF) {
	/* Reassign then new would also exist, just interface is
	 * switching
	 */
	hwsim_check_chanctx_magic(vifs[i].new_ctx);
} else {
	/* SWAP_* then new context does not exist hence set magic.
	 * Also old will no longer exist so clear the magic
	 */
	hwsim_set_chanctx_magic(vifs[i].new_ctx);
	hwsim_clear_chanctx_magic(vifs[i].old_ctx);
}

...

One thing, in patch should I keep those comments or those are noisy? 
Seems noisy to me (if at least mode documentation is referred then 
things are clear already)?

Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for switch_vif_chanctx callback

[Johannes Berg]

On Fri, 2024-02-16 at 08:45 +0530, Aditya Kumar Singh wrote:
> 
> > > +	for (i = 0; i < n_vifs; i++) {
> > > +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
> > > +		wiphy_dbg(hw->wiphy,
> > > +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
> > > +			  vifs[i].old_ctx->def.chan->center_freq,
> > > +			  vifs[i].old_ctx->def.width,
> > > +			  vifs[i].old_ctx->def.center_freq1,
> > > +			  vifs[i].old_ctx->def.center_freq2,
> > > +			  vifs[i].new_ctx->def.chan->center_freq,
> > > +			  vifs[i].new_ctx->def.width,
> > > +			  vifs[i].new_ctx->def.center_freq1,
> > > +			  vifs[i].new_ctx->def.center_freq2);
> > > +		hwsim_set_chanctx_magic(vifs[i].new_ctx);
> > 
> > 
> > hwsim_set_chanctx_magic() is only partially correct, I think, this
> > depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
> > already exist as well, so should also be hwsim_check_chanctx_magic() in
> > that case?
> > 
> 
> Oh yeah missed the mode dependency here. Thanks for pointing it out. So 
> it should be something like -
> 
> ...
> 
> /* old already exist so check magic */
> hwsim_check_chanctx_magic(vifs[i].old_ctx);
> 
> if (mode == CHANCTX_SWMODE_REASSIGN_VIF) {
> 	/* Reassign then new would also exist, just interface is
> 	 * switching
> 	 */
> 	hwsim_check_chanctx_magic(vifs[i].new_ctx);
> } else {
> 	/* SWAP_* then new context does not exist hence set magic.
> 	 * Also old will no longer exist so clear the magic
> 	 */
> 	hwsim_set_chanctx_magic(vifs[i].new_ctx);
> 	hwsim_clear_chanctx_magic(vifs[i].old_ctx);
> }
> 
> ...
> 
> One thing, in patch should I keep those comments or those are noisy? 
> Seems noisy to me (if at least mode documentation is referred then 
> things are clear already)?
> 

I'm not sure I care all that much, but I'd say even with the reference
to the modes, it's fairly easy to figure out at least by looking at the
mode docs?

I'd probably go for a switch () statement on the modes and even WARN on
invalid mode, while at it? hwsim is a test vehicle, after all.

No strong opinions on either (comments and switch) though.

johannes

Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for switch_vif_chanctx callback

[Aditya Kumar Singh]

On 2/16/24 12:42, Johannes Berg wrote:
> On Fri, 2024-02-16 at 08:45 +0530, Aditya Kumar Singh wrote:
>>
>>>> +	for (i = 0; i < n_vifs; i++) {
>>>> +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
>>>> +		wiphy_dbg(hw->wiphy,
>>>> +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
>>>> +			  vifs[i].old_ctx->def.chan->center_freq,
>>>> +			  vifs[i].old_ctx->def.width,
>>>> +			  vifs[i].old_ctx->def.center_freq1,
>>>> +			  vifs[i].old_ctx->def.center_freq2,
>>>> +			  vifs[i].new_ctx->def.chan->center_freq,
>>>> +			  vifs[i].new_ctx->def.width,
>>>> +			  vifs[i].new_ctx->def.center_freq1,
>>>> +			  vifs[i].new_ctx->def.center_freq2);
>>>> +		hwsim_set_chanctx_magic(vifs[i].new_ctx);
>>>
>>>
>>> hwsim_set_chanctx_magic() is only partially correct, I think, this
>>> depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
>>> already exist as well, so should also be hwsim_check_chanctx_magic() in
>>> that case?
>>>
>>
>> Oh yeah missed the mode dependency here. Thanks for pointing it out. So
>> it should be something like -
>>
>> ...
>>
>> /* old already exist so check magic */
>> hwsim_check_chanctx_magic(vifs[i].old_ctx);
>>
>> if (mode == CHANCTX_SWMODE_REASSIGN_VIF) {
>> 	/* Reassign then new would also exist, just interface is
>> 	 * switching
>> 	 */
>> 	hwsim_check_chanctx_magic(vifs[i].new_ctx);
>> } else {
>> 	/* SWAP_* then new context does not exist hence set magic.
>> 	 * Also old will no longer exist so clear the magic
>> 	 */
>> 	hwsim_set_chanctx_magic(vifs[i].new_ctx);
>> 	hwsim_clear_chanctx_magic(vifs[i].old_ctx);
>> }
>>
>> ...
>>
>> One thing, in patch should I keep those comments or those are noisy?
>> Seems noisy to me (if at least mode documentation is referred then
>> things are clear already)?
>>
> 
> I'm not sure I care all that much, but I'd say even with the reference
> to the modes, it's fairly easy to figure out at least by looking at the
> mode docs?
> 

Yes correct. I will drop inline comments. Not required tbh.

> I'd probably go for a switch () statement on the modes and even WARN on
> invalid mode, while at it? hwsim is a test vehicle, after all.
> 

Yes, good point :). Thanks will incorporate and send v2 soon.