Re: [jic23-iio:testing 6/10] drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (un...

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 20 Jul 2023 19:18:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 20 Jul 2023 11:29:55 +0200
> "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > On Wed, Jun 21, 2023, at 10:25, Andy Shevchenko wrote:  
> > > On Wed, Jun 21, 2023 at 10:19 AM kernel test robot <lkp@intel.com> wrote:    
> > >>
> > >> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
> > >> head:   25e201cc6ff270abc062e13ff912292097cb2827
> > >> commit: d3e93b67f934a477c5851d575a2278f07c6242fb [6/10] iio: adc: max14001: New driver
> > >> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/config)
> > >> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
> > >> reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/reproduce)
> > >>
> > >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > >> the same patch/commit), kindly add following tags
> > >> | Reported-by: kernel test robot <lkp@intel.com>
> > >> | Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/
> > >>
> > >> All warnings (new ones prefixed by >>):    
> > >
> > > Okay, you even haven't compiled your code :-(
> > > This should probably use one of the functions from bitfield.h.
> > > Like u32_encode_bits().    
> > 
> > I see the warning is now in linux-next. I see it only shows up with
> > clang and not gcc, but it would be nice to address it.  
> 
> Sorry, I managed to forget this one was still outstanding and picked up the driver.
> 
> >   
> > >    
> > >> >> drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]    
> > >>            reg_data = FIELD_PREP(mask, val);
> > >>                       ^~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> > >>                    __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
> > >>                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > >>                    BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
> > >>                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > >>    include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > >>    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > >>                                        ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > >>    include/linux/compiler_types.h:397:22: note: expanded from macro 'compiletime_assert'
> > >>            _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > >>            ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/compiler_types.h:385:23: note: expanded from macro '_compiletime_assert'
> > >>            __compiletime_assert(condition, msg, prefix, suffix)
> > >>            ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/compiler_types.h:377:9: note: expanded from macro '__compiletime_assert'
> > >>                    if (!(condition))                                       \
> > >>                          ^~~~~~~~~
> > >>    1 warning generated.    
> > 
> > It looks like the problem is the use of FIELD_PREP() with a variable
> > 'val' instead of a constant. I tried marking the function __always_inline
> > so the compiler can see the actual value ('1') that gets passed in
> > as a constant, but that did not change anything.
> > 
> > 
> > It also looks suspicious that the value first gets read from the 
> > register and then replaced with the FIELD_PREP() output, while the
> > original value gets discarded. Is this intentional?  
> 
> There are a lot of other fields in that register, so unlikely this was the intent.
> A number of them default to non 0 as well.
> 
> > 
> >         ret = max14001_read(st, reg_addr, &reg_data);
> >         if (ret)
> >                 return ret;
> >         reg_data = FIELD_PREP(mask, val);
> >         ret = max14001_write(st, reg_addr, reg_data);
> > 
> > 
> > I have managed to shut up the warning by rearranging the code like:
> > 
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > index 18ace09601985..f2ce8f8947982 100644
> > --- a/drivers/iio/adc/max14001.c
> > +++ b/drivers/iio/adc/max14001.c
> > @@ -117,11 +117,9 @@ static int max14001_write_verification_reg(struct max14001_state *st,
> >  
> >  static int max14001_reg_update(struct max14001_state *st,
> >                                 unsigned int reg_addr,
> > -                               unsigned int mask,
> > -                               unsigned int val)
> > +                               unsigned int reg_data)
> >  {
> >         int ret;
> > -       unsigned int reg_data;
> >  
> >         /* Enable SPI Registers Write */
> >         ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> > @@ -132,8 +130,6 @@ static int max14001_reg_update(struct max14001_state *st,
> >         if (ret)
> >                 return ret;
> >  
> > -       reg_data = FIELD_PREP(mask, val);
> > -
> >         ret = max14001_write(st, reg_addr, reg_data);
> >         if (ret)
> >                 return ret;
> > @@ -299,7 +295,7 @@ static int max14001_probe(struct spi_device *spi)
> >  
> >                 /* select external voltage reference source for the ADC */
> >                 ret = max14001_reg_update(st, MAX14001_CFG,
> > -                                         MAX14001_CFG_EXRF, 1);
> > +                                         FIELD_PREP(MAX14001_CFG_EXRF, 1));
> >  
> >                 if (ret < 0)
> >                         return ret;  
> 
> I'd prefer to just drop this reg_update function entirely.  Put the call inline
> so that we can use FIELD_PREP() directly rather than (after fixing the probably bug)
> passing in both the value and the mask.
> 
> > 
> > but it looks like there is still a bug in max14001_reg_update(),
> > so I'd prefer Kim Seer Paller to revisit this issue and submit
> > a properly tested patch.  
> 
> Absolutely agree.   If it's outstanding in few weeks though we can go
> with an educated 'guess' for the fix but I'd really rather not if Kim
> can post a fix in the meantime.
> 
On closer inspection of my inbox, there are other reported problems that
need resolving so I'll do a rebase and drop the driver until this stuff is resolved.

Jonathan

> Jonathan
> 
> > 
> >        Arnd  
>