| Message ID | 20170102132747.3491-3-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
> + prop = of_find_property(np, "ieee80211-freq-limit", &i); > + if (!prop) > + return 0; > + > + i = i / sizeof(u32); What if it's not even a multiple of sizeof(u32)? Shouldn't you check that, in case it's completely bogus? > + if (i % 2) { > + dev_err(dev, "ieee80211-freq-limit wrong value"); say "wrong format" perhaps? we don't care (much) above the values. > + return -EPROTO; > + } > + wiphy->n_freq_limits = i / 2; I don't like this use of the 'i' variable - use something like 'len[gth]' instead? > + 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; > + } > + } You should also reject nonsense like empty ranges, or ranges with a higher beginning than end, etc. I think put return 0; here. > +out: > + if (err) { then you can make that a pure "error" label and remove the "if (err)" check. > +void wiphy_freq_limits_apply(struct wiphy *wiphy) I don't see any point in having this here rather than in reg.c, which is the only user. > + 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; This seems wrong. The sband and channels can be static data and be shared across different wiphys for the same driver. If the driver has custom regulatory etc. then this can't work, but that's up to the driver. OF data is handled here though, so if OF data for one device disables a channel, this would also cause the channel to be disabled for another device, if the data is shared. To avoid this, you'd have to have drivers that never share it - but you can't really guarantee that at this level. In order to fix that, you probably need to memdup the sband/channel structs during wiphy registration. Then, if you set it up the right way, you can actually simply edit them according to the OF data directly there, so that *orig_flags* (rather than just flags) already gets the DISABLED bit - and that allows you to get rid of the reg.c hooks entirely since it'll look the same to reg.c independent of the driver or the OF stuff doing this. That can actually be inefficient though, since drivers may already have copied the channel data somewhere and then you copy it again since you can't know. Perhaps a better approach would be to not combine this with wiphy registration, but require drivers that may use this to call a new helper function *before* wiphy registration (and *after* calling set_wiphy_dev()), like e.g. ieee80211_read_of_data(wiphy); You can then also make this an inline when OF is not configured in (something which you haven't really taken into account now, you should have used dev_of_node() too instead of dev->of_node) Yes, this would mean that it doesn't automatically get applied to arbitrary drivers, but it seems unlikely that arbitrary drivers like realtek USB would suddenly get OF node entries ... so that's not necessarily a bad thing. In the documentation for this function you could then document that it will modify flags, and as such must not be called when the sband and channel data is shared, getting rid of the waste/complexity of the copy you otherwise have to make in cfg80211. johannes
On 2 January 2017 at 15:04, Johannes Berg <johannes@sipsolutions.net> wrote: >> + prop = of_find_property(np, "ieee80211-freq-limit", &i); >> + if (!prop) >> + return 0; >> + >> + i = i / sizeof(u32); > > What if it's not even a multiple of sizeof(u32)? Shouldn't you check > that, in case it's completely bogus? > >> + if (i % 2) { >> + dev_err(dev, "ieee80211-freq-limit wrong value"); > > say "wrong format" perhaps? we don't care (much) above the values. > >> + return -EPROTO; >> + } >> + wiphy->n_freq_limits = i / 2; > > I don't like this use of the 'i' variable - use something like > 'len[gth]' instead? > >> + 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; >> + } >> + } > > You should also reject nonsense like empty ranges, or ranges with a > higher beginning than end, etc. I think > > > put > > return 0; > > here. > >> +out: >> + if (err) { > > then you can make that a pure "error" label and remove the "if (err)" > check. > >> +void wiphy_freq_limits_apply(struct wiphy *wiphy) > > I don't see any point in having this here rather than in reg.c, which > is the only user. > >> + 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; > > This seems wrong. > > The sband and channels can be static data and be shared across > different wiphys for the same driver. If the driver has custom > regulatory etc. then this can't work, but that's up to the driver. OF > data is handled here though, so if OF data for one device disables a > channel, this would also cause the channel to be disabled for another > device, if the data is shared. > > To avoid this, you'd have to have drivers that never share it - but you > can't really guarantee that at this level. > > In order to fix that, you probably need to memdup the sband/channel > structs during wiphy registration. Then, if you set it up the right > way, you can actually simply edit them according to the OF data > directly there, so that *orig_flags* (rather than just flags) already > gets the DISABLED bit - and that allows you to get rid of the reg.c > hooks entirely since it'll look the same to reg.c independent of the > driver or the OF stuff doing this. > > > That can actually be inefficient though, since drivers may already have > copied the channel data somewhere and then you copy it again since you > can't know. > > Perhaps a better approach would be to not combine this with wiphy > registration, but require drivers that may use this to call a new > helper function *before* wiphy registration (and *after* calling > set_wiphy_dev()), like e.g. > > ieee80211_read_of_data(wiphy); > > You can then also make this an inline when OF is not configured in > (something which you haven't really taken into account now, you should > have used dev_of_node() too instead of dev->of_node) > > Yes, this would mean that it doesn't automatically get applied to > arbitrary drivers, but it seems unlikely that arbitrary drivers like > realtek USB would suddenly get OF node entries ... so that's not > necessarily a bad thing. > > In the documentation for this function you could then document that it > will modify flags, and as such must not be called when the sband and > channel data is shared, getting rid of the waste/complexity of the copy > you otherwise have to make in cfg80211. Thank you, I appreciate your review a lot, I'll work on this according to your comments!
On 2 January 2017 at 15:04, Johannes Berg <johannes@sipsolutions.net> wrote: > Perhaps a better approach would be to not combine this with wiphy > registration, but require drivers that may use this to call a new > helper function *before* wiphy registration (and *after* calling > set_wiphy_dev()), like e.g. > > ieee80211_read_of_data(wiphy); > > (...) > > Yes, this would mean that it doesn't automatically get applied to > arbitrary drivers, but it seems unlikely that arbitrary drivers like > realtek USB would suddenly get OF node entries ... so that's not > necessarily a bad thing. > > In the documentation for this function you could then document that it > will modify flags, and as such must not be called when the sband and > channel data is shared, getting rid of the waste/complexity of the copy > you otherwise have to make in cfg80211. I just think it may be better to stick to something like ieee80211_read_of_freq_limits or wiphy_read_of_freq_limits As you noted this function will be a bit specific because of modifying (possibly shared) band channels. At some point we may want to add more helpers for other OF properties which won't have extra requirements for driver code. Some drivers may want to use them while not necessary risking have shared band channels modified.
On Mon, 2017-01-02 at 16:05 +0100, Rafał Miłecki wrote: > On 2 January 2017 at 15:04, Johannes Berg <johannes@sipsolutions.net> > wrote: > > Perhaps a better approach would be to not combine this with wiphy > > registration, but require drivers that may use this to call a new > > helper function *before* wiphy registration (and *after* calling > > set_wiphy_dev()), like e.g. > > > > ieee80211_read_of_data(wiphy); > > > > (...) > I just think it may be better to stick to something like > ieee80211_read_of_freq_limits > or > wiphy_read_of_freq_limits I have no objection to that. > As you noted this function will be a bit specific because of > modifying (possibly shared) band channels. At some point we may want > to add more helpers for other OF properties which won't have extra > requirements for driver code. Some drivers may want to use them while > not necessary risking have shared band channels modified. That makes sense, although at that time we might still wish to have a common "read it all" with the combined requirements. But we can cross that bridge when we get to it. johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index e952cca..6609c39 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); }; diff --git a/net/wireless/core.c b/net/wireless/core.c index 398922a..c2a5c81 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -87,6 +87,113 @@ struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx) return &rdev->wiphy; } +static int wiphy_freq_limits_init(struct wiphy *wiphy) +{ + struct device *dev = wiphy_dev(wiphy); + struct device_node *np; + struct property *prop; + const __be32 *p; + int i; + int err = 0; + + if (wiphy->n_freq_limits) + return 0; + + if (!dev) + return 0; + np = dev->of_node; + if (!np) + return 0; + + prop = of_find_property(np, "ieee80211-freq-limit", &i); + if (!prop) + return 0; + + i = i / sizeof(u32); + if (i % 2) { + dev_err(dev, "ieee80211-freq-limit wrong value"); + return -EPROTO; + } + wiphy->n_freq_limits = i / 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; + } + } + +out: + if (err) { + dev_err(dev, "Failed to get limits: %d\n", err); + kfree(wiphy->freq_limits); + wiphy->n_freq_limits = 0; + } + return err; +} + +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; +} + +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; + } + } + } +} + static int cfg80211_dev_check_name(struct cfg80211_registered_device *rdev, const char *newname) { @@ -481,6 +588,8 @@ struct wiphy *wiphy_new_nm(struct device *dev, const struct cfg80211_ops *ops, init_waitqueue_head(&rdev->dev_wait); + wiphy_freq_limits_init(&rdev->wiphy); + /* * Initialize wiphy parameters to IEEE 802.11 MIB default values. * Fragmentation and RTS threshold are disabled by default with the @@ -942,6 +1051,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/core.h b/net/wireless/core.h index af6e023..ce94f82 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -271,6 +271,8 @@ struct cfg80211_iface_destroy { u32 nlportid; }; +void wiphy_freq_limits_apply(struct wiphy *wiphy); + void cfg80211_destroy_ifaces(struct cfg80211_registered_device *rdev); /* free object */ diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 5dbac37..cb5a264 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -748,8 +748,8 @@ static bool is_valid_rd(const struct ieee80211_regdomain *rd) return true; } -static bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_range, - u32 center_freq_khz, u32 bw_khz) +bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_range, + u32 center_freq_khz, u32 bw_khz) { u32 start_freq_khz, end_freq_khz; @@ -1693,6 +1693,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 +1802,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. diff --git a/net/wireless/reg.h b/net/wireless/reg.h index f6ced31..90eddc3 100644 --- a/net/wireless/reg.h +++ b/net/wireless/reg.h @@ -25,6 +25,8 @@ extern const struct ieee80211_regdomain __rcu *cfg80211_regdomain; bool reg_is_valid_request(const char *alpha2); bool is_world_regdom(const char *alpha2); +bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_range, + u32 center_freq_khz, u32 bw_khz); bool reg_supported_dfs_region(enum nl80211_dfs_regions dfs_region); enum nl80211_dfs_regions reg_get_dfs_region(struct wiphy *wiphy);