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: <df0b368c-3b84-653e-6ef8-5446b75ce797@quicinc.com>
Date:   Wed, 19 Oct 2022 09:54:44 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [PATCH 4/7] cfg80211: add NL command to set 6 GHz power mode
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
References: <20220704102341.5692-1-quic_adisi@quicinc.com>
 <20220704102341.5692-5-quic_adisi@quicinc.com>
 <b36799bc1129baebbb64032deeaae72e4ca1af7b.camel@sipsolutions.net>
From:   Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <b36799bc1129baebbb64032deeaae72e4ca1af7b.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/6/2022 16:35, Johannes Berg wrote:
> On Mon, 2022-07-04 at 15:53 +0530, Aditya Kumar Singh wrote:
>>
>> + * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
>> + *	for access points. Referenced from &enum ieee80211_ap_reg_power.
>> + *
>> + * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
>> + *	mode for clients. Referenced from &enum ieee80211_client_reg_power.
> 
> I don't really see a good reason to have two attributes for this, rather
> than validating their value based on the iftype?
> 
The policy for each varies. For AP power mode, it can vary from 0 to 2 
(total 3 power modes currently), and for clients 0 to 1 (total 2 power 
modes). So, if we have just 1 NL_ATTR, while parsing obviously we can do 
based on iftype but in NL_ATTR policy validation, for clients it will 
pass value 2 where actually it should not. Will that be fine?


>>   	err = __cfg80211_stop_ap(rdev, dev, link_id, notify);
>> +
>> +	if (wdev->reg_6ghz_pwr_configured)
>> +		wdev->reg_6ghz_pwr_configured = false;
> 
> no need to check first
Sure, will improvise in next version.

> 
>>
...
> 
> Btw, all of that probably also needs to be per *link* rather than per
> *interface* now, with MLO?
Yes, it should be per *link*. Im working on the changes, will send next 
version soon.

> 
>> +	if (!info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] &&
>> +	    !info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE])
>> +		return -EINVAL;
>> +
>> +	wdev_lock(wdev);
>> +	if (wdev->reg_6ghz_pwr_configured) {
>> +		wdev_unlock(wdev);
>> +		return -EALREADY;
>> +	}
>> +
>> +	if (iftype == NL80211_IFTYPE_AP &&
>> +	    info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]) {
>> +		wdev->ap_6ghz_power =
>> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]);
>> +		ret = 0;
>> +	}
>> +
>> +	if (iftype == NL80211_IFTYPE_STATION &&
>> +	    info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]) {
>> +		wdev->client_6ghz_power =
>> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]);
>> +		ret = 0;
>> +	}
>> +
>> +	if (!ret)
>> +		wdev->reg_6ghz_pwr_configured = true;
> 
> and honestly that logic here with the two attributes is pretty
> strange...
> 
> I'd even say you should remove the union in the wdev struct since you
> only can have one of them at a time anyway
Sure will do.

> 
>> +	{
>> +		.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),
> 
> if you have netdev then you have wdev too, later
Yes, got it.

> 
>>   
>> +	if (wdev->reg_6ghz_pwr_configured)
>> +		wdev->reg_6ghz_pwr_configured = false;
>>
> 
> No need for if
Got it, thanks. Will rectify.

> 
> johannes

Aditya