| Message ID | 20201106100708.4609-1-cjhuang@codeaurora.org |
|---|---|
| Headers | show |
| Series | add common API to configure SAR | expand |
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
On 2020-11-06 18:25, Johannes Berg wrote: > Hi, > > Looks pretty good. Some comments, mostly nits, below. > Thank you for the comments, Johannes. I don't understand below well, please help explain: > 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? > > >> +/** >> + * nl80211_sar_attrs - Attributes for SAR spec > > missing enum > sure >> + * >> + * @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. > sure >> +/** >> + * 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. > Looks 0 and negative value are not practical as it means <= 1mw, but I can use S32 instead. Not sure if a magic value is needed? If it's needed, then perhaps 0x7fffffff is good for it? > And wait, I thought we agreed to remove the index? Now I'm confused. > Using index in SET operation doesn't add burden to userspace and kernel, but it provides some flexibility so userspace can skip some certain ranges. > 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? > I don't understand what means here. Use nla_type for what? >> + * >> + * 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. > sure > > 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. > Yes, will use NULL instead of info->extack >> + 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. > sure >> + 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. > Good suggestion, will change to switch case. >> + 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 ... > I'll move this to a separate function. > johannes
Hi, This patch looks good to me, there is one small nit, If the maintainer can take care of it then probably we don't need a new rev. > @@ -329,6 +336,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { > .fw_diag_ce_download = true, > .tx_stats_over_pktlog = false, > .supports_peer_stats_info = true, > + .dynamic_sar_support = true, > }, Orthogonal to this patch, other people might probably differ, I guess putting dynamic sar support as a firmware feature capability should be more scalable and then we don't need a structure table for each firmware. This might hold for other firmware features as well. > + ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower 2g:%d 5g:%d\n", > + tx_power_2g, tx_power_5g); just a nit: space after colon. This might throw a warning in checkpatch.pl -Abhishek
Hi, Johannes has some good comments, apart for that I have some nits. > > And wait, I thought we agreed to remove the index? Now I'm confused. > > > Using index in SET operation doesn't add burden to userspace and kernel, > but it provides some flexibility so userspace can skip some certain > ranges. I agree with Carl's comment, we do need the frequency index. If the frequency index is provided, then the order is not important which makes the data more clear or the set_sar_spec function needs to parse the frequency ranges (and ofcourse userspace has to populate that as well). If the frequency index is not provided, then the driver has to assume that the userspace is not making any error in mapping of the power and desired frequency. Other reason is, might be a bit unlikely, but if in future there are new subbands, then it gives a flexibility to the userspace to explicitly provide the band for which it needs to set the power for. > + * used with %NL80211_CMD_SET_SAR_SPECS. The message contains fileds > + * of %nl80211_sar_attrs which specifies the sar type and related typo: fileds .. you mean fields -Abhishek
On 2020-11-20 04:25, Abhishek Kumar wrote: > Hi, > > Johannes has some good comments, apart for that I have some nits. >> > And wait, I thought we agreed to remove the index? Now I'm confused. >> > >> Using index in SET operation doesn't add burden to userspace and >> kernel, >> but it provides some flexibility so userspace can skip some certain >> ranges. > > I agree with Carl's comment, we do need the frequency index. If the > frequency index is provided, then the order is not important which > makes the data more clear or the set_sar_spec function needs to parse > the frequency ranges (and ofcourse userspace has to populate that as > well). If the frequency index is not provided, then the driver has to > assume that the userspace is not making any error in mapping of the > power and desired frequency. > Other reason is, might be a bit unlikely, but if in future there are > new subbands, then it gives a flexibility to the userspace to > explicitly provide the band for which it needs to set the power for. > >> + * used with %NL80211_CMD_SET_SAR_SPECS. The message contains >> fileds >> + * of %nl80211_sar_attrs which specifies the sar type and related > > typo: fileds .. you mean fields > I will fix all the spelling errors and send V2. > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k