Message-ID: <20140504200403.GA5886@lianli>
Date:	Sun, 4 May 2014 22:04:03 +0200
From:	Emil Goode <emilgoode@...il.com>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Cc:	Larry Finger <Larry.Finger@...inger.net>,
	Chaoming Li <chaoming_li@...lsil.com.cn>,
	"John W. Linville" <linville@...driver.com>, netdev@...r.kernel.org
Subject: Re: Found some errors and other oddities, largely by means of a
 static code analysis program

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
> 
> The static code analysis program called cppcheck.
> http://cppcheck.sourceforge.net/
> 
> 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
> 
> I have created a patch, and inluderat the error file generated by cppcheck.
> 
> 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 :)
> 
> 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.
> 
> 
> Best regards
> Rickard Strandqvist

> From 0ef1cda18e05aa6d0b0ea745ce194f33d8f03973 Mon Sep 17 00:00:00 2001
> From: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
> Date: Wed, 30 Apr 2014 16:27:31 +0200
> Subject: [PATCH] Found same errors using a static code analysis program
>  called cppcheck.
> 

You need to add what subsystem the patch is intended for to the subject line. 
The following command will list the subject lines used for a particular file:

git log --pretty=oneline 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.

For more information read SubmittingPatches in the Documentation folder.

> ---
>  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 ändrade, 9 tillägg(+), 9 borttagningar(-)
> 
> diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/trx.c b/drivers/net/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(struct 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 = false, addr;

There is a fix for this one already in the linux-next tree:

commit 328e203fc35f0b4f6df1c4943f74cf553bcc04f8
Author: Colin Ian King <colin.king@...onical.com>
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.git

>  
>  	tmp_buf = skb->data + pstatus->rx_drvinfo_size + pstatus->rx_bufshift;
>  
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/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 ieee80211_hw *hw)
>  		txpktbuf_bndy = 246;
>  		value8 = 0;
>  		value32 = 0x80bf0d29;
> -	} else if (rtlpriv->rtlhal.macphymode != SINGLEMAC_SINGLEPHY) {
> +	} else {
>  		maxPage = 127;
>  		txpktbuf_bndy = 123;
>  		value8 = 0;
> @@ -1074,7 +1074,6 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
>  	u8 bt_msr = rtl_read_byte(rtlpriv, MSR);
>  	enum led_ctl_mode ledaction = LED_CTL_NO_LINK;
> -	u8 bcnfunc_enable;
>  
>  	bt_msr &= 0xfc;
>  
> @@ -1091,31 +1090,27 @@ static int _rtl92de_set_media_status(struct ieee80211_hw *hw,
>  			 "Set HW_VAR_MEDIA_STATUS: No such media status(%x)\n",
>  			 type);
>  	}
> -	bcnfunc_enable = rtl_read_byte(rtlpriv, REG_BCN_CTRL);
> +	rtl_read_byte(rtlpriv, REG_BCN_CTRL);
>  	switch (type) {
>  	case NL80211_IFTYPE_UNSPECIFIED:
>  		bt_msr |= MSR_NOLINK;
>  		ledaction = LED_CTL_LINK;
> -		bcnfunc_enable &= 0xF7;
>  		RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
>  			 "Set Network type to NO LINK!\n");
>  		break;
>  	case NL80211_IFTYPE_ADHOC:
>  		bt_msr |= MSR_ADHOC;
> -		bcnfunc_enable |= 0x08;
>  		RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
>  			 "Set Network type to Ad Hoc!\n");
>  		break;
>  	case NL80211_IFTYPE_STATION:
>  		bt_msr |= MSR_INFRA;
>  		ledaction = LED_CTL_LINK;
> -		bcnfunc_enable &= 0xF7;
>  		RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
>  			 "Set Network type to STA!\n");
>  		break;
>  	case NL80211_IFTYPE_AP:
>  		bt_msr |= MSR_AP;
> -		bcnfunc_enable |= 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 ieee80211_hw *hw,
>  	}
>  	rtl_write_byte(rtlpriv, REG_CR + 2, bt_msr);
>  	rtlpriv->cfg->ops->led_control(hw, ledaction);
> +    /*
> +     * Expression '(X & 0xfc) == 0x3' is always false.
> +     */
> +#if 0
>  	if ((bt_msr & 0xfc) == 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/net/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 = rtl_priv(hw);
>  	struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
>  	u32 returnvalue, originalvalue, bitshift;
> -	u8 dbi_direct;
>  
>  	RT_TRACE(rtlpriv, COMP_RF, DBG_TRACE, "regaddr(%#x), bitmask(%#x)\n",
>  		 regaddr, bitmask);
>  	if (rtlhal->during_mac1init_radioa || rtlhal->during_mac0init_radiob) {
> +		u8 dbi_direct = 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

> -- 
> 1.7.10.4
> 

> drivers/net/wireless/rtlwifi/rtl8188ee/trx.c : 322] :  (error) Uninitialized variable :  packet_beacon
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 559] :  (error) Uninitialized variable :  value8
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 560] :  (error) Uninitialized variable :  value32
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 1118] :  (style) Variable 'bcnfunc_enable' is assigned a value that is never used.
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c : 1131] :  (style) Expression '(X & 0xfc) == 0x3' is always false.
> drivers/net/wireless/rtlwifi/rtl8192de/phy.c : 218] :  (error) Uninitialized variable :  dbi_direct

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists