On Fri, Oct 14, 2016 at 1:17 PM, Crt Mori <cmo@xxxxxxxxxxx> wrote:
> 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()
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
--
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
