Re: [bug report] iio: fetch and enable regulators unconditionally

[Lars-Peter Clausen <lars@metafoo.de>]

On 10/13/2016 10:25 AM, Crt Mori wrote:
> Hello Dan,
> 
> On 12 October 2016 at 08:07, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> Hello Crt Mori,
>>
>> The patch 67516074884b: "iio: fetch and enable regulators
>> unconditionally" from Sep 5, 2016, leads to the following static
>> checker warning:
>>
>>         drivers/iio/pressure/ms5611_core.c:419 ms5611_init()
>>         error: 'st->vdd' dereferencing possible ERR_PTR()
>>
>> drivers/iio/pressure/ms5611_core.c
>>    388  static int ms5611_init(struct iio_dev *indio_dev)
>>    389  {
>>    390          int ret;
>>    391          struct ms5611_state *st = iio_priv(indio_dev);
>>    392
>>    393          /* Enable attached regulator if any. */
>>    394          st->vdd = devm_regulator_get(indio_dev->dev.parent, "vdd");
>>    395          if (!IS_ERR(st->vdd)) {
>>    396                  ret = regulator_enable(st->vdd);
>>    397                  if (ret) {
>>    398                          dev_err(indio_dev->dev.parent,
>>    399                                  "failed to enable Vdd supply: %d\n", ret);
>>    400                          return ret;
>>    401                  }
>>    402          } else {
>>    403                  ret = PTR_ERR(st->vdd);
>>    404                  if (ret != -ENODEV)
>>    405                          return ret;
>>
>> You probably want to update this chunk as well?  Otherwise static
>> checkers think we're dereferencing -ENODEV.
> 
> Can you explain a bit more why they would think of it like that and
> what would better proposal be like?
> 
> ret is a return value and it is st->vdd pointer is checked for error
> before we deduct error information out, so I do not see where
> dereferencing takes part?

The thing is, if PTR_ERR(st->vdd) is -ENODEV, we do not return an error and
just continue in the probe function. And then if one of the cleanup
functions is called st->vdd is passed to regulator_disable() which will
crash if it is passed -ENODEV instead of a valid pointer.

I think the ENODEV check should be dropped and the whole thing should be
turned into

if (IS_ERR(st->vdd))
	retrurn PTR_ERR(st->vdd))

ret = regulator_enable()
...

This is the behavior that we expect for normal supply regulators.