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: <d86d8f47-dbb2-1da6-794a-c020c317b4b5@quicinc.com>
Date:   Thu, 27 Jul 2023 09:47:21 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 2/3] wifi: ath11k: qmi: refactor ath11k_qmi_m3_load()
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>, <mhi@lists.linux.dev>
CC:     <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20230727100430.3603551-1-kvalo@kernel.org>
 <20230727100430.3603551-3-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230727100430.3603551-3-kvalo@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/27/2023 3:04 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
> 
> Simple refactoring to make it easier to add firmware-2.bin support in the
> following patch.
> 
> Earlier ath11k_qmi_m3_load() supported changing m3.bin contents while ath11k is
> running. But that's not going to actually work, m3.bin is supposed to the be

nit: s/to the be/to be the/

> same during the lifetime of ath11k, for example we don't support changing the
> firmware capabilities on the fly. Due to this ath11k requests m3.bin firmware
> file first and only then checks m3_mem->vaddr, so we are basically requesting
> the firmware file even if it's not needed. Reverse the code so that m3_mem
> buffer is checked first, and only if it doesn't exist, then m3.bin is requested
> from user space.
> 
> Checking for m3_mem->size is redundant when m3_mem->vaddr is NULL, we would
> not be able to use the buffer in that case. So remove the check for size.
> 
> Simplify the exit handling and use 'goto out'.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9
> 
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
>   drivers/net/wireless/ath/ath11k/qmi.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
> index d4eaf7d2ba84..9b13628fa3c7 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.c
> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
> @@ -2506,6 +2506,10 @@ static int ath11k_qmi_m3_load(struct ath11k_base *ab)
>   	char path[100];
>   	int ret;
>   
> +	if (m3_mem->vaddr)
> +		/* m3 firmware buffer is already available in the DMA buffer */
> +		return 0;
> +
>   	fw = ath11k_core_firmware_request(ab, ATH11K_M3_FILE);
>   	if (IS_ERR(fw)) {
>   		ret = PTR_ERR(fw);
> @@ -2515,25 +2519,25 @@ static int ath11k_qmi_m3_load(struct ath11k_base *ab)
>   		return ret;
>   	}
>   
> -	if (m3_mem->vaddr || m3_mem->size)
> -		goto skip_m3_alloc;
> -
>   	m3_mem->vaddr = dma_alloc_coherent(ab->dev,
>   					   fw->size, &m3_mem->paddr,
>   					   GFP_KERNEL);
>   	if (!m3_mem->vaddr) {
>   		ath11k_err(ab, "failed to allocate memory for M3 with size %zu\n",
>   			   fw->size);
> -		release_firmware(fw);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto out;
>   	}
>   
> -skip_m3_alloc:
>   	memcpy(m3_mem->vaddr, fw->data, fw->size);
>   	m3_mem->size = fw->size;
> +
> +	ret = 0;
> +
> +out:
>   	release_firmware(fw);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static void ath11k_qmi_m3_free(struct ath11k_base *ab)