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 <[email protected]>
---
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
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
> -----Original Message-----
> From: lilinmao <[email protected]>
> Sent: Friday, January 5, 2024 6:46 PM
> To: [email protected]
> Cc: Ping-Ke Shih <[email protected]>; [email protected]; lilinmao <[email protected]>
> 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 <[email protected]>
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
lilinmao <[email protected]> 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
> -----Original Message-----
> From: Kalle Valo <[email protected]>
> Sent: Monday, January 8, 2024 11:24 PM
> To: lilinmao <[email protected]>
> Cc: Ping-Ke Shih <[email protected]>; [email protected]? <[email protected]>
> Subject: Re: [PATCH] wifi: rtw89: 8852b: fix cppcheck issues
>
> lilinmao <[email protected]> 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. :-)