| Message ID | 20220902161143.5ce3dad3be7c.I92e9f7a6c120cd4a3631baf486ad8b6aafcd796f@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | another set of MLO patches | expand |
On 9/2/2022 10:12 PM, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > In order to let the driver select active links and properly > make multi-link connections, as a first step isolate the > driver from inactive links, and set the active links to be > only the association link for client-side interfaces. For > AP side nothing changes since APs always have to have all > their links active. > > To simplify things, update the for_each_sta_active_link() > API to include the appropriate vif pointer. > > This also implies not allocating a chanctx for an inactive > link, which requires a few more changes. > > Since we now no longer try to program multiple links to the > driver, remove the check in the MLME code. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > include/net/mac80211.h | 30 +++---- > net/mac80211/chan.c | 6 ++ > net/mac80211/driver-ops.c | 172 ++++++++++++++++++++++++++++++++++++++ > net/mac80211/driver-ops.h | 165 ++++++------------------------------ > net/mac80211/key.c | 8 ++ > net/mac80211/link.c | 66 ++++++++++++--- > net/mac80211/mlme.c | 25 ++---- > net/mac80211/util.c | 2 +- > 8 files changed, 286 insertions(+), 188 deletions(-) > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index d4e1d73d88cc..20a2f25a38fa 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1799,6 +1799,9 @@ struct ieee80211_vif_cfg { > * @link_conf: in case of MLD, the per-link BSS configuration, > * indexed by link ID > * @valid_links: bitmap of valid links, or 0 for non-MLO. > + * @active_links: The bitmap of active links, or 0 for non-MLO. > + * The driver shouldn't change this directly, but use the > + * API calls meant for that purpose. > * @addr: address of this interface > * @p2p: indicates whether this AP or STA interface is a p2p > * interface, i.e. a GO or p2p-sta respectively > @@ -1834,7 +1837,7 @@ struct ieee80211_vif { > struct ieee80211_vif_cfg cfg; > struct ieee80211_bss_conf bss_conf; > struct ieee80211_bss_conf __rcu *link_conf[IEEE80211_MLD_MAX_NUM_LINKS]; > - u16 valid_links; > + u16 valid_links, active_links; > u8 addr[ETH_ALEN] __aligned(2); > bool p2p; > ... > @@ -123,11 +132,38 @@ static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata) > return 0; > } > > +static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, > + u16 links) > +{ > + sdata->vif.valid_links = links; > + > + if (!links) { > + sdata->vif.active_links = 0; > + return; > + } > + > + switch (sdata->vif.type) { > + case NL80211_IFTYPE_AP: > + /* in an AP all links are always active */ > + sdata->vif.active_links = links; > + break; > + case NL80211_IFTYPE_STATION: > + if (sdata->vif.active_links) > + break; > + WARN_ON(hweight16(links) > 1); > + sdata->vif.active_links = links; > + break; > + default: > + WARN_ON(1); > + } > +} > + Now I found it only active the primay link(the link for authentication/assoc request) in my station MLO test, change_vif_links of struct ieee80211_ops *ops of driver will only be called one time for the primary link. it means only one link for MLO. I plan to revert this patch in my local test now. Will you implement muti-links later? > ...
On Thu, 2022-09-08 at 23:23 +0800, Wen Gong wrote: > > Now I found it only active the primay link(the link for > authentication/assoc request) in my station MLO test, Yes, that's intentional. It gives the driver choice about which links to activate; first of all because we don't have interface/link combinations stuff yet (waiting for your side on that), and secondly because we might very well (want to) negotiate more links than we can concurrently have active, e.g. a NIC that can have two active might still want to negotiate four and switch dynamically. > change_vif_links of struct ieee80211_ops *ops of driver will only be > called one time for the primary link. Correct. > it means only one link for MLO. Right. > I plan to revert this patch in my local test now. > > Will you implement muti-links later? Yes. I have patches pending to add API that the driver can call to pick the active links (as a bitmap). I'll send it out when I can, likely tomorrow. johannes >
On 9/8/2022 11:36 PM, Johannes Berg wrote: > On Thu, 2022-09-08 at 23:23 +0800, Wen Gong wrote: >> Now I found it only active the primay link(the link for >> authentication/assoc request) in my station MLO test, > Yes, that's intentional. It gives the driver choice about which links to > activate; first of all because we don't have interface/link combinations > stuff yet (waiting for your side on that), and secondly because we might > very well (want to) negotiate more links than we can concurrently have > active, e.g. a NIC that can have two active might still want to > negotiate four and switch dynamically. > >> change_vif_links of struct ieee80211_ops *ops of driver will only be >> called one time for the primary link. > Correct. > >> it means only one link for MLO. > Right. > >> I plan to revert this patch in my local test now. >> >> Will you implement muti-links later? > Yes. I have patches pending to add API that the driver can call to pick > the active links (as a bitmap). > > I'll send it out when I can, likely tomorrow. > > johannes Thanks. Another thing is what is the local MLD addr and local primary link(send authentication/assoc requset) addr relation? I think they are same address for station, right? And the others local link address is random generated by eth_random_addr in ieee80211_mgd_assoc() , right?
On Thu, 2022-09-08 at 23:51 +0800, Wen Gong wrote: > > Another thing is what is the local MLD addr and local primary link(send > authentication/assoc requset) addr relation? > I think they are same address for station, right? No, they aren't, and shouldn't be. > And the others local link address is random generated by eth_random_addr > in ieee80211_mgd_assoc() , right? Yes, at least for now all the link addresses are randomly generated. johannes
On 9/8/2022 11:52 PM, Johannes Berg wrote: > On Thu, 2022-09-08 at 23:51 +0800, Wen Gong wrote: >> Another thing is what is the local MLD addr and local primary link(send >> authentication/assoc requset) addr relation? >> I think they are same address for station, right? > No, they aren't, and shouldn't be. IEEE P802.11be™/D2.0 35.3.3 Multi-link device addressing An MLD has an MLD MAC address that singly identifies the MLD. Each STA affiliated with an MLD shall have a different MAC address. NOTE 1—The MLD MAC address of an MLD might be the same as the MAC address of one affiliated STA or different from the MAC address of any affiliated STA. This means the MLD address can be same with one link. I suggest to set primary link local addr same with MLD address for station. reason is: When station up, one link interface of driver will be created with the addr of struct ieee80211_vif, it is used for scan and non-MLO connection. If station start to do MLO connection now, then random local link addr will be generated by below call stack. for the 1st link. This lead driver must change the link interface local address to this random addr. After disconnect MLO connection, driver also need to change the link interface local address back to addr of struct ieee80211_vif. It increased the complexity and driver need to sync the link interface if this is a scan running at this moment. ieee80211_mgd_auth() ->ieee80211_prep_connection() ->ieee80211_vif_set_links() ->ieee80211_vif_update_links() ->ieee80211_link_setup() ->ieee80211_mgd_setup_link() eth_random_addr(link->conf->addr);//sdata->u.mgd.assoc_data is null at this point >> And the others local link address is random generated by eth_random_addr >> in ieee80211_mgd_assoc() , right? > Yes, at least for now all the link addresses are randomly generated. > > johannes >
Hi, > > No, they aren't, and shouldn't be. > IEEE P802.11be™/D2.0 > 35.3.3 Multi-link device addressing > An MLD has an MLD MAC address that singly identifies the MLD. > Each STA affiliated with an MLD shall have a different MAC address. > NOTE 1—The MLD MAC address of an MLD might be the same as the MAC > address of one affiliated STA or different > from the MAC address of any affiliated STA. Right. I was over-simplifying, that was basically the "tl;dr" version of my statement, without the longer one ;-) > This means the MLD address can be same with one link. True. > I suggest to set primary link local addr same with MLD address for station. I wouldn't suggest that, but YMMV. > reason is: > When station up, one link interface of driver will be created with the > addr of struct ieee80211_vif, > it is used for scan and non-MLO connection. > If station start to do MLO connection now, then random local link addr > will be generated by below call stack. > for the 1st link. This lead driver must change the link interface local > address to this random addr. Well, that depends how you treat "address of an interface", no? I don't think there's really any need to "install" a MAC address to the NIC until you even start any kind of operation. True, if you cannot scan using the MLD address while you also have a different link address, you might be in trouble - but I find this unrealistic because you would want to be able to scan on any part of the hardware that is doing any of the links? In any case, changing this makes the receive logic a bit different. You would have to ensure that your driver does indeed indicate the link a frame was received on, I think? Also, ieee80211_rx_for_interface() might have to change, something like the below maybe? If we just change the first link's address to the same as the MLD address without any changes then the code without the changes below would overwrite the link ID because it can find the link STA address, even if the device already did address translation. Of course this is only relevant if it does address translation w/o indicating the link, which it shouldn't ... hence the patch. In any case, I expect this will end up being some kind of driver policy, so I can imagine that we could make a relatively simple patch with a new method to let drivers set the link address that gets used. It cannot be changing the link address when it's added to the driver since this patch that this thread is based on means the driver doesn't get to know about the links until it's far too late (and even before this patch, the links were only created after assoc, when the link addresses were already sent to the AP) johannes diff --git a/include/net/mac80211.h b/include/net/mac80211.h index ac2bad57933f..648b2de8dd3e 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding { * @ampdu_delimiter_crc: A-MPDU delimiter CRC * @zero_length_psdu_type: radiotap type of the 0-length PSDU * @link_valid: if the link which is identified by @link_id is valid. This flag - * is set only when connection is MLO. + * is set only when connection is MLO. Note that setting this also implies + * address translation was done. * @link_id: id of the link used to receive the packet. This is used along with * @link_valid. */ diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index a57811372027..963de5d880d7 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw, static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx, struct sk_buff *skb, bool consume) { - struct link_sta_info *link_sta; + struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); struct ieee80211_hdr *hdr = (void *)skb->data; + struct link_sta_info *link_sta = NULL; /* - * Look up link station first, in case there's a - * chance that they might have a link address that - * is identical to the MLD address, that way we'll - * have the link information if needed. + * Unless the driver did addr translation and provided the link + * ID, look up link station first. Note that if we get a frame + * without link ID in the status and the device happens to use + * identical addresses for one of the links and the MLD, then + * we cannot identify whether it was translated already or not. */ - link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); + if (!status->link_valid) + link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); + if (link_sta) { rx->sta = link_sta->sta; rx->link_id = link_sta->link_id; } else { - struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); - rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2); if (rx->sta) { if (status->link_valid &&
On 9/9/2022 3:28 PM, Johannes Berg wrote: > Hi, > >>> No, they aren't, and shouldn't be. >> IEEE P802.11be™/D2.0 >> 35.3.3 Multi-link device addressing >> An MLD has an MLD MAC address that singly identifies the MLD. >> Each STA affiliated with an MLD shall have a different MAC address. >> NOTE 1—The MLD MAC address of an MLD might be the same as the MAC >> address of one affiliated STA or different >> from the MAC address of any affiliated STA. > Right. I was over-simplifying, that was basically the "tl;dr" version of > my statement, without the longer one ;-) > >> This means the MLD address can be same with one link. > True. > >> I suggest to set primary link local addr same with MLD address for station. > I wouldn't suggest that, but YMMV. > >> reason is: >> When station up, one link interface of driver will be created with the >> addr of struct ieee80211_vif, >> it is used for scan and non-MLO connection. >> If station start to do MLO connection now, then random local link addr >> will be generated by below call stack. >> for the 1st link. This lead driver must change the link interface local >> address to this random addr. > Well, that depends how you treat "address of an interface", no? I don't > think there's really any need to "install" a MAC address to the NIC > until you even start any kind of operation. > > True, if you cannot scan using the MLD address while you also have a > different link address, you might be in trouble - but I find this > unrealistic because you would want to be able to scan on any part of the > hardware that is doing any of the links? Scan probe request needs the local address, so we must fill one address to it. And we use the same local address to scan for 2.4 GHz/5 GHz/6 GHz band. > > > In any case, changing this makes the receive logic a bit different. You > would have to ensure that your driver does indeed indicate the link a > frame was received on, I think? Also, ieee80211_rx_for_interface() might > have to change, something like the below maybe? I looked the ieee80211_rx_for_interface(), it is to find struct link_sta_info with the source address of an rx frame. For station, the hdr->addr2 means the address of the AP, so the the change of mac80211/wireless will not effect the ieee80211_rx_for_interface(). Because it is the MLD/link address of the AP(maybe it is same addr for MLD/one link) when we use as station. > > If we just change the first link's address to the same as the MLD > address without any changes then the code without the changes below > would overwrite the link ID because it can find the link STA address, > even if the device already did address translation. Of course this is > only relevant if it does address translation w/o indicating the link, > which it shouldn't ... hence the patch. > > In any case, I expect this will end up being some kind of driver policy, > so I can imagine that we could make a relatively simple patch with a new > method to let drivers set the link address that gets used. It cannot be > changing the link address when it's added to the driver since this patch > that this thread is based on means the driver doesn't get to know about > the links until it's far too late (and even before this patch, the links > were only created after assoc, when the link addresses were already sent > to the AP) > > johannes > > > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index ac2bad57933f..648b2de8dd3e 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding { > * @ampdu_delimiter_crc: A-MPDU delimiter CRC > * @zero_length_psdu_type: radiotap type of the 0-length PSDU > * @link_valid: if the link which is identified by @link_id is valid. This flag > - * is set only when connection is MLO. > + * is set only when connection is MLO. Note that setting this also implies > + * address translation was done. > * @link_id: id of the link used to receive the packet. This is used along with > * @link_valid. > */ > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c > index a57811372027..963de5d880d7 100644 > --- a/net/mac80211/rx.c > +++ b/net/mac80211/rx.c > @@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw, > static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx, > struct sk_buff *skb, bool consume) > { > - struct link_sta_info *link_sta; > + struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); > struct ieee80211_hdr *hdr = (void *)skb->data; > + struct link_sta_info *link_sta = NULL; > > /* > - * Look up link station first, in case there's a > - * chance that they might have a link address that > - * is identical to the MLD address, that way we'll > - * have the link information if needed. > + * Unless the driver did addr translation and provided the link > + * ID, look up link station first. Note that if we get a frame > + * without link ID in the status and the device happens to use > + * identical addresses for one of the links and the MLD, then > + * we cannot identify whether it was translated already or not. > */ > - link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); > + if (!status->link_valid) > + link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); > + > if (link_sta) { > rx->sta = link_sta->sta; > rx->link_id = link_sta->link_id; > } else { > - struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); > - > rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2); > if (rx->sta) { > if (status->link_valid && Thanks. Below patch has said driver should report link id if addr translated. https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?h=mld&id=ea9d807b56428d65cf43030cbd7ae5a580077147 wifi: mac80211: add link information in ieee80211_rx_status In MLO, when the address translation from link to MLD is done in fw/hw, it is necessary to be able to have some information on the link on which the frame has been received. Extend the rx API to include link_id and a valid flag in ieee80211_rx_status. Also make chanes to mac80211 rx APIs to make use of the reported link_id after sanity checks.
On 9/9/2022 3:28 PM, Johannes Berg wrote: > Hi, > >>> No, they aren't, and shouldn't be. >> IEEE P802.11be™/D2.0 >> 35.3.3 Multi-link device addressing >> An MLD has an MLD MAC address that singly identifies the MLD. >> Each STA affiliated with an MLD shall have a different MAC address. >> NOTE 1—The MLD MAC address of an MLD might be the same as the MAC >> address of one affiliated STA or different >> from the MAC address of any affiliated STA. > Right. I was over-simplifying, that was basically the "tl;dr" version of > my statement, without the longer one ;-) > >> This means the MLD address can be same with one link. > True. > >> I suggest to set primary link local addr same with MLD address for station. > I wouldn't suggest that, but YMMV. > >> reason is: >> When station up, one link interface of driver will be created with the >> addr of struct ieee80211_vif, >> it is used for scan and non-MLO connection. >> If station start to do MLO connection now, then random local link addr >> will be generated by below call stack. >> for the 1st link. This lead driver must change the link interface local >> address to this random addr. > Well, that depends how you treat "address of an interface", no? I don't > think there's really any need to "install" a MAC address to the NIC > until you even start any kind of operation. > > True, if you cannot scan using the MLD address while you also have a > different link address, you might be in trouble - but I find this > unrealistic because you would want to be able to scan on any part of the > hardware that is doing any of the links? Scan probe request needs the local address, so we must fill one address to it. And we use the same local address to scan for 2.4 GHz/5 GHz/6 GHz band. > > > In any case, changing this makes the receive logic a bit different. You > would have to ensure that your driver does indeed indicate the link a > frame was received on, I think? Also, ieee80211_rx_for_interface() might > have to change, something like the below maybe? I looked the ieee80211_rx_for_interface(), it is to find struct link_sta_info with the source address of an rx frame. For station, the hdr->addr2 means the address of the AP, so the the change of mac80211/wireless will not effect the ieee80211_rx_for_interface(). Because it is the MLD/link address of the AP(maybe it is same addr for MLD/one link) when we as station. > If we just change the first link's address to the same as the MLD > address without any changes then the code without the changes below > would overwrite the link ID because it can find the link STA address, > even if the device already did address translation. Of course this is > only relevant if it does address translation w/o indicating the link, > which it shouldn't ... hence the patch. > > In any case, I expect this will end up being some kind of driver policy, > so I can imagine that we could make a relatively simple patch with a new > method to let drivers set the link address that gets used. It cannot be > changing the link address when it's added to the driver since this patch > that this thread is based on means the driver doesn't get to know about > the links until it's far too late (and even before this patch, the links > were only created after assoc, when the link addresses were already sent > to the AP) Thanks for the incoming new method to let drivers set the link address. It is better to let driver to fill all the links' address in load phase. And then it never change again. And one of the address array is always used for primary link. > > johannes > > > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index ac2bad57933f..648b2de8dd3e 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding { > * @ampdu_delimiter_crc: A-MPDU delimiter CRC > * @zero_length_psdu_type: radiotap type of the 0-length PSDU > * @link_valid: if the link which is identified by @link_id is valid. This flag > - * is set only when connection is MLO. > + * is set only when connection is MLO. Note that setting this also implies > + * address translation was done. > * @link_id: id of the link used to receive the packet. This is used along with > * @link_valid. > */ > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c > index a57811372027..963de5d880d7 100644 > --- a/net/mac80211/rx.c > +++ b/net/mac80211/rx.c > @@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw, > static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx, > struct sk_buff *skb, bool consume) > { > - struct link_sta_info *link_sta; > + struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); > struct ieee80211_hdr *hdr = (void *)skb->data; > + struct link_sta_info *link_sta = NULL; > > /* > - * Look up link station first, in case there's a > - * chance that they might have a link address that > - * is identical to the MLD address, that way we'll > - * have the link information if needed. > + * Unless the driver did addr translation and provided the link > + * ID, look up link station first. Note that if we get a frame > + * without link ID in the status and the device happens to use > + * identical addresses for one of the links and the MLD, then > + * we cannot identify whether it was translated already or not. > */ > - link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); > + if (!status->link_valid) > + link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2); > + > if (link_sta) { > rx->sta = link_sta->sta; > rx->link_id = link_sta->link_id; > } else { > - struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); > - > rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2); > if (rx->sta) { > if (status->link_valid && Thanks. Below patch has said driver should report link id if addr translated. https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?h=mld&id=ea9d807b56428d65cf43030cbd7ae5a580077147 wifi: mac80211: add link information in ieee80211_rx_status In MLO, when the address translation from link to MLD is done in fw/hw, it is necessary to be able to have some information on the link on which the frame has been received. Extend the rx API to include link_id and a valid flag in ieee80211_rx_status. Also make chanes to mac80211 rx APIs to make use of the reported link_id after sanity checks.
Hi johannes, May I know some more info/status about the "incoming new method to let drivers set the link address"? On 9/9/2022 4:58 PM, Wen Gong wrote: > On 9/9/2022 3:28 PM, Johannes Berg wrote: > ... >> If we just change the first link's address to the same as the MLD >> address without any changes then the code without the changes below >> would overwrite the link ID because it can find the link STA address, >> even if the device already did address translation. Of course this is >> only relevant if it does address translation w/o indicating the link, >> which it shouldn't ... hence the patch. >> >> In any case, I expect this will end up being some kind of driver policy, >> so I can imagine that we could make a relatively simple patch with a new >> method to let drivers set the link address that gets used. It cannot be >> changing the link address when it's added to the driver since this patch >> that this thread is based on means the driver doesn't get to know about >> the links until it's far too late (and even before this patch, the links >> were only created after assoc, when the link addresses were already sent >> to the AP) > Thanks for the incoming new method to let drivers set the link address. > It is better to let driver to fill all the links' address in load phase. > And then it never change again. And one of the address array is always > used for primary link. > ...
Hi, Sorry - still catching up from PF related matters ... > May I know some more info/status about the "incoming new method to let > drivers set the link address"? > I wasn't actually planning to work on that myself, FWIW. johannes
On 9/28/2022 11:28 PM, Johannes Berg wrote: ... > >> May I know some more info/status about the "incoming new method to let >> drivers set the link address"? >> > I wasn't actually planning to work on that myself, FWIW. > > johannes OK. So has some body will work for that now?
On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote: > On 9/28/2022 11:28 PM, Johannes Berg wrote: > ... > > > > > May I know some more info/status about the "incoming new method to let > > > drivers set the link address"? > > > > > I wasn't actually planning to work on that myself, FWIW. > > > > johannes > > OK. So has some body will work for that now?
On 10/11/2022 3:26 PM, Johannes Berg wrote: > On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote: >> On 9/28/2022 11:28 PM, Johannes Berg wrote: >> ... >>>> May I know some more info/status about the "incoming new method to let >>>> drivers set the link address"? >>>> >>> I wasn't actually planning to work on that myself, FWIW. >>> >>> johannes >> OK. So has some body will work for that now?
On 9/8/2022 11:36 PM, Johannes Berg wrote: > On Thu, 2022-09-08 at 23:23 +0800, Wen Gong wrote: >> Now I found it only active the primay link(the link for >> authentication/assoc request) in my station MLO test, > Yes, that's intentional. It gives the driver choice about which links to > activate; first of all because we don't have interface/link combinations > stuff yet (waiting for your side on that), and secondly because we might > very well (want to) negotiate more links than we can concurrently have > active, e.g. a NIC that can have two active might still want to > negotiate four and switch dynamically. > >> change_vif_links of struct ieee80211_ops *ops of driver will only be >> called one time for the primary link. > Correct. > >> it means only one link for MLO. > Right. > >> I plan to revert this patch in my local test now. >> >> Will you implement muti-links later? > Yes. I have patches pending to add API that the driver can call to pick > the active links (as a bitmap). > > I'll send it out when I can, likely tomorrow. > > johannes The patches should be "wifi: mac80211: implement link switching" for ieee80211_set_active_links(). May I also add a field such as "u16 active_links_count" in struct wiphy_iftype_ext_capab, and add logic in function ieee80211_set_vif_links_bitmaps() for station like this ?: if (active_links_count && hweight16(links) <= active_links_count) then sdata->vif.active_links = links;
On Tue, 2023-04-04 at 10:54 +0800, Wen Gong wrote: > On 10/11/2022 3:26 PM, Johannes Berg wrote: > > On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote: > > > On 9/28/2022 11:28 PM, Johannes Berg wrote: > > > ... > > > > > May I know some more info/status about the "incoming new method to let > > > > > drivers set the link address"? > > > > > > > > > I wasn't actually planning to work on that myself, FWIW. > > > > > > > > johannes > > > OK. So has some body will work for that now?
On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: > > May I also add a field such as "u16 active_links_count" in struct > wiphy_iftype_ext_capab, > and add logic in function ieee80211_set_vif_links_bitmaps() for station > like this ?: > if (active_links_count && hweight16(links) <= active_links_count) > then sdata->vif.active_links = links; > Also here, not sure it makes sense in cfg80211 level? Though I'm not sure what the idea here is at all - you can refuse to link switch etc, what would you use this for? Then again, we haven't really designed out all the link selection stuff, do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So depending on what end up doing there, we will obviously need to advertise some level of link-concurrency to userspace. johannes
On 4/11/2023 3:32 PM, Johannes Berg wrote: > On Tue, 2023-04-04 at 10:54 +0800, Wen Gong wrote: >> On 10/11/2022 3:26 PM, Johannes Berg wrote: >>> On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote: >>>> On 9/28/2022 11:28 PM, Johannes Berg wrote: >>>> ... >>>>>> May I know some more info/status about the "incoming new method to let >>>>>> drivers set the link address"? >>>>>> >>>>> I wasn't actually planning to work on that myself, FWIW. >>>>> >>>>> johannes >>>> OK. So has some body will work for that now?
On 4/11/2023 3:38 PM, Johannes Berg wrote: > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: >> May I also add a field such as "u16 active_links_count" in struct >> wiphy_iftype_ext_capab, >> and add logic in function ieee80211_set_vif_links_bitmaps() for station >> like this ?: >> if (active_links_count && hweight16(links) <= active_links_count) >> then sdata->vif.active_links = links; >> > Also here, not sure it makes sense in cfg80211 level? > > Though I'm not sure what the idea here is at all - you can refuse to > link switch etc, what would you use this for? If I use ieee80211_set_active_links(), then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver. I would like to active all links while assoc, then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to lower-driver from mac80211. > > Then again, we haven't really designed out all the link selection stuff, > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So > depending on what end up doing there, we will obviously need to > advertise some level of link-concurrency to userspace. > > johannes
On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote: > > OK. So I will try to put this in mac80211 layer, is it OK? > I guess? I'm still not really sure why you even want it, but hey, that's up to you in a way. I really didn't like the suggestion with wiphy_iftype_ext_capab (or any other capability for that matter), it feels like it should be more dynamic, like maybe a new "add link" callback or something? At least then you can't blame mac80211 for when it breaks when you have two 5 GHz links ... johannes
On Mon, 2023-04-17 at 22:13 +0800, Wen Gong wrote: > On 4/11/2023 3:38 PM, Johannes Berg wrote: > > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: > > > May I also add a field such as "u16 active_links_count" in struct > > > wiphy_iftype_ext_capab, > > > and add logic in function ieee80211_set_vif_links_bitmaps() for station > > > like this ?: > > > if (active_links_count && hweight16(links) <= active_links_count) > > > then sdata->vif.active_links = links; > > > > > Also here, not sure it makes sense in cfg80211 level? > > > > Though I'm not sure what the idea here is at all - you can refuse to > > link switch etc, what would you use this for? > If I use ieee80211_set_active_links(), > then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver. > > I would like to active all links while assoc, > then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to > lower-driver from mac80211. I'm not convinced that makes sense. You're going to have to be able to deal with changing links after association _anyway_, unless you plan on breaking the entire connection once any of the links is getting out of range or something? So anyway you're going to have to be able to this for new links anyway? I mean doing key management when link switching, and "association" (in quotes, because as a term doesn't even make sense since this state is on the MLD level, not the link level)... So not sure I get it? johannes
On 4/18/2023 4:15 PM, Johannes Berg wrote: > On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote: >> OK. So I will try to put this in mac80211 layer, is it OK? >> > I guess? I'm still not really sure why you even want it, but hey, that's > up to you in a way. I really didn't like the suggestion with > wiphy_iftype_ext_capab (or any other capability for that matter), it > feels like it should be more dynamic, like maybe a new "add link" > callback or something? At least then you can't blame mac80211 for when > it breaks when you have two 5 GHz links ... ok, so I would like to add callback such as "add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct ieee80211_bss_conf *link_conf, unsigned int link_id)" in struct ieee80211_ops, and mac80211 call it in ieee80211_mgd_setup_link()/ieee80211_vif_update_links, then lower-drvier could dynamic set the local addr of assoc link_conf(also for 2nd link_conf), is it OK? > > johannes
On Tue, 2023-04-18 at 16:59 +0800, Wen Gong wrote: > On 4/18/2023 4:15 PM, Johannes Berg wrote: > > On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote: > > > OK. So I will try to put this in mac80211 layer, is it OK? > > > > > I guess? I'm still not really sure why you even want it, but hey, that's > > up to you in a way. I really didn't like the suggestion with > > wiphy_iftype_ext_capab (or any other capability for that matter), it > > feels like it should be more dynamic, like maybe a new "add link" > > callback or something? At least then you can't blame mac80211 for when > > it breaks when you have two 5 GHz links ... > > ok, so I would like to add callback such as > > "add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct > ieee80211_bss_conf *link_conf, unsigned int link_id)" > > in struct ieee80211_ops, and mac80211 call it in > ieee80211_mgd_setup_link()/ieee80211_vif_update_links, > > then lower-drvier could dynamic set the local addr of assoc > link_conf(also for 2nd link_conf), is it OK? > Seems OK, but I'm not sure that _works_? After all, we first set the addresses in assoc_data, when we don't have a link_conf yet, no? Just what we were discussing in the other thread about the leak. johannes
On 4/18/2023 5:11 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 16:59 +0800, Wen Gong wrote: >> On 4/18/2023 4:15 PM, Johannes Berg wrote: >>> On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote: >>>> OK. So I will try to put this in mac80211 layer, is it OK? >>>> >>> I guess? I'm still not really sure why you even want it, but hey, that's >>> up to you in a way. I really didn't like the suggestion with >>> wiphy_iftype_ext_capab (or any other capability for that matter), it >>> feels like it should be more dynamic, like maybe a new "add link" >>> callback or something? At least then you can't blame mac80211 for when >>> it breaks when you have two 5 GHz links ... >> ok, so I would like to add callback such as >> >> "add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct >> ieee80211_bss_conf *link_conf, unsigned int link_id)" >> >> in struct ieee80211_ops, and mac80211 call it in >> ieee80211_mgd_setup_link()/ieee80211_vif_update_links, >> >> then lower-drvier could dynamic set the local addr of assoc >> link_conf(also for 2nd link_conf), is it OK? >> > Seems OK, but I'm not sure that _works_? > > After all, we first set the addresses in assoc_data, when we don't have > a link_conf yet, no? Just what we were discussing in the other thread > about the leak. > > johannes It should work, I will test it later. For the 1st assoc link, the data->u.mgd.assoc_data is empty in ieee80211_mgd_setup_link(), because ieee80211_mgd_setup_link() is called from nl80211_authenticate() for the 1st assoc link. So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link. For the 2nd link, ieee80211_mgd_setup_link() is called from nl80211_associate(), the sdata->u.mgd.assoc_data is NOT empty, and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, it is addr by eth_random_addr(assoc_data->link[i].addr) in ieee80211_mgd_assoc().
On 4/18/2023 4:18 PM, Johannes Berg wrote: > On Mon, 2023-04-17 at 22:13 +0800, Wen Gong wrote: >> On 4/11/2023 3:38 PM, Johannes Berg wrote: >>> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: >>>> May I also add a field such as "u16 active_links_count" in struct >>>> wiphy_iftype_ext_capab, >>>> and add logic in function ieee80211_set_vif_links_bitmaps() for station >>>> like this ?: >>>> if (active_links_count && hweight16(links) <= active_links_count) >>>> then sdata->vif.active_links = links; >>>> >>> Also here, not sure it makes sense in cfg80211 level? >>> >>> Though I'm not sure what the idea here is at all - you can refuse to >>> link switch etc, what would you use this for? >> If I use ieee80211_set_active_links(), >> then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver. >> >> I would like to active all links while assoc, >> then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to >> lower-driver from mac80211. > I'm not convinced that makes sense. You're going to have to be able to > deal with changing links after association _anyway_, unless you plan on > breaking the entire connection once any of the links is getting out of > range or something? > > So anyway you're going to have to be able to this for new links anyway? > I mean doing key management when link switching, and "association" (in > quotes, because as a term doesn't even make sense since this state is on > the MLD level, not the link level)... > > So not sure I get it? > > johannes Yes, you are right. Now lower driver I used do not store the key and do not trigger BSS_CHANGED_ASSOC for new links after assoc. So my suggestion is a way to active all links while assoc, this way is simple for lower driver I used. Also ieee80211_set_active_links() is another way to active all links after assoc.
On Tue, 2023-04-18 at 17:22 +0800, Wen Gong wrote: > > It should work, I will test it later. > > For the 1st assoc link, the data->u.mgd.assoc_data is empty in > ieee80211_mgd_setup_link(), Yeah for the first link it should work. > because ieee80211_mgd_setup_link() is called from nl80211_authenticate() > for the 1st assoc link. > > So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link. Right. > For the 2nd link, ieee80211_mgd_setup_link() is called from > nl80211_associate() I don't think so, it should only be called from ieee80211_assoc_success()? > the sdata->u.mgd.assoc_data is NOT empty, > > and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, > > it is addr by eth_random_addr(assoc_data->link[i].addr) in > ieee80211_mgd_assoc(). > Exactly, so we've already decided on the address long before we actually add the link data structure, so your callback would be much too late. We'd need to have it called from ieee80211_mgd_assoc() already? johannes
On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote: > > Now lower driver I used do not store the key > Sure, that's fine. > and do not trigger > BSS_CHANGED_ASSOC for new links after assoc. I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC causes is likely no longer correct in MLO. Again, the assoc state *itself* is only changed once, when the whole MLD associated. > So my suggestion is a way to active all links while assoc, this way is > simple for lower driver I used. Sure, and we do that. But that's not what you're asking - you're asking to re-do some *MLD* state when a new link is added, and I'm saying that it doesn't make sense to "add" (again) a key to the MLD that was already added, nor calling a vif (MLD!) level method saying the MLD changed state to associated (again). I really think you should solve this in the driver, that doesn't mean you have to _store_ he key, you can use one of the iteration functions as well. > Also ieee80211_set_active_links() is another way to active all links > after assoc. > Sure. johannes
On 4/18/2023 5:31 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 17:22 +0800, Wen Gong wrote: >> It should work, I will test it later. >> >> For the 1st assoc link, the data->u.mgd.assoc_data is empty in >> ieee80211_mgd_setup_link(), > Yeah for the first link it should work. > >> because ieee80211_mgd_setup_link() is called from nl80211_authenticate() >> for the 1st assoc link. >> >> So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link. > Right. > >> For the 2nd link, ieee80211_mgd_setup_link() is called from >> nl80211_associate() > I don't think so, it should only be called from > ieee80211_assoc_success()? Yes, I checked again, you are right. It is not from nl80211_associate(). >> the sdata->u.mgd.assoc_data is NOT empty, >> >> and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, >> >> it is addr by eth_random_addr(assoc_data->link[i].addr) in >> ieee80211_mgd_assoc(). >> > Exactly, so we've already decided on the address long before we actually > add the link data structure, so your callback would be much too late. > We'd need to have it called from ieee80211_mgd_assoc() already? For the 2nd link, is it OK for me to use the random addr which is set in ieee80211_mgd_assoc(). I only need to set the 1st assoc link in low driver. > johannes
On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote: > > > the sdata->u.mgd.assoc_data is NOT empty, > > > > > > and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, > > > > > > it is addr by eth_random_addr(assoc_data->link[i].addr) in > > > ieee80211_mgd_assoc(). > > > > > Exactly, so we've already decided on the address long before we actually > > add the link data structure, so your callback would be much too late. > > We'd need to have it called from ieee80211_mgd_assoc() already? > > For the 2nd link, is it OK for me to use the random addr which is set in > ieee80211_mgd_assoc(). > > I only need to set the 1st assoc link in low driver. > Ah. But does it make sense to restrict the API for that? I mean, if you just change the prototype a little bit and call it without the link conf, you can easily solve this problem too, no? Then your driver just has to call eth_radnom_addr() when it's "don't care", but that's OK? johannes
On 4/18/2023 5:38 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote: > > >>>> the sdata->u.mgd.assoc_data is NOT empty, >>>> >>>> and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, >>>> >>>> it is addr by eth_random_addr(assoc_data->link[i].addr) in >>>> ieee80211_mgd_assoc(). >>>> >>> Exactly, so we've already decided on the address long before we actually >>> add the link data structure, so your callback would be much too late. >>> We'd need to have it called from ieee80211_mgd_assoc() already? >> For the 2nd link, is it OK for me to use the random addr which is set in >> ieee80211_mgd_assoc(). >> >> I only need to set the 1st assoc link in low driver. >> > Ah. But does it make sense to restrict the API for that? I mean, if you > just change the prototype a little bit and call it without the link > conf, you can easily solve this problem too, no? Sorry, I am not sure how to solve this problem by remove the link conf in prototype. > > Then your driver just has to call eth_radnom_addr() when it's "don't > care", but that's OK? Yes, it is also OK for me to call eth_radnom_addr()for 2nd link. > > johannes
On 4/18/2023 5:34 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote: >> Now lower driver I used do not store the key >> > Sure, that's fine. > >> and do not trigger >> BSS_CHANGED_ASSOC for new links after assoc. > I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC > causes is likely no longer correct in MLO. Again, the assoc state > *itself* is only changed once, when the whole MLD associated. > >> So my suggestion is a way to active all links while assoc, this way is >> simple for lower driver I used. > Sure, and we do that. > > But that's not what you're asking - you're asking to re-do some *MLD* > state when a new link is added, and I'm saying that it doesn't make > sense to "add" (again) a key to the MLD that was already added, nor > calling a vif (MLD!) level method saying the MLD changed state to > associated (again). My purpose it to: add logic in function ieee80211_set_vif_links_bitmaps() for station, and to active all link as same as AP type. > > I really think you should solve this in the driver, that doesn't mean > you have to _store_ he key, you can use one of the iteration functions > as well. Ok, I will try to find the iteration functions. > >> Also ieee80211_set_active_links() is another way to active all links >> after assoc. >> > Sure. > > johannes
On Tue, 2023-04-18 at 17:44 +0800, Wen Gong wrote: > On 4/18/2023 5:38 PM, Johannes Berg wrote: > > On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote: > > > > > > > > > the sdata->u.mgd.assoc_data is NOT empty, > > > > > > > > > > and the sdata->u.mgd.assoc_data->link[link_id].addr is valid, > > > > > > > > > > it is addr by eth_random_addr(assoc_data->link[i].addr) in > > > > > ieee80211_mgd_assoc(). > > > > > > > > > Exactly, so we've already decided on the address long before we actually > > > > add the link data structure, so your callback would be much too late. > > > > We'd need to have it called from ieee80211_mgd_assoc() already? > > > For the 2nd link, is it OK for me to use the random addr which is set in > > > ieee80211_mgd_assoc(). > > > > > > I only need to set the 1st assoc link in low driver. > > > > > Ah. But does it make sense to restrict the API for that? I mean, if you > > just change the prototype a little bit and call it without the link > > conf, you can easily solve this problem too, no? > Sorry, I am not sure how to solve this problem by remove the link conf > in prototype. Why, then you can have an output parameter for the address, and call it in mac80211 wherever it calls eth_random_addr() today, no? johannes >
(removing HTML) On Tue, 2023-04-18 at 18:42 +0800, Wen Gong wrote: > > > Why, then you can have an output parameter for the address, and call > > it > > in mac80211 wherever it calls eth_random_addr() today, no? > > > OK. Got it. So it is like this, right? > add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, u8 > *link_local_addr, unsigned int link_id)" Seems like that could work, yeah. johannes
On 4/11/2023 3:38 PM, Johannes Berg wrote: > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: >> May I also add a field such as "u16 active_links_count" in struct >> wiphy_iftype_ext_capab, >> and add logic in function ieee80211_set_vif_links_bitmaps() for station >> like this ?: >> if (active_links_count && hweight16(links) <= active_links_count) >> then sdata->vif.active_links = links; >> > Also here, not sure it makes sense in cfg80211 level? > > Though I'm not sure what the idea here is at all - you can refuse to > link switch etc, what would you use this for? > > Then again, we haven't really designed out all the link selection stuff, > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So > depending on what end up doing there, we will obviously need to > advertise some level of link-concurrency to userspace. So will you plan to do something to let wpa_s/userspace app active/deactive links? Or you already have implemented that? > > johannes
On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote: > On 4/11/2023 3:38 PM, Johannes Berg wrote: > > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: > > > May I also add a field such as "u16 active_links_count" in struct > > > wiphy_iftype_ext_capab, > > > and add logic in function ieee80211_set_vif_links_bitmaps() for station > > > like this ?: > > > if (active_links_count && hweight16(links) <= active_links_count) > > > then sdata->vif.active_links = links; > > > > > Also here, not sure it makes sense in cfg80211 level? > > > > Though I'm not sure what the idea here is at all - you can refuse to > > link switch etc, what would you use this for? > > > > Then again, we haven't really designed out all the link selection stuff, > > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So > > depending on what end up doing there, we will obviously need to > > advertise some level of link-concurrency to userspace. > > So will you plan to do something to let wpa_s/userspace app > active/deactive links? > > Or you already have implemented that? > No plans right now, and honestly not sure what the right thing even is. johannes
On 5/10/2023 7:24 PM, Johannes Berg wrote: > On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote: >> On 4/11/2023 3:38 PM, Johannes Berg wrote: >>> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: >>>> May I also add a field such as "u16 active_links_count" in struct >>>> wiphy_iftype_ext_capab, >>>> and add logic in function ieee80211_set_vif_links_bitmaps() for station >>>> like this ?: >>>> if (active_links_count && hweight16(links) <= active_links_count) >>>> then sdata->vif.active_links = links; >>>> >>> Also here, not sure it makes sense in cfg80211 level? >>> >>> Though I'm not sure what the idea here is at all - you can refuse to >>> link switch etc, what would you use this for? >>> >>> Then again, we haven't really designed out all the link selection stuff, >>> do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So >>> depending on what end up doing there, we will obviously need to >>> advertise some level of link-concurrency to userspace. >> So will you plan to do something to let wpa_s/userspace app >> active/deactive links? >> >> Or you already have implemented that? >> > No plans right now, and honestly not sure what the right thing even is. OK. For "advertise some level of link-concurrency to userspace", do you have any plan/idea? > > johannes
On Wed, 2023-05-10 at 20:25 +0800, Wen Gong wrote: > On 5/10/2023 7:24 PM, Johannes Berg wrote: > > On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote: > > > On 4/11/2023 3:38 PM, Johannes Berg wrote: > > > > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote: > > > > > May I also add a field such as "u16 active_links_count" in struct > > > > > wiphy_iftype_ext_capab, > > > > > and add logic in function ieee80211_set_vif_links_bitmaps() for station > > > > > like this ?: > > > > > if (active_links_count && hweight16(links) <= active_links_count) > > > > > then sdata->vif.active_links = links; > > > > > > > > > Also here, not sure it makes sense in cfg80211 level? > > > > > > > > Though I'm not sure what the idea here is at all - you can refuse to > > > > link switch etc, what would you use this for? > > > > > > > > Then again, we haven't really designed out all the link selection stuff, > > > > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So > > > > depending on what end up doing there, we will obviously need to > > > > advertise some level of link-concurrency to userspace. > > > So will you plan to do something to let wpa_s/userspace app > > > active/deactive links? > > > > > > Or you already have implemented that? > > > > > No plans right now, and honestly not sure what the right thing even is. > OK. For "advertise some level of link-concurrency to userspace", do you > have any plan/idea? No, not really. johannes >
On 4/18/2023 5:34 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote: >> Now lower driver I used do not store the key >> > Sure, that's fine. > >> and do not trigger >> BSS_CHANGED_ASSOC for new links after assoc. > I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC > causes is likely no longer correct in MLO. Again, the assoc state > *itself* is only changed once, when the whole MLD associated. > >> So my suggestion is a way to active all links while assoc, this way is >> simple for lower driver I used. > Sure, and we do that. > > But that's not what you're asking - you're asking to re-do some *MLD* > state when a new link is added, and I'm saying that it doesn't make > sense to "add" (again) a key to the MLD that was already added, nor > calling a vif (MLD!) level method saying the MLD changed state to > associated (again). > > I really think you should solve this in the driver, that doesn't mean > you have to _store_ he key, you can use one of the iteration functions > as well. > >> Also ieee80211_set_active_links() is another way to active all links >> after assoc. >> > Sure. Hi Johannes, May I add a new ops in struct ieee80211_ops? like this: u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 new_links)" then ieee80211_set_vif_links_bitmaps() call the ops to get the links for station and set the sdata->vif.active_links with the return value from lower driver, it means lower driver will dynamic select the links count at this moment. If lower driver not register ops active_links, then keep current logic. > > johannes
On 4/18/2023 5:34 PM, Johannes Berg wrote: > On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote: >> Now lower driver I used do not store the key >> > Sure, that's fine. > >> and do not trigger >> BSS_CHANGED_ASSOC for new links after assoc. > I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC > causes is likely no longer correct in MLO. Again, the assoc state > *itself* is only changed once, when the whole MLD associated. > >> So my suggestion is a way to active all links while assoc, this way is >> simple for lower driver I used. > Sure, and we do that. > > But that's not what you're asking - you're asking to re-do some *MLD* > state when a new link is added, and I'm saying that it doesn't make > sense to "add" (again) a key to the MLD that was already added, nor > calling a vif (MLD!) level method saying the MLD changed state to > associated (again). > > I really think you should solve this in the driver, that doesn't mean > you have to _store_ he key, you can use one of the iteration functions > as well. > >> Also ieee80211_set_active_links() is another way to active all links >> after assoc. >> > Sure. May I add a new ops in struct ieee80211_ops? like this: u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 new_links)" then ieee80211_set_vif_links_bitmaps() call the ops to get the links for station and set the sdata->vif.active_links with the return value from lower driver, it means lower driver will dynamic select the links count at this moment. If lower driver not register ops active_links, then keep current logic. > johannes
On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: > > May I add a new ops in struct ieee80211_ops? like this: > > u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 > new_links)" > > then ieee80211_set_vif_links_bitmaps() call the ops to get the links for > station and set the sdata->vif.active_links with the return value from > lower driver, > it means lower driver will dynamic select the links count at this moment. > > If lower driver not register ops active_links, then keep current logic. > I guess you can can send patches for whatever you want :) But I have no idea what you're trying to do? Why would you need to have a callback? Was this for link selection in the driver? We should have a patch somewhere that adds a BSS_CHANGE flag for when the valid links change, so the driver can select others. johannes
On 6/15/2023 2:32 AM, Johannes Berg wrote: > On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: >> May I add a new ops in struct ieee80211_ops? like this: >> >> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 >> new_links)" >> >> then ieee80211_set_vif_links_bitmaps() call the ops to get the links for >> station and set the sdata->vif.active_links with the return value from >> lower driver, >> it means lower driver will dynamic select the links count at this moment. >> >> If lower driver not register ops active_links, then keep current logic. >> > I guess you can can send patches for whatever you want :) > > But I have no idea what you're trying to do? Why would you need to have > a callback? Currently driver could use ieee80211_set_active_links_async() to active links after connection completed. But I would like to allow driver to select active links in a early time, it will be more convenient for driver. > > Was this for link selection in the driver? We should have a patch > somewhere that adds a BSS_CHANGE flag for when the valid links change, > so the driver can select others. > > johannes Yes, it is for link selection in driver at a early time before connection completed. Could you tell detail about how the BSS_CHANGE flag works?
On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote: > On 6/15/2023 2:32 AM, Johannes Berg wrote: > > On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: > > > May I add a new ops in struct ieee80211_ops? like this: > > > > > > u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 > > > new_links)" > > > > > > then ieee80211_set_vif_links_bitmaps() call the ops to get the links for > > > station and set the sdata->vif.active_links with the return value from > > > lower driver, > > > it means lower driver will dynamic select the links count at this moment. > > > > > > If lower driver not register ops active_links, then keep current logic. > > > > > I guess you can can send patches for whatever you want :) > > > > But I have no idea what you're trying to do? Why would you need to have > > a callback? > > Currently driver could use ieee80211_set_active_links_async() to active > links after connection completed. Right. > But I would like to allow driver to select active links in a early time, > it will be more convenient for driver. How so? All you have to do is look for the connection becoming authorized (e.g. sta state for the AP moving to authorized) and then selecting the links you want. We've already been working on that, it's really easy? On the flip-side, it would be highly inconvenient for mac80211 to try to enable more links *during* the association process, and actually it's not even allowed by spec until the 4-way-HS finishes. So the earliest possible time is pretty much when you can just do it in the driver as I just described. > > Was this for link selection in the driver? We should have a patch > > somewhere that adds a BSS_CHANGE flag for when the valid links change, > > so the driver can select others. > > > > johannes > > Yes, it is for link selection in driver at a early time before > connection completed. This is not really allowed ... At least not without also finding ways to really transmit the 802.1X and 4-way-HS only on the right link, etc. > Could you tell detail about how the BSS_CHANGE flag works?
On 6/15/2023 3:56 PM, Johannes Berg wrote: > On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote: >> On 6/15/2023 2:32 AM, Johannes Berg wrote: >>> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: >>>> May I add a new ops in struct ieee80211_ops? like this: >>>> >>>> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 >>>> new_links)" >>>> >>>> then ieee80211_set_vif_links_bitmaps() call the ops to get the links for >>>> station and set the sdata->vif.active_links with the return value from >>>> lower driver, >>>> it means lower driver will dynamic select the links count at this moment. >>>> >>>> If lower driver not register ops active_links, then keep current logic. >>>> >>> I guess you can can send patches for whatever you want :) >>> >>> But I have no idea what you're trying to do? Why would you need to have >>> a callback? >> Currently driver could use ieee80211_set_active_links_async() to active >> links after connection completed. > Right. > >> But I would like to allow driver to select active links in a early time, >> it will be more convenient for driver. > How so? All you have to do is look for the connection becoming > authorized (e.g. sta state for the AP moving to authorized) and then > selecting the links you want. We've already been working on that, it's > really easy? It is more complex for ath12k drivers. > > On the flip-side, it would be highly inconvenient for mac80211 to try to > enable more links *during* the association process, and actually it's > not even allowed by spec until the 4-way-HS finishes. So the earliest > possible time is pretty much when you can just do it in the driver as I > just described. > >>> Was this for link selection in the driver? We should have a patch >>> somewhere that adds a BSS_CHANGE flag for when the valid links change, >>> so the driver can select others. >>> >>> johannes >> Yes, it is for link selection in driver at a early time before >> connection completed. > This is not really allowed ... At least not without also finding ways to > really transmit the 802.1X and 4-way-HS only on the right link, etc. Yes, I also found this in section "2.7.6.1 General" of IEEE P802.11be: "For MLO, if RSNA has not been established, each message of the 4-way handshake shall be sent on the same link used by the latest exchange of successful (Re)Association Request/Response frames." For ath12k drivers, only the primary link(the link used by the latest exchange of successful (Re)Association Request/Response frames) is active before key installed, so the 802.1X and 4-way-HS will always sent on the primary link, so it will meet the rule above of section "2.7.6.1 General". It means: lower driver will ensure the rule when it implmented the active_links callback such as ath12k. >> Could you tell detail about how the BSS_CHANGE flag works?
On 6/21/2023 3:55 PM, Wen Gong wrote: > On 6/15/2023 3:56 PM, Johannes Berg wrote: >> On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote: >>> On 6/15/2023 2:32 AM, Johannes Berg wrote: >>>> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: >>>>> May I add a new ops in struct ieee80211_ops? like this: >>>>> >>>>> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif >>>>> vif, u16 >>>>> new_links)" >>>>> >>>>> then ieee80211_set_vif_links_bitmaps() call the ops to get the >>>>> links for >>>>> station and set the sdata->vif.active_links with the return value >>>>> from >>>>> lower driver, >>>>> it means lower driver will dynamic select the links count at this >>>>> moment. >>>>> >>>>> If lower driver not register ops active_links, then keep current >>>>> logic. >>>>> >>>> I guess you can can send patches for whatever you want :) >>>> >>>> But I have no idea what you're trying to do? Why would you need to >>>> have >>>> a callback? >>> Currently driver could use ieee80211_set_active_links_async() to active >>> links after connection completed. >> Right. >> >>> But I would like to allow driver to select active links in a early >>> time, >>> it will be more convenient for driver. >> How so? All you have to do is look for the connection becoming >> authorized (e.g. sta state for the AP moving to authorized) and then >> selecting the links you want. We've already been working on that, it's >> really easy? > It is more complex for ath12k drivers. >> >> On the flip-side, it would be highly inconvenient for mac80211 to try to >> enable more links *during* the association process, and actually it's >> not even allowed by spec until the 4-way-HS finishes. So the earliest >> possible time is pretty much when you can just do it in the driver as I >> just described. >> >>>> Was this for link selection in the driver? We should have a patch >>>> somewhere that adds a BSS_CHANGE flag for when the valid links change, >>>> so the driver can select others. >>>> >>>> johannes >>> Yes, it is for link selection in driver at a early time before >>> connection completed. >> This is not really allowed ... At least not without also finding ways to >> really transmit the 802.1X and 4-way-HS only on the right link, etc. > Yes, I also found this in section "2.7.6.1 General" of IEEE P802.11be: > "For MLO, if RSNA has not been established, each message of the 4-way > handshake shall be sent on > the same link used by the latest exchange of successful > (Re)Association Request/Response frames." > > For ath12k drivers, only the primary link(the link used by the latest > exchange of successful (Re)Association > Request/Response frames) is active before key installed, so the 802.1X > and 4-way-HS will always sent on > the primary link, so it will meet the rule above of section "2.7.6.1 > General". > > It means: lower driver will ensure the rule when it implmented the > active_links callback such as ath12k. Johannes, do you have more comments for my answer above?
On 6/15/2023 3:56 PM, Johannes Berg wrote: > On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote: >> On 6/15/2023 2:32 AM, Johannes Berg wrote: >>> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote: >>> ... >> Could you tell detail about how the BSS_CHANGE flag works?
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index d4e1d73d88cc..20a2f25a38fa 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1799,6 +1799,9 @@ struct ieee80211_vif_cfg { * @link_conf: in case of MLD, the per-link BSS configuration, * indexed by link ID * @valid_links: bitmap of valid links, or 0 for non-MLO. + * @active_links: The bitmap of active links, or 0 for non-MLO. + * The driver shouldn't change this directly, but use the + * API calls meant for that purpose. * @addr: address of this interface * @p2p: indicates whether this AP or STA interface is a p2p * interface, i.e. a GO or p2p-sta respectively @@ -1834,7 +1837,7 @@ struct ieee80211_vif { struct ieee80211_vif_cfg cfg; struct ieee80211_bss_conf bss_conf; struct ieee80211_bss_conf __rcu *link_conf[IEEE80211_MLD_MAX_NUM_LINKS]; - u16 valid_links; + u16 valid_links, active_links; u8 addr[ETH_ALEN] __aligned(2); bool p2p; @@ -1861,12 +1864,11 @@ struct ieee80211_vif { u8 drv_priv[] __aligned(sizeof(void *)); }; -/* FIXME: for now loop over all the available links; later will be changed - * to loop only over the active links. - */ -#define for_each_vif_active_link(vif, link, link_id) \ - for (link_id = 0; link_id < ARRAY_SIZE((vif)->link_conf); link_id++) \ - if ((link = rcu_dereference((vif)->link_conf[link_id]))) +#define for_each_vif_active_link(vif, link, link_id) \ + for (link_id = 0; link_id < ARRAY_SIZE((vif)->link_conf); link_id++) \ + if ((!(vif)->active_links || \ + (vif)->active_links & BIT(link_id)) && \ + (link = rcu_dereference((vif)->link_conf[link_id]))) static inline bool ieee80211_vif_is_mesh(struct ieee80211_vif *vif) { @@ -2264,13 +2266,13 @@ struct ieee80211_sta { u8 drv_priv[] __aligned(sizeof(void *)); }; -/* FIXME: need to loop only over links which are active and check the actual - * lock - */ -#define for_each_sta_active_link(sta, link_sta, link_id) \ - for (link_id = 0; link_id < ARRAY_SIZE((sta)->link); link_id++) \ - if (((link_sta) = rcu_dereference_protected((sta)->link[link_id],\ - 1))) \ +/* FIXME: check the locking correctly */ +#define for_each_sta_active_link(vif, sta, link_sta, link_id) \ + for (link_id = 0; link_id < ARRAY_SIZE((sta)->link); link_id++) \ + if ((!(vif)->active_links || \ + (vif)->active_links & BIT(link_id)) && \ + ((link_sta) = rcu_dereference_protected((sta)->link[link_id],\ + 1))) /** * enum sta_notify_cmd - sta notify command diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index f247daa41563..e72cf0749d49 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -1799,6 +1799,12 @@ int ieee80211_link_use_channel(struct ieee80211_link_data *link, lockdep_assert_held(&local->mtx); + if (sdata->vif.active_links && + !(sdata->vif.active_links & BIT(link->link_id))) { + ieee80211_link_update_chandef(link, chandef); + return 0; + } + mutex_lock(&local->chanctx_mtx); ret = cfg80211_chandef_dfs_required(local->hw.wiphy, diff --git a/net/mac80211/driver-ops.c b/net/mac80211/driver-ops.c index 9b61dc7889c2..5392ffa18270 100644 --- a/net/mac80211/driver-ops.c +++ b/net/mac80211/driver-ops.c @@ -192,6 +192,10 @@ int drv_conf_tx(struct ieee80211_local *local, if (!check_sdata_in_driver(sdata)) return -EIO; + if (sdata->vif.active_links && + !(sdata->vif.active_links & BIT(link->link_id))) + return 0; + if (params->cw_min == 0 || params->cw_min > params->cw_max) { /* * If we can't configure hardware anyway, don't warn. We may @@ -272,6 +276,60 @@ void drv_reset_tsf(struct ieee80211_local *local, trace_drv_return_void(local); } +int drv_assign_vif_chanctx(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + struct ieee80211_chanctx *ctx) +{ + int ret = 0; + + drv_verify_link_exists(sdata, link_conf); + if (!check_sdata_in_driver(sdata)) + return -EIO; + + if (sdata->vif.active_links && + !(sdata->vif.active_links & BIT(link_conf->link_id))) + return 0; + + trace_drv_assign_vif_chanctx(local, sdata, link_conf, ctx); + if (local->ops->assign_vif_chanctx) { + WARN_ON_ONCE(!ctx->driver_present); + ret = local->ops->assign_vif_chanctx(&local->hw, + &sdata->vif, + link_conf, + &ctx->conf); + } + trace_drv_return_int(local, ret); + + return ret; +} + +void drv_unassign_vif_chanctx(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + struct ieee80211_chanctx *ctx) +{ + might_sleep(); + + drv_verify_link_exists(sdata, link_conf); + if (!check_sdata_in_driver(sdata)) + return; + + if (sdata->vif.active_links && + !(sdata->vif.active_links & BIT(link_conf->link_id))) + return; + + trace_drv_unassign_vif_chanctx(local, sdata, link_conf, ctx); + if (local->ops->unassign_vif_chanctx) { + WARN_ON_ONCE(!ctx->driver_present); + local->ops->unassign_vif_chanctx(&local->hw, + &sdata->vif, + link_conf, + &ctx->conf); + } + trace_drv_return_void(local); +} + int drv_switch_vif_chanctx(struct ieee80211_local *local, struct ieee80211_vif_chanctx_switch *vifs, int n_vifs, enum ieee80211_chanctx_switch_mode mode) @@ -346,3 +404,117 @@ int drv_ampdu_action(struct ieee80211_local *local, return ret; } + +void drv_link_info_changed(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *info, + int link_id, u64 changed) +{ + might_sleep(); + + if (WARN_ON_ONCE(changed & (BSS_CHANGED_BEACON | + BSS_CHANGED_BEACON_ENABLED) && + sdata->vif.type != NL80211_IFTYPE_AP && + sdata->vif.type != NL80211_IFTYPE_ADHOC && + sdata->vif.type != NL80211_IFTYPE_MESH_POINT && + sdata->vif.type != NL80211_IFTYPE_OCB)) + return; + + if (WARN_ON_ONCE(sdata->vif.type == NL80211_IFTYPE_P2P_DEVICE || + sdata->vif.type == NL80211_IFTYPE_NAN || + (sdata->vif.type == NL80211_IFTYPE_MONITOR && + !sdata->vif.bss_conf.mu_mimo_owner && + !(changed & BSS_CHANGED_TXPOWER)))) + return; + + if (!check_sdata_in_driver(sdata)) + return; + + if (sdata->vif.active_links && + !(sdata->vif.active_links & BIT(link_id))) + return; + + trace_drv_link_info_changed(local, sdata, info, changed); + if (local->ops->link_info_changed) + local->ops->link_info_changed(&local->hw, &sdata->vif, + info, changed); + else if (local->ops->bss_info_changed) + local->ops->bss_info_changed(&local->hw, &sdata->vif, + info, changed); + trace_drv_return_void(local); +} + +int drv_set_key(struct ieee80211_local *local, + enum set_key_cmd cmd, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + struct ieee80211_key_conf *key) +{ + int ret; + + might_sleep(); + + sdata = get_bss_sdata(sdata); + if (!check_sdata_in_driver(sdata)) + return -EIO; + + if (WARN_ON(key->link_id >= 0 && sdata->vif.active_links && + !(sdata->vif.active_links & BIT(key->link_id)))) + return -ENOLINK; + + trace_drv_set_key(local, cmd, sdata, sta, key); + ret = local->ops->set_key(&local->hw, cmd, &sdata->vif, sta, key); + trace_drv_return_int(local, ret); + return ret; +} + +int drv_change_vif_links(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + u16 old_links, u16 new_links, + struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS]) +{ + int ret = -EOPNOTSUPP; + + might_sleep(); + + if (!check_sdata_in_driver(sdata)) + return -EIO; + + if (old_links == new_links) + return 0; + + trace_drv_change_vif_links(local, sdata, old_links, new_links); + if (local->ops->change_vif_links) + ret = local->ops->change_vif_links(&local->hw, &sdata->vif, + old_links, new_links, old); + trace_drv_return_int(local, ret); + + return ret; +} + +int drv_change_sta_links(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + u16 old_links, u16 new_links) +{ + int ret = -EOPNOTSUPP; + + might_sleep(); + + if (!check_sdata_in_driver(sdata)) + return -EIO; + + old_links &= sdata->vif.active_links; + new_links &= sdata->vif.active_links; + + if (old_links == new_links) + return 0; + + trace_drv_change_sta_links(local, sdata, sta, old_links, new_links); + if (local->ops->change_sta_links) + ret = local->ops->change_sta_links(&local->hw, &sdata->vif, sta, + old_links, new_links); + trace_drv_return_int(local, ret); + + return ret; +} diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index 482f5c97a72b..81e40b0a3b16 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -165,40 +165,10 @@ static inline void drv_vif_cfg_changed(struct ieee80211_local *local, trace_drv_return_void(local); } -static inline void drv_link_info_changed(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_bss_conf *info, - int link_id, u64 changed) -{ - might_sleep(); - - if (WARN_ON_ONCE(changed & (BSS_CHANGED_BEACON | - BSS_CHANGED_BEACON_ENABLED) && - sdata->vif.type != NL80211_IFTYPE_AP && - sdata->vif.type != NL80211_IFTYPE_ADHOC && - sdata->vif.type != NL80211_IFTYPE_MESH_POINT && - sdata->vif.type != NL80211_IFTYPE_OCB)) - return; - - if (WARN_ON_ONCE(sdata->vif.type == NL80211_IFTYPE_P2P_DEVICE || - sdata->vif.type == NL80211_IFTYPE_NAN || - (sdata->vif.type == NL80211_IFTYPE_MONITOR && - !sdata->vif.bss_conf.mu_mimo_owner && - !(changed & BSS_CHANGED_TXPOWER)))) - return; - - if (!check_sdata_in_driver(sdata)) - return; - - trace_drv_link_info_changed(local, sdata, info, changed); - if (local->ops->link_info_changed) - local->ops->link_info_changed(&local->hw, &sdata->vif, - info, changed); - else if (local->ops->bss_info_changed) - local->ops->bss_info_changed(&local->hw, &sdata->vif, - info, changed); - trace_drv_return_void(local); -} +void drv_link_info_changed(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *info, + int link_id, u64 changed); static inline u64 drv_prepare_multicast(struct ieee80211_local *local, struct netdev_hw_addr_list *mc_list) @@ -256,25 +226,11 @@ static inline int drv_set_tim(struct ieee80211_local *local, return ret; } -static inline int drv_set_key(struct ieee80211_local *local, - enum set_key_cmd cmd, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_sta *sta, - struct ieee80211_key_conf *key) -{ - int ret; - - might_sleep(); - - sdata = get_bss_sdata(sdata); - if (!check_sdata_in_driver(sdata)) - return -EIO; - - trace_drv_set_key(local, cmd, sdata, sta, key); - ret = local->ops->set_key(&local->hw, cmd, &sdata->vif, sta, key); - trace_drv_return_int(local, ret); - return ret; -} +int drv_set_key(struct ieee80211_local *local, + enum set_key_cmd cmd, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + struct ieee80211_key_conf *key); static inline void drv_update_tkip_key(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata, @@ -945,52 +901,14 @@ static inline void drv_verify_link_exists(struct ieee80211_sub_if_data *sdata, sdata_assert_lock(sdata); } -static inline int drv_assign_vif_chanctx(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_bss_conf *link_conf, - struct ieee80211_chanctx *ctx) -{ - int ret = 0; - - drv_verify_link_exists(sdata, link_conf); - if (!check_sdata_in_driver(sdata)) - return -EIO; - - trace_drv_assign_vif_chanctx(local, sdata, link_conf, ctx); - if (local->ops->assign_vif_chanctx) { - WARN_ON_ONCE(!ctx->driver_present); - ret = local->ops->assign_vif_chanctx(&local->hw, - &sdata->vif, - link_conf, - &ctx->conf); - } - trace_drv_return_int(local, ret); - - return ret; -} - -static inline void drv_unassign_vif_chanctx(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_bss_conf *link_conf, - struct ieee80211_chanctx *ctx) -{ - might_sleep(); - - drv_verify_link_exists(sdata, link_conf); - if (!check_sdata_in_driver(sdata)) - return; - - trace_drv_unassign_vif_chanctx(local, sdata, link_conf, ctx); - if (local->ops->unassign_vif_chanctx) { - WARN_ON_ONCE(!ctx->driver_present); - local->ops->unassign_vif_chanctx(&local->hw, - &sdata->vif, - link_conf, - &ctx->conf); - } - trace_drv_return_void(local); -} - +int drv_assign_vif_chanctx(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + struct ieee80211_chanctx *ctx); +void drv_unassign_vif_chanctx(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_bss_conf *link_conf, + struct ieee80211_chanctx *ctx); int drv_switch_vif_chanctx(struct ieee80211_local *local, struct ieee80211_vif_chanctx_switch *vifs, int n_vifs, enum ieee80211_chanctx_switch_mode mode); @@ -1552,46 +1470,13 @@ static inline int drv_net_fill_forward_path(struct ieee80211_local *local, return ret; } -static inline int drv_change_vif_links(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - u16 old_links, u16 new_links, - struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS]) -{ - int ret = -EOPNOTSUPP; - - might_sleep(); - - if (!check_sdata_in_driver(sdata)) - return -EIO; - - trace_drv_change_vif_links(local, sdata, old_links, new_links); - if (local->ops->change_vif_links) - ret = local->ops->change_vif_links(&local->hw, &sdata->vif, - old_links, new_links, old); - trace_drv_return_int(local, ret); - - return ret; -} - -static inline int drv_change_sta_links(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata, - struct ieee80211_sta *sta, - u16 old_links, u16 new_links) -{ - int ret = -EOPNOTSUPP; - - might_sleep(); - - if (!check_sdata_in_driver(sdata)) - return -EIO; - - trace_drv_change_sta_links(local, sdata, sta, old_links, new_links); - if (local->ops->change_sta_links) - ret = local->ops->change_sta_links(&local->hw, &sdata->vif, sta, - old_links, new_links); - trace_drv_return_int(local, ret); - - return ret; -} +int drv_change_vif_links(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + u16 old_links, u16 new_links, + struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS]); +int drv_change_sta_links(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + u16 old_links, u16 new_links); #endif /* __MAC80211_DRIVER_OPS */ diff --git a/net/mac80211/key.c b/net/mac80211/key.c index d89ec93b243b..f6f0f65fb255 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -177,6 +177,10 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) } } + if (key->conf.link_id >= 0 && sdata->vif.active_links && + !(sdata->vif.active_links & BIT(key->conf.link_id))) + return 0; + ret = drv_set_key(key->local, SET_KEY, sdata, sta ? &sta->sta : NULL, &key->conf); @@ -246,6 +250,10 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) sta = key->sta; sdata = key->sdata; + if (key->conf.link_id >= 0 && sdata->vif.active_links && + !(sdata->vif.active_links & BIT(key->conf.link_id))) + return; + if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC | IEEE80211_KEY_FLAG_PUT_MIC_SPACE | IEEE80211_KEY_FLAG_RESERVE_TAILROOM))) diff --git a/net/mac80211/link.c b/net/mac80211/link.c index 096f313c2a6e..8df348a5edce 100644 --- a/net/mac80211/link.c +++ b/net/mac80211/link.c @@ -73,28 +73,37 @@ struct link_container { struct ieee80211_bss_conf conf; }; -static void ieee80211_free_links(struct ieee80211_sub_if_data *sdata, - struct link_container **links) +static void ieee80211_tear_down_links(struct ieee80211_sub_if_data *sdata, + struct link_container **links, u16 mask) { + struct ieee80211_link_data *link; LIST_HEAD(keys); unsigned int link_id; for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) { - if (!links[link_id]) + if (!(mask & BIT(link_id))) continue; - ieee80211_remove_link_keys(&links[link_id]->data, &keys); + link = &links[link_id]->data; + if (link_id == 0 && !link) + link = &sdata->deflink; + if (WARN_ON(!link)) + continue; + ieee80211_remove_link_keys(link, &keys); + ieee80211_link_stop(link); } synchronize_rcu(); ieee80211_free_key_list(sdata->local, &keys); +} - for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) { - if (!links[link_id]) - continue; - ieee80211_link_stop(&links[link_id]->data); +static void ieee80211_free_links(struct ieee80211_sub_if_data *sdata, + struct link_container **links) +{ + unsigned int link_id; + + for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) kfree(links[link_id]); - } } static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata) @@ -123,11 +132,38 @@ static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata) return 0; } +static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, + u16 links) +{ + sdata->vif.valid_links = links; + + if (!links) { + sdata->vif.active_links = 0; + return; + } + + switch (sdata->vif.type) { + case NL80211_IFTYPE_AP: + /* in an AP all links are always active */ + sdata->vif.active_links = links; + break; + case NL80211_IFTYPE_STATION: + if (sdata->vif.active_links) + break; + WARN_ON(hweight16(links) > 1); + sdata->vif.active_links = links; + break; + default: + WARN_ON(1); + } +} + static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata, struct link_container **to_free, u16 new_links) { u16 old_links = sdata->vif.valid_links; + u16 old_active = sdata->vif.active_links; unsigned long add = new_links & ~old_links; unsigned long rem = old_links & ~new_links; unsigned int link_id; @@ -195,13 +231,17 @@ static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata, ieee80211_link_init(sdata, -1, &sdata->deflink, &sdata->vif.bss_conf); - sdata->vif.valid_links = new_links; - ret = ieee80211_check_dup_link_addrs(sdata); if (!ret) { + /* for keys we will not be able to undo this */ + ieee80211_tear_down_links(sdata, to_free, rem); + + ieee80211_set_vif_links_bitmaps(sdata, new_links); + /* tell the driver */ ret = drv_change_vif_links(sdata->local, sdata, - old_links, new_links, + old_links & old_active, + new_links & sdata->vif.active_links, old); } @@ -209,7 +249,7 @@ static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata, /* restore config */ memcpy(sdata->link, old_data, sizeof(old_data)); memcpy(sdata->vif.link_conf, old, sizeof(old)); - sdata->vif.valid_links = old_links; + ieee80211_set_vif_links_bitmaps(sdata, old_links); /* and free (only) the newly allocated links */ memset(to_free, 0, sizeof(links)); goto free; diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 4c8bbf0bd25b..30edac8724d5 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -4043,11 +4043,11 @@ static bool ieee80211_assoc_config_link(struct ieee80211_link_data *link, goto out; } - sband = ieee80211_get_link_sband(link); - if (!sband) { + if (WARN_ON(!link->conf->chandef.chan)) { ret = false; goto out; } + sband = local->hw.wiphy->bands[link->conf->chandef.chan->band]; if (!(link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_HE) && (!elems->he_cap || !elems->he_operation)) { @@ -4871,8 +4871,10 @@ static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata, err = ieee80211_prep_channel(sdata, link, assoc_data->link[link_id].bss, &link->u.mgd.conn_flags); - if (err) + if (err) { + link_info(link, "prep_channel failed\n"); goto out_err; + } } err = ieee80211_mgd_setup_link_sta(link, sta, link_sta, @@ -6876,23 +6878,6 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) size += req->links[i].elems_len; - if (req->ap_mld_addr) { - for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) { - if (!req->links[i].bss) - continue; - if (i == assoc_link_id) - continue; - /* - * For now, support only a single link in MLO, we - * don't have the necessary parsing of the multi- - * link element in the association response, etc. - */ - sdata_info(sdata, - "refusing MLO association with >1 links\n"); - return -EINVAL; - } - } - assoc_data = kzalloc(size, GFP_KERNEL); if (!assoc_data) return -ENOMEM; diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 8b4c6b7abafa..0eed18189491 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -2902,7 +2902,7 @@ void ieee80211_recalc_min_chandef(struct ieee80211_sub_if_data *sdata, */ rcu_read_unlock(); - if (WARN_ON_ONCE(!chanctx_conf)) + if (!chanctx_conf) goto unlock; chanctx = container_of(chanctx_conf, struct ieee80211_chanctx,