Received-SPF: pass (google.com: domain of linux-wireless+bounces-4206-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <788f1df8-64e1-4b3c-ae8e-00c67be1c3de@quicinc.com>
Date: Wed, 28 Feb 2024 07:54:20 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath11k: fix few -Wmaybe-uninitialized warnings
Content-Language: en-US
To: Dmitry Antipov <dmantipov@yandex.ru>,
        Baochen Qiang
	<quic_bqiang@quicinc.com>
CC: Kalle Valo <kvalo@kernel.org>, <ath11k@lists.infradead.org>,
        <linux-wireless@vger.kernel.org>, <lvc-project@linuxtesting.org>
References: <20240228131406.165786-1-dmantipov@yandex.ru>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240228131406.165786-1-dmantipov@yandex.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 2/28/2024 5:14 AM, Dmitry Antipov wrote:
> When compiling with gcc version 14.0.1 20240226 (experimental) and
> W=12, I've noticed the following warnings:
> 
> drivers/net/wireless/ath/ath11k/mac.c: In function 'ath11k_mac_op_remain_on_channel':
> drivers/net/wireless/ath/ath11k/mac.c:9230:12: warning: 'ret' may be used uninitialized
> [-Wmaybe-uninitialized]
>  9230 |         if (ret)
> 
> drivers/net/wireless/ath/ath11k/qmi.c: In function 'ath11k_qmi_load_file_target_mem':
> drivers/net/wireless/ath/ath11k/qmi.c:2401:16: warning: 'ret' may be used uninitialized
> [-Wmaybe-uninitialized]
>  2401 |         return ret;
> 
> drivers/net/wireless/ath/ath11k/qmi.c: In function 'ath11k_qmi_load_bdf_qmi':
> drivers/net/wireless/ath/ath11k/qmi.c:2494:17: warning: 'fw_entry' may be used uninitialized
> [-Wmaybe-uninitialized]
>  2494 |                 release_firmware(fw_entry);
> 
> And a bunch of them traced to uninitialized fields of the same
> variable, e.g.:
> 
> drivers/net/wireless/ath/ath11k/spectral.c: In function 'ath11k_spectral_process_data':
> drivers/net/wireless/ath/ath11k/spectral.c:700:47: warning: 'summ_rpt.meta.freq1' may
> be used uninitialized [-Wmaybe-uninitialized]
>   700 |         struct ath11k_spectral_summary_report summ_rpt;
> 
> Fix all of the above by using 0 and NULL initializers where appropriate.
> Note there are few more (less obvious) -Wmaybe-uninitialized warnings
> still remains, but they're hardly possible to fix without running on
> a physical hardware. Compile tested oly.
> 
> Also noticed by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/ath/ath11k/mac.c      | 2 +-
>  drivers/net/wireless/ath/ath11k/qmi.c      | 4 ++--
>  drivers/net/wireless/ath/ath11k/spectral.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
> index a6a37d67a50a..b89bc7ceaaa7 100644
> --- a/drivers/net/wireless/ath/ath11k/mac.c
> +++ b/drivers/net/wireless/ath/ath11k/mac.c
> @@ -9201,7 +9201,7 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw,
>  	struct ath11k *ar = hw->priv;
>  	struct ath11k_vif *arvif = ath11k_vif_to_arvif(vif);
>  	struct scan_req_params *arg;
> -	int ret;
> +	int ret = 0;

NAK
the only time ret would be uninitialized is if the initial switch()
processed an unknown state, in which case we should stop processing, not
process as successful.

the correct fix for this is to add a default: to the switch() which
warns and sets ret to an appropriate errno

Please submit as a separate patch

>  	u32 scan_time_msec;
>  
>  	mutex_lock(&ar->conf_mutex);
> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
> index 5006f81f779b..4477f652e068 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.c
> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
> @@ -2293,7 +2293,7 @@ static int ath11k_qmi_load_file_target_mem(struct ath11k_base *ab,
>  	struct qmi_txn txn;
>  	const u8 *temp = data;
>  	void __iomem *bdf_addr = NULL;
> -	int ret;
> +	int ret = 0;

I hate this since the only time ret would be uninitialized is if len is
0 (and hence we never execute the while(remaining) loop), but nothing
ever calls this static function with len == 0

but the alternative to this is to add a guard check and that seems like
overkill, so this one is ok

>  	u32 remaining = len;
>  
>  	req = kzalloc(sizeof(*req), GFP_KERNEL);
> @@ -2406,7 +2406,7 @@ static int ath11k_qmi_load_bdf_qmi(struct ath11k_base *ab,
>  {
>  	struct device *dev = ab->dev;
>  	char filename[ATH11K_QMI_MAX_BDF_FILE_NAME_SIZE];
> -	const struct firmware *fw_entry;
> +	const struct firmware *fw_entry = NULL;
>  	struct ath11k_board_data bd;
>  	u32 fw_size, file_type;
>  	int ret = 0, bdf_type;
> diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c
> index 79e091134515..4c826b539404 100644
> --- a/drivers/net/wireless/ath/ath11k/spectral.c
> +++ b/drivers/net/wireless/ath/ath11k/spectral.c
> @@ -697,7 +697,7 @@ static int ath11k_spectral_process_data(struct ath11k *ar,
>  	struct ath11k_base *ab = ar->ab;
>  	struct spectral_tlv *tlv;
>  	struct spectral_summary_fft_report *summary = NULL;
> -	struct ath11k_spectral_summary_report summ_rpt;
> +	struct ath11k_spectral_summary_report summ_rpt = { 0 };

prefer just {} since that works correctly if we ever want to add a
non-scalar as the first member

>  	struct fft_sample_ath11k *fft_sample = NULL;
>  	u8 *data;
>  	u32 data_len, i;