Received-SPF: pass (google.com: domain of linux-wireless+bounces-7054-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <305a93c9-c5f0-a2a2-86c6-95a3f7f218d7@quicinc.com>
Date: Tue, 30 Apr 2024 20:14:16 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCHv5] wifi: ath11k: skip status ring entry processing
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>, Jeff Johnson <quic_jjohnson@quicinc.com>
CC: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        Venkateswara Naralasetty <quic_vnaralas@quicinc.com>
References: <20240429073624.736147-1-quic_tamizhr@quicinc.com>
 <35f114c4-1ff7-4a4b-aadf-ed147f19e170@quicinc.com>
 <87cyq7ota5.fsf@kernel.org>
From: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
In-Reply-To: <87cyq7ota5.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/30/2024 7:18 PM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> 
>> On 4/29/2024 12:36 AM, Tamizh Chelvam Raja wrote:
>>
>>> From: Venkateswara Naralasetty <quic_vnaralas@quicinc.com>
>>>
>>> If STATUS_BUFFER_DONE is not set for a monitor status ring entry,
>>> we don't process the status ring until STATUS_BUFFER_DONE set
>>> for that status ring entry.
>>>
>>> During LMAC reset it may happen that hardware will not write
>>> STATUS_BUFFER_DONE tlv in status buffer, in that case we end up
>>> waiting for STATUS_BUFFER_DONE leading to backpressure on monitor
>>> status ring.
>>>
>>> To fix the issue, when HP(Head Pointer) + 1 entry is peeked and if DMA
>>> is not done and if HP + 2 entry's DMA done is set,
>>> replenish HP + 1 entry and start processing in next interrupt.
>>> If HP + 2 entry's DMA done is not set, poll onto HP + 1 entry DMA
>>> done to be set.
>>>
>>> Also, during monitor attach HP points to the end of the ring and
>>> TP(Tail Pointer) points to the start of the ring.
>>> Using ath11k_hal_srng_src_peek() may result in processing invalid buffer
>>> for the very first interrupt. Since, HW starts writing buffer from TP.
>>>
>>> To avoid this issue call ath11k_hal_srng_src_next_peek() instead of
>>> calling ath11k_hal_srng_src_peek().
>>>
>>> Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.6.0.1-00861-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com>
>>> Co-developed-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
>>> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
>>
>> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>>
>> however note...
>>
>>> +
>>> +				/* If done status is missing:
>>> +				 * 1. As per MAC team's suggestion,
>>> +				 *    when HP + 1 entry is peeked and if DMA
>>> +				 *    is not done and if HP + 2 entry's DMA done
>>> +				 *    is set. skip HP + 1 entry and
>>> +				 *    start processing in next interrupt.
>>> +				 * 2. If HP + 2 entry's DMA done is not set,
>>> +				 *    poll onto HP + 1 entry DMA done to be set.
>>> +				 *    Check status for same buffer for next time
>>> +				 *    dp_rx_mon_status_srng_process
>>> +				 */
>>> +
>>> + reap_status = ath11k_dp_rx_mon_handle_status_buf_done(ab, srng,
>>> + rx_ring);
>>
>> ath11k-check reports:
>>
>> drivers/net/wireless/ath/ath11k/dp_rx.c:3116: line length of 95 exceeds 90 columns
>> drivers/net/wireless/ath/ath11k/dp_rx.c:3117: line length of 95 exceeds 90 columns
> 
> Tamizh, please ALWAYS run ath11k-check. We are wasting time for trivial
> stuff like this.
Sure Kalle.
> 
>> Kalle, in this case we may want to make an exception since I don't think there
>> is a clean way to fix this other than refactoring.
> 
> The new function name looked quite long so I shortened it to
> ath11k_dp_rx_mon_buf_done() and the warning is now gone. Does that look
> reasonable name?
> 
> Also I removed one unrelated change and removed unnecessary else. Please
> check my changes:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=6e88d559268779107715008c51e006f7a5f62045
Change looks good!
> 
> But now I noticed that the warning message needs some work:
> 
> ath11k_warn(ab, "mon status DONE not set %lx, buf_id %d\n",
> 	    FIELD_GET(HAL_TLV_HDR_TAG, tlv->tl),
> 	    buf_id);
> 

> Please use understandable warning and error messages in plain english.
> Any suggestions? I can edit the commit in the pending branch.
>
"hal rx status buffer done is not set in tlv %lx buf_id %d" -> is this fine?
 
>> FWIW I'd like to see this function refactored to avoid the excessive
>> indentation, but that should be a separate exercise.
> 
> Indeed, the indentation in this function is getting close to the limit.
> 


Tamizh