* [bug report] iio: adc: ad7949: add vref selection support
@ 2021-09-21 6:35 Dan Carpenter
2021-09-22 5:10 ` Liam Beguin
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-09-21 6:35 UTC (permalink / raw)
To: lvb; +Cc: linux-iio
Hello Liam Beguin,
The patch 379306506049: "iio: adc: ad7949: add vref selection
support" from Aug 15, 2021, leads to the following
Smatch static checker warning:
drivers/iio/adc/ad7949.c:387 ad7949_spi_probe()
error: 'ad7949_adc->vref' dereferencing possible ERR_PTR()
drivers/iio/adc/ad7949.c
309 static int ad7949_spi_probe(struct spi_device *spi)
310 {
311 u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
312 struct device *dev = &spi->dev;
313 const struct ad7949_adc_spec *spec;
314 struct ad7949_adc_chip *ad7949_adc;
315 struct iio_dev *indio_dev;
316 u32 tmp;
317 int ret;
318
319 indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
320 if (!indio_dev) {
321 dev_err(dev, "can not allocate iio device\n");
322 return -ENOMEM;
323 }
324
325 indio_dev->info = &ad7949_spi_info;
326 indio_dev->name = spi_get_device_id(spi)->name;
327 indio_dev->modes = INDIO_DIRECT_MODE;
328 indio_dev->channels = ad7949_adc_channels;
329 spi_set_drvdata(spi, indio_dev);
330
331 ad7949_adc = iio_priv(indio_dev);
332 ad7949_adc->indio_dev = indio_dev;
333 ad7949_adc->spi = spi;
334
335 spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
336 indio_dev->num_channels = spec->num_channels;
337 ad7949_adc->resolution = spec->resolution;
338
339 /* Set SPI bits per word */
340 if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
341 spi->bits_per_word = ad7949_adc->resolution;
342 } else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
343 spi->bits_per_word = 16;
344 } else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
345 spi->bits_per_word = 8;
346 } else {
347 dev_err(dev, "unable to find common BPW with spi controller\n");
348 return -EINVAL;
349 }
350
351 /* Setup internal voltage reference */
352 tmp = 4096000;
353 device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
354
355 switch (tmp) {
356 case 2500000:
357 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
358 break;
359 case 4096000:
360 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
361 break;
362 default:
363 dev_err(dev, "unsupported internal voltage reference\n");
364 return -EINVAL;
365 }
366
367 /* Setup external voltage reference, buffered? */
368 ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
Apparenty it's really rare to have an optional regulator? This function
is very tricky. It should return NULL if the option is not like all the
other optional features in the kernel. But the regulator code is really
not set up for not having a regulator. If it were then there would be a
lot of NULL checks in the regulator code. But since it's not then you
have to add them to the driver instead.
369 if (IS_ERR(ad7949_adc->vref)) {
370 ret = PTR_ERR(ad7949_adc->vref);
371 if (ret != -ENODEV)
372 return ret;
373 /* unbuffered? */
374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
375 if (IS_ERR(ad7949_adc->vref)) {
376 ret = PTR_ERR(ad7949_adc->vref);
377 if (ret != -ENODEV)
378 return ret;
Instead of returning NULL when the regulator is disabled it returns
-ENODEV. How do you differentiate from an -ENODEV which is caused by an
error vs an -ENODEV which is caused because the optional regulator is
disabled? You'll just have to hope that the errors are less common and
assume it means disabled.
You might be doubting that devm_regulator_get_optional() can return
-ENODEV on error? Look at the code and prepare your heart for sadness.
379 } else {
380 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
381 }
382 } else {
383 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
384 }
385
386 if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
--> 387 ret = regulator_enable(ad7949_adc->vref);
^^^^^^^^^^^^^^^^
Every reference to ->vref will crash if the optional regulator is
disabled.
388 if (ret < 0) {
389 dev_err(dev, "fail to enable regulator\n");
390 return ret;
391 }
392
393 ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
394 ad7949_adc->vref);
395 if (ret)
396 return ret;
397 }
398
399 mutex_init(&ad7949_adc->lock);
400
401 ret = ad7949_spi_init(ad7949_adc);
402 if (ret) {
403 dev_err(dev, "enable to init this device: %d\n", ret);
404 return ret;
405 }
406
407 ret = devm_iio_device_register(dev, indio_dev);
408 if (ret)
409 dev_err(dev, "fail to register iio device: %d\n", ret);
410
411 return ret;
412 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iio: adc: ad7949: add vref selection support
2021-09-21 6:35 [bug report] iio: adc: ad7949: add vref selection support Dan Carpenter
@ 2021-09-22 5:10 ` Liam Beguin
2021-09-22 6:00 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Liam Beguin @ 2021-09-22 5:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: lvb, linux-iio, liambeguin
Hi Dan,
On Tue, Sep 21, 2021 at 09:35:09AM +0300, Dan Carpenter wrote:
> Hello Liam Beguin,
>
> The patch 379306506049: "iio: adc: ad7949: add vref selection
> support" from Aug 15, 2021, leads to the following
> Smatch static checker warning:
>
> drivers/iio/adc/ad7949.c:387 ad7949_spi_probe()
> error: 'ad7949_adc->vref' dereferencing possible ERR_PTR()
>
> drivers/iio/adc/ad7949.c
> 309 static int ad7949_spi_probe(struct spi_device *spi)
> 310 {
> 311 u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> 312 struct device *dev = &spi->dev;
> 313 const struct ad7949_adc_spec *spec;
> 314 struct ad7949_adc_chip *ad7949_adc;
> 315 struct iio_dev *indio_dev;
> 316 u32 tmp;
> 317 int ret;
> 318
> 319 indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> 320 if (!indio_dev) {
> 321 dev_err(dev, "can not allocate iio device\n");
> 322 return -ENOMEM;
> 323 }
> 324
> 325 indio_dev->info = &ad7949_spi_info;
> 326 indio_dev->name = spi_get_device_id(spi)->name;
> 327 indio_dev->modes = INDIO_DIRECT_MODE;
> 328 indio_dev->channels = ad7949_adc_channels;
> 329 spi_set_drvdata(spi, indio_dev);
> 330
> 331 ad7949_adc = iio_priv(indio_dev);
> 332 ad7949_adc->indio_dev = indio_dev;
> 333 ad7949_adc->spi = spi;
> 334
> 335 spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> 336 indio_dev->num_channels = spec->num_channels;
> 337 ad7949_adc->resolution = spec->resolution;
> 338
> 339 /* Set SPI bits per word */
> 340 if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> 341 spi->bits_per_word = ad7949_adc->resolution;
> 342 } else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> 343 spi->bits_per_word = 16;
> 344 } else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> 345 spi->bits_per_word = 8;
> 346 } else {
> 347 dev_err(dev, "unable to find common BPW with spi controller\n");
> 348 return -EINVAL;
> 349 }
> 350
> 351 /* Setup internal voltage reference */
> 352 tmp = 4096000;
> 353 device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> 354
> 355 switch (tmp) {
> 356 case 2500000:
> 357 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
> 358 break;
> 359 case 4096000:
> 360 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
> 361 break;
> 362 default:
> 363 dev_err(dev, "unsupported internal voltage reference\n");
> 364 return -EINVAL;
> 365 }
> 366
> 367 /* Setup external voltage reference, buffered? */
> 368 ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
>
> Apparenty it's really rare to have an optional regulator? This function
> is very tricky. It should return NULL if the option is not like all the
> other optional features in the kernel. But the regulator code is really
> not set up for not having a regulator. If it were then there would be a
> lot of NULL checks in the regulator code. But since it's not then you
> have to add them to the driver instead.
>
> 369 if (IS_ERR(ad7949_adc->vref)) {
> 370 ret = PTR_ERR(ad7949_adc->vref);
> 371 if (ret != -ENODEV)
> 372 return ret;
> 373 /* unbuffered? */
> 374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> 375 if (IS_ERR(ad7949_adc->vref)) {
> 376 ret = PTR_ERR(ad7949_adc->vref);
> 377 if (ret != -ENODEV)
> 378 return ret;
>
> Instead of returning NULL when the regulator is disabled it returns
> -ENODEV. How do you differentiate from an -ENODEV which is caused by an
> error vs an -ENODEV which is caused because the optional regulator is
> disabled? You'll just have to hope that the errors are less common and
> assume it means disabled.
I see.. So far, I've only used fixed-regulators to provide a constant
voltage reference here, and I guess those are quite unlikely to fail.
> You might be doubting that devm_regulator_get_optional() can return
> -ENODEV on error? Look at the code and prepare your heart for sadness.
Thanks for the warning, I see what you meant now.
I wasn't able to use smatch to reproduce the error with the following:
make O=builds/smatch CHECK="~/dev/smatch/smatch -p=kernel" C=1 Image.gz
Would you have any pointer for that?
Anyway, I believe the following would address the error you mentioned.
-- >8 --
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 44bb5fde83de..3613f4e55e1c 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -368,12 +368,14 @@ static int ad7949_spi_probe(struct spi_device *spi)
ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
if (IS_ERR(ad7949_adc->vref)) {
ret = PTR_ERR(ad7949_adc->vref);
+ ad7949_adc->vref = NULL;
if (ret != -ENODEV)
return ret;
/* unbuffered? */
ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
if (IS_ERR(ad7949_adc->vref)) {
ret = PTR_ERR(ad7949_adc->vref);
+ ad7949_adc->vref = NULL;
if (ret != -ENODEV)
return ret;
} else {
-- >8 --
I'd like to be able to reproduce the error to make sure everything is okay but
otherwise I can still send a patch.
Thanks,
Liam
> 379 } else {
> 380 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
> 381 }
> 382 } else {
> 383 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
> 384 }
> 385
> 386 if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
> --> 387 ret = regulator_enable(ad7949_adc->vref);
> ^^^^^^^^^^^^^^^^
> Every reference to ->vref will crash if the optional regulator is
> disabled.
>
> 388 if (ret < 0) {
> 389 dev_err(dev, "fail to enable regulator\n");
> 390 return ret;
> 391 }
> 392
> 393 ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
> 394 ad7949_adc->vref);
> 395 if (ret)
> 396 return ret;
> 397 }
> 398
> 399 mutex_init(&ad7949_adc->lock);
> 400
> 401 ret = ad7949_spi_init(ad7949_adc);
> 402 if (ret) {
> 403 dev_err(dev, "enable to init this device: %d\n", ret);
> 404 return ret;
> 405 }
> 406
> 407 ret = devm_iio_device_register(dev, indio_dev);
> 408 if (ret)
> 409 dev_err(dev, "fail to register iio device: %d\n", ret);
> 410
> 411 return ret;
> 412 }
>
> regards,
> dan carpenter
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] iio: adc: ad7949: add vref selection support
2021-09-22 5:10 ` Liam Beguin
@ 2021-09-22 6:00 ` Dan Carpenter
2021-09-22 14:48 ` Liam Beguin
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-09-22 6:00 UTC (permalink / raw)
To: Liam Beguin; +Cc: lvb, linux-iio
On Wed, Sep 22, 2021 at 01:10:52AM -0400, Liam Beguin wrote:
> >
> > 369 if (IS_ERR(ad7949_adc->vref)) {
> > 370 ret = PTR_ERR(ad7949_adc->vref);
> > 371 if (ret != -ENODEV)
> > 372 return ret;
> > 373 /* unbuffered? */
> > 374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > 375 if (IS_ERR(ad7949_adc->vref)) {
> > 376 ret = PTR_ERR(ad7949_adc->vref);
> > 377 if (ret != -ENODEV)
> > 378 return ret;
> >
> > Instead of returning NULL when the regulator is disabled it returns
> > -ENODEV. How do you differentiate from an -ENODEV which is caused by an
> > error vs an -ENODEV which is caused because the optional regulator is
> > disabled? You'll just have to hope that the errors are less common and
> > assume it means disabled.
>
> I see.. So far, I've only used fixed-regulators to provide a constant
> voltage reference here, and I guess those are quite unlikely to fail.
>
> > You might be doubting that devm_regulator_get_optional() can return
> > -ENODEV on error? Look at the code and prepare your heart for sadness.
>
> Thanks for the warning, I see what you meant now.
>
> I wasn't able to use smatch to reproduce the error with the following:
>
> make O=builds/smatch CHECK="~/dev/smatch/smatch -p=kernel" C=1 Image.gz
>
> Would you have any pointer for that?
This requires building the cross function Database:
~/dev/smatch/smatch_scripts/build_kernel_data.sh
The command takes 5 hours to run so here is a short cut which just
builds the minimum two files:
~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/core.c | tee out
~/dev/smatch/smatch_data/db/create_db.sh -p=kernel out
~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/devres.c | tee out
~/dev/smatch/smatch_data/db/reload_partial.sh out
~/dev/smatch/smatch_scripts/kchecker --spammy drivers/iio/adc/ad7949.c
>
> Anyway, I believe the following would address the error you mentioned.
>
> -- >8 --
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 44bb5fde83de..3613f4e55e1c 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -368,12 +368,14 @@ static int ad7949_spi_probe(struct spi_device *spi)
> ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
> if (IS_ERR(ad7949_adc->vref)) {
> ret = PTR_ERR(ad7949_adc->vref);
> + ad7949_adc->vref = NULL;
This is not required because it will just be reassigned in a couple
lines.
> if (ret != -ENODEV)
> return ret;
> /* unbuffered? */
> ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> if (IS_ERR(ad7949_adc->vref)) {
> ret = PTR_ERR(ad7949_adc->vref);
> + ad7949_adc->vref = NULL;
But this also won't work. Passing a NULL to regulator_enable() will
cause an Oops. All the reference to ->vref need checks. :/
> if (ret != -ENODEV)
> return ret;
> } else {
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iio: adc: ad7949: add vref selection support
2021-09-22 6:00 ` Dan Carpenter
@ 2021-09-22 14:48 ` Liam Beguin
2021-09-23 5:47 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Liam Beguin @ 2021-09-22 14:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: lvb, linux-iio
Hi Dan,
On Wed, Sep 22, 2021 at 09:00:26AM +0300, Dan Carpenter wrote:
> On Wed, Sep 22, 2021 at 01:10:52AM -0400, Liam Beguin wrote:
> > >
> > > 369 if (IS_ERR(ad7949_adc->vref)) {
> > > 370 ret = PTR_ERR(ad7949_adc->vref);
> > > 371 if (ret != -ENODEV)
> > > 372 return ret;
> > > 373 /* unbuffered? */
> > > 374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > 375 if (IS_ERR(ad7949_adc->vref)) {
> > > 376 ret = PTR_ERR(ad7949_adc->vref);
> > > 377 if (ret != -ENODEV)
> > > 378 return ret;
> > >
> > > Instead of returning NULL when the regulator is disabled it returns
> > > -ENODEV. How do you differentiate from an -ENODEV which is caused by an
> > > error vs an -ENODEV which is caused because the optional regulator is
> > > disabled? You'll just have to hope that the errors are less common and
> > > assume it means disabled.
> >
> > I see.. So far, I've only used fixed-regulators to provide a constant
> > voltage reference here, and I guess those are quite unlikely to fail.
> >
> > > You might be doubting that devm_regulator_get_optional() can return
> > > -ENODEV on error? Look at the code and prepare your heart for sadness.
> >
> > Thanks for the warning, I see what you meant now.
> >
> > I wasn't able to use smatch to reproduce the error with the following:
> >
> > make O=builds/smatch CHECK="~/dev/smatch/smatch -p=kernel" C=1 Image.gz
> >
> > Would you have any pointer for that?
>
> This requires building the cross function Database:
>
> ~/dev/smatch/smatch_scripts/build_kernel_data.sh
>
> The command takes 5 hours to run so here is a short cut which just
> builds the minimum two files:
>
> ~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/core.c | tee out
> ~/dev/smatch/smatch_data/db/create_db.sh -p=kernel out
> ~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/devres.c | tee out
> ~/dev/smatch/smatch_data/db/reload_partial.sh out
> ~/dev/smatch/smatch_scripts/kchecker --spammy drivers/iio/adc/ad7949.c
Thanks, I appreciate the shortcuts! I was able to reproduce the error
with these steps.
> > Anyway, I believe the following would address the error you mentioned.
> >
> > -- >8 --
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 44bb5fde83de..3613f4e55e1c 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -368,12 +368,14 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
> > if (IS_ERR(ad7949_adc->vref)) {
> > ret = PTR_ERR(ad7949_adc->vref);
> > + ad7949_adc->vref = NULL;
>
> This is not required because it will just be reassigned in a couple
> lines.
Right, sorry about that.
> > if (ret != -ENODEV)
> > return ret;
> > /* unbuffered? */
> > ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > if (IS_ERR(ad7949_adc->vref)) {
> > ret = PTR_ERR(ad7949_adc->vref);
> > + ad7949_adc->vref = NULL;
>
> But this also won't work. Passing a NULL to regulator_enable() will
> cause an Oops. All the reference to ->vref need checks. :/
I believe it still work since these conditions around
devm_regulator_get_optional() also set ad7949_adc->refsel.
ad7949_adc->refsel is then checked before calling regulator_enable() and
regulator_get_voltage().
Even without the patch, I don't think we can call regulor_enable()
without having it be defined. Am I missing something else?
Thanks,
Liam
> > if (ret != -ENODEV)
> > return ret;
> > } else {
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iio: adc: ad7949: add vref selection support
2021-09-22 14:48 ` Liam Beguin
@ 2021-09-23 5:47 ` Dan Carpenter
2021-09-24 0:21 ` Liam Beguin
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-09-23 5:47 UTC (permalink / raw)
To: Liam Beguin; +Cc: lvb, linux-iio
On Wed, Sep 22, 2021 at 10:48:45AM -0400, Liam Beguin wrote:
> Hi Dan,
>
> On Wed, Sep 22, 2021 at 09:00:26AM +0300, Dan Carpenter wrote:
> > On Wed, Sep 22, 2021 at 01:10:52AM -0400, Liam Beguin wrote:
> > > >
> > > > 369 if (IS_ERR(ad7949_adc->vref)) {
> > > > 370 ret = PTR_ERR(ad7949_adc->vref);
> > > > 371 if (ret != -ENODEV)
> > > > 372 return ret;
> > > > 373 /* unbuffered? */
> > > > 374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > > 375 if (IS_ERR(ad7949_adc->vref)) {
> > > > 376 ret = PTR_ERR(ad7949_adc->vref);
> > > > 377 if (ret != -ENODEV)
> > > > 378 return ret;
> > > >
> > > > Instead of returning NULL when the regulator is disabled it returns
> > > > -ENODEV. How do you differentiate from an -ENODEV which is caused by an
> > > > error vs an -ENODEV which is caused because the optional regulator is
> > > > disabled? You'll just have to hope that the errors are less common and
> > > > assume it means disabled.
> > >
> > > I see.. So far, I've only used fixed-regulators to provide a constant
> > > voltage reference here, and I guess those are quite unlikely to fail.
> > >
> > > > You might be doubting that devm_regulator_get_optional() can return
> > > > -ENODEV on error? Look at the code and prepare your heart for sadness.
> > >
> > > Thanks for the warning, I see what you meant now.
> > >
> > > I wasn't able to use smatch to reproduce the error with the following:
> > >
> > > make O=builds/smatch CHECK="~/dev/smatch/smatch -p=kernel" C=1 Image.gz
> > >
> > > Would you have any pointer for that?
> >
> > This requires building the cross function Database:
> >
> > ~/dev/smatch/smatch_scripts/build_kernel_data.sh
> >
> > The command takes 5 hours to run so here is a short cut which just
> > builds the minimum two files:
> >
> > ~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/core.c | tee out
> > ~/dev/smatch/smatch_data/db/create_db.sh -p=kernel out
> > ~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/devres.c | tee out
> > ~/dev/smatch/smatch_data/db/reload_partial.sh out
> > ~/dev/smatch/smatch_scripts/kchecker --spammy drivers/iio/adc/ad7949.c
>
> Thanks, I appreciate the shortcuts! I was able to reproduce the error
> with these steps.
>
> > > Anyway, I believe the following would address the error you mentioned.
> > >
> > > -- >8 --
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 44bb5fde83de..3613f4e55e1c 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -368,12 +368,14 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > > ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
> > > if (IS_ERR(ad7949_adc->vref)) {
> > > ret = PTR_ERR(ad7949_adc->vref);
> > > + ad7949_adc->vref = NULL;
> >
> > This is not required because it will just be reassigned in a couple
> > lines.
>
> Right, sorry about that.
>
> > > if (ret != -ENODEV)
> > > return ret;
> > > /* unbuffered? */
> > > ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > if (IS_ERR(ad7949_adc->vref)) {
> > > ret = PTR_ERR(ad7949_adc->vref);
> > > + ad7949_adc->vref = NULL;
> >
> > But this also won't work. Passing a NULL to regulator_enable() will
> > cause an Oops. All the reference to ->vref need checks. :/
>
> I believe it still work since these conditions around
> devm_regulator_get_optional() also set ad7949_adc->refsel.
>
> ad7949_adc->refsel is then checked before calling regulator_enable() and
> regulator_get_voltage().
>
> Even without the patch, I don't think we can call regulor_enable()
> without having it be defined. Am I missing something else?
Actually, you're right. This warning is a 100% false positive. Smatch
doesn't handle bit wise tests very well. I've been meaning to write
that code but I haven't done it yet. When I do the false positive will
go away.
Sorry for the noise on this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] iio: adc: ad7949: add vref selection support
2021-09-23 5:47 ` Dan Carpenter
@ 2021-09-24 0:21 ` Liam Beguin
0 siblings, 0 replies; 6+ messages in thread
From: Liam Beguin @ 2021-09-24 0:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: lvb, linux-iio
On Thu, Sep 23, 2021 at 08:47:52AM +0300, Dan Carpenter wrote:
> > > > if (ret != -ENODEV)
> > > > return ret;
> > > > /* unbuffered? */
> > > > ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > > if (IS_ERR(ad7949_adc->vref)) {
> > > > ret = PTR_ERR(ad7949_adc->vref);
> > > > + ad7949_adc->vref = NULL;
> > >
> > > But this also won't work. Passing a NULL to regulator_enable() will
> > > cause an Oops. All the reference to ->vref need checks. :/
> >
> > I believe it still work since these conditions around
> > devm_regulator_get_optional() also set ad7949_adc->refsel.
> >
> > ad7949_adc->refsel is then checked before calling regulator_enable() and
> > regulator_get_voltage().
> >
> > Even without the patch, I don't think we can call regulor_enable()
> > without having it be defined. Am I missing something else?
Hi Dan,
> Actually, you're right. This warning is a 100% false positive. Smatch
> doesn't handle bit wise tests very well. I've been meaning to write
> that code but I haven't done it yet. When I do the false positive will
> go away.
>
> Sorry for the noise on this.
No worries, thanks for your support on this.
> regards,
> dan carpenter
Liam
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-24 0:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 6:35 [bug report] iio: adc: ad7949: add vref selection support Dan Carpenter
2021-09-22 5:10 ` Liam Beguin
2021-09-22 6:00 ` Dan Carpenter
2021-09-22 14:48 ` Liam Beguin
2021-09-23 5:47 ` Dan Carpenter
2021-09-24 0:21 ` Liam Beguin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).