Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CACT4ouecdXk3SQrgUNKnr4u2WAaiBUjgou5u_H1bEubTcrGtFQ@mail.gmail.com>
 <CACT4oudp9Je55zjg7N8QFDWi5h3kmzMj6syfdi3KgAqQOVgPMA@mail.gmail.com>
 <1620216779.15370.10.camel@realtek.com> <CACT4ouehaQsGr1UGqncvAFgay0v40Zv=O=oz5f8W=E+YmV=SYg@mail.gmail.com>
 <1620223407.28283.2.camel@realtek.com> <CACT4ouePVqwkFRYRLCE7TznfWG87OKwhR2ukAYxO+ijOgZ26Dg@mail.gmail.com>
 <1620225209.1966.4.camel@realtek.com>
In-Reply-To: <1620225209.1966.4.camel@realtek.com>
From:   Inigo Huguet <ihuguet@redhat.com>
Date:   Wed, 5 May 2021 16:36:14 +0200
Message-ID: <CACT4oueTiwZPSvrZFZJpgkMQ04AKKP5qGB8ag2guTY==Y-KmVg@mail.gmail.com>
Subject: Re: rtlwifi: potential bugs
To:     Pkshih <pkshih@realtek.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "ivecera@redhat.com" <ivecera@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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.co=
m> 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/dri=
vers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2813:
> > > > > > > identical_branches: The same code is executed regardless of w=
hether
> > > > > > > "bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH || bt_rssi_state =
=3D=3D
> > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'el=
se'
> > > > > > > branches are identical. Should one of the branches be modifie=
d, or the
> > > > > > > entire 'if' statement replaced?
> > > > > > > # 2811|   }
> > > > > > > # 2812|
> > > > > > > # 2813|-> if ((bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH) ||
> > > > > > > # 2814|      (bt_rssi_state =3D=3D 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/dri=
vers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:2947:
> > > > > > > identical_branches: The same code is executed regardless of w=
hether
> > > > > > > "bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH || bt_rssi_state =
=3D=3D
> > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'el=
se'
> > > > > > > branches are identical. Should one of the branches be modifie=
d, or the
> > > > > > > entire 'if' statement replaced?
> > > > > > > # 2945|   }
> > > > > > > # 2946|
> > > > > > > # 2947|-> if ((bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH) ||
> > > > > > > # 2948|      (bt_rssi_state =3D=3D 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/dri=
vers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3135:
> > > > > > > identical_branches: The same code is executed regardless of w=
hether
> > > > > > > "wifi_bw =3D=3D BTC_WIFI_BW_LEGACY" is true, because the 'the=
n' and 'else'
> > > > > > > branches are identical. Should one of the branches be modifie=
d, or the
> > > > > > > entire 'if' statement replaced?
> > > > > > > # 3133|   btcoexist->btc_get(btcoexist, BTC_GET_U4_WIFI_BW, &=
wifi_bw);
> > > > > > > # 3134|
> > > > > > > # 3135|-> if (wifi_bw =3D=3D BTC_WIFI_BW_LEGACY) {
> > > > > > > # 3136|   /* for HID at 11b/g mode */
> > > > > > > # 3137|   btc8821a2ant_coex_table(btcoexist, NORMAL_EXEC, 0x5=
5ff55ff,
> > > > > > >
> > > > > > > Error: IDENTICAL_BRANCHES (CWE-398): [#def215]
> > > > > > > kernel-5.11.0-0.rc7.151/linux-5.11.0-0.rc7.151.el9.x86_64/dri=
vers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8821a2ant.c:3324:
> > > > > > > identical_branches: The same code is executed regardless of w=
hether
> > > > > > > "bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH || bt_rssi_state =
=3D=3D
> > > > > > > BTC_RSSI_STATE_STAY_HIGH" is true, because the 'then' and 'el=
se'
> > > > > > > branches are identical. Should one of the branches be modifie=
d, or the
> > > > > > > entire 'if' statement replaced?
> > > > > > > # 3322|   }
> > > > > > > # 3323|
> > > > > > > # 3324|-> if ((bt_rssi_state =3D=3D BTC_RSSI_STATE_HIGH) ||
> > > > > > > # 3325|      (bt_rssi_state =3D=3D 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 i=
f-else
> > > > > > > because they strongly depend on magic numbers, which are diff=
erent
> > > > > > > configurations for the hw, I guess.
> > > > > > >
> > > > > > > Can the maintainers confirm if these are real bugs and see ho=
w to fix them?
> > > > > > >
> > > > > > > Regards
> > > > > > > --
> > > > > > > =C3=8D=C3=B1igo 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 recei=
ved.
> > > > > > 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 b=
andwidth 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 d=
on't lower
> > > > > than low bound.
> > > > >
> > > > >
> > >
> > > Hi,
> > >
> > > I can add comments. Do you need any keyword within comment to avoid y=
our
> > > 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
>


--=20
=C3=8D=C3=B1igo Huguet