Re: [PATCH v3] wifi: ath11k: Add rx histogram stats

From: Kalle Valo <kvalo@kernel.org>
To: Maharaja Kennadyrajan <quic_mkenna@quicinc.com>
Cc: Tyler Stachecki <stachecki.tyler@gmail.com>,
	 "open list\:QUALCOMM ATHEROS ATH11K WIRELESS DRIVER"
	<ath11k@lists.infradead.org>,
	 "open list\:NETWORKING DRIVERS \(WIRELESS\)"
	<linux-wireless@vger.kernel.org>,  <quic_akalaise@quicinc.com>,
	 <quic_mpubbise@quicinc.com>
Subject: Re: [PATCH v3] wifi: ath11k: Add rx histogram stats
Date: Wed, 24 May 2023 12:36:16 +0300	[thread overview]
Message-ID: <87h6s2kr5b.fsf@kernel.org> (raw)
In-Reply-To: <d6f794e3-7463-da7d-721e-00869faf4c17@quicinc.com> (Maharaja Kennadyrajan's message of "Wed, 24 May 2023 14:33:03 +0530")

Maharaja Kennadyrajan <quic_mkenna@quicinc.com> writes:

> On 5/21/2023 11:21 PM, Tyler Stachecki wrote:
>
>> Was this really tested on QCN9074 as the commit text suggests...?
>>
>>> const struct ath11k_hw_ops ipq6018_ops = {
>>> @@ -1132,6 +1147,7 @@ const struct ath11k_hw_ops wcn6750_ops = {
>>>   .rx_desc_get_msdu_payload = ath11k_hw_qcn9074_rx_desc_get_msdu_payload,
>>>   .reo_setup = ath11k_hw_wcn6855_reo_setup,
>>>   .mpdu_info_get_peerid = ath11k_hw_ipq8074_mpdu_info_get_peerid,
>>> + .mpdu_info_get_mpdu_len = ath11k_hw_qcn9074_mpdu_info_get_mpdu_len,
>> ...
>>
>>> +static u32 ath11k_hal_rx_mpduinfo_get_mpdu_len(struct ath11k_base *ab,
>>> +       struct hal_rx_mpdu_info *mpdu_info)
>>> +{
>>> + return ab->hw_params.hw_ops->mpdu_info_get_mpdu_len(mpdu_info);
>>> +}
>> I think you want to put this under qcn9074_ops. As of now, when
>> QCN9074 is present, it attempts to jump to a NULL pointer as
>> mpdu_info_get_mpdu_len remains uninitialized for qcn9074_ops.
>>
>> And, do you not need to define mpdu_info_get_mpdu_len for all the
>> other hw_ops? If so, please be careful about defining it for
>> WCN6855/WCN6750 as there was a recent regression due to how the RX
>> MPDU info is provided by those firmwares as it differed from
>> IPQ8074/QCN9074. I personally do not have the appropriate literature
>> to determine whether or not this is consequential or not here as well,
>> though it seems like it would be:
>> https://lore.kernel.org/linux-wireless/20230404072234.18503-3-quic_youghand@quicinc.com/
>>
>> Tyler
>
> Thanks for your comments. Will fix this in the upcoming patchset.

I have some comments as well but I haven't been able to send them yet. I
recommend waiting for them before sending the next version.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k