Received-SPF: pass (google.com: domain of linux-wireless+bounces-1526-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <84a1fba8-b709-4bf5-8fc0-9d32d245f9ff@quicinc.com>
Date: Fri, 5 Jan 2024 10:36:58 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath12k: add firmware-2.bin support
Content-Language: en-US
To: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240105164207.2906093-1-quic_rajkbhag@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240105164207.2906093-1-quic_rajkbhag@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 1/5/2024 8:42 AM, Raj Kumar Bhagat wrote:
> Firmware IE containers can dynamically provide various information
> what firmware supports. Also it can embed more than one image so
> updating firmware is easy, user just needs to update one file in
> /lib/firmware/.
> 
> The firmware API 2 or higher will use the IE container format, the
> current API 1 will not use the new format but it still is supported
> for some time. Firmware API 2 files are named as firmware-2.bin
> (which contains both amss.bin and m3.bin images) and API 1 files are
> amss.bin and m3.bin.
> 
> Currently ath12k PCI driver provides firmware binary (amss.bin) path to
> MHI driver, MHI driver reads firmware from filesystem and boots it. Add
> provision to read firmware files from ath12k driver and provide the amss.bin
> firmware data and size to MHI using a pointer.
> 
> Currently enum ath12k_fw_features is empty, the patches adding features will
> add the flags.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/Makefile |   3 +-
>  drivers/net/wireless/ath/ath12k/core.c   |  10 +-
>  drivers/net/wireless/ath/ath12k/core.h   |  16 ++-
>  drivers/net/wireless/ath/ath12k/fw.c     | 165 +++++++++++++++++++++++
>  drivers/net/wireless/ath/ath12k/fw.h     |  27 ++++
>  drivers/net/wireless/ath/ath12k/mhi.c    |  20 ++-
>  drivers/net/wireless/ath/ath12k/qmi.c    |  51 ++++---
>  7 files changed, 267 insertions(+), 25 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath12k/fw.c
>  create mode 100644 drivers/net/wireless/ath/ath12k/fw.h
> 
...
> +int ath12k_fw_map(struct ath12k_base *ab)
> +{
> +	int ret;
> +
> +	ret = ath12k_fw_request_firmware_api_n(ab, ATH12K_FW_API2_FILE);
> +	if (ret == 0)
> +		ab->fw.api_version = 2;
> +	else
> +		ab->fw.api_version = 1;
> +
> +	ath12k_dbg(ab, ATH12K_DBG_BOOT, "using fw api %d\n",
> +		   ab->fw.api_version);
> +
> +	return 0;

since this always returns 0 perhaps make this a void function and remove
the error checking from the caller?

> +}
> +
> +void ath12k_fw_unmap(struct ath12k_base *ab)
> +{
> +	release_firmware(ab->fw.fw);

should we memset the entire ab->fw struct to 0 so that there aren't any
dangling pointers into the firmware buffer?

> +}