* rtlwifi: potential bugs
@ 2021-04-23 12:56 Inigo Huguet
2021-05-05 11:23 ` Inigo Huguet
0 siblings, 1 reply; 8+ messages in thread
From: Inigo Huguet @ 2021-04-23 12:56 UTC (permalink / raw)
To: pkshih, linux-wireless; +Cc: Ivan Vecera
Hello,
Executing some static analysis on the kernel, we've got this results
affecting rtlwifi drivers:
Error: IDENTICAL_BRANCHES (CWE-398): [#def212]
kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813:
identical_branches: The same code is executed regardless of whether
"bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state ==
BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else'
branches are identical. Should one of the branches be modified, or the
entire 'if' statement replaced?
# 2811| }
# 2812|
# 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) ||
# 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
# 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23);
Error: IDENTICAL_BRANCHES (CWE-398): [#def213]
kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947:
identical_branches: The same code is executed regardless of whether
"bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state ==
BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else'
branches are identical. Should one of the branches be modified, or the
entire 'if' statement replaced?
# 2945| }
# 2946|
# 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) ||
# 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH))
# 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26);
Error: IDENTICAL_BRANCHES (CWE-398): [#def214]
kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135:
identical_branches: The same code is executed regardless of whether
"wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else'
branches are identical. Should one of the branches be modified, or the
entire 'if' statement replaced?
# 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw);
# 3134|
# 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) {
# 3136| /* for HID at 11b/g mode */
# 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff,
Error: IDENTICAL_BRANCHES (CWE-398): [#def215]
kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324:
identical_branches: The same code is executed regardless of whether
"bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state ==
BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else'
branches are identical. Should one of the branches be modified, or the
entire 'if' statement replaced?
# 3322| }
# 3323|
# 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) ||
# 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
# 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23);
In my opinion, they seem to be real bugs. However, it's very difficult
to imagine what actions must be taken on each branch of the if-else
because they strongly depend on magic numbers, which are different
configurations for the hw, I guess.
Can the maintainers confirm if these are real bugs and see how to fix them?
Regards
--
Íñigo Huguet
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: rtlwifi: potential bugs 2021-04-23 12:56 rtlwifi: potential bugs Inigo Huguet @ 2021-05-05 11:23 ` Inigo Huguet 2021-05-05 12:13 ` Pkshih 0 siblings, 1 reply; 8+ messages in thread From: Inigo Huguet @ 2021-05-05 11:23 UTC (permalink / raw) To: pkshih, linux-wireless; +Cc: Ivan Vecera On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > Hello, > > Executing some static analysis on the kernel, we've got this results > affecting rtlwifi drivers: > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > identical_branches: The same code is executed regardless of whether > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > branches are identical. Should one of the branches be modified, or the > entire 'if' statement replaced? > # 2811| } > # 2812| > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > identical_branches: The same code is executed regardless of whether > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > branches are identical. Should one of the branches be modified, or the > entire 'if' statement replaced? > # 2945| } > # 2946| > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > identical_branches: The same code is executed regardless of whether > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > branches are identical. Should one of the branches be modified, or the > entire 'if' statement replaced? > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > # 3134| > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > # 3136| /* for HID at 11b/g mode */ > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > identical_branches: The same code is executed regardless of whether > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > branches are identical. Should one of the branches be modified, or the > entire 'if' statement replaced? > # 3322| } > # 3323| > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > In my opinion, they seem to be real bugs. However, it's very difficult > to imagine what actions must be taken on each branch of the if-else > because they strongly depend on magic numbers, which are different > configurations for the hw, I guess. > > Can the maintainers confirm if these are real bugs and see how to fix them? > > Regards > -- > Íñigo Huguet Hello, A few weeks ago I sent the message above notifying a potential bug in rtlwifi module. I just wanted to be sure that it has been received. Can the maintainers acknowledge whether they have seen it? Thanks! -- Íñigo Huguet ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 11:23 ` Inigo Huguet @ 2021-05-05 12:13 ` Pkshih 2021-05-05 13:01 ` Inigo Huguet 0 siblings, 1 reply; 8+ messages in thread From: Pkshih @ 2021-05-05 12:13 UTC (permalink / raw) To: linux-wireless, ihuguet; +Cc: ivecera On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > Hello, > > > > Executing some static analysis on the kernel, we've got this results > > affecting rtlwifi drivers: > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > identical_branches: The same code is executed regardless of whether > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > branches are identical. Should one of the branches be modified, or the > > entire 'if' statement replaced? > > # 2811| } > > # 2812| > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > identical_branches: The same code is executed regardless of whether > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > branches are identical. Should one of the branches be modified, or the > > entire 'if' statement replaced? > > # 2945| } > > # 2946| > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > identical_branches: The same code is executed regardless of whether > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > branches are identical. Should one of the branches be modified, or the > > entire 'if' statement replaced? > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > # 3134| > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > # 3136| /* for HID at 11b/g mode */ > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > identical_branches: The same code is executed regardless of whether > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > branches are identical. Should one of the branches be modified, or the > > entire 'if' statement replaced? > > # 3322| } > > # 3323| > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > to imagine what actions must be taken on each branch of the if-else > > because they strongly depend on magic numbers, which are different > > configurations for the hw, I guess. > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > Regards > > -- > > Íñigo Huguet > > Hello, > > A few weeks ago I sent the message above notifying a potential bug in > rtlwifi module. I just wanted to be sure that it has been received. > Can the maintainers acknowledge whether they have seen it? > Hi, Not real bugs. The coexistence programmers preserve the same code of branches intentionally to fine tune performance easier, because bandwidth and RSSI strength are highly related to coexistence performance. The basic rule of performance tuning is to assign most time slot to BT for realtime application, and WiFi uses remaining time slot but don't lower than low bound. -- Ping-Ke ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 12:13 ` Pkshih @ 2021-05-05 13:01 ` Inigo Huguet 2021-05-05 14:03 ` Pkshih 0 siblings, 1 reply; 8+ messages in thread From: Inigo Huguet @ 2021-05-05 13:01 UTC (permalink / raw) To: Pkshih; +Cc: linux-wireless, ivecera Hi, Thanks for the info. Maybe we should consider adding some comments to clarify this? Other people might also think these are bugs... Regards, On Wed, May 5, 2021 at 2:13 PM Pkshih <pkshih@realtek.com> wrote: > > On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > > > Hello, > > > > > > Executing some static analysis on the kernel, we've got this results > > > affecting rtlwifi drivers: > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > > identical_branches: The same code is executed regardless of whether > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > branches are identical. Should one of the branches be modified, or the > > > entire 'if' statement replaced? > > > # 2811| } > > > # 2812| > > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > > identical_branches: The same code is executed regardless of whether > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > branches are identical. Should one of the branches be modified, or the > > > entire 'if' statement replaced? > > > # 2945| } > > > # 2946| > > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > > identical_branches: The same code is executed regardless of whether > > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > > branches are identical. Should one of the branches be modified, or the > > > entire 'if' statement replaced? > > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > > # 3134| > > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > > # 3136| /* for HID at 11b/g mode */ > > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > > identical_branches: The same code is executed regardless of whether > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > branches are identical. Should one of the branches be modified, or the > > > entire 'if' statement replaced? > > > # 3322| } > > > # 3323| > > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > > to imagine what actions must be taken on each branch of the if-else > > > because they strongly depend on magic numbers, which are different > > > configurations for the hw, I guess. > > > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > > > Regards > > > -- > > > Íñigo Huguet > > > > Hello, > > > > A few weeks ago I sent the message above notifying a potential bug in > > rtlwifi module. I just wanted to be sure that it has been received. > > Can the maintainers acknowledge whether they have seen it? > > > > Hi, > > Not real bugs. The coexistence programmers preserve the same code of > branches intentionally to fine tune performance easier, because bandwidth and > RSSI strength are highly related to coexistence performance. > The basic rule of performance tuning is to assign most time slot to BT > for realtime application, and WiFi uses remaining time slot but don't lower > than low bound. > > -- > Ping-Ke -- Íñigo Huguet ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 13:01 ` Inigo Huguet @ 2021-05-05 14:03 ` Pkshih 2021-05-05 14:20 ` Inigo Huguet 0 siblings, 1 reply; 8+ messages in thread From: Pkshih @ 2021-05-05 14:03 UTC (permalink / raw) To: ihuguet; +Cc: linux-wireless, ivecera On Wed, 2021-05-05 at 13:01 +0000, Inigo Huguet wrote: > Hi, > > Thanks for the info. Maybe we should consider adding some comments to > clarify this? Other people might also think these are bugs... > > Regards, > > On Wed, May 5, 2021 at 2:13 PM Pkshih <pkshih@realtek.com> wrote: > > > > On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > > > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > > > > > Hello, > > > > > > > > Executing some static analysis on the kernel, we've got this results > > > > affecting rtlwifi drivers: > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > > > identical_branches: The same code is executed regardless of whether > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > branches are identical. Should one of the branches be modified, or the > > > > entire 'if' statement replaced? > > > > # 2811| } > > > > # 2812| > > > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > > > identical_branches: The same code is executed regardless of whether > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > branches are identical. Should one of the branches be modified, or the > > > > entire 'if' statement replaced? > > > > # 2945| } > > > > # 2946| > > > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > > > identical_branches: The same code is executed regardless of whether > > > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > > > branches are identical. Should one of the branches be modified, or the > > > > entire 'if' statement replaced? > > > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > > > # 3134| > > > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > > > # 3136| /* for HID at 11b/g mode */ > > > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > > > identical_branches: The same code is executed regardless of whether > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > branches are identical. Should one of the branches be modified, or the > > > > entire 'if' statement replaced? > > > > # 3322| } > > > > # 3323| > > > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > > > to imagine what actions must be taken on each branch of the if-else > > > > because they strongly depend on magic numbers, which are different > > > > configurations for the hw, I guess. > > > > > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > > > > > Regards > > > > -- > > > > Íñigo Huguet > > > > > > Hello, > > > > > > A few weeks ago I sent the message above notifying a potential bug in > > > rtlwifi module. I just wanted to be sure that it has been received. > > > Can the maintainers acknowledge whether they have seen it? > > > > > > > Hi, > > > > Not real bugs. The coexistence programmers preserve the same code of > > branches intentionally to fine tune performance easier, because bandwidth and > > RSSI strength are highly related to coexistence performance. > > The basic rule of performance tuning is to assign most time slot to BT > > for realtime application, and WiFi uses remaining time slot but don't lower > > than low bound. > > > > Hi, I can add comments. Do you need any keyword within comment to avoid your checking tool warns this false alarm? -- Ping-Ke ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 14:03 ` Pkshih @ 2021-05-05 14:20 ` Inigo Huguet 2021-05-05 14:33 ` Pkshih 0 siblings, 1 reply; 8+ messages in thread From: Inigo Huguet @ 2021-05-05 14:20 UTC (permalink / raw) To: Pkshih; +Cc: linux-wireless, ivecera If it's no problem to add them, a comment STARTING WITH `coverity[identical_branches]` should suppress the warnings. Example: /* Explanation why this code is fine * and great */ /* coverity[identical_branches] */ if (...) ... Thanks! On Wed, May 5, 2021 at 4:03 PM Pkshih <pkshih@realtek.com> wrote: > > On Wed, 2021-05-05 at 13:01 +0000, Inigo Huguet wrote: > > Hi, > > > > Thanks for the info. Maybe we should consider adding some comments to > > clarify this? Other people might also think these are bugs... > > > > Regards, > > > > On Wed, May 5, 2021 at 2:13 PM Pkshih <pkshih@realtek.com> wrote: > > > > > > On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > > > > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > > > > > > > Hello, > > > > > > > > > > Executing some static analysis on the kernel, we've got this results > > > > > affecting rtlwifi drivers: > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > > > > identical_branches: The same code is executed regardless of whether > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > branches are identical. Should one of the branches be modified, or the > > > > > entire 'if' statement replaced? > > > > > # 2811| } > > > > > # 2812| > > > > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > > > > identical_branches: The same code is executed regardless of whether > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > branches are identical. Should one of the branches be modified, or the > > > > > entire 'if' statement replaced? > > > > > # 2945| } > > > > > # 2946| > > > > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > > > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > > > > identical_branches: The same code is executed regardless of whether > > > > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > > > > branches are identical. Should one of the branches be modified, or the > > > > > entire 'if' statement replaced? > > > > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > > > > # 3134| > > > > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > > > > # 3136| /* for HID at 11b/g mode */ > > > > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > > > > identical_branches: The same code is executed regardless of whether > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > branches are identical. Should one of the branches be modified, or the > > > > > entire 'if' statement replaced? > > > > > # 3322| } > > > > > # 3323| > > > > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > > > > to imagine what actions must be taken on each branch of the if-else > > > > > because they strongly depend on magic numbers, which are different > > > > > configurations for the hw, I guess. > > > > > > > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > > > > > > > Regards > > > > > -- > > > > > Íñigo Huguet > > > > > > > > Hello, > > > > > > > > A few weeks ago I sent the message above notifying a potential bug in > > > > rtlwifi module. I just wanted to be sure that it has been received. > > > > Can the maintainers acknowledge whether they have seen it? > > > > > > > > > > Hi, > > > > > > Not real bugs. The coexistence programmers preserve the same code of > > > branches intentionally to fine tune performance easier, because bandwidth and > > > RSSI strength are highly related to coexistence performance. > > > The basic rule of performance tuning is to assign most time slot to BT > > > for realtime application, and WiFi uses remaining time slot but don't lower > > > than low bound. > > > > > > > > Hi, > > I can add comments. Do you need any keyword within comment to avoid your > checking tool warns this false alarm? > > -- > Ping-Ke > -- Íñigo Huguet ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 14:20 ` Inigo Huguet @ 2021-05-05 14:33 ` Pkshih 2021-05-05 14:36 ` Inigo Huguet 0 siblings, 1 reply; 8+ messages in thread From: Pkshih @ 2021-05-05 14:33 UTC (permalink / raw) To: ihuguet; +Cc: linux-wireless, ivecera On Wed, 2021-05-05 at 16:20 +0200, Inigo Huguet wrote: > If it's no problem to add them, a comment STARTING WITH > `coverity[identical_branches]` should suppress the warnings. > > Example: > > /* Explanation why this code is fine > * and great > */ > /* coverity[identical_branches] */ > if (...) > ... > > Thanks! > > On Wed, May 5, 2021 at 4:03 PM Pkshih <pkshih@realtek.com> wrote: > > > > On Wed, 2021-05-05 at 13:01 +0000, Inigo Huguet wrote: > > > Hi, > > > > > > Thanks for the info. Maybe we should consider adding some comments to > > > clarify this? Other people might also think these are bugs... > > > > > > Regards, > > > > > > On Wed, May 5, 2021 at 2:13 PM Pkshih <pkshih@realtek.com> wrote: > > > > > > > > On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > > > > > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > > > > > > > > > Hello, > > > > > > > > > > > > Executing some static analysis on the kernel, we've got this results > > > > > > affecting rtlwifi drivers: > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > entire 'if' statement replaced? > > > > > > # 2811| } > > > > > > # 2812| > > > > > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > entire 'if' statement replaced? > > > > > > # 2945| } > > > > > > # 2946| > > > > > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > > > > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > entire 'if' statement replaced? > > > > > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > > > > > # 3134| > > > > > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > > > > > # 3136| /* for HID at 11b/g mode */ > > > > > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > entire 'if' statement replaced? > > > > > > # 3322| } > > > > > > # 3323| > > > > > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > > > > > to imagine what actions must be taken on each branch of the if-else > > > > > > because they strongly depend on magic numbers, which are different > > > > > > configurations for the hw, I guess. > > > > > > > > > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > > > > > > > > > Regards > > > > > > -- > > > > > > Íñigo Huguet > > > > > > > > > > Hello, > > > > > > > > > > A few weeks ago I sent the message above notifying a potential bug in > > > > > rtlwifi module. I just wanted to be sure that it has been received. > > > > > Can the maintainers acknowledge whether they have seen it? > > > > > > > > > > > > > Hi, > > > > > > > > Not real bugs. The coexistence programmers preserve the same code of > > > > branches intentionally to fine tune performance easier, because bandwidth and > > > > RSSI strength are highly related to coexistence performance. > > > > The basic rule of performance tuning is to assign most time slot to BT > > > > for realtime application, and WiFi uses remaining time slot but don't lower > > > > than low bound. > > > > > > > > > > > > Hi, > > > > I can add comments. Do you need any keyword within comment to avoid your > > checking tool warns this false alarm? > > I do "git grep coverity | wc -l" and there are only 8 instances. I'm not sure if I can add comments with "coverity" marker. -- Ping-Ke ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rtlwifi: potential bugs 2021-05-05 14:33 ` Pkshih @ 2021-05-05 14:36 ` Inigo Huguet 0 siblings, 0 replies; 8+ messages in thread From: Inigo Huguet @ 2021-05-05 14:36 UTC (permalink / raw) To: Pkshih; +Cc: linux-wireless, ivecera Don't worry then, the explanation for humans is more than enough. On Wed, May 5, 2021 at 4:33 PM Pkshih <pkshih@realtek.com> wrote: > > On Wed, 2021-05-05 at 16:20 +0200, Inigo Huguet wrote: > > If it's no problem to add them, a comment STARTING WITH > > `coverity[identical_branches]` should suppress the warnings. > > > > Example: > > > > /* Explanation why this code is fine > > * and great > > */ > > /* coverity[identical_branches] */ > > if (...) > > ... > > > > Thanks! > > > > On Wed, May 5, 2021 at 4:03 PM Pkshih <pkshih@realtek.com> wrote: > > > > > > On Wed, 2021-05-05 at 13:01 +0000, Inigo Huguet wrote: > > > > Hi, > > > > > > > > Thanks for the info. Maybe we should consider adding some comments to > > > > clarify this? Other people might also think these are bugs... > > > > > > > > Regards, > > > > > > > > On Wed, May 5, 2021 at 2:13 PM Pkshih <pkshih@realtek.com> wrote: > > > > > > > > > > On Wed, 2021-05-05 at 11:23 +0000, Inigo Huguet wrote: > > > > > > On Fri, Apr 23, 2021 at 2:56 PM Inigo Huguet <ihuguet@redhat.com> wrote: > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > Executing some static analysis on the kernel, we've got this results > > > > > > > affecting rtlwifi drivers: > > > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def212] > > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813: > > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > > entire 'if' statement replaced? > > > > > > > # 2811| } > > > > > > > # 2812| > > > > > > > # 2813|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > > # 2814| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > > > # 2815| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def213] > > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947: > > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > > entire 'if' statement replaced? > > > > > > > # 2945| } > > > > > > > # 2946| > > > > > > > # 2947|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > > # 2948| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) > > > > > > > # 2949| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 26); > > > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def214] > > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135: > > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > > "wifi_bw == BTC_WIFI_BW_LEGACY" is true, because the 'then' and 'else' > > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > > entire 'if' statement replaced? > > > > > > > # 3133| btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &wifi_bw); > > > > > > > # 3134| > > > > > > > # 3135|-> if (wifi_bw == BTC_WIFI_BW_LEGACY) { > > > > > > > # 3136| /* for HID at 11b/g mode */ > > > > > > > # 3137| btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x55ff55ff, > > > > > > > > > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215] > > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324: > > > > > > > identical_branches: The same code is executed regardless of whether > > > > > > > "bt_rssi_state == BTC_RSSI_STATE_HIGH || bt_rssi_state == > > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'else' > > > > > > > branches are identical. Should one of the branches be modified, or the > > > > > > > entire 'if' statement replaced? > > > > > > > # 3322| } > > > > > > > # 3323| > > > > > > > # 3324|-> if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) || > > > > > > > # 3325| (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) { > > > > > > > # 3326| btc8821a2ant_ps_tdma(btcoexist, NORMAL_EXEC, true, 23); > > > > > > > > > > > > > > > > > > > > > In my opinion, they seem to be real bugs. However, it's very difficult > > > > > > > to imagine what actions must be taken on each branch of the if-else > > > > > > > because they strongly depend on magic numbers, which are different > > > > > > > configurations for the hw, I guess. > > > > > > > > > > > > > > Can the maintainers confirm if these are real bugs and see how to fix them? > > > > > > > > > > > > > > Regards > > > > > > > -- > > > > > > > Íñigo Huguet > > > > > > > > > > > > Hello, > > > > > > > > > > > > A few weeks ago I sent the message above notifying a potential bug in > > > > > > rtlwifi module. I just wanted to be sure that it has been received. > > > > > > Can the maintainers acknowledge whether they have seen it? > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > Not real bugs. The coexistence programmers preserve the same code of > > > > > branches intentionally to fine tune performance easier, because bandwidth and > > > > > RSSI strength are highly related to coexistence performance. > > > > > The basic rule of performance tuning is to assign most time slot to BT > > > > > for realtime application, and WiFi uses remaining time slot but don't lower > > > > > than low bound. > > > > > > > > > > > > > > > > Hi, > > > > > > I can add comments. Do you need any keyword within comment to avoid your > > > checking tool warns this false alarm? > > > > > I do "git grep coverity | wc -l" and there are only 8 instances. > I'm not sure if I can add comments with "coverity" marker. > > -- > Ping-Ke > -- Íñigo Huguet ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-05 14:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-23 12:56 rtlwifi: potential bugs Inigo Huguet 2021-05-05 11:23 ` Inigo Huguet 2021-05-05 12:13 ` Pkshih 2021-05-05 13:01 ` Inigo Huguet 2021-05-05 14:03 ` Pkshih 2021-05-05 14:20 ` Inigo Huguet 2021-05-05 14:33 ` Pkshih 2021-05-05 14:36 ` Inigo Huguet
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.