* [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.