[RFC,v3,8/8] wifi: mac80211: add wiphy radio assignment and validation

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(-)

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