diff mbox series

mt7601u: fix always true expression

Message ID 20210225183241.1002129-1-colin.king@canonical.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series mt7601u: fix always true expression | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Colin King Feb. 25, 2021, 6:32 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Currently the expression ~nic_conf1 is always true because nic_conf1
is a u16 and according to 6.5.3.3 of the C standard the ~ operator
promotes the u16 to an integer before flipping all the bits. Thus
the top 16 bits of the integer result are all set so the expression
is always true.  If the intention was to flip all the bits of nic_conf1
then casting the integer result back to a u16 is a suitabel fix.

Interestingly static analyzers seem to thing a bitwise ! should be
used instead of ~ for this scenario, so I think the original intent
of the expression may need some extra consideration.

Addresses-Coverity: ("Logical vs. bitwise operator")
Fixes: c869f77d6abb ("add mt7601u driver")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/wireless/mediatek/mt7601u/eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Feb. 25, 2021, 6:51 p.m. UTC | #1 
On Thu, 25 Feb 2021 18:32:41 +0000 Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the expression ~nic_conf1 is always true because nic_conf1
> is a u16 and according to 6.5.3.3 of the C standard the ~ operator
> promotes the u16 to an integer before flipping all the bits. Thus
> the top 16 bits of the integer result are all set so the expression
> is always true.  If the intention was to flip all the bits of nic_conf1
> then casting the integer result back to a u16 is a suitabel fix.
> 
> Interestingly static analyzers seem to thing a bitwise ! should be
> used instead of ~ for this scenario

In what way? The condition is nic_conf1 != 0xffff AFAICT, how would we
do that with !?

> so I think the original intent
> of the expression may need some extra consideration.
> 
> Addresses-Coverity: ("Logical vs. bitwise operator")
> Fixes: c869f77d6abb ("add mt7601u driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Acked-by: Jakub Kicinski <kubakici@wp.pl>

Thanks for the fix!
Kalle Valo April 11, 2021, 9:30 a.m. UTC | #2 
Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the expression ~nic_conf1 is always true because nic_conf1
> is a u16 and according to 6.5.3.3 of the C standard the ~ operator
> promotes the u16 to an integer before flipping all the bits. Thus
> the top 16 bits of the integer result are all set so the expression
> is always true.  If the intention was to flip all the bits of nic_conf1
> then casting the integer result back to a u16 is a suitabel fix.
> 
> Interestingly static analyzers seem to thing a bitwise ! should be
> used instead of ~ for this scenario, so I think the original intent
> of the expression may need some extra consideration.
> 
> Addresses-Coverity: ("Logical vs. bitwise operator")
> Fixes: c869f77d6abb ("add mt7601u driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Acked-by: Jakub Kicinski <kubakici@wp.pl>

Patch applied to wireless-drivers-next.git, thanks.

87fce88658ba mt7601u: fix always true expression
diff mbox series

Patch

diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index c868582c5d22..aa3b64902cf9 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -99,7 +99,7 @@  mt7601u_has_tssi(struct mt7601u_dev *dev, u8 *eeprom)
 {
 	u16 nic_conf1 = get_unaligned_le16(eeprom + MT_EE_NIC_CONF_1);
 
-	return ~nic_conf1 && (nic_conf1 & MT_EE_NIC_CONF_1_TX_ALC_EN);
+	return (u16)~nic_conf1 && (nic_conf1 & MT_EE_NIC_CONF_1_TX_ALC_EN);
 }
 
 static void