| Message ID | 20170102163209.2445-2-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
> +static void wiphy_freq_limits_apply(struct wiphy *wiphy) [...] > + if (!wiphy_freq_limits_valid_chan(wiphy, > chan)) { > + pr_debug("Disabling freq %d MHz as > it's out of OF limits\n", > + chan->center_freq); > + chan->flags |= > IEEE80211_CHAN_DISABLED; I think you didn't address the problem in the best way now. The problem with the channel sharing was the way you're applying the limits - at runtime. This is now OK since the new function shouldn't be called when the channel structs are shared, but hooking it all into the regulatory code is now no longer needed. What you can do now, when reading the OF data, is actually apply it to the channel flags immediately. If done *before* wiphy_register(), these flags will be preserved forever, so you no longer need any hooks in regulatory code at all - you can just set the original channel flags according to the OF data. I think this greatly simplifies the flow, since you can also remove wiphy->freq_limits (and n_freq_limits) completely, since now the only effect of the function would be to modify the channel list, and later regulatory updates would always preserve the flags. johannes
On 02-01-17 18:52, Johannes Berg wrote: >> +static void wiphy_freq_limits_apply(struct wiphy *wiphy) > [...] >> + if (!wiphy_freq_limits_valid_chan(wiphy, >> chan)) { >> + pr_debug("Disabling freq %d MHz as >> it's out of OF limits\n", >> + chan->center_freq); >> + chan->flags |= >> IEEE80211_CHAN_DISABLED; > > I think you didn't address the problem in the best way now. > > The problem with the channel sharing was the way you're applying the > limits - at runtime. This is now OK since the new function shouldn't be > called when the channel structs are shared, but hooking it all into thes > regulatory code is now no longer needed. > > What you can do now, when reading the OF data, is actually apply it to > the channel flags immediately. If done *before* wiphy_register(), these > flags will be preserved forever, so you no longer need any hooks in > regulatory code at all - you can just set the original channel flags > according to the OF data. I suppose this then can also be done early in the wiphy_register() function itself, right? > I think this greatly simplifies the flow, since you can also remove > wiphy->freq_limits (and n_freq_limits) completely, since now the only > effect of the function would be to modify the channel list, and later > regulatory updates would always preserve the flags. So does it mean the function can go in core.c again :-p If it is likely there will be other properties being added it might justify adding a new source file, eg. of.c, and only compile it when CONFIG_OF is set. Just a thought. Regards, Arend
On 2 January 2017 at 18:52, Johannes Berg <johannes@sipsolutions.net> wrote: >> +static void wiphy_freq_limits_apply(struct wiphy *wiphy) > [...] >> + if (!wiphy_freq_limits_valid_chan(wiphy, >> chan)) { >> + pr_debug("Disabling freq %d MHz as >> it's out of OF limits\n", >> + chan->center_freq); >> + chan->flags |= >> IEEE80211_CHAN_DISABLED; > > I think you didn't address the problem in the best way now. > > The problem with the channel sharing was the way you're applying the > limits - at runtime. This is now OK since the new function shouldn't be > called when the channel structs are shared, but hooking it all into the > regulatory code is now no longer needed. > > What you can do now, when reading the OF data, is actually apply it to > the channel flags immediately. If done *before* wiphy_register(), these > flags will be preserved forever, so you no longer need any hooks in > regulatory code at all - you can just set the original channel flags > according to the OF data. > > I think this greatly simplifies the flow, since you can also remove > wiphy->freq_limits (and n_freq_limits) completely, since now the only > effect of the function would be to modify the channel list, and later > regulatory updates would always preserve the flags. I need some extra help understanding this :( When driver uses custom regulatory it registers initial channels at init but it can also react to regdom changes using reg_notifier. Is that correct? So I'm looking at brcmf_cfg80211_reg_notifier which calls brcmf_setup_wiphybands which calls brcmf_construct_chaninfo. That last one reworks all channels on every call. It first marks all existing channels as DISABLED then queries firmware for the list of supported channels and updates wiphy channels one by one. So if I understand this correctly, every regdom change can result in rebuilding channels pretty much from the scratch. That's why I believed I need to call wiphy_freq_limits_apply on runtime, not just during the init. Is there some flow in my understanding?
On 2 January 2017 at 21:12, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 02-01-17 18:52, Johannes Berg wrote: >>> +static void wiphy_freq_limits_apply(struct wiphy *wiphy) >> [...] >>> + if (!wiphy_freq_limits_valid_chan(wiphy, >>> chan)) { >>> + pr_debug("Disabling freq %d MHz as >>> it's out of OF limits\n", >>> + chan->center_freq); >>> + chan->flags |= >>> IEEE80211_CHAN_DISABLED; >> >> I think you didn't address the problem in the best way now. >> >> The problem with the channel sharing was the way you're applying the >> limits - at runtime. This is now OK since the new function shouldn't be >> called when the channel structs are shared, but hooking it all into thes >> regulatory code is now no longer needed. >> >> What you can do now, when reading the OF data, is actually apply it to >> the channel flags immediately. If done *before* wiphy_register(), these >> flags will be preserved forever, so you no longer need any hooks in >> regulatory code at all - you can just set the original channel flags >> according to the OF data. > > I suppose this then can also be done early in the wiphy_register() > function itself, right? When driver calls wiphy_apply_custom_regulatory I need to already have limits read from the DT (at least in my current implementation, it may change, but I need help understanding flow first). As wiphy_apply_custom_regulatory has to be called before wiphy_register, I can't read DT so late (in wiphy_register).
> I suppose this then can also be done early in the wiphy_register() > function itself, right? No, because of the shared channel data issue. If this is unconditionally in wiphy_register() then we can no longer guarantee that sharing it is acceptable - which it is today under certain circumstances the driver controls. This would move it out of driver control, making it never possible to share any more. > So does it mean the function can go in core.c again :-p If it is > likely there will be other properties being added it might justify > adding a new source file, eg. of.c, and only compile it when > CONFIG_OF is set. Just a thought. Yeah, whatever :) We can figure that out once we have the mechanisms in place. johannes
> When driver uses custom regulatory it registers initial channels at > init but it can also react to regdom changes using reg_notifier. Is > that correct? We can treat regulatory and OF data as entirely independent, I think. At least that's my suggestion: * use OF data to populate the original channel list, saying which channels are valid (or not) * use regulatory later to further restrict settings of the channels > So I'm looking at brcmf_cfg80211_reg_notifier which calls > brcmf_setup_wiphybands which calls brcmf_construct_chaninfo. > That last one reworks all channels on every call. It first marks all > existing channels as DISABLED then queries firmware for the list of > supported channels and updates wiphy channels one by one. > So if I understand this correctly, every regdom change can result in > rebuilding channels pretty much from the scratch. That's why I > believed I need to call wiphy_freq_limits_apply on runtime, not just > during the init. > > Is there some flow in my understanding? I think maybe there's a problem in my understanding :) All the regulatory code usually takes into account channel->orig_flags. If this code also did, then we could have the original DISABLED flag taken from OF still be valid here. johannes johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index ca2ac1c..5002625 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -3515,6 +3515,9 @@ struct wiphy_iftype_ext_capab { * attribute indices defined in &enum nl80211_bss_select_attr. * * @cookie_counter: unique generic cookie counter, used to identify objects. + * + * @n_freq_limits: number of frequency limits + * @freq_limits: array of extra frequency limits */ struct wiphy { /* assign these fields before you register the wiphy */ @@ -3646,6 +3649,9 @@ struct wiphy { u64 cookie_counter; + unsigned int n_freq_limits; + struct ieee80211_freq_range *freq_limits; + char priv[0] __aligned(NETDEV_ALIGN); }; @@ -4278,6 +4284,23 @@ const u8 *cfg80211_find_vendor_ie(unsigned int oui, int oui_type, */ /** + * wiphy_read_of_freq_limits - read frequency limits from device tree + * + * @wiphy: the wireless device to get extra limits for + * + * Some devices may have extra limitations specified in DT. This may be useful + * for chipsets that normally support more bands but are limited due to board + * design (e.g. by antennas or extermal power amplifier). + * + * This function reads info from DT and uses it to *modify* channels (disable + * unavailable ones) in a regulary code. It's usually a *bad* idea to use it in + * drivers with *shared* channel data as DT limitations are device specific. + * + * As this function access device node it has to be called after set_wiphy_dev. + */ +int wiphy_read_of_freq_limits(struct wiphy *wiphy); + +/** * regulatory_hint - driver hint to the wireless core a regulatory domain * @wiphy: the wireless device giving the hint (used only for reporting * conflicts) diff --git a/net/wireless/core.c b/net/wireless/core.c index 158c59e..c1b5fc7 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -940,6 +940,7 @@ void cfg80211_dev_free(struct cfg80211_registered_device *rdev) void wiphy_free(struct wiphy *wiphy) { + kfree(wiphy->freq_limits); put_device(&wiphy->dev); } EXPORT_SYMBOL(wiphy_free); diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 5dbac37..e00fd5b 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -1158,6 +1158,120 @@ static uint32_t reg_rule_to_chan_bw_flags(const struct ieee80211_regdomain *regd return bw_flags; } +int wiphy_read_of_freq_limits(struct wiphy *wiphy) +{ + struct device *dev = wiphy_dev(wiphy); + struct device_node *np; + struct property *prop; + const __be32 *p; + int len, i, err; + + if (wiphy->n_freq_limits) + return 0; + + if (!dev) + return 0; + np = dev_of_node(dev); + if (!np) + return 0; + + prop = of_find_property(np, "ieee80211-freq-limit", &len); + if (!prop) + return 0; + + if (!len || len % sizeof(u32) || len / sizeof(u32) % 2) { + dev_err(dev, "ieee80211-freq-limit wrong format"); + return -EPROTO; + } + wiphy->n_freq_limits = len / sizeof(u32) / 2; + + wiphy->freq_limits = kzalloc(wiphy->n_freq_limits * sizeof(*wiphy->freq_limits), + GFP_KERNEL); + if (!wiphy->freq_limits) { + err = -ENOMEM; + goto out; + } + + p = NULL; + for (i = 0; i < wiphy->n_freq_limits; i++) { + struct ieee80211_freq_range *limit = &wiphy->freq_limits[i]; + + p = of_prop_next_u32(prop, p, &limit->start_freq_khz); + if (!p) { + err = -EINVAL; + goto out; + } + + p = of_prop_next_u32(prop, p, &limit->end_freq_khz); + if (!p) { + err = -EINVAL; + goto out; + } + + if (!limit->start_freq_khz || + !limit->end_freq_khz || + limit->start_freq_khz >= limit->end_freq_khz) { + err = -EINVAL; + goto out; + } + } + + return 0; + +out: + dev_err(dev, "Failed to get limits: %d\n", err); + kfree(wiphy->freq_limits); + wiphy->n_freq_limits = 0; + + return err; +} +EXPORT_SYMBOL(wiphy_read_of_freq_limits); + +static bool wiphy_freq_limits_valid_chan(struct wiphy *wiphy, + struct ieee80211_channel *chan) +{ + u32 bw = MHZ_TO_KHZ(20); + int i; + + for (i = 0; i < wiphy->n_freq_limits; i++) { + struct ieee80211_freq_range *limit = &wiphy->freq_limits[i]; + + if (reg_does_bw_fit(limit, MHZ_TO_KHZ(chan->center_freq), bw)) + return true; + } + + return false; +} + +static void wiphy_freq_limits_apply(struct wiphy *wiphy) +{ + enum nl80211_band band; + int i; + + if (!wiphy->n_freq_limits) + return; + + for (band = 0; band < NUM_NL80211_BANDS; band++) { + struct ieee80211_supported_band *sband = wiphy->bands[band]; + + if (!sband) + continue; + + for (i = 0; i < sband->n_channels; i++) { + struct ieee80211_channel *chan = &sband->channels[i]; + + if (chan->flags & IEEE80211_CHAN_DISABLED) + continue; + + if (!wiphy_freq_limits_valid_chan(wiphy, chan)) { + pr_debug("Disabling freq %d MHz as it's out of OF limits\n", + chan->center_freq); + chan->flags |= IEEE80211_CHAN_DISABLED; + } + } + } +} + /* * Note that right now we assume the desired channel bandwidth * is always 20 MHz for each individual channel (HT40 uses 20 MHz @@ -1693,6 +1807,8 @@ static void wiphy_update_regulatory(struct wiphy *wiphy, for (band = 0; band < NUM_NL80211_BANDS; band++) handle_band(wiphy, initiator, wiphy->bands[band]); + wiphy_freq_limits_apply(wiphy); + reg_process_beacons(wiphy); reg_process_ht_flags(wiphy); reg_call_notifier(wiphy, lr); @@ -1800,6 +1916,8 @@ void wiphy_apply_custom_regulatory(struct wiphy *wiphy, bands_set++; } + wiphy_freq_limits_apply(wiphy); + /* * no point in calling this if it won't have any effect * on your device's supported bands.