[RFC,v3,0/8] cfg80211/mac80211: support defining multiple radios per wiphy

The prerequisite for MLO support in cfg80211/mac80211 is that all the links
participating in MLO must be from the same wiphy/ieee80211_hw. To meet this
expectation, some drivers may need to group multiple discrete hardware each
acting as a link in MLO under single wiphy.

With this series, the bands and supported frequencies of each individual
radio are reported to user space. This allows user space to figure out the
limitations of what combination of channels can be used concurrently.

Each mac80211 channel context is assigned to a radio based on radio specific
frequency ranges and interface combinations.

This is loosely based on Karthikeyan Periyasamy's series
"[PATCH 00/13] wifi: Add multi physical hardware iface combination support"
with some differences:

 - a struct wiphy_radio is defined, which holds the frequency ranges and
   a full struct ieee80211_iface_combination array
 - a channel context is explicitly assigned to a radio when created
 - both global and per-radio interface combination limits are checked
   and enforced on channel context assignment

Changes since RFC v2:
 - fix uninitialized variables
 - fix multiple radios with DFS
 - add support for per-radio beacon interval checking

Changes since RFC:
 - replace static per-radio number of channels limit with full ifcomb
   checks
 - remove band bitmask in favor of only using freq ranges

Felix Fietkau (8):
  wifi: nl80211: split helper function from nl80211_put_iface_combinations
  wifi: cfg80211: add support for advertising multiple radios belonging to a wiphy
  wifi: cfg80211: extend interface combination check for multi-radio
  wifi: mac80211: add support for DFS with multiple radios
  wifi: mac80211: add radio index to ieee80211_chanctx_conf
  wifi: mac80211: extend ifcomb check functions for multi-radio
  wifi: mac80211: move code in ieee80211_link_reserve_chanctx to a helper
  wifi: mac80211: add wiphy radio assignment and validation

 include/net/cfg80211.h       |  43 +++++++-
 include/net/mac80211.h       |   2 +-
 include/uapi/linux/nl80211.h |  48 ++++++++-
 net/mac80211/cfg.c           |   7 +-
 net/mac80211/chan.c          | 228 +++++++++++++++++++++++-------------
 net/mac80211/ibss.c          |   2 +-
 net/mac80211/ieee80211_i.h   |   6 +-
 net/mac80211/iface.c         |   2 +-
 net/mac80211/main.c          |  32 +++--
 net/mac80211/util.c          | 131 +++++++++++++++------
 net/wireless/nl80211.c       | 190 +++++++++++++++++++++---------
 net/wireless/rdev-ops.h      |  17 +++-
 net/wireless/trace.h         |   5 +-
 net/wireless/util.c          |  36 ++++--
 14 files changed, 560 insertions(+), 189 deletions(-)

base-commit: 5bcd9a0a59953b5ff55ac226f903397319bf7f40

On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
> 
> @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs {
>   *
>   * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
>   * @set_ttlm: set the TID to link mapping.
> + * @get_radio_mask: get bitmask of radios in use
>   */
>  struct cfg80211_ops {
>  	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
> @@ -4938,6 +4941,8 @@ struct cfg80211_ops {
>  				    struct cfg80211_set_hw_timestamp *hwts);
>  	int	(*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
>  			    struct cfg80211_ttlm_params *params);
> +	int	(*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev,
> +				  u32 *mask);


not sure I see the point of this being a callback rather than being
passed in?

(Also, if really needed, do you actually expect a device with 32 radios?
if not you can use a return value instead of u32 *mask out pointer :) )


> +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask,
> +	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
> +	TP_ARGS(wiphy, netdev)
> +);

and if we do need it that really should trace not just the fact that it
happened but also the return value and mask

>  static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int,
>  				       u32 *beacon_int_gcd,
> -				       bool *beacon_int_different)
> +				       bool *beacon_int_different,
> +				       const struct wiphy_radio *radio)
>  {
> +	struct cfg80211_registered_device *rdev;
>  	struct wireless_dev *wdev;
> +	int radio_idx = -1;
>  
>  	*beacon_int_gcd = 0;
>  	*beacon_int_different = false;
> +	if (radio)
> +		radio_idx = radio - wiphy->radio;

This can go oh so wrong ... and technically even be UB. I'd rather pass
the index from the driver, I guess, and validate it against n_radios.
 
> +	rdev = wiphy_to_rdev(wiphy);
>  	list_for_each_entry(wdev, &wiphy->wdev_list, list) {
>  		int wdev_bi;
> +		u32 mask;
>  
>  		/* this feature isn't supported with MLO */
>  		if (wdev->valid_links)
>  			continue;

Are we expecting this to change? because the premise of this patchset is
MLO support, and yet with real MLO we won't get here?

Or is that because non-MLO interfaces could be created on this wiphy?

>  
> +		if (radio_idx >= 0) {
> +			if (rdev_get_radio_mask(rdev, wdev->netdev, &mask))
> +				continue;


here: given that 'radio'/'radio_idx' is passed in, not sure I see why
the mask couldn't also be passed in?

> +			if (!(mask & BIT(radio_idx)))
> +				continue;

that could use a comment

> -	for (i = 0; i < wiphy->n_iface_combinations; i++) {
> -		const struct ieee80211_iface_combination *c;
> +	if (radio) {
> +		c = radio->iface_combinations;
> +		n = radio->n_iface_combinations;
> +	} else {
> +		c = wiphy->iface_combinations;
> +		n = wiphy->n_iface_combinations;
> +	}
> +	for (i = 0; i < n; i++, c++) {

that c++ is a bit too hidden for my taste there, but YMMV and I guess if
I wasn't reading the diff it'd be more obvious :)

johannes

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

On 07.06.24 11:32, Johannes Berg wrote:
> On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
>> 
>> @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs {
>>   *
>>   * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
>>   * @set_ttlm: set the TID to link mapping.
>> + * @get_radio_mask: get bitmask of radios in use
>>   */
>>  struct cfg80211_ops {
>>  	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
>> @@ -4938,6 +4941,8 @@ struct cfg80211_ops {
>>  				    struct cfg80211_set_hw_timestamp *hwts);
>>  	int	(*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
>>  			    struct cfg80211_ttlm_params *params);
>> +	int	(*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev,
>> +				  u32 *mask);
> 
> 
> not sure I see the point of this being a callback rather than being
> passed in?
> 
> (Also, if really needed, do you actually expect a device with 32 radios?
> if not you can use a return value instead of u32 *mask out pointer :) )

I'll update the callback to return u32

>> +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask,
>> +	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
>> +	TP_ARGS(wiphy, netdev)
>> +);
> 
> and if we do need it that really should trace not just the fact that it
> happened but also the return value and mask
> 
>>  static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int,
>>  				       u32 *beacon_int_gcd,
>> -				       bool *beacon_int_different)
>> +				       bool *beacon_int_different,
>> +				       const struct wiphy_radio *radio)
>>  {
>> +	struct cfg80211_registered_device *rdev;
>>  	struct wireless_dev *wdev;
>> +	int radio_idx = -1;
>>  
>>  	*beacon_int_gcd = 0;
>>  	*beacon_int_different = false;
>> +	if (radio)
>> +		radio_idx = radio - wiphy->radio;
> 
> This can go oh so wrong ... and technically even be UB. I'd rather pass
> the index from the driver, I guess, and validate it against n_radios.

Will pass the index in directly.

>> +	rdev = wiphy_to_rdev(wiphy);
>>  	list_for_each_entry(wdev, &wiphy->wdev_list, list) {
>>  		int wdev_bi;
>> +		u32 mask;
>>  
>>  		/* this feature isn't supported with MLO */
>>  		if (wdev->valid_links)
>>  			continue;
> 
> Are we expecting this to change? because the premise of this patchset is
> MLO support, and yet with real MLO we won't get here?
> 
> Or is that because non-MLO interfaces could be created on this wiphy?

Not sure about this. I guess we can revisit it later since it's out of 
scope for this series.

>>  
>> +		if (radio_idx >= 0) {
>> +			if (rdev_get_radio_mask(rdev, wdev->netdev, &mask))
>> +				continue;
> 
> 
> here: given that 'radio'/'radio_idx' is passed in, not sure I see why
> the mask couldn't also be passed in?

mask is supposed to be per wdev, which is iterated in a loop here.

> 
>> +			if (!(mask & BIT(radio_idx)))
>> +				continue;
> 
> that could use a comment
> 
>> -	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>> -		const struct ieee80211_iface_combination *c;
>> +	if (radio) {
>> +		c = radio->iface_combinations;
>> +		n = radio->n_iface_combinations;
>> +	} else {
>> +		c = wiphy->iface_combinations;
>> +		n = wiphy->n_iface_combinations;
>> +	}
>> +	for (i = 0; i < n; i++, c++) {
> 
> that c++ is a bit too hidden for my taste there, but YMMV and I guess if
> I wasn't reading the diff it'd be more obvious :)

Will move it into the loop body to make it more obvious.

Thanks,

- Felix