iio: fetch and enable regulators unconditionally
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.
406 } 407 408 ret = ms5611_reset(indio_dev); 409 if (ret < 0) 410 goto err_regulator_disable; 411 412 ret = ms5611_read_prom(indio_dev); 413 if (ret < 0) 414 goto err_regulator_disable; 415 416 return 0; 417 418 err_regulator_disable: 419 regulator_disable(st->vdd); 420 return ret; 421 }
regards,
dan carpenter
Hello Dan,
Quoted 29 lines · Dan Carpenter
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 intms5611_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;
Quoted 2 lines
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?
Quoted 20 lines
406 } 407 408 ret = ms5611_reset(indio_dev); 409 if (ret < 0) 410 goto err_regulator_disable; 411 412 ret = ms5611_read_prom(indio_dev); 413 if (ret < 0) 414 goto err_regulator_disable; 415 416 return 0; 417 418 err_regulator_disable: 419 regulator_disable(st->vdd); 420 return ret; 421 }regards,
dan carpenter
Quoted 35 lines · Crt Mori
Hello Dan,
Quoted 29 lines · Dan Carpenter
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 intms5611_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;Quoted 2 lines
You probably want to update this chunk as well? Otherwise static checkers think we're dereferencing
-ENODEV.
Quoted 6 lines
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->vddpointer 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.
Quoted 51 lines · Lars-Peter Clausen
Quoted 35 lines · Crt Mori
Hello Dan,
Quoted 29 lines · Dan Carpenter
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 intms5611_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;Quoted 2 lines
You probably want to update this chunk as well? Otherwise static checkers think we're dereferencing
-ENODEV.Quoted 6 lines
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->vddpointer 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 calledst->vddis passed toregulator_disable()which will crash if it is passed-ENODEVinstead of a valid pointer.I think the ENODEV check should be dropped and the whole thing should be turned into
Quoted 7 lines
if (
IS_ERR(st->vdd))retrurn PTR_ERR(st->vdd))ret =
regulator_enable()
...This is the behavior that we expect for normal supply regulators.
This would then make us completely dependent on this behaviour:
If the device tree or board file does not define suitable regulators for the component, it will be substituted by a dummy regulator, or, if regulators are disabled altogether, by stubs.
which I read as that it will not return ENODEV anyway, so we are safe to assume something else went wrong whenever error is returned? I have added Linus Wallerij to the discussion as might also be valid for his solution?
Quoted 49 lines · Crt Mori
Quoted 48 lines · Lars-Peter Clausen
Quoted 35 lines · Crt Mori
Hello Dan,
Quoted 29 lines · Dan Carpenter
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 intms5611_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;Quoted 2 lines
You probably want to update this chunk as well? Otherwise static checkers think we're dereferencing
-ENODEV.Quoted 6 lines
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->vddpointer 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 calledst->vddis passed toregulator_disable()which will crash if it is passed-ENODEVinstead of a valid pointer.
Quoted 7 lines
Quoted 7 lines
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()
Agree.
Quoted 8 lines
Quoted 3 lines
...
This is the behavior that we expect for normal supply regulators.
This would then make us completely dependent on this behaviour:
If the device tree or board file does not define suitable regulators for the component, it will be substituted by a dummy regulator, or, if regulators are disabled altogether, by stubs.
Quoted 4 lines
which I read as that it will not return ENODEV anyway, so we are safe to assume something else went wrong whenever error is returned? I have added Linus Wallerij to the discussion as might also be valid for his solution?
Just handle it the most straight forward way:
data->vdd = devm_regulator_get(dev, "vdd");
if (IS_ERR(data->vdd))
return PTR_ERR(data->vdd);
ret = regulator_enable(data->vdd);
if (ret) {
dev_warn(dev,
"Failed to enable specified Vdd supply\n");
return ret;
}The same in reverse order mutatis mutandis for
taking it down.
- If there is a regulator, it will be handled
- If there are dummies, they will be used yielding NO-OPs
- If regulators are compiled out it will be a static inline NO-OP
Everything else is the errorpath and you should just return with an error.
If the regulator is optional - i.e. if the *hardware* regulator is optional, i.e. if it is optional to supply power on that line at all - that is another thing. We don't do "software optional" which is a common misconception around regulators. If it is *really* hardware-optional to supply this voltage, then use regulator_get_optional() and ONLY in that case you may get a -ENODEV back, that you need to handle.
Yours,
Linus Walleij