Patchwork i2c-mxs: fix compile warning in mxs_i2c_xfer()

login
register
mail settings
Submitter Shawn Guo
Date Nov. 24, 2011, 7:52 a.m.
Message ID <1322121140-5216-1-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/127448/
State New
Headers show

Comments

Shawn Guo - Nov. 24, 2011, 7:52 a.m.
CC      drivers/i2c/busses/i2c-mxs.o
drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/i2c/busses/i2c-mxs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Wolfram Sang - Nov. 24, 2011, 8 a.m.
On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
>   CC      drivers/i2c/busses/i2c-mxs.o
> drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Which compiler? What flags? I don't see that here.
Shawn Guo - Nov. 24, 2011, 8:24 a.m.
On Thu, Nov 24, 2011 at 09:00:04AM +0100, Wolfram Sang wrote:
> On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> >   CC      drivers/i2c/busses/i2c-mxs.o
> > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> 
> Which compiler? What flags? I don't see that here.
> 
gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-8ubuntu3)

$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- -j 2 zImage
Uwe Kleine-König - Nov. 24, 2011, 8:29 a.m.
Hello Shawn,

On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
>   CC      drivers/i2c/busses/i2c-mxs.o
> drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/i2c/busses/i2c-mxs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 7e78f7c..00f098f 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
>  
>  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
>  {
> -	u32 data;
> +	u32 data = 0;
>  	int i;
>  
>  	for (i = 0; i < len; i++) {
Looks like a stupid compiler. Then better use 

	u32 uninitialized_var(data);

Best regards
Uwe
Wolfram Sang - Nov. 24, 2011, 8:37 a.m.
On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:
> Hello Shawn,
> 
> On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> >   CC      drivers/i2c/busses/i2c-mxs.o
> > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> > index 7e78f7c..00f098f 100644
> > --- a/drivers/i2c/busses/i2c-mxs.c
> > +++ b/drivers/i2c/busses/i2c-mxs.c
> > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
> >  
> >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
> >  {
> > -	u32 data;
> > +	u32 data = 0;
> >  	int i;
> >  
> >  	for (i = 0; i < len; i++) {
> Looks like a stupid compiler. Then better use 
> 
> 	u32 uninitialized_var(data);

Leave it as it is and fix the compiler? Will do another test later to make
sure.
Dong Aisheng - Nov. 24, 2011, 9:02 a.m.
> -----Original Message-----

> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-

> kernel-bounces@lists.infradead.org] On Behalf Of Wolfram Sang

> Sent: Thursday, November 24, 2011 4:38 PM

> To: Uwe Kleine-König

> Cc: Shawn Guo; linux-arm-kernel@lists.infradead.org; Ben Dooks; linux-

> i2c@vger.kernel.org

> Subject: Re: [PATCH] i2c-mxs: fix compile warning in mxs_i2c_xfer()

> 

> On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:

> > Hello Shawn,

> >

> > On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:

> > >   CC      drivers/i2c/busses/i2c-mxs.o

> > > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:

> > > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used

> > > uninitialized in this function

> > >

> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> > > ---

> > >  drivers/i2c/busses/i2c-mxs.c |    2 +-

> > >  1 files changed, 1 insertions(+), 1 deletions(-)

> > >

> > > diff --git a/drivers/i2c/busses/i2c-mxs.c

> > > b/drivers/i2c/busses/i2c-mxs.c index 7e78f7c..00f098f 100644

> > > --- a/drivers/i2c/busses/i2c-mxs.c

> > > +++ b/drivers/i2c/busses/i2c-mxs.c

> > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct

> > > mxs_i2c_dev *i2c)

> > >

> > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf,

> > > int len)  {

> > > -	u32 data;

> > > +	u32 data = 0;

> > >  	int i;

> > >

> > >  	for (i = 0; i < len; i++) {

> > Looks like a stupid compiler. Then better use

> >

> > 	u32 uninitialized_var(data);

> 

> Leave it as it is and fix the compiler? Will do another test later to

> make sure.

> 


Compiler is wrong?
Just from the code that it seems to be right the 'data' might be used
uninitialized.

Something I missed?

static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
{
        u32 data;
        int i;

        for (i = 0; i < len; i++) {
                if ((i & 3) == 0) {
                        if (mxs_i2c_wait_for_data(i2c))
                                return -ETIMEDOUT;
                        data = readl(i2c->regs + MXS_I2C_QUEUEDATA);
                }
                buf[i] = data & 0xff;
                data >>= 8;
        }

        return 0;
}

Regards
Dong Aisheng
Uwe Kleine-König - Nov. 24, 2011, 9:06 a.m.
Hello,

On Thu, Nov 24, 2011 at 09:02:21AM +0000, Dong Aisheng-B29396 wrote:
> > > > diff --git a/drivers/i2c/busses/i2c-mxs.c
> > > > b/drivers/i2c/busses/i2c-mxs.c index 7e78f7c..00f098f 100644
> > > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct
> > > > mxs_i2c_dev *i2c)
> > > >
> > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf,
> > > > int len)  {
> > > > -	u32 data;
> > > > +	u32 data = 0;
> > > >  	int i;
> > > >
> > > >  	for (i = 0; i < len; i++) {
> > > Looks like a stupid compiler. Then better use
> > >
> > > 	u32 uninitialized_var(data);
> > 
> > Leave it as it is and fix the compiler? Will do another test later to
> > make sure.
> > 
> 
> Compiler is wrong?
> Just from the code that it seems to be right the 'data' might be used
> uninitialized.
> 
> Something I missed?
> 
> static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
> {
>         u32 data;
>         int i;
> 
>         for (i = 0; i < len; i++) {
>                 if ((i & 3) == 0) {
>                         if (mxs_i2c_wait_for_data(i2c))
>                                 return -ETIMEDOUT;
>                         data = readl(i2c->regs + MXS_I2C_QUEUEDATA);
>                 }
>                 buf[i] = data & 0xff;
>                 data >>= 8;
>         }
When data is used the first time, i is 0, so it went through the body of
the if above before which assigned data. So no, data is initialized when
used to assign buf[i].

Best regards
Uwe
Dong Aisheng - Nov. 24, 2011, 9:09 a.m.
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: Thursday, November 24, 2011 5:07 PM
> To: Dong Aisheng-B29396
> Cc: Wolfram Sang; Shawn Guo; linux-arm-kernel@lists.infradead.org; Ben
> Dooks; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH] i2c-mxs: fix compile warning in mxs_i2c_xfer()
> 
> Hello,
> 
> On Thu, Nov 24, 2011 at 09:02:21AM +0000, Dong Aisheng-B29396 wrote:
> > > > > diff --git a/drivers/i2c/busses/i2c-mxs.c
> > > > > b/drivers/i2c/busses/i2c-mxs.c index 7e78f7c..00f098f 100644
> > > > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct
> > > > > mxs_i2c_dev *i2c)
> > > > >
> > > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8
> > > > > *buf, int len)  {
> > > > > -	u32 data;
> > > > > +	u32 data = 0;
> > > > >  	int i;
> > > > >
> > > > >  	for (i = 0; i < len; i++) {
> > > > Looks like a stupid compiler. Then better use
> > > >
> > > > 	u32 uninitialized_var(data);
> > >
> > > Leave it as it is and fix the compiler? Will do another test later
> > > to make sure.
> > >
> >
> > Compiler is wrong?
> > Just from the code that it seems to be right the 'data' might be used
> > uninitialized.
> >
> > Something I missed?
> >
> > static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int
> > len) {
> >         u32 data;
> >         int i;
> >
> >         for (i = 0; i < len; i++) {
> >                 if ((i & 3) == 0) {
> >                         if (mxs_i2c_wait_for_data(i2c))
> >                                 return -ETIMEDOUT;
> >                         data = readl(i2c->regs + MXS_I2C_QUEUEDATA);
> >                 }
> >                 buf[i] = data & 0xff;
> >                 data >>= 8;
> >         }
> When data is used the first time, i is 0, so it went through the body of
> the if above before which assigned data. So no, data is initialized when
> used to assign buf[i].
> 
Ok, Thanks for the info.
So the compiler should do intelligent checking.

Regards
Dong Aisheng
Lothar Waßmann - Nov. 24, 2011, 9:11 a.m.
Hi,

Dong Aisheng-B29396 writes:
> > -----Original Message-----
> > From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-
> > kernel-bounces@lists.infradead.org] On Behalf Of Wolfram Sang
> > Sent: Thursday, November 24, 2011 4:38 PM
> > To: Uwe Kleine-König
> > Cc: Shawn Guo; linux-arm-kernel@lists.infradead.org; Ben Dooks; linux-
> > i2c@vger.kernel.org
> > Subject: Re: [PATCH] i2c-mxs: fix compile warning in mxs_i2c_xfer()
> > 
> > On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:
> > > Hello Shawn,
> > >
> > > On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> > > >   CC      drivers/i2c/busses/i2c-mxs.o
> > > > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > > > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used
> > > > uninitialized in this function
> > > >
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-mxs.c
> > > > b/drivers/i2c/busses/i2c-mxs.c index 7e78f7c..00f098f 100644
> > > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct
> > > > mxs_i2c_dev *i2c)
> > > >
> > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf,
> > > > int len)  {
> > > > -	u32 data;
> > > > +	u32 data = 0;
> > > >  	int i;
> > > >
> > > >  	for (i = 0; i < len; i++) {
> > > Looks like a stupid compiler. Then better use
> > >
> > > 	u32 uninitialized_var(data);
> > 
> > Leave it as it is and fix the compiler? Will do another test later to
> > make sure.
> > 
> 
> Compiler is wrong?
> Just from the code that it seems to be right the 'data' might be used
> uninitialized.
> 
Only iff you assume that (0 & 3) could be != 0...


Lothar Waßmann
Shawn Guo - Nov. 25, 2011, 1:15 a.m.
On Thu, Nov 24, 2011 at 09:37:47AM +0100, Wolfram Sang wrote:
> On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:
> > Hello Shawn,
> > 
> > On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> > >   CC      drivers/i2c/busses/i2c-mxs.o
> > > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> > > index 7e78f7c..00f098f 100644
> > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
> > >  
> > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
> > >  {
> > > -	u32 data;
> > > +	u32 data = 0;
> > >  	int i;
> > >  
> > >  	for (i = 0; i < len; i++) {
> > Looks like a stupid compiler. Then better use 
> > 
> > 	u32 uninitialized_var(data);
> 
> Leave it as it is and fix the compiler? Will do another test later to make
> sure.
> 
Ben, do you have an opinion on this, leave it there or fix it?
Wolfram Sang - Nov. 25, 2011, 9:08 a.m.
On Fri, Nov 25, 2011 at 09:15:20AM +0800, Shawn Guo wrote:
> On Thu, Nov 24, 2011 at 09:37:47AM +0100, Wolfram Sang wrote:
> > On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:
> > > Hello Shawn,
> > > 
> > > On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> > > >   CC      drivers/i2c/busses/i2c-mxs.o
> > > > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > > > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used uninitialized in this function
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> > > > index 7e78f7c..00f098f 100644
> > > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
> > > >  
> > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
> > > >  {
> > > > -	u32 data;
> > > > +	u32 data = 0;
> > > >  	int i;
> > > >  
> > > >  	for (i = 0; i < len; i++) {
> > > Looks like a stupid compiler. Then better use 
> > > 
> > > 	u32 uninitialized_var(data);
> > 
> > Leave it as it is and fix the compiler? Will do another test later to make
> > sure.
> > 
> Ben, do you have an opinion on this, leave it there or fix it?

Sorry, forgot the tests yesterday. I don't like random annotations
because of faulty compilers. Please fix that one. NACK from me.
Marek Vasut - Dec. 2, 2011, 7:42 p.m.
> On Fri, Nov 25, 2011 at 09:15:20AM +0800, Shawn Guo wrote:
> > On Thu, Nov 24, 2011 at 09:37:47AM +0100, Wolfram Sang wrote:
> > > On Thu, Nov 24, 2011 at 09:29:03AM +0100, Uwe Kleine-König wrote:
> > > > Hello Shawn,
> > > > 
> > > > On Thu, Nov 24, 2011 at 03:52:20PM +0800, Shawn Guo wrote:
> > > > >   CC      drivers/i2c/busses/i2c-mxs.o
> > > > > 
> > > > > drivers/i2c/busses/i2c-mxs.c: In function ‘mxs_i2c_xfer’:
> > > > > drivers/i2c/busses/i2c-mxs.c:196:6: warning: ‘data’ may be used
> > > > > uninitialized in this function
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > ---
> > > > > 
> > > > >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/i2c/busses/i2c-mxs.c
> > > > > b/drivers/i2c/busses/i2c-mxs.c index 7e78f7c..00f098f 100644
> > > > > --- a/drivers/i2c/busses/i2c-mxs.c
> > > > > +++ b/drivers/i2c/busses/i2c-mxs.c
> > > > > @@ -193,7 +193,7 @@ static int mxs_i2c_wait_for_data(struct
> > > > > mxs_i2c_dev *i2c)
> > > > > 
> > > > >  static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf,
> > > > >  int len) {
> > > > > 
> > > > > -	u32 data;
> > > > > +	u32 data = 0;
> > > > > 
> > > > >  	int i;
> > > > >  	
> > > > >  	for (i = 0; i < len; i++) {
> > > > 
> > > > Looks like a stupid compiler. Then better use
> > > > 
> > > > 	u32 uninitialized_var(data);
> > > 
> > > Leave it as it is and fix the compiler? Will do another test later to
> > > make sure.
> > 
> > Ben, do you have an opinion on this, leave it there or fix it?
> 
> Sorry, forgot the tests yesterday. I don't like random annotations
> because of faulty compilers. Please fix that one. NACK from me.

Hi,

I hit this with emdebian gcc 4.6.1 ... I'd be for merging this u32 data = 0; 
stuff, so on my side it's ACK.

M

Patch

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 7e78f7c..00f098f 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -193,7 +193,7 @@  static int mxs_i2c_wait_for_data(struct mxs_i2c_dev *i2c)
 
 static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 {
-	u32 data;
+	u32 data = 0;
 	int i;
 
 	for (i = 0; i < len; i++) {