Found some errors and other oddities, largely by means of a static code analysis program

Found some errors and other oddities, largely by means of a static code analysis program

[Rickard Strandqvist]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Hello again
Sorry!  I failed to include the patch :-)


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/isdn/hisax/l3dss1.c
drivers/isdn/hisax/l3ni1.c
drivers/isdn/mISDN/dsp_cmx.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

[-- Attachment #2: drivers_isdn_3files.patch --]
[-- Type: text/x-patch, Size: 2270 bytes --]

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

---
 drivers/isdn/hisax/l3dss1.c  |    8 ++++++--
 drivers/isdn/hisax/l3ni1.c   |    8 ++++++--
 drivers/isdn/mISDN/dsp_cmx.c |    3 ---
 3 filer ändrade, 12 tillägg(+), 7 borttagningar(-)

diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
index cda7006..3ea6257 100644
--- a/drivers/isdn/hisax/l3dss1.c
+++ b/drivers/isdn/hisax/l3dss1.c
@@ -2208,8 +2208,12 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
 				{ free_invoke_id(st, id);
 					return (-2);
 				}
-			pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
-			pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+
+			if (pc)
+			{
+				pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
+				pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+			}
 
 			if (!(skb = l3_alloc_skb(l)))
 			{ free_invoke_id(st, id);
diff --git a/drivers/isdn/hisax/l3ni1.c b/drivers/isdn/hisax/l3ni1.c
index 0df6691..367b9d6 100644
--- a/drivers/isdn/hisax/l3ni1.c
+++ b/drivers/isdn/hisax/l3ni1.c
@@ -2064,8 +2064,12 @@ static int l3ni1_cmd_global(struct PStack *st, isdn_ctrl *ic)
 				{ free_invoke_id(st, id);
 					return (-2);
 				}
-			pc->prot.ni1.ll_id = ic->parm.ni1_io.ll_id; /* remember id */
-			pc->prot.ni1.proc = ic->parm.ni1_io.proc; /* and procedure */
+
+			if (pc)
+			{
+				pc->prot.ni1.ll_id = ic->parm.ni1_io.ll_id; /* remember id */
+				pc->prot.ni1.proc = ic->parm.ni1_io.proc; /* and procedure */
+			}
 
 			if (!(skb = l3_alloc_skb(l)))
 			{ free_invoke_id(st, id);
diff --git a/drivers/isdn/mISDN/dsp_cmx.c b/drivers/isdn/mISDN/dsp_cmx.c
index a4f05c5..9e9bdc0 100644
--- a/drivers/isdn/mISDN/dsp_cmx.c
+++ b/drivers/isdn/mISDN/dsp_cmx.c
@@ -1511,9 +1511,6 @@ dsp_cmx_send_member(struct dsp *dsp, int len, s32 *c, int members)
 			dsp->tx_R = t;
 			goto send_packet;
 		}
-#ifdef DSP_NEVER_DEFINED
-	}
-#endif
 	/* PROCESS DATA (three or more members) */
 	/* -> if echo is NOT enabled */
 	if (!dsp->echo.software) {
-- 
1.7.10.4

Re: Found some errors and other oddities, largely by means of a static code analysis program

[Emil Goode]

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@spectrumdigital.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@canonical.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

Found some errors and other oddities, largely by means of a static code analysis program

[Rickard Strandqvist]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

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

[-- Attachment #2: drivers_net_wireless_rtlwifi_3files.patch --]
[-- Type: text/x-patch, Size: 4106 bytes --]

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

---
 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;
 
 	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
 		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)
-- 
1.7.10.4


[-- Attachment #3: drivers_net_wireless_rtlwifi_cppcheck.log --]
[-- Type: text/x-log, Size: 634 bytes --]

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

Found some errors and other oddities, largely by means of a static code analysis program

[Rickard Strandqvist]

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/isdn/hisax/l3dss1.c
drivers/isdn/hisax/l3ni1.c
drivers/isdn/mISDN/dsp_cmx.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