mbox series

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

Message ID 20240215162811.506065-1-quic_adisi@quicinc.com
Headers show
Series wifi: mac80211/mac80211_hwsim: support MLO CSA test case | expand

Message

Aditya Kumar Singh Feb. 15, 2024, 4:28 p.m. UTC 
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

Comments

Johannes Berg Feb. 15, 2024, 7:48 p.m. UTC | #1 
> -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
Aditya Kumar Singh Feb. 16, 2024, 3:04 a.m. UTC | #2 
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.