Re: [PATCH V2 3/3] cfg80211: support ieee80211-freq-limit DT property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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ł
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux