Re: [bug report] iio: adc: ad7949: add vref selection support

[Liam Beguin <liambeguin@gmail.com>]

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