Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572017 > unrolled thread

regulator_get_optional() no longer returning NULL?

Back to article view | Back to linux.kernel

Contents

  regulator_get_optional() no longer returning NULL? Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 23:30 +0100
    Re: regulator_get_optional() no longer returning NULL? Mark Brown <broonie@kernel.org> - 2017-02-03 12:30 +0100
      Re: regulator_get_optional() no longer returning NULL? Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 23:00 +0100
        Re: regulator_get_optional() no longer returning NULL? Mark Brown <broonie@kernel.org> - 2017-02-04 11:10 +0100

#1572017 — regulator_get_optional() no longer returning NULL?

Hi Mark,

It appears that [devm_]regulator_get_optional() and
[devm_]gpiod_get_optional() behave irritatingly differently. While the
latter returns NULL for non-existing GPIOs, the former started returning
-ENODEV instead of NULL, starting with commit below.

Why did we do that? It is much more convenient to write:

	data->vcc = devm_regulator_get_optional(dev. "vcc");
	if (IS_ERR(data->vcc))
		return ERR_PTR(data->vcc);
	...
	if (data->vcc)
		do_stuff();

vs.

	data->vcc = devm_regulator_get_optional(dev. "vcc");
	if (IS_ERR(data->vcc)) {
		error = ERR_PTR(data->vcc);
		if (error != -ENODEV && error != <list of vetted codes>)
			return error;

		data->vcc = NULL;
	}

I.e. it is nice to treat *all* codes returned by
devm_regulator_get_optional() as fatal and NULL as special instead of
vetting by hand (and having chance that list of vetted codes will bit
rot).

Can we please revert this patch?

Thanks!

-- 
Dmitry

commit ef60abbb6b406389245225ab4acfe73f66e7d92c
Author: Mark Brown <broonie@linaro.org>
Date:   Mon Sep 23 16:12:52 2013 +0100

    regulator: core: Always use return value when regulator_dev_lookup() fails

    Ensure that the return value is always set when we return now that the
    logic has changed for regulator_get_optional() so we don't get missing
    codes leaking out.

    Reported-by: Thierry Reding <treding@nvidia.com>
    Tested-by: Thierry Reding <treding@nvidia.com>
    Signed-off-by: Mark Brown <broonie@linaro.org>

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 088b41ac9506..a40055edaae4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1263,12 +1263,13 @@ static struct regulator *_regulator_get(struct
device *dev, const char *id,
        if (rdev)
                goto found;
 
+       regulator = ERR_PTR(ret);
+
        /*
         * If we have return value from dev_lookup fail, we do not
         * expect to
         * succeed, so, quit with appropriate error value
         */
        if (ret && ret != -ENODEV) {
-               regulator = ERR_PTR(ret);
                goto out;
        }
 

[toc] | [next] | [standalone]

#1573037

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 01, 2017 at 02:23:31PM -0800, Dmitry Torokhov wrote:

> It appears that [devm_]regulator_get_optional() and
> [devm_]gpiod_get_optional() behave irritatingly differently. While the
> latter returns NULL for non-existing GPIOs, the former started returning
> -ENODEV instead of NULL, starting with commit below.

> Why did we do that? It is much more convenient to write:

As the commit description for that commit says it's fixing a bug with us
not returning the error code we get from regulator_dev_lookup().

> I.e. it is nice to treat *all* codes returned by
> devm_regulator_get_optional() as fatal and NULL as special instead of
> vetting by hand (and having chance that list of vetted codes will bit
> rot).

There's no real risk of bitrot I can see here, the only thing something
using optional regulator is looking for is that the regulator does not
exist and we have the specific error code -ENODEV for that.

> Can we please revert this patch?

That won't do what you want so I don't know why you're asking for it -
the default value of regulator is ERR_PTR(-EPROBE_DEFER) so if we skip
assigning the actual return code you'll get that and not NULL.  I don't
recall that behaviour ever working, it's not what's documented so it
doesn't look like something broke here.  If you wanted to implement it
you'd need to add new code to do it (eg, squash down -ENODEV in
_get_optional()).

However I am a bit dubious about doing that as I'm not thrilled with
adding in more things for people to check.  Even if you check for NULL
with an optional regulator you'd still need to also go and check that
you didn't get an error code like -EPROBE_DEFER, you'd only get a NULL
in cases where the regulator does not exist.  Given that I'm not sure
that it's actually simplifying things.

[toc] | [prev] | [next] | [standalone]

#1573475

On Fri, Feb 03, 2017 at 12:20:01PM +0100, Mark Brown wrote:
> On Wed, Feb 01, 2017 at 02:23:31PM -0800, Dmitry Torokhov wrote:
> 
> > It appears that [devm_]regulator_get_optional() and
> > [devm_]gpiod_get_optional() behave irritatingly differently. While the
> > latter returns NULL for non-existing GPIOs, the former started returning
> > -ENODEV instead of NULL, starting with commit below.
> 
> > Why did we do that? It is much more convenient to write:
> 
> As the commit description for that commit says it's fixing a bug with us
> not returning the error code we get from regulator_dev_lookup().
> 
> > I.e. it is nice to treat *all* codes returned by
> > devm_regulator_get_optional() as fatal and NULL as special instead of
> > vetting by hand (and having chance that list of vetted codes will bit
> > rot).
> 
> There's no real risk of bitrot I can see here, the only thing something
> using optional regulator is looking for is that the regulator does not
> exist and we have the specific error code -ENODEV for that.
> 
> > Can we please revert this patch?
> 
> That won't do what you want so I don't know why you're asking for it -
> the default value of regulator is ERR_PTR(-EPROBE_DEFER) so if we skip
> assigning the actual return code you'll get that and not NULL.  I don't
> recall that behaviour ever working, it's not what's documented so it
> doesn't look like something broke here.  If you wanted to implement it
> you'd need to add new code to do it (eg, squash down -ENODEV in
> _get_optional()).

Yes, you are right. The _regulator_get() is a bit confusing, I have a
patch series that hopefully makes it a bit more straightforward.

Unfortunately it seems some of the drivers are not ready for
regulator_get_optional() to return NULL, I'll have to prepare them
first.

> 
> However I am a bit dubious about doing that as I'm not thrilled with
> adding in more things for people to check.  Even if you check for NULL
> with an optional regulator you'd still need to also go and check that
> you didn't get an error code like -EPROBE_DEFER, you'd only get a NULL
> in cases where the regulator does not exist.  Given that I'm not sure
> that it's actually simplifying things.

Like I said, I want people to simply check for error/!error and have all
errors be fatal, and the rest of the checks (when trying to use said
regulator) should be in form of:

	if (blah->supply)
		regulator_enable(supply).

Or even teach regulator_enable() to like NULLs.

Allowing regulator_get_optional() return NULL will make it work
similarly to gpiod_get_optional() which I think is a good thing.

By the way, I see quite a few drivers using regulator_get_optional() for
cases when regulator is mandatory, but it might be handled by the
firmware. I.e. it seems they can be converted to regulator_get() with
the expectation that it will give us dummy supply. Would such
conversions be OK/accepted?

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1573633

[Multipart message — attachments visible in raw view] — view raw

On Fri, Feb 03, 2017 at 01:51:20PM -0800, Dmitry Torokhov wrote:

> Like I said, I want people to simply check for error/!error and have all
> errors be fatal, and the rest of the checks (when trying to use said
> regulator) should be in form of:

> 	if (blah->supply)
> 		regulator_enable(supply).

I am very dubious about this sort of pattern, it's fairly rare to get
optional supplies at all and when they're there they tend to be more
clearly described at a higher level than just checking for the
regulator.

> Or even teach regulator_enable() to like NULLs.

We definitely should not do this, we already have far too many problems
with people not writing appropriate error handling code and this just
encourages the "we got an error, set the regulator to NULL" behaviour
that seems to go along with this.  For most of the reasonable use cases
we need explicit handling for the case where the regulator isn't there
so it's not particularly optimising anything.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web