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

[Dan Carpenter <dan.carpenter@oracle.com>]

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