| Message ID | 9b331a61b8d53284b044bc594cf9952c60164e5f.1717696995.git-series.nbd@nbd.name (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211/mac80211: support defining multiple radios per wiphy | expand |
On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote: > > +static bool > +ieee80211_radio_freq_match(const struct wiphy_radio *radio, > + const struct ieee80211_chan_req *chanreq) > +{ > + const struct wiphy_radio_freq_range *r; > + u32 freq; > + int i; > + > + freq = ieee80211_channel_to_khz(chanreq->oper.chan); > + for (i = 0; i < radio->n_freq_range; i++) { > + r = &radio->freq_range[i]; > + if (freq >= r->start_freq && freq <= r->end_freq) > + return true; IMHO should be inclusive only on one side of the range. Can always make it work since channels are at least a few MHz apart, but the purist in me says it's easier to grok if the end is not inclusive :) Maybe this should be a cfg80211 helper like struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef); which could also check that the _whole_ chandef fits (though that should probably also be handled elsewhere, like chandef_valid), and you can use it to get the radio pointer and then check for == below. The point would be to have a single place with this kind of range logic. This is only going to get done by mac80211 now, but it'd really be awkward if some other driver had some other logic later. > if (!curr_ctx || (curr_ctx->replace_state == > IEEE80211_CHANCTX_WILL_BE_REPLACED) || > @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local, > if (!list_empty(&ctx->reserved_links)) > continue; > > + if (ctx->conf.radio_idx >= 0) { > + radio = &wiphy->radio[ctx->conf.radio_idx]; > + if (!ieee80211_radio_freq_match(radio, chanreq)) > + continue; > + } something happened to indentation here :) > +static bool > +ieee80211_find_available_radio(struct ieee80211_local *local, > + const struct ieee80211_chan_req *chanreq, > + int *radio_idx) > +{ > + struct wiphy *wiphy = local->hw.wiphy; > + const struct wiphy_radio *radio; > + int i; > + > + *radio_idx = -1; > + if (!wiphy->n_radio) > + return true; > + > + for (i = 0; i < wiphy->n_radio; i++) { > + radio = &wiphy->radio[i]; > + if (!ieee80211_radio_freq_match(radio, chanreq)) > + continue; > + > + if (!ieee80211_can_create_new_chanctx(local, i)) > + continue; > + > + *radio_idx = i; > + return true; > + } > + > + return false; > +} which would also get used here to find the radio first, though would have to differentiate n_radio still, of course. johannes
On 07.06.24 11:44, Johannes Berg wrote: > On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote: >> >> +static bool >> +ieee80211_radio_freq_match(const struct wiphy_radio *radio, >> + const struct ieee80211_chan_req *chanreq) >> +{ >> + const struct wiphy_radio_freq_range *r; >> + u32 freq; >> + int i; >> + >> + freq = ieee80211_channel_to_khz(chanreq->oper.chan); >> + for (i = 0; i < radio->n_freq_range; i++) { >> + r = &radio->freq_range[i]; >> + if (freq >= r->start_freq && freq <= r->end_freq) >> + return true; > > IMHO should be inclusive only on one side of the range. Can always make > it work since channels are at least a few MHz apart, but the purist in > me says it's easier to grok if the end is not inclusive :) Sure, no problem. > Maybe this should be a cfg80211 helper like > > struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef); I didn't add such a helper, in case we get hardware where multiple radios support the same band. That's why ieee80211_find_available_radio loops over all radios until it finds one that matches both the freq range and the ifcomb constraints. > which could also check that the _whole_ chandef fits (though that should > probably also be handled elsewhere, like chandef_valid), and you can use > it to get the radio pointer and then check for == below. > > The point would be to have a single place with this kind of range logic. > This is only going to get done by mac80211 now, but it'd really be > awkward if some other driver had some other logic later. I will move a variation of the freq range match helper to cfg80211. >> if (!curr_ctx || (curr_ctx->replace_state == >> IEEE80211_CHANCTX_WILL_BE_REPLACED) || >> @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local, >> if (!list_empty(&ctx->reserved_links)) >> continue; >> >> + if (ctx->conf.radio_idx >= 0) { >> + radio = &wiphy->radio[ctx->conf.radio_idx]; >> + if (!ieee80211_radio_freq_match(radio, chanreq)) >> + continue; >> + } > > something happened to indentation here :) Will fix :) >> +static bool >> +ieee80211_find_available_radio(struct ieee80211_local *local, >> + const struct ieee80211_chan_req *chanreq, >> + int *radio_idx) >> +{ >> + struct wiphy *wiphy = local->hw.wiphy; >> + const struct wiphy_radio *radio; >> + int i; >> + >> + *radio_idx = -1; >> + if (!wiphy->n_radio) >> + return true; >> + >> + for (i = 0; i < wiphy->n_radio; i++) { >> + radio = &wiphy->radio[i]; >> + if (!ieee80211_radio_freq_match(radio, chanreq)) >> + continue; >> + >> + if (!ieee80211_can_create_new_chanctx(local, i)) >> + continue; >> + >> + *radio_idx = i; >> + return true; >> + } >> + >> + return false; >> +} > > which would also get used here to find the radio first, though would > have to differentiate n_radio still, of course. See above - Felix
On Fri, 2024-06-07 at 11:53 +0200, Felix Fietkau wrote: > > struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef); > > I didn't add such a helper, in case we get hardware where multiple > radios support the same band. That's why ieee80211_find_available_radio > loops over all radios until it finds one that matches both the freq > range and the ifcomb constraints. Ah, fair. Thinking more about the "whole chandef" thing, I think I want to have a check in cfg80211 somewhere that ensures you don't split up ranges that could be used for a wider channel? Say (for a stupid example) you have a device that (only) supports channels 36-40: * 5180 * 5200 but now you say it has two radios: * radio 1 ranges: 5170-5190 * radio 2 ranges: 5190-5210 Now you can't use 40 MHz... but nothing will actually really prevent it. Obviously this is a totally useless case, so I'd argue we should just check during wiphy registration that you don't split the channel list in this way with multiple radios? Even on the potential Qualcomm 5 GHz low/mid/high split radios you'd have gaps between the channels (e.g. no channel 80, no channel 148), so it feels like you should always be able to split it in a way that the radio range boundaries don't land between two adjacent channels in the channel array. Not sure how to implement such a check best, probably easiest to find all non-adjacent channels first: - go over bands - ensure channels are sorted by increasing frequency (not sure we do that today? but every driver probably does) - find adjacent channels: - while more channels: - start_freq = current channel's freq - 10 - end_freq = current channel's freq + 10 - while current channel's freq == end_freq - 10: - go to next channel - check all radio's ranges cover this full or not at all (neither start nor end of a range falls into the calculated [start_freq, end_freq) interval) or something like that? (Also some docs on this I guess!) johannes
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index ac49c2c71d2b..257ee3b1100b 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -696,14 +696,15 @@ static struct ieee80211_chanctx * ieee80211_new_chanctx(struct ieee80211_local *local, const struct ieee80211_chan_req *chanreq, enum ieee80211_chanctx_mode mode, - bool assign_on_failure) + bool assign_on_failure, + int radio_idx) { struct ieee80211_chanctx *ctx; int err; lockdep_assert_wiphy(local->hw.wiphy); - ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1); + ctx = ieee80211_alloc_chanctx(local, chanreq, mode, radio_idx); if (!ctx) return ERR_PTR(-ENOMEM); @@ -1060,6 +1061,24 @@ int ieee80211_link_unreserve_chanctx(struct ieee80211_link_data *link) return 0; } +static bool +ieee80211_radio_freq_match(const struct wiphy_radio *radio, + const struct ieee80211_chan_req *chanreq) +{ + const struct wiphy_radio_freq_range *r; + u32 freq; + int i; + + freq = ieee80211_channel_to_khz(chanreq->oper.chan); + for (i = 0; i < radio->n_freq_range; i++) { + r = &radio->freq_range[i]; + if (freq >= r->start_freq && freq <= r->end_freq) + return true; + } + + return false; +} + static struct ieee80211_chanctx * ieee80211_replace_chanctx(struct ieee80211_local *local, const struct ieee80211_chan_req *chanreq, @@ -1067,6 +1086,8 @@ ieee80211_replace_chanctx(struct ieee80211_local *local, struct ieee80211_chanctx *curr_ctx) { struct ieee80211_chanctx *new_ctx, *ctx; + struct wiphy *wiphy = local->hw.wiphy; + const struct wiphy_radio *radio; if (!curr_ctx || (curr_ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED) || @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local, if (!list_empty(&ctx->reserved_links)) continue; + if (ctx->conf.radio_idx >= 0) { + radio = &wiphy->radio[ctx->conf.radio_idx]; + if (!ieee80211_radio_freq_match(radio, chanreq)) + continue; + } + curr_ctx = ctx; break; } @@ -1125,6 +1152,34 @@ ieee80211_replace_chanctx(struct ieee80211_local *local, return new_ctx; } +static bool +ieee80211_find_available_radio(struct ieee80211_local *local, + const struct ieee80211_chan_req *chanreq, + int *radio_idx) +{ + struct wiphy *wiphy = local->hw.wiphy; + const struct wiphy_radio *radio; + int i; + + *radio_idx = -1; + if (!wiphy->n_radio) + return true; + + for (i = 0; i < wiphy->n_radio; i++) { + radio = &wiphy->radio[i]; + if (!ieee80211_radio_freq_match(radio, chanreq)) + continue; + + if (!ieee80211_can_create_new_chanctx(local, i)) + continue; + + *radio_idx = i; + return true; + } + + return false; +} + int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link, const struct ieee80211_chan_req *chanreq, enum ieee80211_chanctx_mode mode, @@ -1133,6 +1188,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link, struct ieee80211_sub_if_data *sdata = link->sdata; struct ieee80211_local *local = sdata->local; struct ieee80211_chanctx *new_ctx, *curr_ctx; + int radio_idx; lockdep_assert_wiphy(local->hw.wiphy); @@ -1142,9 +1198,10 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link, new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode); if (!new_ctx) { - if (ieee80211_can_create_new_chanctx(local, -1)) + if (ieee80211_can_create_new_chanctx(local, -1) && + ieee80211_find_available_radio(local, chanreq, &radio_idx)) new_ctx = ieee80211_new_chanctx(local, chanreq, mode, - false); + false, radio_idx); else new_ctx = ieee80211_replace_chanctx(local, chanreq, mode, curr_ctx); @@ -1755,6 +1812,7 @@ int _ieee80211_link_use_channel(struct ieee80211_link_data *link, struct ieee80211_chanctx *ctx; u8 radar_detect_width = 0; bool reserved = false; + int radio_idx; int ret; lockdep_assert_wiphy(local->hw.wiphy); @@ -1785,9 +1843,11 @@ int _ieee80211_link_use_channel(struct ieee80211_link_data *link, /* Note: context is now reserved */ if (ctx) reserved = true; + else if (!ieee80211_find_available_radio(local, chanreq, &radio_idx)) + ctx = ERR_PTR(-EBUSY); else ctx = ieee80211_new_chanctx(local, chanreq, mode, - assign_on_failure); + assign_on_failure, radio_idx); if (IS_ERR(ctx)) { ret = PTR_ERR(ctx); goto out;
Validate number of channels and interface combinations per radio. Assign each channel context to a radio. Signed-off-by: Felix Fietkau <nbd@nbd.name> --- net/mac80211/chan.c | 70 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 65 insertions(+), 5 deletions(-)