Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <4f289038-c5ca-ce55-edc8-45ff98efc716@quicinc.com>
Date:   Wed, 1 Jun 2022 12:09:48 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 1/2] ath11k: fix missing srng_access_end in CE
Content-Language: en-US
To:     Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>,
        <ath11k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>
References: <20220528142516.20819-1-ansuelsmth@gmail.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220528142516.20819-1-ansuelsmth@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/28/2022 7:25 AM, Christian 'Ansuel' Marangi wrote:
> When a CE completed send next operation is done, the srng access end is
> never called. Correctly end the srng access to make sure we have the
> correct values in the srng struct.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> ---
>   drivers/net/wireless/ath/ath11k/ce.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index c14c51f38709..665205d2322e 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -490,6 +490,8 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
>   	pipe->src_ring->sw_index = sw_index;
>   
>   err_unlock:
> +	ath11k_hal_srng_access_end(ab, srng);
> +
>   	spin_unlock_bh(&srng->lock);
>   
>   	spin_unlock_bh(&ab->ce.ce_lock);

NAK -- Not calling ath11k_hal_srng_access_end() is intentional.

CE_SRC ring is a special ring, we perform tx_request and tx_completion 
on the same ring. The completion processing frees SKBs associated to the 
ce_desc, this is what ath11k_ce_completed_send_next() does, it does not 
update the head pointer to hw because the ce_desc at hp does not have 
any new data, it is just available for new messages. ath11k_ce_send() 
retrieves a processed (processed by ath11k_ce_completed_send_next) 
ce_desc and sets it up for the new message. Then it updates the hw about 
the newly available ce_desc by calling ath11k_hal_srng_access_end().

If you want to do anything here, adding a comment based upon the above 
would be good.

/jeff