| Message ID | 20240215162811.506065-3-quic_adisi@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211/mac80211_hwsim: support MLO CSA test case | expand |
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
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)?
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
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.
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
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(-)