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: <20120125115705.GF3687@opensource.wolfsonmicro.com>
Date:	Wed, 25 Jan 2012 11:57:05 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Sylwester Nawrocki <s.nawrocki@...sung.com>
Cc:	lrg@...com, linux-kernel@...r.kernel.org, m.szyprowski@...sung.com,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Steve Glendinning <steve.glendinning@...c.com>,
	Richard Purdie <rpurdie@...ys.net>,
	Timur Tabi <timur@...escale.com>,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH/RFC] regulator: Reverse the disable sequence in
 regulator_bulk_disable()

On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:

> Often there is a need for disabling a set of regulators in order opposite
> to the enable order. Currently the function regulator_bulk_disable() walks
> list of regulators in same order as regulator_bulk_enable(). This may cause
> trouble, especially for devices with mixed analogue and digital circuits.
> So reverse the disabling sequence of regulator_bulk_disable().
> While at it, also correct the comment.

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.

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?

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

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

>  - re-implement it in modules that need the order reversed; it is not
>    really helpful in practice since such code would have to be repeated
>    in multiple modules;

>  - create new function, e.g. regulator_bulk_disable_reversed() with the
>    order reversed - not sure if it is not an overkill though;

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.  Typically this will mean either a few
sets of bulk supplies or a single set of bulk supplies and then some
number of individual supplies.  An awful lot of devices don't have any
sequencing constraints at all, apparently including most of those using
the API at present.

BTW, your CC list here is *really* random - please think more about who
you're CCing, it looks like you've done something with get_maintainer.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)