| Message ID | 20221220214318.2041986-5-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] ap: make supported rates a common builder. | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-ci-gitlint | success | GitLint |
Hi James, On 12/20/22 15:43, James Prestwood wrote: > For AP mode its convenient for IWD to choose an appropriate > channel definition rather than require the user provide very > low level parameters such as channel width, center1 frequency > etc. This adds two new APIs to generate a channel definition, one > for legacy and one for HT. > > The legacy API is very simple and chooses the first matching > operating class using 20Mhz channel widths. > > The HT API is a bit more complex since it has to take into account > 40MHz and check any channel restrictions. If an operating class is > found that is supported/not restricted it is marked as 'best' until > a better one is found. In this case 'better' is a larger channel > width. Since this is HT only 20mhz and 40mhz widths are checked. I wonder if the HT one can be implemented using the noHT one? > --- > src/band.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/band.h | 4 ++ > 2 files changed, 163 insertions(+) > > diff --git a/src/band.c b/src/band.c > index d89b2a90..b1e319d7 100644 > --- a/src/band.c > +++ b/src/band.c > @@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct band_chandef *own, uint8_t oci[static 3]) > return -ENOENT; > } > > +static bool freq_in_oper_class(uint32_t freq, > + const struct operating_class_info *info, > + enum band_freq *out_band) > +{ > + uint8_t channel; > + enum band_freq band; > + > + channel = band_freq_to_channel(freq, &band); > + if (!channel) > + return false; You already call band_freq_to_channel right at the top of band_freq_to_chandef (but not band_freq_to_ht_chandef?). Not really a big deal, but why continue calling it inside this function which is being called in a loop? You already know the channel and the band... > + > + switch (band) { > + case BAND_FREQ_2_4_GHZ: > + if (info->starting_frequency > 5000) > + return false; > + break; > + case BAND_FREQ_5_GHZ: > + if (info->starting_frequency < 5000 || > + info->starting_frequency > 5950) > + return false; > + break; > + case BAND_FREQ_6_GHZ: > + if (info->starting_frequency < 5900) > + return false; > + break; > + } So you don't trust band_freq_to_channel? Also, I'd really like to isolate 'magic frequency numbers' use as much as possible. Ideally contain this in band_freq_to_channel and band_channel_to_freq only. And even then maybe we should look into whether we can rework band_channel_to_freq to use the e4 table. If there's a chance that band_freq_to_channel can produce a channel/band combination that isn't in e4, then perhaps we should simply add a 'band' member to 'struct operating_class_info'. That way this could be sanity checked directly. > + > + if (e4_channel_to_frequency(info, channel) < 0) > + return false; > + > + if (out_band) > + *out_band = band; > + > + return true; > +} > + > +/* Find a legacy chandef for the frequency */ > +int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr, > + struct band_chandef *chandef) > +{ > + enum band_freq band; > + uint8_t channel; > + unsigned int i; > + const struct operating_class_info *best = NULL; > + > + channel = band_freq_to_channel(freq, &band); > + if (!channel) > + return -EINVAL; > + > + if (attr->disabled || !attr->supported) > + return -EINVAL; > + > + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { > + const struct operating_class_info *info = > + &e4_operating_classes[i]; > + > + if (!freq_in_oper_class(freq, info, NULL)) > + continue; > + > + if (info->channel_spacing != 20) > + continue; nit, but you might want to check this first. > + > + best = info; > + break; > + } > + > + if (!best) > + return -ENOENT; > + > + chandef->frequency = freq; > + chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT; > + > + return 0; > +} > + > +/* Find an HT chandef for the frequency */ > +int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr, > + struct band_chandef *chandef) > +{ > + enum band_freq band; > + enum band_chandef_width width; > + unsigned int i; > + const struct operating_class_info *best = NULL; > + > + if (attr->disabled || !attr->supported) > + return -EINVAL; > + > + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { > + const struct operating_class_info *info = > + &e4_operating_classes[i]; > + enum band_chandef_width w; > + > + if (!freq_in_oper_class(freq, info, &band)) > + continue; > + > + /* Any restrictions for this channel width? */ > + switch (info->channel_spacing) { > + case 20: > + w = BAND_CHANDEF_WIDTH_20; > + break; > + case 40: > + w = BAND_CHANDEF_WIDTH_40; > + > + /* 6GHz remove the upper/lower 40mhz channel concept */ > + if (band == BAND_FREQ_6_GHZ) > + break; > + > + if (info->flags & PRIMARY_CHANNEL_UPPER && > + attr->no_ht40_plus) > + continue; > + > + if (info->flags & PRIMARY_CHANNEL_LOWER && > + attr->no_ht40_minus) > + continue; > + > + break; > + default: > + continue; > + } > + > + if (!best || best->channel_spacing < info->channel_spacing) { > + best = info; > + width = w; > + } > + } > + > + if (!best) > + return -ENOENT; So why not limit the above loop to 40 mhz channel spacing, and if nothing is found, do something like if (!best) return band_freq_to_chandef(); Or alternatively, pass in a parameter for maximum desired width. > + > + chandef->frequency = freq; > + chandef->channel_width = width; > + > + /* > + * Choose a secondary channel frequency: > + * - 20mhz no secondary > + * - 40mhz we can base the selection off the channel flags, either > + * higher or lower. > + */ > + switch (width) { > + case BAND_CHANDEF_WIDTH_20: > + return 0; > + case BAND_CHANDEF_WIDTH_40: > + if (band == BAND_FREQ_6_GHZ) > + return 0; > + > + if (best->flags & PRIMARY_CHANNEL_UPPER) > + chandef->center1_frequency = freq - 10; > + else > + chandef->center1_frequency = freq + 10; > + > + return 0; > + default: > + /* Should never happen */ > + return -EINVAL; > + } > + > + return 0; > +} > + > uint8_t band_freq_to_channel(uint32_t freq, enum band_freq *out_band) > { > uint32_t channel = 0; Regards, -Denis
On Tue, 2022-12-27 at 12:07 -0600, Denis Kenzior wrote: > Hi James, > > On 12/20/22 15:43, James Prestwood wrote: > > For AP mode its convenient for IWD to choose an appropriate > > channel definition rather than require the user provide very > > low level parameters such as channel width, center1 frequency > > etc. This adds two new APIs to generate a channel definition, one > > for legacy and one for HT. > > > > The legacy API is very simple and chooses the first matching > > operating class using 20Mhz channel widths. > > > > The HT API is a bit more complex since it has to take into account > > 40MHz and check any channel restrictions. If an operating class is > > found that is supported/not restricted it is marked as 'best' until > > a better one is found. In this case 'better' is a larger channel > > width. Since this is HT only 20mhz and 40mhz widths are checked. > > I wonder if the HT one can be implemented using the noHT one? > > > --- > > src/band.c | 159 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > src/band.h | 4 ++ > > 2 files changed, 163 insertions(+) > > > > diff --git a/src/band.c b/src/band.c > > index d89b2a90..b1e319d7 100644 > > --- a/src/band.c > > +++ b/src/band.c > > @@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct > > band_chandef *own, uint8_t oci[static 3]) > > return -ENOENT; > > } > > > > +static bool freq_in_oper_class(uint32_t freq, > > + const struct operating_class_info > > *info, > > + enum band_freq *out_band) > > +{ > > + uint8_t channel; > > + enum band_freq band; > > + > > + channel = band_freq_to_channel(freq, &band); > > + if (!channel) > > + return false; > > You already call band_freq_to_channel right at the top of > band_freq_to_chandef > (but not band_freq_to_ht_chandef?). Not really a big deal, but why > continue > calling it inside this function which is being called in a loop? You > already > know the channel and the band... > > > + > > + switch (band) { > > + case BAND_FREQ_2_4_GHZ: > > + if (info->starting_frequency > 5000) > > + return false; > > + break; > > + case BAND_FREQ_5_GHZ: > > + if (info->starting_frequency < 5000 || > > + info->starting_frequency > 5950) > > + return false; > > + break; > > + case BAND_FREQ_6_GHZ: > > + if (info->starting_frequency < 5900) > > + return false; > > + break; > > + } > > So you don't trust band_freq_to_channel? > > Also, I'd really like to isolate 'magic frequency numbers' use as > much as > possible. Ideally contain this in band_freq_to_channel and > band_channel_to_freq > only. And even then maybe we should look into whether we can rework > band_channel_to_freq to use the e4 table. Yeah this was dumb, I removed this completely and just use e4_has_frequency() in the loop for both functions. > > If there's a chance that band_freq_to_channel can produce a > channel/band > combination that isn't in e4, then perhaps we should simply add a > 'band' member > to 'struct operating_class_info'. That way this could be sanity > checked directly. > > > + > > + if (e4_channel_to_frequency(info, channel) < 0) > > + return false; > > + > > + if (out_band) > > + *out_band = band; > > + > > + return true; > > +} > > + > > +/* Find a legacy chandef for the frequency */ > > +int band_freq_to_chandef(uint32_t freq, const struct > > band_freq_attrs *attr, > > + struct band_chandef *chandef) > > +{ > > + enum band_freq band; > > + uint8_t channel; > > + unsigned int i; > > + const struct operating_class_info *best = NULL; > > + > > + channel = band_freq_to_channel(freq, &band); > > + if (!channel) > > + return -EINVAL; > > + > > + if (attr->disabled || !attr->supported) > > + return -EINVAL; > > + > > + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { > > + const struct operating_class_info *info = > > + &e4_operating_class > > es[i]; > > + > > + if (!freq_in_oper_class(freq, info, NULL)) > > + continue; > > + > > + if (info->channel_spacing != 20) > > + continue; > > nit, but you might want to check this first. Looking at this function again after a break I don't even think we need it. Since the only width for non-HT is 20MHz and we just set the frequency... The caller could just validate the frequency (which it already does) and set into the chandef if HT is not supported. > > > + > > + best = info; > > + break; > > + } > > + > > + if (!best) > > + return -ENOENT; > > + > > + chandef->frequency = freq; > > + chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT; > > + > > + return 0; > > +} > > + > > +/* Find an HT chandef for the frequency */ > > +int band_freq_to_ht_chandef(uint32_t freq, const struct > > band_freq_attrs *attr, > > + struct band_chandef *chandef) > > +{ > > + enum band_freq band; > > + enum band_chandef_width width; > > + unsigned int i; > > + const struct operating_class_info *best = NULL; > > + > > + if (attr->disabled || !attr->supported) > > + return -EINVAL; > > + > > + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { > > + const struct operating_class_info *info = > > + &e4_operating_class > > es[i]; > > + enum band_chandef_width w; > > + > > + if (!freq_in_oper_class(freq, info, &band)) > > + continue; > > + > > + /* Any restrictions for this channel width? */ > > + switch (info->channel_spacing) { > > + case 20: > > + w = BAND_CHANDEF_WIDTH_20; > > + break; > > + case 40: > > + w = BAND_CHANDEF_WIDTH_40; > > + > > + /* 6GHz remove the upper/lower 40mhz > > channel concept */ > > + if (band == BAND_FREQ_6_GHZ) > > + break; > > + > > + if (info->flags & PRIMARY_CHANNEL_UPPER && > > + attr->no_ht40_plus) > > + continue; > > + > > + if (info->flags & PRIMARY_CHANNEL_LOWER && > > + attr- > > >no_ht40_minus) > > + continue; > > + > > + break; > > + default: > > + continue; > > + } > > + > > + if (!best || best->channel_spacing < info- > > >channel_spacing) { > > + best = info; > > + width = w; > > + } > > + } > > + > + if (!best) > > + return -ENOENT; > > So why not limit the above loop to 40 mhz channel spacing, and if > nothing is > found, do something like We still need to support 20Mhz since there is this distinction between 20MHz HT and 20Mhz no-HT. If only 20MHz HT is supported we should pick that rather than the non-HT width. > > if (!best) > return band_freq_to_chandef(); So ap.c just tries to pick the best chandef so in theory we could fall back to non-HT and this wouldn't be totally unexpected (from the users perspective). The problem is we would then need to disable HT in ap.c if we fell back to non-HT. Since I may just remove band_freq_to_chandef() entirely I might instead keep the -ENOENT return and fall back inside ap.c itself. Then I don't have to check chandef->channel_width after a successful return and we know this will always return an HT chandef, or fail. > > Or alternatively, pass in a parameter for maximum desired width. > > > + > > + chandef->frequency = freq; > > + chandef->channel_width = width; > > + > > + /* > > + * Choose a secondary channel frequency: > > + * - 20mhz no secondary > > + * - 40mhz we can base the selection off the channel flags, > > either > > + * higher or lower. > > + */ > > + switch (width) { > > + case BAND_CHANDEF_WIDTH_20: > > + return 0; > > + case BAND_CHANDEF_WIDTH_40: > > + if (band == BAND_FREQ_6_GHZ) > > + return 0; > > + > > + if (best->flags & PRIMARY_CHANNEL_UPPER) > > + chandef->center1_frequency = freq - 10; > > + else > > + chandef->center1_frequency = freq + 10; > > + > > + return 0; > > + default: > > + /* Should never happen */ > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > uint8_t band_freq_to_channel(uint32_t freq, enum band_freq > > *out_band) > > { > > uint32_t channel = 0; > > Regards, > -Denis
Hi James, > We still need to support 20Mhz since there is this distinction between > 20MHz HT and 20Mhz no-HT. If only 20MHz HT is supported we should pick > that rather than the non-HT width. Hmm, but there's no difference in operating class between 20HT and 20NoHT. So this could be done post-factum, no? > > So ap.c just tries to pick the best chandef so in theory we could fall > back to non-HT and this wouldn't be totally unexpected (from the users > perspective). The problem is we would then need to disable HT in ap.c > if we fell back to non-HT. Something like if ap->chandef.channel_width == 20HT && !ap->supports_ht -> set channel_width appropriately. > > Since I may just remove band_freq_to_chandef() entirely I might instead > keep the -ENOENT return and fall back inside ap.c itself. Then I don't > have to check chandef->channel_width after a successful return and we > know this will always return an HT chandef, or fail. > Or do that. Or pass in the ht capability to band_freq_to_chandef() and let it figure it out. Regards, -Denis
diff --git a/src/band.c b/src/band.c index d89b2a90..b1e319d7 100644 --- a/src/band.c +++ b/src/band.c @@ -1194,6 +1194,165 @@ int oci_from_chandef(const struct band_chandef *own, uint8_t oci[static 3]) return -ENOENT; } +static bool freq_in_oper_class(uint32_t freq, + const struct operating_class_info *info, + enum band_freq *out_band) +{ + uint8_t channel; + enum band_freq band; + + channel = band_freq_to_channel(freq, &band); + if (!channel) + return false; + + switch (band) { + case BAND_FREQ_2_4_GHZ: + if (info->starting_frequency > 5000) + return false; + break; + case BAND_FREQ_5_GHZ: + if (info->starting_frequency < 5000 || + info->starting_frequency > 5950) + return false; + break; + case BAND_FREQ_6_GHZ: + if (info->starting_frequency < 5900) + return false; + break; + } + + if (e4_channel_to_frequency(info, channel) < 0) + return false; + + if (out_band) + *out_band = band; + + return true; +} + +/* Find a legacy chandef for the frequency */ +int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr, + struct band_chandef *chandef) +{ + enum band_freq band; + uint8_t channel; + unsigned int i; + const struct operating_class_info *best = NULL; + + channel = band_freq_to_channel(freq, &band); + if (!channel) + return -EINVAL; + + if (attr->disabled || !attr->supported) + return -EINVAL; + + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { + const struct operating_class_info *info = + &e4_operating_classes[i]; + + if (!freq_in_oper_class(freq, info, NULL)) + continue; + + if (info->channel_spacing != 20) + continue; + + best = info; + break; + } + + if (!best) + return -ENOENT; + + chandef->frequency = freq; + chandef->channel_width = BAND_CHANDEF_WIDTH_20NOHT; + + return 0; +} + +/* Find an HT chandef for the frequency */ +int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr, + struct band_chandef *chandef) +{ + enum band_freq band; + enum band_chandef_width width; + unsigned int i; + const struct operating_class_info *best = NULL; + + if (attr->disabled || !attr->supported) + return -EINVAL; + + for (i = 0; i < L_ARRAY_SIZE(e4_operating_classes); i++) { + const struct operating_class_info *info = + &e4_operating_classes[i]; + enum band_chandef_width w; + + if (!freq_in_oper_class(freq, info, &band)) + continue; + + /* Any restrictions for this channel width? */ + switch (info->channel_spacing) { + case 20: + w = BAND_CHANDEF_WIDTH_20; + break; + case 40: + w = BAND_CHANDEF_WIDTH_40; + + /* 6GHz remove the upper/lower 40mhz channel concept */ + if (band == BAND_FREQ_6_GHZ) + break; + + if (info->flags & PRIMARY_CHANNEL_UPPER && + attr->no_ht40_plus) + continue; + + if (info->flags & PRIMARY_CHANNEL_LOWER && + attr->no_ht40_minus) + continue; + + break; + default: + continue; + } + + if (!best || best->channel_spacing < info->channel_spacing) { + best = info; + width = w; + } + } + + if (!best) + return -ENOENT; + + chandef->frequency = freq; + chandef->channel_width = width; + + /* + * Choose a secondary channel frequency: + * - 20mhz no secondary + * - 40mhz we can base the selection off the channel flags, either + * higher or lower. + */ + switch (width) { + case BAND_CHANDEF_WIDTH_20: + return 0; + case BAND_CHANDEF_WIDTH_40: + if (band == BAND_FREQ_6_GHZ) + return 0; + + if (best->flags & PRIMARY_CHANNEL_UPPER) + chandef->center1_frequency = freq - 10; + else + chandef->center1_frequency = freq + 10; + + return 0; + default: + /* Should never happen */ + return -EINVAL; + } + + return 0; +} + uint8_t band_freq_to_channel(uint32_t freq, enum band_freq *out_band) { uint32_t channel = 0; diff --git a/src/band.h b/src/band.h index a4652fd8..f7f0c318 100644 --- a/src/band.h +++ b/src/band.h @@ -101,6 +101,10 @@ int band_estimate_nonht_rate(const struct band *band, const uint8_t *supported_rates, const uint8_t *ext_supported_rates, int32_t rssi, uint64_t *out_data_rate); +int band_freq_to_chandef(uint32_t freq, const struct band_freq_attrs *attr, + struct band_chandef *chandef); +int band_freq_to_ht_chandef(uint32_t freq, const struct band_freq_attrs *attr, + struct band_chandef *chandef); int oci_to_frequency(uint32_t operating_class, uint32_t channel);