Hi,
Just a quick first pass.
> + * @NL80211_CMD_SET_REG: Set current regulatory domain. CRDA sends this command
> + * after being queried by the kernel. CRDA replies by sending a regulatory
> + * domain structure which consists of %NL80211_ATTR_REG_ALPHA set to our
> + * current alpha2 if it found a match. It also provides %NL80211_ATTR_REG_NUM,
> + * NL80211_ATTR_REG_RULE_FLAGS, and a set of regulatory rules. Each
> + * regulatory rule is a nested set of attributes given by
> + * %NL80211_ATTR_REG_RULE_FREQ_[START|END] and
> + * %NL80211_ATTR_FREQ_RANGE_MAX_BW with an attached power rule given by
> + * %NL80211_ATTR_REG_RULE_POWER_MAX_ANT_GAIN and
> + * %NL80211_ATTR_REG_RULE_POWER_MAX_EIRP. We provide CRDA with a receipt,
> + * %NL80211_ATTR_REG_UUID, which CRDA sends us back to ensure the request
> + * command is valid and came from us.
The UUID thing confuses me. Why is this necessary? And if that's there,
why does crda provide a country code back to the kernel? It shouldn't
ever reply with something different...
> + * @NL80211_ATTR_NUM_REG_RULES: the number of regulatory rules nested in
> + * %NL80211_ATTR_REG_RULES, we pass this to not abuse the stack
> + * and to gracefully use only one memory area in nl80211_set_reg().
> + * This value shall never exceed NL80211_MAX_SUPP_REG_RULES. We can
> + * increase this value as regulatory rules becomes more complex.
That isn't needed. You can just count the number of nested attributes
in NL80211_ATTR_REG_RULES.
> + * @NL80211_ATTR_POWER_RULE_MAX_ANT_GAIN: the maximum allowed antenna gain
> + * for a given frequency range. The value is in mBi (100 * dBi).
> + * If you set this to 0 it means there is no available known limit.
No way. If there's no known limit, just leave out the attribute
completel.
> +/**
> + * enum nl80211_reg_rule_flags - regulatory rule flags. These should match
> + * the latest regdb.h regulatory rule flags from CRDA.
I don't think such a comment belongs in the kernel, it's a detail of the
userspace implementation and not part of the kernel API. Hence, we can
do whatever we want here and have userspace follow, not the other way
around.
> +/**
> + * enum reg_set_by - Indicates who is trying to set the regulatory domain
> + * @REGDOM_SET_BY_INIT: regultory domain was set by initialization. We will be
^^^^^^^^^
typo
> + * @REGDOM_SET_BY_80211D: the wireless core has received an 802.11 country
> + * information element with regulotory information it thinks we
^^^^^^^^^^
typo
> struct ieee80211_channel {
> enum ieee80211_band band;
> u16 center_freq;
> + u8 max_bandwidth;
> u16 hw_value;
> u32 flags;
> int max_antenna_gain;
reorder please so the struct is smaller.
> +struct list_head regulatory_requests;
> +
> +/* Central wireless core regulatory domains, we only need two,
> + * the current one and a world regulatory domain in case we have no
> + * information to give us an alpha2 */
> +struct ieee80211_regdomain *cfg80211_regdomain;
> +
> +/* CRDA can provide a dynamic world regulatory domain, we keep
> + * this static one updated as often as we can in case of the absense
absence, but the sentence is a bit confused anyway :)
> + [NL80211_ATTR_REG_UUID] = { .type = NLA_BINARY, .len = 16 },
> + [NL80211_ATTR_REG_ALPHA2] = { .type = NLA_STRING, .len = 2 },
shouldn't alpha2 be considered binary as well?
> + reg_rule_policy[NL80211_REG_RULE_ATTR_MAX + 1] = {
> + [NL80211_ATTR_REG_RULE_FLAGS] = { .type = NLA_U32 },
I thought we agreed to use actual NLA flags for the flags, in a nested
attribute, instead of using bitmaps.
> + uuid = nla_data(info->attrs[NL80211_ATTR_REG_UUID]);
> + alpha2 = nla_data(info->attrs[NL80211_ATTR_REG_ALPHA2]);
> + num_rules = nla_get_u32(info->attrs[NL80211_ATTR_NUM_REG_RULES]);
please don't indent like that, it makes grepping unnecessarily hard.
> + printk("nl80211: incorectly formatted regulatory domain\n");
why bother printing something?
> + unsigned char uuid_arg[16];
> + sprintf(uuid_arg,
> + "%02x%02x%02x%02x"
> + "%02x%02x%02x%02x"
> + "%02x%02x%02x%02x"
> + "%02x%02x%02x%02x",
> + uuid[0], uuid[1], uuid[2], uuid[3],
> + uuid[4], uuid[5], uuid[6], uuid[7],
> + uuid[8], uuid[9], uuid[10], uuid[11],
> + uuid[12], uuid[13], uuid[14], uuid[15]);
buffer overflow.
> +/**
> + * regulatory_hint - hint to wireless core a regulatory domain
> + * @alpha2: the ISO-3166 alpha2 the driver thinks we're on
> + * @wiphy: the driver's very own &struct wiphy
> + *
> + * Wireles drivers can use this function to hint to the wireles core
type in both instances :)
> +static int is_alpha_upper(char letter)
> +{
> + /* ASCII A - Z */
> + if (letter >= 65 && letter <= 90)
> + return 1;
> + return 0;
> +}
> +static int is_an_alpha2(char *alpha2)
> +{
> + if (is_alpha_upper(alpha2[0]) && is_alpha_upper(alpha2[1]))
> + return 1;
> + return 0;
Why's that so important?
> + printk("Country: %c%c\n", rd->alpha2[0], rd->alpha2[1]);
most of your printks are unnecessary, and most of them are lacking level
annotations.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
