Re: [RFC 1/2] cfg80211: add support for advertising multiple radios belonging to a wiphy

[Felix Fietkau <nbd@nbd.name>]

On 22.05.24 16:23, Ben Greear wrote:
> On 5/22/24 05:30, Felix Fietkau wrote:
>> 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 change, 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.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   include/net/cfg80211.h       | 34 ++++++++++++++++
>>   include/uapi/linux/nl80211.h | 48 +++++++++++++++++++++++
>>   net/wireless/nl80211.c       | 75 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 157 insertions(+)
>> 
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index f2ca495ac9f8..58d8375ffa11 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -5407,6 +5407,34 @@ struct wiphy_iftype_akm_suites {
>>   	int n_akm_suites;
>>   };
>>   
>> +/**
>> + * struct wiphy_radio_freq_range - wiphy frequency range
>> + * @start_freq:  start range edge frequency (kHz)
>> + * @end_freq:    end range edge frequency (kHz)
>> + */
>> +struct wiphy_radio_freq_range {
>> +	u32 start_freq;
>> +	u32 end_freq;
>> +};
>> +
>> +
>> +/**
>> + * struct wiphy_radio - This structure describes a physical radio belonging
>> + * to a wiphy. It is used to describe concurrent-channel capabilities of the
>> + * phy. Only one channel can be active on the radio described by struct
>> + * wiphy_radio.
>> + *
>> + * @band_mask: Mask of enum nl80211_band describing the bands that the radio
>> + *	can operate on.
>> + * @num_freq_range: number of elements in @freq_range
>> + * @freq_range: frequency range that the radio can operate on (optional)
>> + */
>> +struct wiphy_radio {
>> +	u16 band_mask;
>> +	u16 n_freq_range;
>> +	const struct wiphy_radio_freq_range *freq_range;
>> +};
> 
> Do you think we might should add the radio_idx in here so that we don't
> depend on position in the array in case we need to add/remove radios
> for some reason?
> 
> Or radio MAC addr or some other more persistent way to detect exactly
> what this is in user-space?

Why would radios be added/removed at run time?

>>   #define CFG80211_HW_TIMESTAMP_ALL_PEERS	0xffff
>>   
>>   /**
>> @@ -5625,6 +5653,9 @@ struct wiphy_iftype_akm_suites {
>>    *	A value of %CFG80211_HW_TIMESTAMP_ALL_PEERS indicates the driver
>>    *	supports enabling HW timestamping for all peers (i.e. no need to
>>    *	specify a mac address).
>> + *
>> + * @radio: radios belonging to this wiphy
>> + * @n_radio: number of radios
>>    */
>>   struct wiphy {
>>   	struct mutex mtx;
>> @@ -5775,6 +5806,9 @@ struct wiphy {
>>   
>>   	u16 hw_timestamp_max_peers;
>>   
>> +	const struct wiphy_radio *radio;
>> +	int n_radio;
>> +
>>   	char priv[] __aligned(NETDEV_ALIGN);
>>   };
>>   
>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
>> index f917bc6c9b6f..dfb01d673094 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -3401,6 +3401,8 @@ enum nl80211_attrs {
>>   
>>   	NL80211_ATTR_ASSOC_SPP_AMSDU,
>>   
>> +	NL80211_ATTR_RADIOS,
>> +
>>   	/* add attributes here, update the policy in nl80211.c */
>>   
>>   	__NL80211_ATTR_AFTER_LAST,
>> @@ -7999,4 +8001,50 @@ enum nl80211_ap_settings_flags {
>>   	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
>>   };
>>   
>> +/**
>> + * enum nl80211_wiphy_radio_attrs - wiphy radio attributes
>> + *
>> + * @__NL80211_WIPHY_RADIO_ATTR_INVALID: Invalid
>> + *
>> + * @NL80211_WIPHY_RADIO_ATTR_BAND_MASK: Bitmask of bands supported by this
>> + *	radio.
>> + *
>> + * @NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES: Nested array of frequency ranges
>> + *	supported by this radio.
>> + *
>> + * @__NL80211_WIPHY_RADIO_ATTR_LAST: Internal
>> + * @NL80211_WIPHY_RADIO_ATTR_MAX: Highest attribute
>> + */
>> +enum nl80211_wiphy_radio_attrs {
>> +	__NL80211_WIPHY_RADIO_ATTR_INVALID,
>> +
>> +	NL80211_WIPHY_RADIO_ATTR_BAND_MASK,
>> +	NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES,
>> +
>> +	/* keep last */
>> +	__NL80211_WIPHY_RADIO_ATTR_LAST,
>> +	NL80211_WIPHY_RADIO_ATTR_MAX = __NL80211_WIPHY_RADIO_ATTR_LAST - 1,
>> +};
>> +
>> +/**
>> + * enum nl80211_wiphy_radio_freq_range - wiphy radio frequency range
>> + *
>> + * @__NL80211_WIPHY_RADIO_FREQ_ATTR_INVALID: Invalid
>> + *
>> + * @NL80211_WIPHY_RADIO_FREQ_ATTR_START: Frequency range start
>> + * @NL80211_WIPHY_RADIO_FREQ_ATTR_END: Frequency range end
>> + *
>> + * @__NL80211_WIPHY_RADIO_FREQ_ATTR_LAST: Internal
>> + * @NL80211_WIPHY_RADIO_FREQ_ATTR_MAX: Highest attribute
>> + */
>> +enum nl80211_wiphy_radio_freq_range {
>> +	__NL80211_WIPHY_RADIO_FREQ_ATTR_INVALID,
>> +
>> +	NL80211_WIPHY_RADIO_FREQ_ATTR_START,
>> +	NL80211_WIPHY_RADIO_FREQ_ATTR_END,
>> +
>> +	__NL80211_WIPHY_RADIO_FREQ_ATTR_LAST,
>> +	NL80211_WIPHY_RADIO_FREQ_ATTR_MAX = __NL80211_WIPHY_RADIO_FREQ_ATTR_LAST - 1,
>> +};
>> +
>>   #endif /* __LINUX_NL80211_H */
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index 3c0bca4238d3..c07abdf104ec 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -2392,6 +2392,78 @@ static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg)
>>   	return -ENOBUFS;
>>   }
>>   
>> +static int nl80211_put_radio(struct wiphy *wiphy, struct sk_buff *msg, int idx)
>> +{
>> +	const struct wiphy_radio *r = &wiphy->radio[idx];
> 
> Maybe allow radio[] array to be sparse (ie, idx 0 is there, idx 2 is there, idx 1 is NULL
> because user wants to remove that radio from MLO consideration for whatever reason?

Why should the user want to do this?

- Felix