Skip to contentSkip to conversation

ath9k: fix ath9k_hw_nvram_check_version()

PATCH New Dan Carpenter 5 participants 7 messages Dec 19, 2015 – Jan 7, 2016

Subsystems (2)
Kernel janitors Qualcomm Atheros ATH9K wireless driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ath9k: fix ath9k_hw_nvram_check_version()

There is a type bug so it always returns success.

Fixes 6fa658fd5ab2 ('ath9k: Simplify and fix eeprom endianness swapping')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/ath/ath9k/eeprom.c +1 -1
index f8c5065..a7afdee 100644
@@ -206,7 +206,7 @@ bool ath9k_hw_nvram_check_version(struct ath_hw *ah, int version, int minrev)
 		ath_err(common, "Bad EEPROM VER 0x%04x or REV 0x%04x\n",
 			ah->eep_ops->get_eeprom_ver(ah),
 			ah->eep_ops->get_eeprom_rev(ah));
-		return -EINVAL;
+		return false;
 	}
 
 	return true;
#2 lore raw

PATCH 1/1 ath9k: fix ath9k_hw_nvram_check_version()

In reply to Dan Carpenter
Quoted 1 line · Dan Carpenter

There is a type bug so it always returns success.

How many false positives do you have to sift
through to find this sort of error?

Quoted 10 lines

+++ b/drivers/net/wireless/ath/ath9k/eeprom.c

(quoted diff) +1 -1
@@ -206,7 +206,7 @@ bool ath9k_hw_nvram_check_version(struct ath_hw *ah, int version, int minrev)
 ?		ath_err(common, "Bad EEPROM VER 0x%04x or REV 0x%04x\n",
 ?			ah->eep_ops->get_eeprom_ver(ah),
 ?			ah->eep_ops->get_eeprom_rev(ah));
-		return -EINVAL;
+		return false;
 ?	}

?
? return true;

#3 lore raw

PATCH 1/1 ath9k: fix ath9k_hw_nvram_check_version()

Quoted 5 lines · Joe Perches
Quoted 1 line · Dan Carpenter

There is a type bug so it always returns success.

How many false positives do you have to sift
through to find this sort of error?

The return type is thoughtfully bool, so it should be easy in this case. The function has a return -EINVAL and a return true, so even without the return type it would be locally apparent that there is an inconsistency.

julia

Quoted 17 lines
Quoted 11 lines

+++ b/drivers/net/wireless/ath/ath9k/eeprom.c

(quoted diff) +1 -1
@@ -206,7 +206,7 @@ bool ath9k_hw_nvram_check_version(struct ath_hw *ah, int version, int minrev)
 ?		ath_err(common, "Bad EEPROM VER 0x%04x or REV 0x%04x\n",
 ?			ah->eep_ops->get_eeprom_ver(ah),
 ?			ah->eep_ops->get_eeprom_rev(ah));
-		return -EINVAL;
+		return false;
 ?	}

?
? return true;

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

#4 lore raw

PATCH 1/1 ath9k: fix ath9k_hw_nvram_check_version()

Quoted 11 lines · Julia Lawall
Quoted 5 lines · Joe Perches
Quoted 1 line · Dan Carpenter

There is a type bug so it always returns success.

?
How many false positives do you have to sift
through to find this sort of error?

The return type is thoughtfully bool, so it should be easy in this case.?? The function has a return -EINVAL and a return true, so even without the? return type it would be locally apparent that there is an inconsistency.

True, -EINVAL is a non-bool constant, but
bool returns can be a variable int.

Dan, was the check any constant non-bool?

author
#5 lore raw

PATCH 1/1 ath9k: fix ath9k_hw_nvram_check_version()

Quoted 9 lines · Joe Perches
Quoted 8 lines · Julia Lawall
Quoted 5 lines · Joe Perches
Quoted 1 line · Dan Carpenter

There is a type bug so it always returns success.

?
How many false positives do you have to sift
through to find this sort of error?

Quoted 8 lines
Quoted 3 lines

The return type is thoughtfully bool, so it should be easy in this case.?? The function has a return -EINVAL and a return true, so even without the? return type it would be locally apparent that there is an inconsistency.

True, -EINVAL is a non-bool constant, but
bool returns can be a variable int.

Dan, was the check any constant non-bool?

This warning is for returning negative with unsigned types smaller than int. It's doing cross function flow analysis so it could warning about other things besides constants, but it's 90% constants. It warns about ds2482_w1_triplet() for example.

There aren't many false positives.

regards,
dan carpenter