| Message ID | 20240105104542.463834-1-lilinmao@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: rtw89: 8852b: fix cppcheck issues | expand |
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 <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
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.
> -----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. :-)
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;
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(-)