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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 12 Oct 2019 21:41:45 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, Oct 12, 2019 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > 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.
> >  
> 
> If it helps, let's drop the ADIS patches for this round, and I can
> take a closer look as well.

Makes sense.   As I had queued a few other changes you sent
later on top of this series, I'll drop those for now as well
rather than trying to work out which ones have dependencies on this and
which don't.

So I'll drop all of:

iio: gyro: adis16260: replace mlock with ADIS lib's state_lock
iio: imu: adis: add a note better explaining state_lock
iio: imu: adis: update `adis_data` struct doc-string
iio: imu: adis: add doc-string for `adis` struct
iio: imu: adis16400: fix compiler warnings

+ original series
iio: imu: adis16480: use state lock for filter freq set	
iio: gyro: adis16136: rework locks using ADIS library's state lock
iio: imu: adis16400: rework locks using ADIS library's state lock
iio: imu: adis: group single conversion under a single state lock
iio: imu: adis: protect initial startup routine with state lock
iio: imu: adis: create an unlocked version of adis_reset()
iio: imu: adis: create an unlocked version of adis_check_status()
iio: imu: adis[16480]: group RW into a single lock in adis_enable_irq()
iio: imu: adis: add unlocked read/write function versions
iio: imu: adis: rename txrx_lock -> state_lock

If you could do me a single series with those as well that would
be great.

Thanks for sorting this out,

Jonathan

> The cleanup does seem to have revealed a few gaps in our CI in
> relation to upstreaming things.
> 
> We use Travis-CI for our stuff and stuff is public:
> https://travis-ci.org/analogdevicesinc/linux
> 
> So, if anyone sees anything we should do better, I'm open to
> improvements/suggestions.
> I am in the process of adding sparse-builds, maybe some build
> hardening is next (stronger compile/build flags), adding our patches
> on top of a newer kernel (that's partially done).
> That should help us catch things a bit earlier.
> 
> Sorry for the noise from my side as-well.
> Alex
> 
> 
> > Jonathan