2006-08-19 17:33:40

by Eric Sesterhenn

Subject: [Patch] 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]>

--- linux-2.6.18-rc4/drivers/net/phy/phy_device.c.orig 2006-08-19 18:22:56.000000000 +0200
+++ linux-2.6.18-rc4/drivers/net/phy/phy_device.c 2006-08-19 18:24:49.000000000 +0200
@@ -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

2006-08-20 04:05:06

by Richard Knutsson

Subject: Re: [Patch] Signedness issue in drivers/net/phy/phy_device.c

Eric Sesterhenn wrote:

>hi,
>
>
hello

>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]>
>
>--- linux-2.6.18-rc4/drivers/net/phy/phy_device.c.orig 2006-08-19 18:22:56.000000000 +0200
>+++ linux-2.6.18-rc4/drivers/net/phy/phy_device.c 2006-08-19 18:24:49.000000000 +0200
>@@ -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

2006-08-20 18:36:06

by Eric Sesterhenn

Subject: Re: [Patch] Signedness issue in drivers/net/phy/phy_device.c

* Richard Knutsson ([email protected]) wrote:
> Eric Sesterhenn wrote:
hi,

> 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

2006-08-20 19:16:55

by Dave Jones

Subject: Re: [Patch] Signedness issue in drivers/net/phy/phy_device.c

On Sun, Aug 20, 2006 at 08:36:00PM +0200, Eric Sesterhenn / Snakebyte wrote:
> * Richard Knutsson ([email protected]) wrote:
> > Eric Sesterhenn wrote:
> hi,
>
> > 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

--
http://www.codemonkey.org.uk

2006-08-20 20:19:59

by Richard Knutsson

Subject: Re: [Patch] Signedness issue in drivers/net/phy/phy_device.c

Eric Sesterhenn / Snakebyte wrote:

>* 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.

>Eric
>
>
Richard

2006-08-20 20:39:19

by Eric Sesterhenn

Subject: Re: [Patch] Signedness issue in drivers/net/phy/phy_device.c

On Sun, 2006-08-20 at 15:16 -0400, Dave Jones wrote:
> On Sun, Aug 20, 2006 at 08:36:00PM +0200, Eric Sesterhenn / Snakebyte wrote:
> > * Richard Knutsson ([email protected]) wrote:
> > > Eric Sesterhenn wrote:
> > hi,
> >
> > > 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]>

--- linux-2.6.18-rc4/drivers/net/phy/phy_device.c.orig 2006-08-20 22:05:26.000000000 +0200
+++ linux-2.6.18-rc4/drivers/net/phy/phy_device.c 2006-08-20 22:05:42.000000000 +0200
@@ -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