| Message ID | cover.c104c0bb3a14f4ac26aee71f4979846f6ad87742.1717611760.git-series.nbd@nbd.name |
|---|---|
| Headers | show |
| Series | cfg80211/mac80211: support defining multiple radios per wiphy | expand |
On 6/6/2024 12:01 AM, Felix Fietkau wrote: > > /* > * This is a bit strange, since the iteration used to rely only on > @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy *wiphy, > * cfg80211 already - the only thing not would appear to be any new > * interfaces (while being brought up) and channel/radar data. > */ > - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, > - &beacon_int_gcd, &beacon_int_different); > + if (!radio) > + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, > + &beacon_int_gcd, > + &beacon_int_different); > Why its avoid for radio specific iface combination ?
On 06.06.24 09:20, Karthikeyan Periyasamy wrote: > > > On 6/6/2024 12:01 AM, Felix Fietkau wrote: >> >> /* >> * This is a bit strange, since the iteration used to rely only on >> @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy *wiphy, >> * cfg80211 already - the only thing not would appear to be any new >> * interfaces (while being brought up) and channel/radar data. >> */ >> - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >> - &beacon_int_gcd, &beacon_int_different); >> + if (!radio) >> + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >> + &beacon_int_gcd, >> + &beacon_int_different); >> > > Why its avoid for radio specific iface combination ? Because it iterates over all wdevs within cfg80211. I didn't think this was necessary, given that it already excludes MLO wdevs. - Felix
On 6/6/2024 1:25 PM, Felix Fietkau wrote: > On 06.06.24 09:20, Karthikeyan Periyasamy wrote: >> >> >> On 6/6/2024 12:01 AM, Felix Fietkau wrote: >>> /* >>> * This is a bit strange, since the iteration used to rely only on >>> @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy >>> *wiphy, >>> * cfg80211 already - the only thing not would appear to be any >>> new >>> * interfaces (while being brought up) and channel/radar data. >>> */ >>> - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>> - &beacon_int_gcd, &beacon_int_different); >>> + if (!radio) >>> + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>> + &beacon_int_gcd, >>> + &beacon_int_different); >> >> Why its avoid for radio specific iface combination ? > > Because it iterates over all wdevs within cfg80211. I didn't think this > was necessary, given that it already excludes MLO wdevs. > Dont tie the radio specific iface advertisement with MLO. Usually the existing code consider "params->new_beacon_int" the MLO scenario also.
On 06.06.24 10:56, Karthikeyan Periyasamy wrote: > > > On 6/6/2024 1:25 PM, Felix Fietkau wrote: >> On 06.06.24 09:20, Karthikeyan Periyasamy wrote: >>> >>> >>> On 6/6/2024 12:01 AM, Felix Fietkau wrote: >>>> /* >>>> * This is a bit strange, since the iteration used to rely only on >>>> @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy >>>> *wiphy, >>>> * cfg80211 already - the only thing not would appear to be any >>>> new >>>> * interfaces (while being brought up) and channel/radar data. >>>> */ >>>> - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>> - &beacon_int_gcd, &beacon_int_different); >>>> + if (!radio) >>>> + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>> + &beacon_int_gcd, >>>> + &beacon_int_different); >>> >>> Why its avoid for radio specific iface combination ? >> >> Because it iterates over all wdevs within cfg80211. I didn't think this >> was necessary, given that it already excludes MLO wdevs. >> > > Dont tie the radio specific iface advertisement with MLO. > > Usually the existing code consider "params->new_beacon_int" the MLO > scenario also. For your hardware, do beacon intervals need to be matched/aligned per radio or globally? - Felix
On 6/6/2024 2:28 PM, Felix Fietkau wrote: > On 06.06.24 10:56, Karthikeyan Periyasamy wrote: >> >> >> On 6/6/2024 1:25 PM, Felix Fietkau wrote: >>> On 06.06.24 09:20, Karthikeyan Periyasamy wrote: >>>> >>>> >>>> On 6/6/2024 12:01 AM, Felix Fietkau wrote: >>>>> /* >>>>> * This is a bit strange, since the iteration used to rely >>>>> only on >>>>> @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy >>>>> *wiphy, >>>>> * cfg80211 already - the only thing not would appear to be >>>>> any new >>>>> * interfaces (while being brought up) and channel/radar data. >>>>> */ >>>>> - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>>> - &beacon_int_gcd, &beacon_int_different); >>>>> + if (!radio) >>>>> + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>>> + &beacon_int_gcd, >>>>> + &beacon_int_different); >>>> >>>> Why its avoid for radio specific iface combination ? >>> >>> Because it iterates over all wdevs within cfg80211. I didn't think >>> this was necessary, given that it already excludes MLO wdevs. >>> >> >> Dont tie the radio specific iface advertisement with MLO. >> >> Usually the existing code consider "params->new_beacon_int" the MLO >> scenario also. > > For your hardware, do beacon intervals need to be matched/aligned per > radio or globally? > Our hardware supports radio aligned beacon interval. Currently, ath12k use use same beacon interval configuration all radio iface combination. Even in radio specific iface combination, we should check the beacon interval for the non MLO VAPs. so dont avoid the beacon interval check.
On 06.06.24 11:52, Karthikeyan Periyasamy wrote: > > > On 6/6/2024 2:28 PM, Felix Fietkau wrote: >> On 06.06.24 10:56, Karthikeyan Periyasamy wrote: >>> >>> >>> On 6/6/2024 1:25 PM, Felix Fietkau wrote: >>>> On 06.06.24 09:20, Karthikeyan Periyasamy wrote: >>>>> >>>>> >>>>> On 6/6/2024 12:01 AM, Felix Fietkau wrote: >>>>>> /* >>>>>> * This is a bit strange, since the iteration used to rely >>>>>> only on >>>>>> @@ -2384,8 +2383,10 @@ int cfg80211_iter_combinations(struct wiphy >>>>>> *wiphy, >>>>>> * cfg80211 already - the only thing not would appear to be >>>>>> any new >>>>>> * interfaces (while being brought up) and channel/radar data. >>>>>> */ >>>>>> - cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>>>> - &beacon_int_gcd, &beacon_int_different); >>>>>> + if (!radio) >>>>>> + cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, >>>>>> + &beacon_int_gcd, >>>>>> + &beacon_int_different); >>>>> >>>>> Why its avoid for radio specific iface combination ? >>>> >>>> Because it iterates over all wdevs within cfg80211. I didn't think >>>> this was necessary, given that it already excludes MLO wdevs. >>>> >>> >>> Dont tie the radio specific iface advertisement with MLO. >>> >>> Usually the existing code consider "params->new_beacon_int" the MLO >>> scenario also. >> >> For your hardware, do beacon intervals need to be matched/aligned per >> radio or globally? >> > > Our hardware supports radio aligned beacon interval. > > Currently, ath12k use use same beacon interval configuration all radio > iface combination. > > Even in radio specific iface combination, we should check the beacon > interval for the non MLO VAPs. > > so dont avoid the beacon interval check. Okay, I'll look into making this work. - Felix
On 6/6/24 00:01, 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 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: > - replace static per-radio number of channels limit with full ifcomb > checks > - remove band bitmask in favor of only using freq ranges What about handling 2 GHz + 5 GHz issue we discussed in v1 related to radar detection width and num chan ctx? Is that taken care?
On 06.06.24 12:00, Aditya Kumar Singh wrote: > On 6/6/24 00:01, 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 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: >> - replace static per-radio number of channels limit with full ifcomb >> checks >> - remove band bitmask in favor of only using freq ranges > > What about handling 2 GHz + 5 GHz issue we discussed in v1 related to > radar detection width and num chan ctx? Is that taken care? Seems that I missed that one. I will take care of it in the next version. Thanks, - Felix