On 14 October 2016 at 11:01, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 10/13/2016 10:25 AM, Crt Mori wrote:
>> Hello Dan,
>>
>> On 12 October 2016 at 08:07, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
