Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <608c1629-f003-ec7d-3331-0968ed675aee@quicinc.com>
Date:   Wed, 30 Aug 2023 13:20:31 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH v3 4/9] wifi: cfg80211: add NL command to set 6 GHz power
 mode
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <20230315132904.31779-1-quic_adisi@quicinc.com>
 <20230315132904.31779-5-quic_adisi@quicinc.com>
 <59e2364e9085f725c03d05e3f720f8fae7b336ce.camel@sipsolutions.net>
 <d3740fa2-a59e-d6e2-394c-9b289564b832@quicinc.com>
 <321872041974a787db71bbbf660adeb40c494d71.camel@sipsolutions.net>
From:   Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <321872041974a787db71bbbf660adeb40c494d71.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/30/23 13:11, Johannes Berg wrote:
> On Wed, 2023-08-30 at 10:35 +0530, Aditya Kumar Singh wrote:
>> On 8/29/23 23:21, Johannes Berg wrote:
>>> On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
>>>>
>>>> +	{
>>>> +		.cmd = NL80211_CMD_SET_6GHZ_POWER_MODE,
>>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>> +		.doit = nl80211_set_6ghz_power_mode,
>>>> +		.flags = GENL_UNS_ADMIN_PERM,
>>>> +		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV |
>>>> +					 NL80211_FLAG_MLO_VALID_LINK_ID),
>>>> +	},
>>>
>>> Why is this even a new command, rather than a parameter to start AP or
>>> similar?
>>>
>> A new command was introduced because to give user space a knob to change
>> power mode as and when required. Let's suppose AFC response has not yet
>> arrived, AP could be started in Low Power mode. Now once AFC rules are
>> applied (not going in detail here how that will happen) and user space
>> knows about it, it can send command to switch to Standard Power Mode right?
> 
> At least for AP that could also be a beacon update, like most other
> operational parameters. You have to update the beacon _anyway_ since you
> indicate in the beacon what you're doing.
Yeah this option can be explored too. Thanks for the suggestion.

> 
> So that by itself doesn't really introduce the requirement to have a
> separate command.
Yes I see your point. Let me re-work on this aspect as well.

> 
>>> Why do we even set it in client mode from userspace?
> 
> And you didn't comment on this - what userspace do you expect to use it
> in client mode? I can see hostapd in AP mode, I guess, but in client
> mode I don't see how this could be used? Anyway you're tied to what the
> AP is doing, no?
Yes, as you saw already replied to the similar question in the cover 
letter. Will reply to the last question here in that thread it self (to 
keep client related discussion in same place).


> 
>>>>    static struct genl_family nl80211_fam __ro_after_init = {
>>>> @@ -17409,7 +17473,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
>>>>    	.n_ops = ARRAY_SIZE(nl80211_ops),
>>>>    	.small_ops = nl80211_small_ops,
>>>>    	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
>>>> -	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
>>>> +	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
>>>>
>>>
>>> Obviously, this should not be done.
>> If this hunk is not there, the command was not going through. Upon
>> digging further found out that the number of commands declared in the
>> array and the count provided here has some relation. And that too with
>> the last element added. Since a new element was added, modified it
>> accordingly.
> 
> No no, that wasn't a discussion. I'm telling you, this is wrong, and I
> will not apply a patch that does this. If you needed it, you did
> something wrong in userspace.
Oh okay. Fine, let me check again.

> 
> johannes