| Message ID | 20230906103458.24092-3-quic_wgong@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mac80211: change to match driver to support MLO connection | expand |
On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: > There are 2 fields valid_links and active_links in struct ieee80211_vif > of mac80211. For station mode, valid_links is always include the bitmap > of active_links. valid_links stores the bitmap of links which is created > in mac80211, and mac80211 only indicate the info of links which is existed > in active_links to driver. Finally, the active_links is the bitmap of > links which station can interactive with MLO AP it is connected to. > > Currently the active links is always only contain the primary link, > primary link means the link used by the latest exchange of successful > (Re)Association Request/Response frames. Then it will always have only > one link in driver after connected. > > Hence add this ops in struct ieee80211_ops to allow driver to determine > the active links bit map dynamically while connecting to MLO AP. This commit log does nothing to address the question "why do we need this", particularly since we already have ieee80211_set_active_links(/_async), so the driver can pretty much immediately enable all the links it wants. I see no value in this patch whatsoever. > @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, > WARN_ON(dormant_links); > break; > case NL80211_IFTYPE_STATION: > + active_links = drv_calculate_active_links(sdata->local, sdata, > + valid_links & ~dormant_links); > + if (active_links) { > + sdata->vif.active_links = active_links; > + break; > + } > I also _really_ don't think this should operate at this low-level infrastructure. johannes
On 9/13/2023 4:58 PM, Johannes Berg wrote: > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote: >> There are 2 fields valid_links and active_links in struct ieee80211_vif >> of mac80211. For station mode, valid_links is always include the bitmap >> of active_links. valid_links stores the bitmap of links which is created >> in mac80211, and mac80211 only indicate the info of links which is existed >> in active_links to driver. Finally, the active_links is the bitmap of >> links which station can interactive with MLO AP it is connected to. >> >> Currently the active links is always only contain the primary link, >> primary link means the link used by the latest exchange of successful >> (Re)Association Request/Response frames. Then it will always have only >> one link in driver after connected. >> >> Hence add this ops in struct ieee80211_ops to allow driver to determine >> the active links bit map dynamically while connecting to MLO AP. > This commit log does nothing to address the question "why do we need > this", particularly since we already have > ieee80211_set_active_links(/_async), so the driver can pretty much > immediately enable all the links it wants. > > I see no value in this patch whatsoever. For ieee80211_set_active_links(), driver need save and install pairwise keys for the other links as you said in link below. But driver do not save the key data currently. So driver could not install the pairwise keys by itself. https://lore.kernel.org/linux-wireless/50719d34bc48d816d00b56d3d9efdb59e3e51a16.camel@sipsolutions.net/ And driver could ensure this rule in spec as I said before in link below: "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." https://lore.kernel.org/linux-wireless/b6314b61-0700-0e1c-7b39-21c305dc28b3@quicinc.com/ It will be complext for driver/ath12k to use ieee80211_set_active_links(/_async). > >> @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, >> WARN_ON(dormant_links); >> break; >> case NL80211_IFTYPE_STATION: >> + active_links = drv_calculate_active_links(sdata->local, sdata, >> + valid_links & ~dormant_links); >> + if (active_links) { >> + sdata->vif.active_links = active_links; >> + break; >> + } >> > I also _really_ don't think this should operate at this low-level > infrastructure. I really want to know the reason why active *only* the assoc link for station, but active *all* links for AP here😁? > johannes
> For ieee80211_set_active_links(), driver need save and install pairwise > keys for the other links as you said in link below. But driver do not > save the key data currently. So driver could not install the pairwise > keys by itself. > https://lore.kernel.org/linux-wireless/50719d34bc48d816d00b56d3d9efdb59e3e51a16.camel@sipsolutions.net/ Well it can still easily iterate the keys, but OK, I can buy this argument. You should probably mention it all in the commit log though. > > > @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, > > > WARN_ON(dormant_links); > > > break; > > > case NL80211_IFTYPE_STATION: > > > + active_links = drv_calculate_active_links(sdata->local, sdata, > > > + valid_links & ~dormant_links); > > > + if (active_links) { > > > + sdata->vif.active_links = active_links; > > > + break; > > > + } > > > > > I also _really_ don't think this should operate at this low-level > > infrastructure. > > I really want to know the reason why active *only* the assoc link for > station, but > > active *all* links for AP here😁? Well, fair point. I was somehow anyway thinking that it should be the other way around, I mean, in the sense that this code here is just mechanism, and the policy should've been in higher-level code ... but somehow that's not what we ended up doign here. It really feels wrong to me to have the driver involved in this low- level infrastructure, but OTOH that really is how it works now? Maybe really what you want is a driver flag saying IEEE80211_HW_MANAGES_ACTIVE_LINKS or something, and then from a mac80211 perspective you don't set active links at all? Though I'm not sure that works with TTLM, for instance? And maybe you'd still want the debugfs? But in fact, if you do still want something from mac80211 for debugfs or TTLM, then this code here is not really right, since you'd interact with _all_ attempts in mac80211 to activate or deactivate links - when all you really wanted was the very first one. Which today mac80211 achieves by bailing out "if (sdata->vif.active_links)", and while you could do the same in the driver, it feels really brittle / error-prone to me. I think it'd be better than all these callbacks to encode the specific behaviour you need in a flag, e.g. "activate all STA links on assoc" or something? johannes
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index c7f92a3db359..c50f1a99ccd5 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -3732,6 +3732,14 @@ struct ieee80211_prep_tx_info { * Note: this callback is called if @vif_cfg_changed or @link_info_changed * are not implemented. * + * @calculate_active_links: Prepare for bit maps of links to active. + * This callback is optional. The @new_links parameter is all links bit map + * that mac80211 has capability to activate. Returns non-zero if driver handled + * the @new_links, and the returned non-zero value is the bit map of the links + * that driver allows to active. The bitmap of returned non-zero value may be + * a subset of the @new_links. Return zero if driver not handled this. + * This callback can sleep. + * * @vif_cfg_changed: Handler for configuration requests related to interface * (MLD) parameters from &struct ieee80211_vif_cfg that vary during the * lifetime of the interface (e.g. assoc status, IP addresses, etc.) @@ -4295,6 +4303,8 @@ struct ieee80211_ops { struct ieee80211_vif *vif, struct ieee80211_bss_conf *info, u64 changed); + u16 (*calculate_active_links)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, + u16 new_links); void (*vif_cfg_changed)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, u64 changed); diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index da164d4ca9a1..d2a8bf5341b5 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -950,6 +950,27 @@ static inline void drv_verify_link_exists(struct ieee80211_sub_if_data *sdata, sdata_assert_lock(sdata); } +static inline u16 drv_calculate_active_links(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + u16 new_links) +{ + u16 active_links = 0; + + might_sleep(); + + if (!check_sdata_in_driver(sdata)) + return active_links; + + trace_drv_calculate_active_links(local, sdata, new_links); + if (local->ops->calculate_active_links) + active_links = local->ops->calculate_active_links(&local->hw, + &sdata->vif, + new_links); + + trace_drv_return_int(local, active_links); + return active_links; +} + int drv_assign_vif_chanctx(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata, struct ieee80211_bss_conf *link_conf, diff --git a/net/mac80211/link.c b/net/mac80211/link.c index 6148208b320e..7ce229a64a7b 100644 --- a/net/mac80211/link.c +++ b/net/mac80211/link.c @@ -146,6 +146,7 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, { sdata->vif.valid_links = valid_links; sdata->vif.dormant_links = dormant_links; + u16 active_links; if (!valid_links || WARN((~valid_links & dormant_links) || @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata, WARN_ON(dormant_links); break; case NL80211_IFTYPE_STATION: + active_links = drv_calculate_active_links(sdata->local, sdata, + valid_links & ~dormant_links); + if (active_links) { + sdata->vif.active_links = active_links; + break; + } + if (sdata->vif.active_links) break; sdata->vif.active_links = valid_links & ~dormant_links; diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h index b140a4e70df9..eb458c236101 100644 --- a/net/mac80211/trace.h +++ b/net/mac80211/trace.h @@ -652,6 +652,29 @@ TRACE_EVENT(drv_set_key, ) ); +TRACE_EVENT(drv_calculate_active_links, + TP_PROTO(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + u16 new_links), + + TP_ARGS(local, sdata, new_links), + + TP_STRUCT__entry( + LOCAL_ENTRY + VIF_ENTRY + __field(unsigned int, new_links) + ), + + TP_fast_assign( + LOCAL_ASSIGN; + VIF_ASSIGN; + __entry->new_links = new_links; + ), + + TP_printk(LOCAL_PR_FMT ", " VIF_PR_FMT ", new_links: %u", + LOCAL_PR_ARG, VIF_PR_ARG, __entry->new_links) +); + TRACE_EVENT(drv_update_tkip_key, TP_PROTO(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata,
There are 2 fields valid_links and active_links in struct ieee80211_vif of mac80211. For station mode, valid_links is always include the bitmap of active_links. valid_links stores the bitmap of links which is created in mac80211, and mac80211 only indicate the info of links which is existed in active_links to driver. Finally, the active_links is the bitmap of links which station can interactive with MLO AP it is connected to. Currently the active links is always only contain the primary link, primary link means the link used by the latest exchange of successful (Re)Association Request/Response frames. Then it will always have only one link in driver after connected. Hence add this ops in struct ieee80211_ops to allow driver to determine the active links bit map dynamically while connecting to MLO AP. Signed-off-by: Wen Gong <quic_wgong@quicinc.com> --- include/net/mac80211.h | 10 ++++++++++ net/mac80211/driver-ops.h | 21 +++++++++++++++++++++ net/mac80211/link.c | 8 ++++++++ net/mac80211/trace.h | 23 +++++++++++++++++++++++ 4 files changed, 62 insertions(+)