[PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[lilinmao]

cppcheck reports
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1414:22: error: Array
 'iqk_info->iqk_mcc_ch[2][4]' accessed at index iqk_info->iqk_mcc_ch[2][*],
which is out of bounds. [arrayIndexOutOfBounds]
 iqk_info->iqk_mcc_ch[idx][path] = chan->channel;
                     ^
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1393:2: note: After for
loop, idx has value 2
 for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++) {
 ^
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1414:22: note: Array index
out of bounds
 iqk_info->iqk_mcc_ch[idx][path] = chan->channel;
                     ^
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1424:38: error: Array
'iqk_info->iqk_mcc_ch[2][4]' accessed at index iqk_info->iqk_mcc_ch[2][*],
which is out of bounds. [arrayIndexOutOfBounds]
      idx, path, iqk_info->iqk_mcc_ch[idx][path]);
                                     ^
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1393:2: note: After for
loop, idx has value 2
 for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++) {
 ^
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c:1424:38: note: Array index
out of bounds
      idx, path, iqk_info->iqk_mcc_ch[idx][path]);
                                     ^

Fixes: f2abe804e823 ("wifi: rtw89: 8852b: rfk: add IQK")
Signed-off-by: lilinmao <lilinmao@kylinos.cn>
---
 drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
index 259df67836a0..03dec3f4e7ba 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
@@ -1388,17 +1388,15 @@ static void _iqk_get_ch_info(struct rtw89_dev *rtwdev, enum rtw89_phy_idx phy, u
 	u32 reg_rf18;
 	u32 reg_35c;
 	u8 idx;
-	u8 get_empty_table = false;
 
 	for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++) {
 		if (iqk_info->iqk_mcc_ch[idx][path] == 0) {
-			get_empty_table = true;
 			break;
 		}
 	}
 	rtw89_debug(rtwdev, RTW89_DBG_RFK, "[IQK] (1)idx = %x\n", idx);
 
-	if (!get_empty_table) {
+	if (idx > RTW89_IQK_CHS_NR - 1) {
 		idx = iqk_info->iqk_table_idx[path] + 1;
 		if (idx > 1)
 			idx = 0;
-- 
2.25.1

Re: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[Johannes Berg]

On Fri, 2024-01-05 at 18:45 +0800, lilinmao wrote:
> cppcheck reports

[snip]

Look ... you really should write up an explanation of what the patch is
doing, whether the tool is correct or not, etc.

Please don't blindly paste some checker messages and make random changes
to the code to suppress them. We get too many such patches. Convince us
with a useful commit message that you've actually thought about it.

> Fixes: f2abe804e823 ("wifi: rtw89: 8852b: rfk: add IQK")

Really not much point in that, since it clearly doesn't actually _fix_
anything.


johannes

RE: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[Ping-Ke Shih]

> -----Original Message-----
> From: lilinmao <lilinmao@kylinos.cn>
> Sent: Friday, January 5, 2024 6:46 PM
> To: linux-wireless@vger.kernel.org
> Cc: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org; lilinmao <lilinmao@kylinos.cn>
> Subject: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues

The subject doesn't address the problem or describe the changes you made. 
Maybe, you can say "fix out of bounds of iqk_mcc_ch[][] array", but actually
cppcheck reported a false alarm, doesn't it?

[...]

> Signed-off-by: lilinmao <lilinmao@kylinos.cn>

You should give your real name here as well as "From:" field. 

> ---
>  drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
> b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
> index 259df67836a0..03dec3f4e7ba 100644
> --- a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
> +++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
> @@ -1388,17 +1388,15 @@ static void _iqk_get_ch_info(struct rtw89_dev *rtwdev, enum rtw89_phy_idx phy, u
>         u32 reg_rf18;
>         u32 reg_35c;
>         u8 idx;
> -       u8 get_empty_table = false;
> 
>         for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++) {
>                 if (iqk_info->iqk_mcc_ch[idx][path] == 0) {
> -                       get_empty_table = true;
>                         break;
>                 }
>         }
>         rtw89_debug(rtwdev, RTW89_DBG_RFK, "[IQK] (1)idx = %x\n", idx);
> 
> -       if (!get_empty_table) {
> +       if (idx > RTW89_IQK_CHS_NR - 1) {
>                 idx = iqk_info->iqk_table_idx[path] + 1;
>                 if (idx > 1)
>                         idx = 0;

The original logic looks like

bool found = false;

for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++)
     if (expr) {
		found = true;
		break;
	}

if (!found) { 
	... [A]
}

So, idx >= RTW89_IQK_CHS_NR must fall into branch [A]. Can you report this
pattern to cppcheck?

Ping-Ke 

Re: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[Kalle Valo]

lilinmao <lilinmao@kylinos.cn> writes:

> I'm very sorry for the various issues encountered during my first patch submission.
>
> My patch didn't change the original logic of the code.Perhaps I just changed the way 
> of writing the code to avoid the cppcheck issue.
>
>>The original logic looks like 
>> 
>>bool found = false; 
>> 
>>for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++) 
>>if (expr) { 
>>found = true; 
>>break; 
>>} 
>> 
>>if (!found) { 
>>... [A] 
>>}
>
> After the 'for' loop ends, 'if (idx > RTW89_IQK_CHS_NR - 1)' is
> equivalent to 'if (!found). Cppcheck might not have detected the
> changes to 'idx' within branch [A] which leads it to believe later
> that 'idx' could be greater than or equal to 'RTW89_IQK_CHS_NR'.

Our lists drop all html mail, so please use text/plain format and don't
top post. More info in the wiki link below.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues

[Ping-Ke Shih]

> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Monday, January 8, 2024 11:24 PM
> To: lilinmao <lilinmao@kylinos.cn>
> Cc: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kern… <linux-wireless@vger.kernel.org>
> Subject: Re: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues
> 
> lilinmao <lilinmao@kylinos.cn> writes:
> 
> > I'm very sorry for the various issues encountered during my first patch submission.
> >
> > My patch didn't change the original logic of the code.Perhaps I just changed the way
> > of writing the code to avoid the cppcheck issue.

Yes. I think you didn't change the logic, so explain this and what you made
in commit message as Johannes mentioned. 

> >
> >>The original logic looks like
> >>
> >>bool found = false;
> >>
> >>for (idx = 0; idx < RTW89_IQK_CHS_NR; idx++)
> >>if (expr) {
> >>found = true;
> >>break;
> >>}
> >>
> >>if (!found) {
> >>... [A]
> >>}
> >
> > After the 'for' loop ends, 'if (idx > RTW89_IQK_CHS_NR - 1)' is
> > equivalent to 'if (!found). 

I prefer 'if (idx >= RTW89_IQK_CHS_NR)'

> > Cppcheck might not have detected the
> > changes to 'idx' within branch [A] which leads it to believe later
> > that 'idx' could be greater than or equal to 'RTW89_IQK_CHS_NR'.

So, can you refine cppcheck? 

> 
> Our lists drop all html mail, so please use text/plain format and don't
> top post. More info in the wiki link below.
> 

Thank you, Kalle. With your reformatting, I can simply reply this. :-)