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