From: Aditya Kumar Singh <quic_adisi@quicinc.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v5 2/3] wifi: mac80211: add support for AP channel switch with MLO
Date: Sat, 27 Jan 2024 10:28:37 +0530 [thread overview]
Message-ID: <033151e5-7857-4c44-9b12-d8d8317a4139@quicinc.com> (raw)
In-Reply-To: <d4e77541479560147d453b7b5c06de3f940bad35.camel@sipsolutions.net>
On 1/26/24 15:01, Johannes Berg wrote:
> On Thu, 2024-01-25 at 18:34 +0530, Aditya Kumar Singh wrote:
>> Currently, during channel switch, deflink (or link_id 0)
>
> The parenthetical is wrong: deflink isn't even a valid/used link *at
> all* in MLD data structures. Speaking about "link_id 0" would be a valid
> thing to do even for MLD, but speaking about "deflink" isn't.
>
Okay got it. Will address in next version.
>
>> +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
>> @@ -2285,7 +2285,7 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
>> }
>>
>> if (link_conf->csa_active && ieee80211_beacon_cntdwn_is_complete(vif))
>> - ieee80211_csa_finish(vif);
>> + ieee80211_csa_finish(vif, link_id);
>
> Might make sense to keep the actual logic changes out of this patch?
> It's pretty big, and this is pretty hidden...
>
Sure so I will move this ieee80211_csa_finish() changes to a different
patch.
>> +++ b/net/mac80211/cfg.c
>> @@ -1588,6 +1588,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev,
>> link->csa_block_tx = false;
>> }
>>
>> + wiphy_work_cancel(wiphy, &link->csa_finalize_work);
>
> You don't _really_ need that here (if it runs, it'll just find
> csa_active==false), and I'd feel better about it if we actually had this
> in ieee80211_link_stop() in some way, that's called from
> ieee80211_tear_down_links() and then it's really obvious to see that
> this is removed before freeing the link.
>
Yeah makes sense. Let me see what I can do here. Thanks for the inputs! :)
>> - if (sdata->vif.bss_conf.eht_puncturing != sdata->vif.bss_conf.csa_punct_bitmap) {
>> - sdata->vif.bss_conf.eht_puncturing =
>> - sdata->vif.bss_conf.csa_punct_bitmap;
>> + if (link_conf->eht_puncturing != link_conf->csa_punct_bitmap) {
>> + link_conf->eht_puncturing = link_conf->csa_punct_bitmap;
>> changed |= BSS_CHANGED_EHT_PUNCTURING;
>> }
>
> Hm. I'm going to send some patches soon that remove the puncturing stuff
> and move it into the chandef (as we discussed elsewhere), but just
> noting that - doesn't need to concern you here, I think.
>
Oh okay! Let's see if at all it conflicts, will send a re-based once
those gets merged.
>> @@ -3875,16 +3894,23 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>> if (sdata->wdev.cac_started)
>> return -EBUSY;
>>
>> - if (cfg80211_chandef_identical(¶ms->chandef,
>> - &sdata->vif.bss_conf.chandef))
>> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>> + return -EINVAL;
>> +
>> + link_data = wiphy_dereference(wiphy, sdata->link[link_id]);
>> + if (!link_data)
>> + return -ENOLINK;
>> +
>> + link_conf = link_data->conf;
>> +
>> + if (cfg80211_chandef_identical(¶ms->chandef, &link_conf->chandef))
>> return -EINVAL;
>
> Also another thing unrelated to your patch - I'm thinking about removing
> that identical() check entirely - you might want to switch to the same
> channel with quiet=1. At least for testing that'd be really useful, and
> I don't think it really serves any purpose to forbid it.
>
Yeah, we can do. But is there any actual use case? Also, what if some
notorious user space application simply sends NL command without even
quiet=1? There should be some check I guess?
next prev parent reply other threads:[~2024-01-27 4:58 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-25 13:04 [PATCH v5 0/3] wifi: cfg80211/mac80211: add link_id handling in AP channel switch during Multi-Link Operation Aditya Kumar Singh
2024-01-25 13:04 ` [PATCH v5 1/3] wifi: cfg80211: send link id in channel_switch ops Aditya Kumar Singh
2024-01-26 9:18 ` Johannes Berg
2024-01-27 4:52 ` Aditya Kumar Singh
2024-01-25 13:04 ` [PATCH v5 2/3] wifi: mac80211: add support for AP channel switch with MLO Aditya Kumar Singh
2024-01-26 9:31 ` Johannes Berg
2024-01-27 4:58 ` Aditya Kumar Singh [this message]
2024-01-30 10:51 ` Johannes Berg
2024-01-25 13:04 ` [PATCH v5 3/3] wifi: mac80211: update beacon counters per link basis Aditya Kumar Singh
2024-01-26 9:31 ` Johannes Berg
2024-01-27 5:01 ` Aditya Kumar Singh
-- strict thread matches above, loose matches on Subject: below --
2024-01-25 12:58 [PATCH 0/3] wifi: cfg80211/mac80211: add support to flush stations based on link ID Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH v5 2/3] wifi: mac80211: add support for AP channel switch with MLO Aditya Kumar Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=033151e5-7857-4c44-9b12-d8d8317a4139@quicinc.com \
--to=quic_adisi@quicinc.com \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).