* [bug report] iio: fetch and enable regulators unconditionally
@ 2016-10-12 6:07 Dan Carpenter
2016-10-13 8:25 ` Crt Mori
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2016-10-12 6:07 UTC (permalink / raw)
To: cmo; +Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] iio: fetch and enable regulators unconditionally 2016-10-12 6:07 [bug report] iio: fetch and enable regulators unconditionally Dan Carpenter @ 2016-10-13 8:25 ` Crt Mori 2016-10-14 9:01 ` Lars-Peter Clausen 0 siblings, 1 reply; 5+ messages in thread From: Crt Mori @ 2016-10-13 8:25 UTC (permalink / raw) To: Dan Carpenter Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Linux Iio 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? > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] iio: fetch and enable regulators unconditionally 2016-10-13 8:25 ` Crt Mori @ 2016-10-14 9:01 ` Lars-Peter Clausen 2016-10-14 11:17 ` Crt Mori 0 siblings, 1 reply; 5+ messages in thread From: Lars-Peter Clausen @ 2016-10-14 9:01 UTC (permalink / raw) To: Crt Mori, Dan Carpenter; +Cc: Hartmut Knaack, Peter Meerwald-Stadler, Linux Iio 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] iio: fetch and enable regulators unconditionally 2016-10-14 9:01 ` Lars-Peter Clausen @ 2016-10-14 11:17 ` Crt Mori 2016-10-14 13:10 ` Linus Walleij 0 siblings, 1 reply; 5+ messages in thread From: Crt Mori @ 2016-10-14 11:17 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Dan Carpenter, Hartmut Knaack, Peter Meerwald-Stadler, Linux Iio, Linus Walleij On 14 October 2016 at 11:01, Lars-Peter Clausen <lars@metafoo.de> wrote: > 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. 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] iio: fetch and enable regulators unconditionally 2016-10-14 11:17 ` Crt Mori @ 2016-10-14 13:10 ` Linus Walleij 0 siblings, 0 replies; 5+ messages in thread From: Linus Walleij @ 2016-10-14 13:10 UTC (permalink / raw) To: Crt Mori Cc: Lars-Peter Clausen, Dan Carpenter, Hartmut Knaack, Peter Meerwald-Stadler, Linux Iio, Mark Brown On Fri, Oct 14, 2016 at 1:17 PM, Crt Mori <cmo@melexis.com> wrote: > On 14 October 2016 at 11:01, Lars-Peter Clausen <lars@metafoo.de> wrote: >> 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() Agree. >> ... >> >> 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? 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-14 13:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-12 6:07 [bug report] iio: fetch and enable regulators unconditionally Dan Carpenter 2016-10-13 8:25 ` Crt Mori 2016-10-14 9:01 ` Lars-Peter Clausen 2016-10-14 11:17 ` Crt Mori 2016-10-14 13:10 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.