All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] staging: ks7010: fix style issues
@ 2017-03-22  0:59 Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 1/4] staging: ks7010: fix memcmp() bug Tobin C. Harding
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Tobin C. Harding @ 2017-03-22  0:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Tobin C. Harding, driverdev-devel, Wolfram Sang

Review from previous (merged) patch set made comment on a couple of
style issues. In the same review a bug was reported. We start off
fixing the bug then move on to fix the style issues. None of these
issues are reported by checkpatch, Sparse, or Smatch. Some instances
were introduced by Tobin Harding in the afore mentioned patch set. An
attempt at fixing the whole driver was made, however, an audit of
every 'if' statement was not conducted. 

Patch 01 fixes incorrect usage of return value of memcmp().

Patch 02 adds explicit checks to calls to memcmp() across the whole
driver.

Patch 03 adds previously removed explicit checks to 'size' variables.

Patch 04 adds previously removed braces to multi-line single statement
code blocks.

Code has not been tested. Patch set applies and builds on x86_64 and
PowerPC.

Tobin C. Harding (4):
  staging: ks7010: fix memcmp() bug
  staging: ks7010: add explicit check to memcmp() calls
  staging: ks7010: add explicit check to 'size' variables
  staging: ks7010: add braces to multi-line indent

 drivers/staging/ks7010/ks7010_sdio.c |  8 ++++----
 drivers/staging/ks7010/ks_hostif.c   | 35 ++++++++++++++++++-----------------
 drivers/staging/ks7010/ks_wlan_net.c | 19 +++++++++----------
 3 files changed, 31 insertions(+), 31 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH 1/4] staging: ks7010: fix memcmp() bug
  2017-03-22  0:59 [PATCH 0/4] staging: ks7010: fix style issues Tobin C. Harding
@ 2017-03-22  0:59 ` Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 2/4] staging: ks7010: add explicit check to memcmp() calls Tobin C. Harding
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Tobin C. Harding @ 2017-03-22  0:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Tobin C. Harding, driverdev-devel, Wolfram Sang

Call site to memcmp() treats return value as if it were an error code,
it is not. If memcmp() finds inputs to be not the same, an error
return code should be set explicitly.

Correctly handle return value from call to memcmp(), set error code
explicitly.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks7010_sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 28b91be..dbb1f05 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -695,8 +695,8 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
 	if (ret)
 		goto err_free_read_buf;
 
-	ret = memcmp(data, read_buf, size);
-	if (ret) {
+	if (memcmp(data, read_buf, size) != 0) {
+		ret = -EIO;
 		DPRINTK(0, "data compare error (%d)\n", ret);
 		goto err_free_read_buf;
 	}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/4] staging: ks7010: add explicit check to memcmp() calls
  2017-03-22  0:59 [PATCH 0/4] staging: ks7010: fix style issues Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 1/4] staging: ks7010: fix memcmp() bug Tobin C. Harding
@ 2017-03-22  0:59 ` Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 3/4] staging: ks7010: add explicit check to 'size' variables Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 4/4] staging: ks7010: add braces to multi-line indent Tobin C. Harding
  3 siblings, 0 replies; 5+ messages in thread
From: Tobin C. Harding @ 2017-03-22  0:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Wolfram Sang, driverdev-devel

Calls to functions memcmp() and strcmp() are more clearly readable
when the return value is explicitly checked. i.e

if (memcmp(foo, bar, size) == 0)

Modify driver to use an explicit check on the value returned by
memcmp().

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c   | 15 +++++++--------
 drivers/staging/ks7010/ks_wlan_net.c | 13 ++++++-------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 460ab13..dc730a3 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -271,7 +271,7 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 			memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
 			break;
 		case 221:	/* WPA */
-			if (!memcmp(bp + 2, "\x00\x50\xf2\x01", 4)) {	/* WPA OUI check */
+			if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {	/* WPA OUI check */
 				ap->wpa_ie.id = *bp;
 				if (*(bp + 1) <= RSN_IE_BODY_MAX) {
 					ap->wpa_ie.size = *(bp + 1);
@@ -325,7 +325,7 @@ int hostif_data_indication_wpa(struct ks_wlan_private *priv,
 	eth_proto = ntohs(eth_hdr->h_proto);
 
 	/* source address check */
-	if (!memcmp(&eth_hdr->h_source[0], &priv->eth_addr[0], ETH_ALEN))
+	if (memcmp(&eth_hdr->h_source[0], &priv->eth_addr[0], ETH_ALEN) == 0)
 		return 0;
 
 	if (eth_hdr->h_dest_snap != eth_hdr->h_source_snap) {
@@ -353,7 +353,7 @@ int hostif_data_indication_wpa(struct ks_wlan_private *priv,
 					   (uint8_t)0,	/* priority */
 					   (uint8_t *)michael_mic.Result);
 		}
-		if (memcmp(michael_mic.Result, RecvMIC, 8)) {
+		if (memcmp(michael_mic.Result, RecvMIC, 8) != 0) {
 			now = jiffies;
 			mic_failure = &priv->wpa.mic_failure;
 			/* MIC FAILURE */
@@ -421,7 +421,7 @@ void hostif_data_indication(struct ks_wlan_private *priv)
 	DPRINTK(3, "ether protocol = %04X\n", eth_proto);
 
 	/* source address check */
-	if (!memcmp(&priv->eth_addr[0], eth_hdr->h_source, ETH_ALEN)) {
+	if (memcmp(&priv->eth_addr[0], eth_hdr->h_source, ETH_ALEN) == 0) {
 		DPRINTK(1, "invalid : source is own mac address !!\n");
 		DPRINTK(1,
 			"eth_hdrernet->h_dest=%02X:%02X:%02X:%02X:%02X:%02X\n",
@@ -836,9 +836,8 @@ void hostif_scan_indication(struct ks_wlan_private *priv)
 
 	if (priv->scan_ind_count) {
 		for (i = 0; i < priv->aplist.size; i++) {	/* bssid check */
-			if (!memcmp
-			    (ap_info->bssid,
-			     priv->aplist.ap[i].bssid, ETH_ALEN)) {
+			if (memcmp(ap_info->bssid,
+				   priv->aplist.ap[i].bssid, ETH_ALEN) == 0) {
 				if (ap_info->frame_type ==
 				    FRAME_TYPE_PROBE_RESP)
 					get_ap_information(priv, ap_info,
@@ -1168,7 +1167,7 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *packet)
 
 	/* packet check */
 	eth = (struct ethhdr *)packet->data;
-	if (memcmp(&priv->eth_addr[0], eth->h_source, ETH_ALEN)) {
+	if (memcmp(&priv->eth_addr[0], eth->h_source, ETH_ALEN) != 0) {
 		DPRINTK(1, "invalid mac address !!\n");
 		DPRINTK(1, "ethernet->h_source=%pM\n", eth->h_source);
 		ret = -ENXIO;
diff --git a/drivers/staging/ks7010/ks_wlan_net.c b/drivers/staging/ks7010/ks_wlan_net.c
index 3f9eba4..c097ecd 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -1924,9 +1924,8 @@ static int ks_wlan_set_pmksa(struct net_device *dev,
 		if (list_empty(&priv->pmklist.head)) {	/* new list */
 			for (i = 0; i < PMK_LIST_MAX; i++) {
 				pmk = &priv->pmklist.pmk[i];
-				if (!memcmp
-				    ("\x00\x00\x00\x00\x00\x00", pmk->bssid,
-				     ETH_ALEN))
+				if (memcmp("\x00\x00\x00\x00\x00\x00",
+					   pmk->bssid, ETH_ALEN) == 0)
 					break; /* loop */
 			}
 			memcpy(pmk->bssid, pmksa->bssid.sa_data, ETH_ALEN);
@@ -1938,7 +1937,7 @@ static int ks_wlan_set_pmksa(struct net_device *dev,
 		/* search cache data */
 		list_for_each(ptr, &priv->pmklist.head) {
 			pmk = list_entry(ptr, struct pmk_t, list);
-			if (!memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN)) {	/* match address! list move to head. */
+			if (memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN) == 0) {
 				memcpy(pmk->pmkid, pmksa->pmkid, IW_PMKID_LEN);
 				list_move(&pmk->list, &priv->pmklist.head);
 				break; /* list_for_each */
@@ -1950,8 +1949,8 @@ static int ks_wlan_set_pmksa(struct net_device *dev,
 		if (priv->pmklist.size < PMK_LIST_MAX) {	/* new cache data */
 			for (i = 0; i < PMK_LIST_MAX; i++) {
 				pmk = &priv->pmklist.pmk[i];
-				if (!memcmp("\x00\x00\x00\x00\x00\x00",
-					    pmk->bssid, ETH_ALEN))
+				if (memcmp("\x00\x00\x00\x00\x00\x00",
+					    pmk->bssid, ETH_ALEN) == 0)
 					break; /* loop */
 			}
 			memcpy(pmk->bssid, pmksa->bssid.sa_data, ETH_ALEN);
@@ -1973,7 +1972,7 @@ static int ks_wlan_set_pmksa(struct net_device *dev,
 		/* search cache data */
 		list_for_each(ptr, &priv->pmklist.head) {
 			pmk = list_entry(ptr, struct pmk_t, list);
-			if (!memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN)) {	/* match address! list del. */
+			if (memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN) == 0) {
 				eth_zero_addr(pmk->bssid);
 				memset(pmk->pmkid, 0, IW_PMKID_LEN);
 				list_del_init(&pmk->list);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 3/4] staging: ks7010: add explicit check to 'size' variables
  2017-03-22  0:59 [PATCH 0/4] staging: ks7010: fix style issues Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 1/4] staging: ks7010: fix memcmp() bug Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 2/4] staging: ks7010: add explicit check to memcmp() calls Tobin C. Harding
@ 2017-03-22  0:59 ` Tobin C. Harding
  2017-03-22  0:59 ` [PATCH 4/4] staging: ks7010: add braces to multi-line indent Tobin C. Harding
  3 siblings, 0 replies; 5+ messages in thread
From: Tobin C. Harding @ 2017-03-22  0:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Wolfram Sang, driverdev-devel

When checking the value of a variable that holds a 0 an explicit check
is good style. i.e

  -    if (!size)
  +    if (size == 0)

Update checks on 'numerical' variables to use explicit checks.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks7010_sdio.c | 4 ++--
 drivers/staging/ks7010/ks_hostif.c   | 2 +-
 drivers/staging/ks7010/ks_wlan_net.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index dbb1f05..8829989 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -591,7 +591,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 			}
 			DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
 			rsize = rw_data & RSIZE_MASK;
-			if (rsize) {	/* Read schedule */
+			if (rsize != 0) {	/* Read schedule */
 				ks_wlan_hw_rx((void *)priv,
 					      (uint16_t)(rsize << 4));
 			}
@@ -829,7 +829,7 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
 		DPRINTK(1, "wait time out!! SME_START\n");
 	}
 
-	if (priv->mac_address_valid && priv->version_size)
+	if (priv->mac_address_valid && priv->version_size != 0)
 		priv->dev_state = DEVICE_STATE_PREINIT;
 
 	hostif_sme_enqueue(priv, SME_GET_EEPROM_CKSUM);
diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index dc730a3..24feee3 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -129,7 +129,7 @@ int get_current_ap(struct ks_wlan_private *priv, struct link_ap_info_t *ap_info)
 	memcpy(ap->rate_set.body, ap_info->rate_set.body,
 	       ap_info->rate_set.size);
 	ap->rate_set.size = ap_info->rate_set.size;
-	if (ap_info->ext_rate_set.size) {
+	if (ap_info->ext_rate_set.size != 0) {
 		/* rate_set */
 		memcpy(&ap->rate_set.body[ap->rate_set.size],
 		       ap_info->ext_rate_set.body,
diff --git a/drivers/staging/ks7010/ks_wlan_net.c b/drivers/staging/ks7010/ks_wlan_net.c
index c097ecd..5e68699 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -335,7 +335,7 @@ static int ks_wlan_get_essid(struct net_device *dev,
 	/* Note : if dwrq->flags != 0, we should
 	 * get the relevant SSID from the SSID list...
 	 */
-	if (priv->reg.ssid.size) {
+	if (priv->reg.ssid.size != 0) {
 		/* Get the current SSID */
 		memcpy(extra, priv->reg.ssid.body, priv->reg.ssid.size);
 
@@ -928,7 +928,7 @@ static int ks_wlan_set_encode(struct net_device *dev,
 			/* Do we want to just set the transmit key index ? */
 			if ((index >= 0) && (index < 4)) {
 				/* set_wep_key(priv, index, 0, 0, 1);   xxx */
-				if (priv->reg.wep_key[index].size) {
+				if (priv->reg.wep_key[index].size != 0) {
 					priv->reg.wep_index = index;
 					priv->need_commit |= SME_WEP_INDEX;
 				} else {
@@ -1531,7 +1531,7 @@ static int ks_wlan_get_scan(struct net_device *dev,
 		return -EAGAIN;
 	}
 
-	if (!priv->aplist.size) {
+	if (priv->aplist.size == 0) {
 		/* Client error, no scan results...
 		 * The caller need to restart the scan.
 		 */
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 4/4] staging: ks7010: add braces to multi-line indent
  2017-03-22  0:59 [PATCH 0/4] staging: ks7010: fix style issues Tobin C. Harding
                   ` (2 preceding siblings ...)
  2017-03-22  0:59 ` [PATCH 3/4] staging: ks7010: add explicit check to 'size' variables Tobin C. Harding
@ 2017-03-22  0:59 ` Tobin C. Harding
  3 siblings, 0 replies; 5+ messages in thread
From: Tobin C. Harding @ 2017-03-22  0:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Wolfram Sang, driverdev-devel

The addition of curly braces around single statements that span
multiple lines makes the code more readable in general.

Add curly braces to multi-line indented statement.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 24feee3..38ea7a2 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -2455,16 +2455,18 @@ void hostif_sme_execute(struct ks_wlan_private *priv, int event)
 		hostif_phy_information_request(priv);
 		break;
 	case SME_MIC_FAILURE_REQUEST:
-		if (priv->wpa.mic_failure.failure == 1)
-			hostif_mic_failure_request(
-				priv, priv->wpa.mic_failure.failure - 1, 0);
-		else if (priv->wpa.mic_failure.failure == 2)
-			hostif_mic_failure_request(
-				priv, priv->wpa.mic_failure.failure - 1,
-				priv->wpa.mic_failure.counter);
-		else
+		if (priv->wpa.mic_failure.failure == 1) {
+			hostif_mic_failure_request(priv,
+						   priv->wpa.mic_failure.failure - 1,
+						   0);
+		} else if (priv->wpa.mic_failure.failure == 2) {
+			hostif_mic_failure_request(priv,
+						   priv->wpa.mic_failure.failure - 1,
+						   priv->wpa.mic_failure.counter);
+		} else {
 			DPRINTK(4, "SME_MIC_FAILURE_REQUEST: failure count=%u error?\n",
 				priv->wpa.mic_failure.failure);
+		}
 		break;
 	case SME_MIC_FAILURE_CONFIRM:
 		if (priv->wpa.mic_failure.failure == 2) {
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-22  1:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-22  0:59 [PATCH 0/4] staging: ks7010: fix style issues Tobin C. Harding
2017-03-22  0:59 ` [PATCH 1/4] staging: ks7010: fix memcmp() bug Tobin C. Harding
2017-03-22  0:59 ` [PATCH 2/4] staging: ks7010: add explicit check to memcmp() calls Tobin C. Harding
2017-03-22  0:59 ` [PATCH 3/4] staging: ks7010: add explicit check to 'size' variables Tobin C. Harding
2017-03-22  0:59 ` [PATCH 4/4] staging: ks7010: add braces to multi-line indent Tobin C. Harding

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.