Received-SPF: pass (google.com: domain of linux-wireless+bounces-8006-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <667cea80-9b62-4ca4-9d28-c5940582832a@quicinc.com>
Date: Thu, 23 May 2024 20:34:51 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/5] wifi: ath12k: Fix Pdev id in HTT stats request for
 WCN7850
To: Kalle Valo <kvalo@kernel.org>,
        Ramya Gnanasekar
	<quic_rgnanase@quicinc.com>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240510050806.514126-1-quic_rgnanase@quicinc.com>
 <20240510050806.514126-4-quic_rgnanase@quicinc.com>
 <87pltfd2bt.fsf@kernel.org>
Content-Language: en-US
From: Lingbo Kong <quic_lingbok@quicinc.com>
In-Reply-To: <87pltfd2bt.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/5/21 15:57, Kalle Valo wrote:
> Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:
> 
>> From: Lingbo Kong <quic_lingbok@quicinc.com>
>>
>> Pdev id from mac phy capabilities will be sent as a part of
>> HTT stats request to firmware. This causes issue with single pdev
>> devices where fimrware does not respond to the stats request
>> sent from host.
>>
>> Single pdev devices firmware expects pdev id as 1 for 5GHz/6GHz
>> phy and 2 for 2GHz band. Handle pdev id for single phy device
>> while sending HTT stats request message to firmware.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>
>> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
>> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
> 
> [...]
> 
>> @@ -1029,7 +1030,12 @@ ath12k_dp_tx_htt_h2t_ext_stats_req(struct ath12k *ar, u8 type,
>>   	memset(cmd, 0, sizeof(*cmd));
>>   	cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_EXT_STATS_CFG;
>>   
>> -	cmd->hdr.pdev_mask = 1 << ar->pdev->pdev_id;
>> +	if (ab->hw_params->single_pdev_only)
>> +		pdev_id = ath12k_mac_get_target_pdev_id(ar);
>> +	else
>> +		pdev_id = ar->pdev->pdev_id;
> 
> Wouldn't it be cleaner to have the single_pdev_only check in
> ath12k_mac_get_target_pdev_id()?
> 

yes, i'll put the single_pdev_only check in 
ath12k_mac_get_target_pdev_id() function.

>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
>> +{
>> +	struct ath12k *ar;
>> +	struct ath12k_pdev *pdev;
>> +	struct ath12k_vif *arvif;
>> +	int i;
>> +
>> +	for (i = 0; i < ab->num_radios; i++) {
>> +		pdev = &ab->pdevs[i];
>> +		ar = pdev->ar;
>> +		list_for_each_entry(arvif, &ar->arvifs, list) {
>> +			if (arvif->is_up)
>> +				return arvif;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> I'm not seeing any protection here, is that on purpose?
> 

you means there need to add lockdep_assert_held(&ar->conf_mutex)?

>> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
>> +{
>> +	struct ath12k_vif *arvif;
>> +
>> +	arvif = ath12k_mac_get_vif_up(ar->ab);
>> +
>> +	if (arvif)
>> +		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
>> +	else
>> +		return ar->ab->fw_pdev[0].pdev_id;
>> +}
> 
> No need to have else after return.
>