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: <5a053fc3-7fd0-f8ea-7512-e473815814f4@quicinc.com>
Date:   Thu, 13 Jul 2023 09:06:49 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] wifi: ath10k: fix PCI and SNOC RX pipe operations
Content-Language: en-US
To:     Dmitry Antipov <dmantipov@yandex.ru>, Kalle Valo <kvalo@kernel.org>
CC:     =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>,
        Arnd Bergmann <arnd@arndb.de>, <linux-wireless@vger.kernel.org>
References: <20230713160101.333483-1-dmantipov@yandex.ru>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230713160101.333483-1-dmantipov@yandex.ru>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/13/2023 9:00 AM, Dmitry Antipov wrote:
> Since 'mod_timer()' expects expiration time in jiffies, add
> missing 'msecs_to_jiffies()' in 'ath10k_pci_rx_post_pipe()'
> and 'ath10k_snoc_rx_post_pipe()'. Also simplify the loops
> and fix number of loop iterations in the former (it should
> be equal to 'num' (i.e. number of buffers) and not 'num + 1'.
> 
> Fixes: 728f95eef523 ("ath10k: rework posting pci rx buffers")
> Fixes: a6e149a9ff03 ("ath10k: add hif start/stop methods for wcn3990 snoc layer")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>   drivers/net/wireless/ath/ath10k/pci.c  | 16 +++++++---------
>   drivers/net/wireless/ath/ath10k/snoc.c | 14 +++++++-------
>   2 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index a7f44f6335fb..c004c87d3211 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -818,17 +818,15 @@ static void ath10k_pci_rx_post_pipe(struct ath10k_pci_pipe *pipe)
>   	num = __ath10k_ce_rx_num_free_bufs(ce_pipe);
>   	spin_unlock_bh(&ce->ce_lock);
>   
> -	while (num >= 0) {
> +	while (num--) {
>   		ret = __ath10k_pci_rx_post_buf(pipe);
> -		if (ret) {
> -			if (ret == -ENOSPC)
> -				break;
> -			ath10k_warn(ar, "failed to post pci rx buf: %d\n", ret);
> -			mod_timer(&ar_pci->rx_post_retry, jiffies +
> -				  ATH10K_PCI_RX_POST_RETRY_MS);
> +		if (ret)
>   			break;
> -		}
> -		num--;
> +	}
> +	if (ret != -ENOSPC) {

doesn't this do incorrect processing for the case where ret == 0?

> +		ath10k_warn(ar, "failed to post PCI rx buf: %d\n", ret);
> +		mod_timer(&ar_pci->rx_post_retry, jiffies +
> +			  msecs_to_jiffies(ATH10K_PCI_RX_POST_RETRY_MS));
>   	}
>   }
>   
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 26214c00cd0d..9ced24b1b177 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -544,16 +544,16 @@ static void ath10k_snoc_rx_post_pipe(struct ath10k_snoc_pipe *pipe)
>   	spin_lock_bh(&ce->ce_lock);
>   	num = __ath10k_ce_rx_num_free_bufs(ce_pipe);
>   	spin_unlock_bh(&ce->ce_lock);
> +
>   	while (num--) {
>   		ret = __ath10k_snoc_rx_post_buf(pipe);
> -		if (ret) {
> -			if (ret == -ENOSPC)
> -				break;
> -			ath10k_warn(ar, "failed to post rx buf: %d\n", ret);
> -			mod_timer(&ar_snoc->rx_post_retry, jiffies +
> -				  ATH10K_SNOC_RX_POST_RETRY_MS);
> +		if (ret)
>   			break;
> -		}
> +	}
> +	if (ret != -ENOSPC) {

doesn't this do incorrect processing for the case where ret == 0?

> +		ath10k_warn(ar, "failed to post SNOC rx buf: %d\n", ret);
> +		mod_timer(&ar_snoc->rx_post_retry, jiffies +
> +			  msecs_to_jiffies(ATH10K_SNOC_RX_POST_RETRY_MS));
>   	}
>   }
>