linux-kernel - Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk

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: <CADkCAuudPtdb04HXMgHzgDpFWrr9xPJu3yMRe9pE4YDLD+_cUw@mail.gmail.com>
Date:	Wed, 25 Jan 2012 14:35:25 +0100
From:	Bill Gatliff <bgat@...lgatliff.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC] regulator: Reverse the disable sequence in regulator_bulk_disable()

Guys:

On Wed, Jan 25, 2012 at 12:57 PM, Mark Brown
<broonie@...nsource.wolfsonmicro.com> wrote:
> On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:
>
>
> So, I've applied this since it shouldn't do any harm and probably is
> more what we meant to do but note that the bulk APIs don't make any
> guarantees about ordering - in particular when we do the enable we fire
> off a bunch of threads to bring the regulators up in parallel so the
> ordering really is going to be unreliable as it depends on the scheduler
> and the rates at which the various regulators ramp.  This is done so
> that we can enable faster as we don't have to wait for each regulator to
> ramp in series.

Presumably, then, the notifier mechanism will remain positive
confirmation that the associated regulator has indeed come up?  And
that multithreading thing will be interesting to implement given
parent-child relationships of regulator sources at times...

> Whatever driver inspired you to submit this change is therefore probably
> buggy or fragile at the minute - is it something that's in mainline or
> next right now?

I think he's probably dealing with a "VLOGIC <= VDD"-type requirement,
which isn't uncommon.  I'm dealing with it myself right now, actually.

In addition to the sequencing that imposes, it also has implications
for recalculating VLOGIC when VDD changes--- which means a notifier
somewhere.  I might be convinced that implementing this logic inside
the API itself might be of benefit, though I don't see right now how
to do it in a clear and generic way.

> At some point I'd like to enhance things further so we can coalesce
> register writes where multiple regulators have their enable bits in the
> same register but that's a relatively large amount of work for a small
> benefit unless we do something cute with regmap (and that is likely to
> be too cute).

That sounds awfully cute for such marginal benefit.  :)  I really like
the correct-by-inspection-ness of the current implementation.

>> The alternatives to directly modifying regulator_bulk_disable() could be:

I really don't have a problem with the disable order being the reverse
of the enable order, as it generally follows common sense for people
who work with e.g. multiple mutexes.  I kind of would have expected it
to be that way, actually.

> The third option is that where devices really care about the power
> sequencing they should explicitly write that in code and only use the
> bulk APIs where they don't care.

This is probably the best way, since it keeps the "care about" part in
front of the author's eyes and implements it in the chip-specific
code, where it is immune from changes in how the regulator API might
behave in the future.  Changes like the one we are discussing, for
example. :)

b.g.
-- 
Bill Gatliff
bgat@...lgatliff.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/