Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <1c1391b6-9710-44c6-97b2-68ef23409c1b@quicinc.com>
Date:   Mon, 25 Sep 2023 11:11:48 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 13/13] wifi: ath11k: send TPC power to firmware for 6
 GHz station
To:     Wen Gong <quic_wgong@quicinc.com>, <ath11k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>, <kvalo@kernel.org>,
        <quic_jjohnson@quicinc.com>
References: <20230920082349.29111-1-quic_wgong@quicinc.com>
 <20230920082349.29111-14-quic_wgong@quicinc.com>
 <eb08e0f2-c932-4d79-b2ee-813c2999d1a5@quicinc.com>
 <fbad2af6-9c3f-c241-b820-7820b4200bf4@quicinc.com>
 <60aaa1fc-99cf-476c-af51-e5ad425792f9@quicinc.com>
 <145e9534-6610-9c11-b2b9-87fb8e50ef81@quicinc.com>
Content-Language: en-US
From:   Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <145e9534-6610-9c11-b2b9-87fb8e50ef81@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9/25/23 07:45, Wen Gong wrote:
> On 9/22/2023 9:25 PM, Aditya Kumar Singh wrote:
>> On 9/22/23 15:42, Wen Gong wrote:
>>> On 9/22/2023 5:24 PM, Aditya Kumar Singh wrote:
>>>> On 9/20/23 13:53, Wen Gong wrote:
>>>>> When station is connected to a 6 GHz AP, it has 2 way to configure
>>>>> the power limit to firmware. The first way is to send 2 wmi command
>>>>> WMI_PDEV_PARAM_TXPOWER_LIMIT2G/WMI_PDEV_PARAM_TXPOWER_LIMIT5G to
>>>>> firmware, the second way is to send WMI_VDEV_SET_TPC_POWER_CMDID to
>>>>> firmware which include more parameters for power control.
>>>>>
>>>>> The first way is disabled in previous patch
>>>>> "ath11k: discard BSS_CHANGED_TXPOWER when EXT_TPC_REG_SUPPORT for 6 
>>>>> GHz".
>>>>>
>>>>> Prepare the parameter for wmi command WMI_VDEV_SET_TPC_POWER_CMDID and
>>>>> send the firmware after vdev start response success from firmware, it
>>>>> is for the second way of power control.
>>>>>
>>>>> Tested-on: WCN6855 hw2.0 PCI 
>>>>> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
>>>>>
>>>>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
>>>>> ---
>>>>>   drivers/net/wireless/ath/ath11k/mac.c | 8 +++++++-
>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/ath11k/mac.c 
>>>>> b/drivers/net/wireless/ath/ath11k/mac.c
>>>>> index a8ae281d2635..f8b907a758b1 100644
>>>>> --- a/drivers/net/wireless/ath/ath11k/mac.c
>>>>> +++ b/drivers/net/wireless/ath/ath11k/mac.c
>>>>> @@ -7296,6 +7296,12 @@ ath11k_mac_vdev_start_restart(struct 
>>>>> ath11k_vif *arvif,
>>>>>           return ret;
>>>>>       }
>>>>>   +    if (ath11k_mac_supports_station_tpc(ar, arvif, chandef)) {
>>>>> +        ath11k_mac_fill_reg_tpc_info(ar, arvif->vif, 
>>>>> &arvif->chanctx);
>>>> So we are passing local copy of channel context stored in 
>>>> arvif->chanctx. Do we need to update it when channel changes?
>>>>
>>>> I see that during assignment time, we are copying/updating it and 
>>>> accordingly the command will be sent to firmware, but what about 
>>>> when STA moves channel? arvif->chanctx should be updated and tpc 
>>>> command should be sent again in that case?
>>>
>>> This has been discussed before here per question of Johannes:"Could 
>>> this information change? Should we track it in beacons?":
>>>
>>> [PATCH 9/9] mac80211: save transmit power envelope element and power 
>>> constraint
>>>
>>> https://lore.kernel.org/linux-wireless/38e7d9d2eebafa7245a36a0a0396094526eb3efd.camel@sipsolutions.net/
>> That's fine. That's w.r.t to TX power change. I'm saying here about 
>> CSA? What when AP tries to switch channel? For that client need not 
>> disassociate and associate back right?
>>
>> In that case, channel context in mac80211 layer will change. But our 
>> driver's arvif->chanctx will have previous one only. We are using 
>> channel context to get the ieee80211_channel which has the PSD value, 
>> and that value we are sending to firmware via TPC command during 
>> intial association time. So when channel changes, firmware also should 
>> be updated with the latest PSD values via TPC command for the latest 
>> channel right?
>>
> You are right. CSA may be change channel bandwidth.
> 
> Currently we could keep NOT support CSA as well as CSA for MLO since 
> these are the basic TPC support feature.
> 
> We could make new patch later to support CSA fro TPC power, OK?
Not supporting w.r.t MLO CSA is okay. That's WIP. But for non-MLO case, 
I think it should be supported. And if not, then may be explicitly 
mention it somewhere as _TODO_ ? What if by the time patch is sent for 
that, meanwhile a bug is filed?

Anyways, I will let maintainers take a call on this. I don't have any 
further comments.