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: <67961f9b-1062-88e3-d634-35f015dfbead@quicinc.com>
Date:   Mon, 24 Jul 2023 11:11:53 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v4 04/11] wifi: ath12k: propagate EHT capabilities to
 userspace
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>
CC:     <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        "Pradeep Kumar Chitrapu" <quic_pradeepc@quicinc.com>
References: <20230602235820.23912-1-quic_alokad@quicinc.com>
 <20230602235820.23912-5-quic_alokad@quicinc.com> <87pm5xaqp8.fsf@kernel.org>
From:   Aloka Dixit <quic_alokad@quicinc.com>
In-Reply-To: <87pm5xaqp8.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/15/2023 4:51 AM, Kalle Valo wrote:
> Aloka Dixit <quic_alokad@quicinc.com> writes:
> 
>> Propagate EHT capabilities to the userspace using a new member
>> 'eht_cap' in structure ieee80211_sband_iftype_data.
>>
>> MCS-NSS capabilities are copied depending on the supported bandwidths
>> for the given band.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>> Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
>> Signed-off-by: Pradeep Kumar Chitrapu<quic_pradeepc@quicinc.com>
> 
> [...]
> 
>> +static void ath12k_mac_copy_eht_ppet_ru(u32 ppet, u8 ppe_thres[], int ru)
>> +{
>> +	int i;
>> +	u32 val = 0;
>> +	u8 ppet_size_ru = IEEE80211_EHT_PPE_THRES_INFO_PPET_SIZE * 2;
>> +	u8 bit = IEEE80211_EHT_PPE_THRES_INFO_HEADER_SIZE;
>> +
>> +	u32p_replace_bits(&val, ppet >> (ru * ppet_size_ru),
>> +			  GENMASK(ppet_size_ru - 1, 0));
>> +
>> +	val = ((val >> IEEE80211_EHT_PPE_THRES_INFO_PPET_SIZE) & GENMASK(2, 0)) |
>> +	      ((val & GENMASK(2, 0)) << IEEE80211_EHT_PPE_THRES_INFO_PPET_SIZE);
> 
> This shifting of val looks weird. I didn't check the spec, what does it
> do? Is there any cleaner way to do this? And should have a define for
> GENMASK(2, 0)?
> 

I have a follow-up ready, based on top of the fixes you already made in 
the master pending branch, which cleans this up.

Also noticed that need to use hweight16 instead of hweight8 for 
IEEE80211_EHT_PPE_THRES_RU_INDEX_BITMASK_MASK. The new version fixes it 
in this patch as well as in patch #6.

Should I send the next version?