Hi, Looks pretty good. Some comments, mostly nits, below. > +/** > + * nl80211_sar_attrs - Attributes for SAR spec missing enum > + * > + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in %nl80211_sar_type. better use &enum nl80211_sar_type for a link in docs > + * > + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power > + * limit specifications. Each specification contains a set > + * of %nl80211_sar_specs_attrs. > + * > + * For SET operation, it contains array of NL80211_SAR_ATTR_SPECS_POWER some odd indent? Usually we just use a single tab. > +/** > + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs again, enum missing > + * > + * @NL80211_SAR_ATTR_SPECS_POWER: Required (u32)value to specify the actual > + * power limit value in units of 0.25 dBm if type is > + * NL80211_SAR_TYPE_POWER. (i.e., a value of 44 represents 11 dBm). > + * 0 means userspace doesn't have SAR limitation on this associated range. > + * > + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to specify the > + * index of exported freq range table and the associated power limitation > + * is applied to this range. > + * > + * Userspace isn't required to set all the ranges advertised by WLAN driver, > + * and userspace can skip some certain ranges. These skipped ranges don't > + * have SAR limitations, and these are same as setting the > + * %NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at least one range, > + * no matter the power limiation is 0 or not. (typo - limitation) Should "0" really be the magic value? Theoretically, 0 and even negative values are valid. Perhaps we should just use something big (0xffffffff) to indicate no limit, or just not have such a "no limitation" value because userspace can always set it to something very big that means no practical limitation anyway? OK actually you have a U8 now so the high limit is 63.75dBm, but there's not really a good reason for that, since U32 takes the same space in netlink anyway. And wait, I thought we agreed to remove the index? Now I'm confused. And even if we do need the index, then perhaps we should use the (otherwise anyway ignored) nla_type() of the container, instead of an explicit inner attribute? > + * > + * Every SET operation overwrites previous SET operation. > + * > + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start > + * frequency of this range edge when registering SAR capability to wiphy. It's > + * not a channel center frequency. The unit is KHz. "kHz" not "KHz", in a few places other than this too > +static int > +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev, > + struct sk_buff *msg) > +{ > + struct nlattr *sar_capa, *specs, *sub_freq_range; > + u8 num_freq_ranges; extra space? > + for (i = 0; i < num_freq_ranges; i++) { > + sub_freq_range = nla_nest_start(msg, i + 1); > + > + nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_START_FREQ, > + rdev->wiphy.sar_capa->freq_ranges[i].start_freq); > + > + nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_END_FREQ, > + rdev->wiphy.sar_capa->freq_ranges[i].end_freq); Need to check the return values of these three calls. And an aside, unrelated to this particular code: Should we do some kind of validation that the ranges reported actually overlap all supported channels (taking 20 MHz bandwidth into account)? > + nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, info->attrs[NL80211_ATTR_SAR_SPEC], > + sar_policy, info->extack); If you're not checking the return value then no point in passing a policy or extack :-) And yes, it's already validated, so you don't have to do it again. > + sar_spec->type = type; > + specs = 0; > + nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem) { > + if (nla_parse(spec, > + NL80211_SAR_ATTR_SPECS_MAX, > + nla_data(spec_list), > + nla_len(spec_list), > + sar_specs_policy, > + NULL)) { Similar here, don't really need to validate it since it's done by the policy. > + err = -EINVAL; > + goto error; > + } > + > + /* for power type, power value and index must be presented */ > + if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] || > + !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) && > + type == NL80211_SAR_TYPE_POWER) { maybe "switch (type) {...}" or something and return -EINVAL also if it's a type not supported in the code yet, i.e. default case? Otherwise we might add a type, and forget this pretty easily. > + err = -EINVAL; > + goto error; > + } > + > + power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]); > + sar_spec->sub_specs[specs].power = power; and that probably should then be in a sub function or something also inside the particular type. or maybe just all in a separate function? dunno. not really _necessary_, but the lines are getting kinda long already, and one more indentation level with the switch won't help ... johannes