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]

Date:	Wed, 25 Jan 2012 13:44:06 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Bill Gatliff <bgat@...lgatliff.com>
Cc:	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC] regulator: Reverse the disable sequence in
 regulator_bulk_disable()

On Wed, Jan 25, 2012 at 02:35:25PM +0100, Bill Gatliff wrote:
> On Wed, Jan 25, 2012 at 12:57 PM, Mark Brown
> > On Wed, Jan 25, 2012 at 12:35:38PM +0100, Sylwester Nawrocki wrote:

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

> Presumably, then, the notifier mechanism will remain positive
> confirmation that the associated regulator has indeed come up?  And

The notifiers are called after the enable has finished, yes.

> that multithreading thing will be interesting to implement given
> parent-child relationships of regulator sources at times...

This multithreading thing is already implemented, and has been since
v3.1.  It's pretty trivial, it just fires up a bunch of threads in
parallel to call the underlying regulator_enable() API which already
needs locking to handle concurrent users anyway.

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

I'd imagine so, that's one of the common cases for requiring sequencing,
but if the driver is using the bulk APIs for this then it's buggy.

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

I can imagine a bulk set_voltage() that applied constraints and unwound
things when they went wrong but actually working out the values doesn't
feel generic.

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

Right, that's why I applied the patch - I'm just saying we might not
actually wind up implementing that ordering due to the concurrency.

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