Re: [PULL v2] First set of iio new device support etc for the 5.5 cycle

From: Jonathan Cameron <jic23@kernel.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: linux-iio@vger.kernel.org,
	Alexandru Ardelean <alexandru.ardelean@analog.com>
Subject: Re: [PULL v2] First set of iio new device support etc for the 5.5 cycle
Date: Sat, 12 Oct 2019 17:06:15 +0100	[thread overview]
Message-ID: <20191012170615.01546a96@archlinux> (raw)
In-Reply-To: <20191012152841.GB2142233@kroah.com>

On Sat, 12 Oct 2019 17:28:41 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Oct 12, 2019 at 05:27:44PM +0200, Greg KH wrote:
> > On Sat, Oct 12, 2019 at 12:19:46PM +0100, Jonathan Cameron wrote:  
> > > The following changes since commit b73b93a2af3392b9b7b8ba7e818ee767499f9655:
> > > 
> > >   iio: adc: ad7192: Add sysfs ABI documentation (2019-09-08 10:34:49 +0100)
> > > 
> > > are available in the Git repository at:
> > > 
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git tags/iio-for-5.5a-take2  
> > 
> > Better, but I see this now:
> > 
> > drivers/iio/imu/adis.c: In function ‘__adis_check_status’:
> > drivers/iio/imu/adis.c:295:9: warning: ‘status’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >   295 |  status &= adis->data->status_error_mask;
> >       |         ^~
> > 
> > 
> > I'll take this, can you just send a follow-on patch for this?  
> 
> Also I see:
> 
> drivers/iio/imu/adis16480.c: In function ‘adis16480_enable_irq’:
> drivers/iio/imu/adis16480.c:950:6: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   950 |  val &= ~ADIS16480_DRDY_EN_MSK;
>       |      ^~
>   CC [M]  drivers/iio/magnetometer/hmc5843_i2c.o
> drivers/iio/imu/adis16480.c: In function ‘adis16480_write_raw’:
> drivers/iio/imu/adis16480.c:571:7: warning: ‘val’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   571 |   val &= ~enable_mask;
>       |       ^~
> drivers/iio/imu/adis16480.c:557:11: note: ‘val’ was declared here
>   557 |  uint16_t val;
>       |           ^~~
> 
> 
> So did you really fix anything here?
> 
> I'll drop this pull again.
> 
> What version of gcc are you using?  Might I suggest a newer one (i.e. a
> modern one?)

Ah. This is my mistake.  I did see all of these, but still thought we were
in the category of tidying up some compiler version caused issues.

The adis16400 came up in my local tests, so I previously pinged Alex on
basis it was something to do in a follow up. The other two showed up, but
again I still thought these were compiler version issues, particularly
as 0-day didn't highlight them (there were several other issues it
did highlight this week). Hence again I requested a follow up to tidy
it up.

Anyhow, did some digging.  The issue here was a 'fix' I put in to an initial
0-day issue in the inline functions that Alex added.  Note that one
appears to be compiler version dependent as it didn't turn up in my
local builds. There are now inline functions that check if (ret)
and don't set the value if ret is non 0.

Out in the drivers, the check is the more specific (unnecessarily)
if (ret < 0) and hence the compiler is concluded that there might be a path to
val not being set.  Previously it was giving up figuring this out.
So reality is they are a false positive (sort of as in reality ret
is never positive) but the compiler has made a reasonable point
that it can't see that.

Never mind, I'll do a new pull request once fixes are in place.
Given there are two obvious ways of suppressing this and it's Alex's
driver I'll wait until he has time to take a look.

Sorry for wasting your time.

Jonathan