From mboxrd@z Thu Jan 1 00:00:00 1970 From: Emil Goode Subject: Re: Found some errors and other oddities, largely by means of a static code analysis program Date: Sun, 4 May 2014 22:04:03 +0200 Message-ID: <20140504200403.GA5886@lianli> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Larry Finger , Chaoming Li , "John W. Linville" , netdev@vger.kernel.org To: Rickard Strandqvist Return-path: Received: from mail-la0-f43.google.com ([209.85.215.43]:49554 "EHLO mail-la0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752308AbaEDUCu (ORCPT ); Sun, 4 May 2014 16:02:50 -0400 Received: by mail-la0-f43.google.com with SMTP id c6so4235019lan.16 for ; Sun, 04 May 2014 13:02:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Hello Rickard, Thank you for the patch, I have written some comments below. On Sat, May 03, 2014 at 05:50:32PM +0200, Rickard Strandqvist wrote: > Hi >=20 > The static code analysis program called cppcheck. > http://cppcheck.sourceforge.net/ >=20 > I found code that I think are bugs, or at least inappropriate or > unnecessary code, in the files: > drivers/net/wireless/rtlwifi/rtl8188ee/trx.c > drivers/net/wireless/rtlwifi/rtl8192de/hw.c > drivers/net/wireless/rtlwifi/rtl8192de/phy.c >=20 > I have created a patch, and inluderat the error file generated by cpp= check. >=20 > My goal was not to change any functionality, but it does not mean for > example the unused variables can't mean that there are other > problems/mistakes in the code. So a proper code review :) >=20 > Is there anything else I can help with regarding the patch or > cppcheck, do not hesitate to contact me. > If you like this type of code analysis, it is possible to get more > warnings, which are not as serious, but that may well indicate other > mistakes. >=20 >=20 > Best regards > Rickard Strandqvist > From 0ef1cda18e05aa6d0b0ea745ce194f33d8f03973 Mon Sep 17 00:00:00 200= 1 > From: Rickard Strandqvist > Date: Wed, 30 Apr 2014 16:27:31 +0200 > Subject: [PATCH] Found same errors using a static code analysis progr= am > called cppcheck. >=20 You need to add what subsystem the patch is intended for to the subject= line.=20 The following command will list the subject lines used for a particular= file: git log --pretty=3Doneline drivers/net/wireless/rtlwifi/rtl8188ee/trx.c The subject line description should be in imperative mood. "as if you are giving orders to the codebase to change its behaviour" Also you need to write a description for the change log here and there need to be a Signed-off-by line. =46or more information read SubmittingPatches in the Documentation fold= er. > --- > drivers/net/wireless/rtlwifi/rtl8188ee/trx.c | 2 +- > drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 14 +++++++------- > drivers/net/wireless/rtlwifi/rtl8192de/phy.c | 2 +- > 3 filer =C3=A4ndrade, 9 till=C3=A4gg(+), 9 borttagningar(-) >=20 > diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/trx.c b/drivers/n= et/wireless/rtlwifi/rtl8188ee/trx.c > index 06ef47c..5b4c225 100644 > --- a/drivers/net/wireless/rtlwifi/rtl8188ee/trx.c > +++ b/drivers/net/wireless/rtlwifi/rtl8188ee/trx.c > @@ -293,7 +293,7 @@ static void _rtl88ee_translate_rx_signal_stuff(st= ruct ieee80211_hw *hw, > u8 *psaddr; > __le16 fc; > u16 type, ufc; > - bool match_bssid, packet_toself, packet_beacon, addr; > + bool match_bssid, packet_toself, packet_beacon =3D false, addr; There is a fix for this one already in the linux-next tree: commit 328e203fc35f0b4f6df1c4943f74cf553bcc04f8 Author: Colin Ian King Date: Mon Apr 21 17:38:44 2014 +0100 rtlwifi: rtl8188ee: initialize packet_beacon You can find the linux-next tree here: git clone git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next= =2Egit > =20 > tmp_buf =3D skb->data + pstatus->rx_drvinfo_size + pstatus->rx_bufs= hift; > =20 > diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/ne= t/wireless/rtlwifi/rtl8192de/hw.c > index 2b08671..32e41c0 100644 > --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c > +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c > @@ -546,7 +546,7 @@ static bool _rtl92de_llt_table_init(struct ieee80= 211_hw *hw) > txpktbuf_bndy =3D 246; > value8 =3D 0; > value32 =3D 0x80bf0d29; > - } else if (rtlpriv->rtlhal.macphymode !=3D SINGLEMAC_SINGLEPHY) { > + } else { > maxPage =3D 127; > txpktbuf_bndy =3D 123; > value8 =3D 0; > @@ -1074,7 +1074,6 @@ static int _rtl92de_set_media_status(struct iee= e80211_hw *hw, > struct rtl_priv *rtlpriv =3D rtl_priv(hw); > u8 bt_msr =3D rtl_read_byte(rtlpriv, MSR); > enum led_ctl_mode ledaction =3D LED_CTL_NO_LINK; > - u8 bcnfunc_enable; > =20 > bt_msr &=3D 0xfc; > =20 > @@ -1091,31 +1090,27 @@ static int _rtl92de_set_media_status(struct i= eee80211_hw *hw, > "Set HW_VAR_MEDIA_STATUS: No such media status(%x)\n", > type); > } > - bcnfunc_enable =3D rtl_read_byte(rtlpriv, REG_BCN_CTRL); > + rtl_read_byte(rtlpriv, REG_BCN_CTRL); > switch (type) { > case NL80211_IFTYPE_UNSPECIFIED: > bt_msr |=3D MSR_NOLINK; > ledaction =3D LED_CTL_LINK; > - bcnfunc_enable &=3D 0xF7; > RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE, > "Set Network type to NO LINK!\n"); > break; > case NL80211_IFTYPE_ADHOC: > bt_msr |=3D MSR_ADHOC; > - bcnfunc_enable |=3D 0x08; > RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE, > "Set Network type to Ad Hoc!\n"); > break; > case NL80211_IFTYPE_STATION: > bt_msr |=3D MSR_INFRA; > ledaction =3D LED_CTL_LINK; > - bcnfunc_enable &=3D 0xF7; > RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE, > "Set Network type to STA!\n"); > break; > case NL80211_IFTYPE_AP: > bt_msr |=3D MSR_AP; > - bcnfunc_enable |=3D 0x08; > RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE, > "Set Network type to AP!\n"); > break; > @@ -1128,9 +1123,14 @@ static int _rtl92de_set_media_status(struct ie= ee80211_hw *hw, > } > rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr); > rtlpriv->cfg->ops->led_control(hw, ledaction); > + /* > + * Expression '(X & 0xfc) =3D=3D 0x3' is always false. > + */ > +#if 0 > if ((bt_msr & 0xfc) =3D=3D MSR_AP) > rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x00); > else > +#endif I'm not familiar with this code so I don't know what was intended here. Your changes are harmless but perhaps bcnfunc_enable and this dead code was intended to be used somehow. > rtl_write_byte(rtlpriv, REG_BCNTCFG + 1, 0x66); > return 0; > } > diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c b/drivers/n= et/wireless/rtlwifi/rtl8192de/phy.c > index 3d1f0dd..66abaf6 100644 > --- a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c > +++ b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c > @@ -203,11 +203,11 @@ u32 rtl92d_phy_query_bb_reg(struct ieee80211_hw= *hw, u32 regaddr, u32 bitmask) > struct rtl_priv *rtlpriv =3D rtl_priv(hw); > struct rtl_hal *rtlhal =3D rtl_hal(rtlpriv); > u32 returnvalue, originalvalue, bitshift; > - u8 dbi_direct; > =20 > RT_TRACE(rtlpriv, COMP_RF, DBG_TRACE, "regaddr(%#x), bitmask(%#x)\n= ", > regaddr, bitmask); > if (rtlhal->during_mac1init_radioa || rtlhal->during_mac0init_radio= b) { > + u8 dbi_direct =3D 0; > /* mac1 use phy0 read radio_b. */ > /* mac0 use phy1 read radio_b. */ > if (rtlhal->during_mac1init_radioa) Perhaps you would like to resend with the above changes? And hopefully someone that knows this code well will find the time to look at the _rtl92de_set_media_status function. Best regards, Emil Goode > --=20 > 1.7.10.4 >=20 > drivers/net/wireless/rtlwifi/rtl8188ee/trx.c : 322] : (error) Uninit= ialized variable : packet_beacon > drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 559] : (error) Uniniti= alized variable : value8 > drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 560] : (error) Uniniti= alized variable : value32 > drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 1118] : (style) Variab= le 'bcnfunc_enable' is assigned a value that is never used. > drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 1131] : (style) Expres= sion '(X & 0xfc) =3D=3D 0x3' is always false. > drivers/net/wireless/rtlwifi/rtl8192de/phy.c : 218] : (error) Uninit= ialized variable : dbi_direct