lkml.org 
[lkml]   [2023]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v1] iio: adc: MCP3564: fix warn: unsigned '__x' is never less than zero.
    On Wed, 11 Oct 2023 16:41:38 +0000
    <Marius.Cristea@microchip.com> 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@microchip.com> wrote:
    > >
    > > > From: Marius Cristea <marius.cristea@microchip.com>
    > > >
    > > > 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@intel.com>
    > > > Closes:
    > > > https://lore.kernel.org/oe-kbuild-all/202309280738.NWjVfVt4-lkp@intel.com/
    > > > 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@microchip.com>
    > > > ---
    > > >  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@intel.com>"
    > 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

    \
     
     \ /
      Last update: 2023-10-12 09:36    [W:6.305 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site