Patchwork net: usb: initialize tmp in dm9601.c to avoid warning

login
register
mail settings
Submitter Simon Que
Date Jan. 17, 2013, 7:29 p.m.
Message ID <1358450989-25205-1-git-send-email-sque@chromium.org>
Download mbox | patch
Permalink /patch/213353/
State Accepted
Delegated to: David Miller
Headers show

Comments

Simon Que - Jan. 17, 2013, 7:29 p.m.
In two places, tmp is initialized implicitly by being passed as a
pointer during a function call.  However, this is not obvious to the
compiler, which logs a warning.

Signed-off-by: Simon Que <sque@chromium.org>
---
 drivers/net/usb/dm9601.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Peter Korsgaard - Jan. 17, 2013, 10:47 p.m.
>>>>> "Simon" == Simon Que <sque@chromium.org> writes:

 Simon> In two places, tmp is initialized implicitly by being passed as a
 Simon> pointer during a function call.  However, this is not obvious to the
 Simon> compiler, which logs a warning.

What warning and what compiler version are you using?


 Simon> Signed-off-by: Simon Que <sque@chromium.org>
 Simon> ---
 Simon>  drivers/net/usb/dm9601.c |    4 ++--
 Simon>  1 files changed, 2 insertions(+), 2 deletions(-)

 Simon> diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
 Simon> index e0433ce..94e716d 100644
 Simon> --- a/drivers/net/usb/dm9601.c
 Simon> +++ b/drivers/net/usb/dm9601.c
 Simon> @@ -199,7 +199,7 @@ static int dm_read_shared_word(struct usbnet *dev, int phy, u8 reg, __le16 *valu
 Simon>  	dm_write_reg(dev, DM_SHARED_CTRL, phy ? 0xc : 0x4);
 
 Simon>  	for (i = 0; i < DM_TIMEOUT; i++) {
 Simon> -		u8 tmp;
 Simon> +		u8 tmp = 0;
 
 Simon>  		udelay(1);
 Simon>  		ret = dm_read_reg(dev, DM_SHARED_CTRL, &tmp);
 Simon> @@ -242,7 +242,7 @@ static int dm_write_shared_word(struct usbnet *dev, int phy, u8 reg, __le16 valu
 Simon>  	dm_write_reg(dev, DM_SHARED_CTRL, phy ? 0x1a : 0x12);
 
 Simon>  	for (i = 0; i < DM_TIMEOUT; i++) {
 Simon> -		u8 tmp;
 Simon> +		u8 tmp = 0;
 
 Simon>  		udelay(1);
 Simon>  		ret = dm_read_reg(dev, DM_SHARED_CTRL, &tmp);
 Simon> -- 
 Simon> 1.7.8.6
David Miller - Jan. 17, 2013, 10:54 p.m.
From: Peter Korsgaard <jacmet@sunsite.dk>
Date: Thu, 17 Jan 2013 23:47:58 +0100

>>>>>> "Simon" == Simon Que <sque@chromium.org> writes:
> 
>  Simon> In two places, tmp is initialized implicitly by being passed as a
>  Simon> pointer during a function call.  However, this is not obvious to the
>  Simon> compiler, which logs a warning.
> 
> What warning and what compiler version are you using?

I wouldn't bother asking this, unless it's so that you personally
can reproduce this in the future.

The compiler cannot determine with %100 certainty that tmp is
initialized in all paths.  Any time there are conditional
statements guarding setting and the use, the compiler is going
to err on the side of caution and say that if any of those
paths lead to a use without a set it's going to warn.

Even if it's actually impossible.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard - Jan. 18, 2013, 7:37 a.m.
>>>>> "David" == David Miller <davem@davemloft.net> writes:

 >> What warning and what compiler version are you using?

 David> I wouldn't bother asking this, unless it's so that you personally
 David> can reproduce this in the future.

 David> The compiler cannot determine with %100 certainty that tmp is
 David> initialized in all paths.  Any time there are conditional
 David> statements guarding setting and the use, the compiler is going
 David> to err on the side of caution and say that if any of those
 David> paths lead to a use without a set it's going to warn.

Sure, I just wondered as the code hasn't changed in 5+ years, and the
function that gets called is defined just above, so the compiler should
be able to figure it out.
David Miller - Jan. 18, 2013, 8:03 a.m.
From: Peter Korsgaard <jacmet@sunsite.dk>
Date: Fri, 18 Jan 2013 08:37:41 +0100

>>>>>> "David" == David Miller <davem@davemloft.net> writes:
> 
>  >> What warning and what compiler version are you using?
> 
>  David> I wouldn't bother asking this, unless it's so that you personally
>  David> can reproduce this in the future.
> 
>  David> The compiler cannot determine with %100 certainty that tmp is
>  David> initialized in all paths.  Any time there are conditional
>  David> statements guarding setting and the use, the compiler is going
>  David> to err on the side of caution and say that if any of those
>  David> paths lead to a use without a set it's going to warn.
> 
> Sure, I just wondered as the code hasn't changed in 5+ years, and the
> function that gets called is defined just above, so the compiler should
> be able to figure it out.

Nothing guarentees that a piece of memory passed into an external
function has it's contents initialized completely by that function.

The compiler can't see what usbnet_read_cmd() does with the memory
behind the 'data' pointer argument.  So it makes the only assumption
it can make, which is that 'tmp' might be uninitialized.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard - Jan. 18, 2013, 8:44 a.m.
>>>>> "David" == David Miller <davem@davemloft.net> writes:

 >> Sure, I just wondered as the code hasn't changed in 5+ years, and the
 >> function that gets called is defined just above, so the compiler should
 >> be able to figure it out.

 David> Nothing guarentees that a piece of memory passed into an external
 David> function has it's contents initialized completely by that function.

 David> The compiler can't see what usbnet_read_cmd() does with the memory
 David> behind the 'data' pointer argument.  So it makes the only assumption
 David> it can make, which is that 'tmp' might be uninitialized.

True, I had forgotten about the usbnet_read_cmd() change (which is the
change causing the warning). With that:

Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
David Laight - Jan. 18, 2013, 10:07 a.m.
> > Sure, I just wondered as the code hasn't changed in 5+ years, and the
> > function that gets called is defined just above, so the compiler should
> > be able to figure it out.
> 
> Nothing guarentees that a piece of memory passed into an external
> function has it's contents initialized completely by that function.
> 
> The compiler can't see what usbnet_read_cmd() does with the memory
> behind the 'data' pointer argument.  So it makes the only assumption
> it can make, which is that 'tmp' might be uninitialized.

Possibly the called function is being inlined, then the analysis
knows that some paths don't assign a value.
When it isn't inlined it assumes the value is always modified.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 18, 2013, 7:29 p.m.
From: Simon Que <sque@chromium.org>
Date: Thu, 17 Jan 2013 11:29:49 -0800

> In two places, tmp is initialized implicitly by being passed as a
> pointer during a function call.  However, this is not obvious to the
> compiler, which logs a warning.
> 
> Signed-off-by: Simon Que <sque@chromium.org>

Applied to net-next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index e0433ce..94e716d 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -199,7 +199,7 @@  static int dm_read_shared_word(struct usbnet *dev, int phy, u8 reg, __le16 *valu
 	dm_write_reg(dev, DM_SHARED_CTRL, phy ? 0xc : 0x4);
 
 	for (i = 0; i < DM_TIMEOUT; i++) {
-		u8 tmp;
+		u8 tmp = 0;
 
 		udelay(1);
 		ret = dm_read_reg(dev, DM_SHARED_CTRL, &tmp);
@@ -242,7 +242,7 @@  static int dm_write_shared_word(struct usbnet *dev, int phy, u8 reg, __le16 valu
 	dm_write_reg(dev, DM_SHARED_CTRL, phy ? 0x1a : 0x12);
 
 	for (i = 0; i < DM_TIMEOUT; i++) {
-		u8 tmp;
+		u8 tmp = 0;
 
 		udelay(1);
 		ret = dm_read_reg(dev, DM_SHARED_CTRL, &tmp);