Re: [PATCH v1] iio: adc: MCP3564: fix warn: unsigned '__x' is never less than zero.

[Jonathan Cameron]

On Wed, 11 Oct 2023 16:41:38 +0000
<Marius.Cristea@xxxxxxxxxxxxx> wrote:

>   Hi Jonathan,
> 
>  Sorry, I think I've made a "mistake" related to naming the patches and
> also not running the Smatch checker at a point in time.
> 
> 
> 
> On Tue, 2023-10-10 at 10:44 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Mon, 2 Oct 2023 19:16:18 +0300
> > <marius.cristea@xxxxxxxxxxxxx> wrote:
> >   
> > > From: Marius Cristea <marius.cristea@xxxxxxxxxxxxx>
> > > 
> > > The patch efea15e3c65d: "iio: adc: MCP3564: fix the static checker
> > > warning"
> > > leads to the following Smatch static checker warning:
> > > 
> > >    smatch warnings:
> > >    drivers/iio/adc/mcp3564.c:1105 mcp3564_fill_scale_tbls() warn:
> > > unsigned '__x' is never less than zero.
> > > 
> > > vim +/__x +1105 drivers/iio/adc/mcp3564.c
> > > 
> > >    1094
> > >    1095  static void mcp3564_fill_scale_tbls(struct mcp3564_state
> > > *adc)
> > >    1096  {
> > >    .....
> > >    1103          for (i = 0; i < MCP3564_MAX_PGA; i++) {
> > >    1104                  ref = adc->vref_mv;  
> > >  > 1105                  tmp1 = shift_right((u64)ref * NANO, pow);  
> > >    1106                  div_u64_rem(tmp1, NANO, &tmp0);
> > >    1107
> > >    .....
> > >    1113  }
> > > 
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202309280738.NWjVfVt4-lkp@xxxxxxxxx/
> > > Fixes: efea15e3c65d (iio: adc: MCP3564: fix the static checker
> > > warning)  
> > 
> > This fix is fine but can you talk me through how the static checker
> > warning fix
> > in question has anything to do with this one?
> > 
> > Was it just a case of fixing that issue allowing the static checker
> > to
> > get further before giving up?  In which case the description needs
> > modifying.
> > 
> > Or am I missing something in the following fix?
> > 
> > diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
> > index 64145f4ae55c..9ede1a5d5d7b 100644
> > --- a/drivers/iio/adc/mcp3564.c
> > +++ b/drivers/iio/adc/mcp3564.c
> > @@ -1422,11 +1422,8 @@ static int mcp3564_probe(struct spi_device
> > *spi)
> >         struct mcp3564_state *adc;
> > 
> >         indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > -       if (!indio_dev) {
> > -               dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev),
> > -                             "Can't allocate iio device\n");
> > +       if (!indio_dev)
> >                 return -ENOMEM;
> > -       }
> > 
> >   
> 
>   I've got two bugs reported:
> 
> - The first one was reported by Dan Carpenter "Re: [bug report] iio:
> adc: adding support for MCP3564 ADC". This bug was found using the
> "Smatch static checker warning" and it was related to:
> > --> 1426                 dev_err_probe(&indio_dev->dev,  
> PTR_ERR(indio_dev),
> 
> This bug was fixed by the above "[PATCH v1] iio: adc: MCP3564: fix the
> static checker warning" and it was applied on "Applied to the togreg
> branch of iio.git as that's where this driver is at the moment."
> 
> Also my mistake at this point was that I didn't setup and run the
> "Smatch static checker warning"
> 
> 
> > as that's all I'm seeing in that commit.
> >   
> Yes, that commit only handled part of the fix.
> 
> 
> 
> > > Signed-off-by: Marius Cristea <marius.cristea@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/iio/adc/mcp3564.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
> > > index 9ede1a5d5d7b..e3f1de5fcc5a 100644
> > > --- a/drivers/iio/adc/mcp3564.c
> > > +++ b/drivers/iio/adc/mcp3564.c
> > > @@ -1102,7 +1102,7 @@ static void mcp3564_fill_scale_tbls(struct
> > > mcp3564_state *adc)
> > > 
> > >       for (i = 0; i < MCP3564_MAX_PGA; i++) {
> > >               ref = adc->vref_mv;
> > > -             tmp1 = shift_right((u64)ref * NANO, pow);
> > > +             tmp1 = ((u64)ref * NANO) >> pow;
> > >               div_u64_rem(tmp1, NANO, &tmp0);
> > > 
> > >               tmp1 = tmp1 * mcp3564_hwgain_frac[(2 * i) + 1];
> > > 
> > > base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec  
> >   
> 
> - The second bug was reported by "kernel test robot <lkp@xxxxxxxxx>"
> also by running Smatch and it was run on the initial driver (without
> having the first patch applied)
> 
> smatch warnings:
> drivers/iio/adc/mcp3564.c:1105 mcp3564_fill_scale_tbls() warn: unsigned
> '__x' is never less than zero.
> drivers/iio/adc/mcp3564.c:1426 mcp3564_probe() warn: passing zero to
> 'PTR_ERR'
> drivers/iio/adc/mcp3564.c:1426 mcp3564_probe() warn: address of NULL
> pointer 'indio_dev'
> 
> 
> The:"drivers/iio/adc/mcp3564.c:1426 mcp3564_probe() warn: passing zero
> to 'PTR_ERR'" and "drivers/iio/adc/mcp3564.c:1426 mcp3564_probe() warn:
> address of NULL pointer 'indio_dev'" were fixed by the first patch.
> 
> The "drivers/iio/adc/mcp3564.c:1105 mcp3564_fill_scale_tbls() warn:
> unsigned '__x' is never less than zero." is fixed by the last patch
> "[PATCH v1] iio: adc: MCP3564: fix warn: unsigned '__x' is never less
> than zero."
>  by changeing:
> 
> -		tmp1 = shift_right((u64)ref * NANO, pow);
> +		tmp1 = ((u64)ref * NANO) >> pow;
> 
> shift_right function is "Required to safely shift negative values" but
> my value is always unsigned so it doesn't make sense to used it. This
> error was reported when I have run the Smatch over the driver + first
> patch (what was the latest from togreg).
> 
> I have applied the patch on top of what was the "latest" from togreg
> branch and not on the initial driver.
> 
> 
> I could change the description or I could provide a patch to handle
> both warning reporting at once.
If there are multiple issues then should be multiple patches. So starting
point is definitely a version of this one with the correct description.

Thanks,

Jonathan

> 
> Thanks,
> Marius