linux-kernel - Re: regulator_get_optional() no longer returning NULL?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170203215120.GA22933@dtor-ws>
Date:   Fri, 3 Feb 2017 13:51:20 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Thierry Reding <treding@...dia.com>,
        Jelle van der Waa <jelle@...aa.nl>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: regulator_get_optional() no longer returning NULL?

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