rtlwifi: potential bugs

rtlwifi: potential bugs

[Inigo Huguet]

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

Re: rtlwifi: potential bugs

[Inigo Huguet]

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

Re: rtlwifi: potential bugs

[Pkshih]

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

Re: rtlwifi: potential bugs

[Inigo Huguet]

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

Re: rtlwifi: potential bugs

[Pkshih]

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

Re: rtlwifi: potential bugs

[Inigo Huguet]

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

Re: rtlwifi: potential bugs

[Pkshih]

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

Re: rtlwifi: potential bugs

[Inigo Huguet]

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