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
