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

From: Marius.Cristea
Date: Thu Oct 12 2023 - 04:19:01 EST

Next message: Tomasz Figa: "Re: [PATCH v2] media: videobuf2: Fix IS_ERR checking in vb2_dc_put_userptr()"
Previous message: Zhihao Cheng: "Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier"
In reply to: Jonathan Cameron: "Re: [PATCH v1] iio: adc: MCP3564: fix warn: unsigned '__x' is never less than zero."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jonathan,

On Thu, 2023-10-12 at 08:36 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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.
>

Actually there was 2 bug reports and the second one includes the first
bug report.

For the first bug/warming report I already submit a patch that was
applied to togreg branch
(commit efea15e3c65d96bac17a4d8104e3fff7c07cc910).

For the second bug/warming report I have "fixed" only the part that
wasn't fixed before.

I will resubmit this patch updating the description.

> Thanks,
>
> Jonathan
>
> >
> > Thanks,
> > Marius
>

Thanks,
Marius

Next message: Tomasz Figa: "Re: [PATCH v2] media: videobuf2: Fix IS_ERR checking in vb2_dc_put_userptr()"
Previous message: Zhihao Cheng: "Re: [PATCH RFC] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier"
In reply to: Jonathan Cameron: "Re: [PATCH v1] iio: adc: MCP3564: fix warn: unsigned '__x' is never less than zero."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]