Re: [PATCH ath-next 09/15] wifi: ath12k: add unified firmware image support for QCN9575

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] wifi: ath12k: memory leak of mhi_ctrl->irq in
  ath12k_mhi_register() [1]
--

commit 7e637beba6af9aa8998e9035d9033a359dbfb287
Author: Madhavan Ganesan <madhavan.ganesan@oss.qualcomm.com>

wifi: ath12k: add unified firmware image support for QCN9575

This commit adds unified firmware image support for QCN9575 by caching the
OTP board ID in ath12k_mhi_register() and writing it to a PCIe scratch
register with readback verification in ath12k_pci_power_up().

> diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c
> index 56e697fbdad52..90b7774ae6e8c 100644
> --- a/drivers/net/wireless/ath/ath12k/mhi.c
> +++ b/drivers/net/wireless/ath/ath12k/mhi.c
> @@ -211,6 +211,8 @@ int ath12k_mhi_register(struct ath12k_pci *ab_pci)
>  		if (!board_id || (board_id == OTP_INVALID_BOARD_ID)) {
>  			ath12k_dbg(ab, ATH12K_DBG_BOOT,
>  				   "failed to read board id\n");
> +		} else if (ab->hw_params->fw.unified_fw_image) {
> +			ab_pci->unified_fw_board_id = board_id;
>  		} else if (board_id & OTP_VALID_DUALMAC_BOARD_ID_MASK) {
>  			dualmac = true;
>  			ath12k_dbg(ab, ATH12K_DBG_BOOT,
> @@ -218,7 +220,7 @@ int ath12k_mhi_register(struct ath12k_pci *ab_pci)
>  		}
>  	}
>  
> -	if (dualmac) {
> +	if (!ab->hw_params->fw.unified_fw_image && dualmac) {
>  		if (ab->fw.amss_dualmac_data && ab->fw.amss_dualmac_len > 0) {
>  			/* use MHI firmware file from firmware-N.bin */
>  			mhi_ctrl->fw_data = ab->fw.amss_dualmac_data;

[Severity: Medium]
This problem wasn't introduced by this patch, but does ath12k_mhi_register()
leak mhi_ctrl->irq [1] if mhi_register_controller() fails?

ath12k_mhi_get_msi() allocates the irq array with kcalloc() and assigns it
to mhi_ctrl->irq.  If mhi_register_controller() fails later in
ath12k_mhi_register(), the free_controller error path only calls
mhi_free_controller(), which does not free the driver-allocated irq array:

drivers/net/wireless/ath/ath12k/mhi.c:ath12k_mhi_register() {
	...
	ret = ath12k_mhi_get_msi(ab_pci);
	if (ret) {
		ath12k_err(ab, "failed to get msi for mhi\n");
		goto free_controller;
	}
	...
	ret = mhi_register_controller(mhi_ctrl, ab->hw_params->mhi_config);
	if (ret) {
		ath12k_err(ab, "failed to register to mhi bus, err = %d\n", ret);
		goto free_controller;
	}

	return 0;

free_controller:
	mhi_free_controller(mhi_ctrl);
	ab_pci->mhi_ctrl = NULL;
	return ret;
}

Calling kfree(mhi_ctrl->irq) on the error path before mhi_free_controller(),
matching ath12k_mhi_unregister(), would avoid the leak.

[1] https://sashiko.dev/#/bug/linux-7faf2081-d31b-4759-a736-dcc2f8a00de5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-wifi8_qcn9575_bringup-v1-0-3f30fbc2bb69@oss.qualcomm.com?part=9