Re: [PATCH v2 1/3] nl80211: add common API to configure SAR power limitations.

[Carl Huang <cjhuang@codeaurora.org>]

On 2020-12-01 16:37, Abhishek Kumar wrote:
> The V2 patch looks good to me.
> Regarding Brian's comment
> 
>> [1] By the way, you aren't checking for duplicates; so users could
>> pass the same index many times, and it's not clear from the API
>> definition what should happen. It seems the current implementation is
>> that you'll just use the last value provided.
> 
> I don't think we should be adding any logic in the kernel to check for
> duplicates, but rather userspace should take care of those. As long as
> the data provided abides by the data policy, the kernel should bother.
> But I do agree with Brian's other comment that it might be made more
> clear in comment. If at all a V3 is needed, then we should add that,
> or else it looks fine.
> 
> Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>
> 
I didn't notice this comment, I actually added the check in the kernel 
in V3.
I think it's OK to check in kernel. I also add some comments that 
duplicated
index is not allowed.

> Thanks
> Abhishek
> 
> On Mon, Nov 30, 2020 at 2:10 AM Carl Huang <cjhuang@codeaurora.org> 
> wrote:
>> 
>> On 2020-11-21 10:42, Brian Norris wrote:
>> > On Fri, Nov 20, 2020 at 12:53 AM Carl Huang <cjhuang@codeaurora.org>
>> > wrote:
>> >>
>> >> NL80211_CMD_SET_SAR_SPECS is added to configure SAR from
>> >> user space. NL80211_ATTR_SAR_SPEC is used to pass the SAR
>> >> power specification when used with NL80211_CMD_SET_SAR_SPECS.
>> >>
>> >> Wireless driver needs to register SAR type, supported frequency
>> >> ranges to wiphy, so user space can query it. The index in
>> >> frequency range is used to specify which sub band the power
>> >> limitation applies to. The SAR type is for compatibility, so later
>> >> other SAR mechanism can be implemented without breaking the user
>> >> space SAR applications.
>> >>
>> >> Normal process is user space quries the SAR capability, and
>> >> gets the index of supported frequency ranges and associates the
>> >> power limitation with this index and sends to kernel.
>> >>
>> >> Here is an example of message send to kernel:
>> >> 8c 00 00 00 08 00 01 00 00 00 00 00 38 00 2b 81
>> >> 08 00 01 00 00 00 00 00 2c 00 02 80 14 00 00 80
>> >> 08 00 02 00 00 00 00 00 08 00 01 00 38 00 00 00
>> >> 14 00 01 80 08 00 02 00 01 00 00 00 08 00 01 00
>> >> 48 00 00 00
>> >>
>> >> NL80211_CMD_SET_SAR_SPECS:  0x8c
>> >> NL80211_ATTR_WIPHY:     0x01(phy idx is 0)
>> >> NL80211_ATTR_SAR_SPEC:  0x812b (NLA_NESTED)
>> >> NL80211_SAR_ATTR_TYPE:  0x00 (NL80211_SAR_TYPE_POWER)
>> >> NL80211_SAR_ATTR_SPECS: 0x8002 (NLA_NESTED)
>> >> freq range 0 power: 0x38 in 0.25dbm unit (14dbm)
>> >> freq range 1 power: 0x48 in 0.25dbm unit (18dbm)
>> >>
>> >> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
>> >
>> > I think the API is reasonably clear and usable. I'm a little skeptical
>> > that the complexity related to indexes is absolutely necessary [1],
>> > but at least you make clear what should happen in the case of missing
>> > indexes (treated as "max"). But you've addressed my concerns, I think:
>> >
>> > Reviewed-by: Brian Norris <briannorris@chromium.org>
>> >
>> > I haven't done the most thorough review on the implementation pieces
>> > (and ath10k), but I at least wanted to put my thoughts out there.
>> >
>> > Thanks,
>> > Brian
>> >
>> > [1] By the way, you aren't checking for duplicates; so users could
>> > pass the same index many times, and it's not clear from the API
>> > definition what should happen. It seems the current implementation is
>> > that you'll just use the last value provided.
>> Thanks for the comments.
>> It's right the last value is used.
>> I can describe it more clearly if V3 is needed.
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k