Skip to contentSkip to conversation

Signedness issue in drivers/net/phy/phy_device.c

PATCH In review Eric Sesterhenn 3 participants 6 messages Aug 19, 2006 – Aug 20, 2006

Subsystems (2)
Network drivers Ethernet PHY
lore
Share & tools
lore

Eric Sesterhenn

author
#0 lore raw

PATCH 1/1 Signedness issue in drivers/net/phy/phy_device.c

hi,

while checking gcc 4.1 -Wextra warnings, I stumbled across the following two warnings:

drivers/net/phy/phy_device.c:528: warning: comparison of unsigned expression < 0 is always false drivers/net/phy/phy_device.c:546: warning: comparison of unsigned expression < 0 is always false

Since phy_read() returns an integer and can return negative values, it seems to me the best way to get proper error handling working again is to make val an int. Currently it is an u32, so the < 0 check always fails.

Signed-off-by Eric Sesterhenn <[email protected]>
drivers/net/phy/phy_device.c renamed from drivers/net/phy/phy_device.c.orig +1 -1
@@ -513,7 +513,7 @@ EXPORT_SYMBOL(genphy_read_status);
 
 static int genphy_config_init(struct phy_device *phydev)
 {
-	u32 val;
+	int val;
 	u32 features;
 
 	/* For now, I'll claim that the generic driver supports
#1 lore raw
Quoted 3 lines · Eric Sesterhenn

hi,

hello

Quoted 16 lines

while checking gcc 4.1 -Wextra warnings, I stumbled across the following two warnings:

drivers/net/phy/phy_device.c:528: warning: comparison of unsigned expression < 0 is always false drivers/net/phy/phy_device.c:546: warning: comparison of unsigned expression < 0 is always false

Since phy_read() returns an integer and can return negative values, it seems to me the best way to get proper error handling working again is to make val an int. Currently it is an u32, so the < 0 check always fails.

Signed-off-by Eric Sesterhenn <[email protected]>

drivers/net/phy/phy_device.c EXPORT_SYMBOL(genphy_read_status); · line 518

Quoted 7 lines
drivers/net/phy/phy_device.c renamed from drivers/net/phy/phy_device.c.orig +1 -1
@@ -513,7 +513,7 @@ EXPORT_SYMBOL(genphy_read_status);
 
 static int genphy_config_init(struct phy_device *phydev)
 {
-	u32 val;
+	int val;

Would it not be preferable to use a 's32' instead of an 'int'? After all, it seem 'val' needs to be 32 bits.

Just a thought
/Richard

author
#2 lore raw
Quoted 1 line · * Richard Knutsson ([email protected]) wrote:

Eric Sesterhenn wrote:

hi,

Quoted 2 lines

Would it not be preferable to use a 's32' instead of an 'int'? After all, it seem 'val' needs to be 32 bits.

not sure, but wouldnt this collide with platforms where an int is 64 Bits?

Eric

#3 lore raw
Quoted 9 lines · Eric Sesterhenn / Snakebyte
Quoted 1 line · * Richard Knutsson ([email protected]) wrote:

Eric Sesterhenn wrote:

hi,

Quoted 2 lines

Would it not be preferable to use a 's32' instead of an 'int'? After all, it seem 'val' needs to be 32 bits.

not sure, but wouldnt this collide with platforms where an int is 64 Bits?

None of the 64-bit Linux ports use ILP64.

Dave

Signature · 1 line
http://www.codemonkey.org.uk
author
#4 lore raw
Quoted 5 lines · Dave Jones
Quoted 4 lines · Eric Sesterhenn / Snakebyte
Quoted 1 line · * Richard Knutsson ([email protected]) wrote:

Eric Sesterhenn wrote:

hi,

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Would it not be preferable to use a 's32' instead of an 'int'? After all, it seem 'val' needs to be 32 bits.

not sure, but wouldnt this collide with platforms where an int is 64 Bits?

None of the 64-bit Linux ports use ILP64.

Here is an updated patch.

while checking gcc 4.1 -Wextra warnings, I stumbled across the following two warnings:

drivers/net/phy/phy_device.c:528: warning: comparison of unsigned expression < 0 is always false drivers/net/phy/phy_device.c:546: warning: comparison of unsigned expression < 0 is always false

Since phy_read() returns an integer and can return negative values, as proposed by Richard Knutsson this patch changes val to s32. Currently it is an u32, so the < 0 check always fails.

Signed-off-by Eric Sesterhenn <[email protected]>
drivers/net/phy/phy_device.c renamed from drivers/net/phy/phy_device.c.orig +1 -1
@@ -513,7 +513,7 @@ EXPORT_SYMBOL(genphy_read_status);
 
 static int genphy_config_init(struct phy_device *phydev)
 {
-	u32 val;
+	s32 val;
 	u32 features;
 
 	/* For now, I'll claim that the generic driver supports
#5 lore raw
In reply to Eric Sesterhenn / Snakebyte
Quoted 11 lines · Eric Sesterhenn / Snakebyte
Quoted 3 lines · * Richard Knutsson ([email protected]) wrote:

Would it not be preferable to use a 's32' instead of an 'int'? After all, it seem 'val' needs to be 32 bits.

not sure, but wouldnt this collide with platforms where an int is 64 Bits?

Because of 'phy_read'? It shouldn't as long the returning value is within the range of the s32.

Quoted 3 lines

Eric

Richard