| Message ID | 1322121140-5216-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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.
> -----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
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
> -----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
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
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?
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.
> 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
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++) {
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(-)