On 2 January 2017 at 15:04, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> 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!
--
Rafał
