[patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Fix some refcounting issues in the regulator framework, supporting
regulator_disable() for regulators that were enabled at boot time
via machine constraints:

 - Update those regulators' usecounts after enabling, so they
   can cleanly be disabled at that level.

 - Remove the problematic per-consumer usecount, so there's
   only one level of enable/disable.

Buggy consumers could notice different bug symptoms.  The main
example would be refcounting bugs; also, any (out-of-tree) users
of the experimental regulator_set_optimum_mode() stuff which
don't call it when they're done using a regulator.

This is a net minor codeshrink.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Against the regulator-next tree; mainline has similar issues.

 drivers/regulator/core.c |   30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -52,7 +52,6 @@ struct regulator {
 	int uA_load;
 	int min_uV;
 	int max_uV;
-	int enabled; /* count of client enables */
 	char *supply_name;
 	struct device_attribute dev_attr;
 	struct regulator_dev *rdev;
@@ -811,6 +810,7 @@ static int set_machine_constraints(struc
 			rdev->constraints = NULL;
 			goto out;
 		}
+		rdev->use_count = 1;
 	}
 
 	print_constraints(rdev);
@@ -1066,10 +1066,6 @@ void regulator_put(struct regulator *reg
 	mutex_lock(&regulator_list_mutex);
 	rdev = regulator->rdev;
 
-	if (WARN(regulator->enabled, "Releasing supply %s while enabled\n",
-			       regulator->supply_name))
-		_regulator_disable(rdev);
-
 	/* remove any sysfs entries */
 	if (regulator->dev) {
 		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
@@ -1144,12 +1140,7 @@ int regulator_enable(struct regulator *r
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	if (regulator->enabled == 0)
-		ret = _regulator_enable(rdev);
-	else if (regulator->enabled < 0)
-		ret = -EIO;
-	if (ret == 0)
-		regulator->enabled++;
+	ret = _regulator_enable(rdev);
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
@@ -1160,6 +1151,11 @@ static int _regulator_disable(struct reg
 {
 	int ret = 0;
 
+	if (WARN(rdev->use_count <= 0,
+			"unbalanced disables for %s\n",
+			rdev->desc->name))
+		return -EIO;
+
 	/* are we the last user and permitted to disable ? */
 	if (rdev->use_count == 1 && !rdev->constraints->always_on) {
 
@@ -1208,16 +1204,7 @@ int regulator_disable(struct regulator *
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	if (regulator->enabled == 1) {
-		ret = _regulator_disable(rdev);
-		if (ret == 0)
-			regulator->uA_load = 0;
-	} else if (WARN(regulator->enabled <= 0,
-			"unbalanced disables for supply %s\n",
-			regulator->supply_name))
-		ret = -EIO;
-	if (ret == 0)
-		regulator->enabled--;
+	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
@@ -1264,7 +1251,6 @@ int regulator_force_disable(struct regul
 	int ret;
 
 	mutex_lock(&regulator->rdev->mutex);
-	regulator->enabled = 0;
 	regulator->uA_load = 0;
 	ret = _regulator_force_disable(regulator->rdev);
 	mutex_unlock(&regulator->rdev->mutex);

[patch 2.6.29-rc7 regulator-next] regulator: init fixes

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Make the regulator setup code cope more consistently with
regulators undesirably left enabled by the bootloader.

Building on the previous refcount patch:

 * Unless the "boot_on" or "always_on" machine constraints
   were set, disable() the regulator.  This gives drivers
   a clean start state:  enable state matches usecount,
   regardless of boot_on/always_on flag state.

 * To help make some integration stages easier, add a new
   "devmode" machine constraint where state the bootloader
   left isn't touched, but enable state and usecount may
   not match.  (System boots but some drivers act odd ...
   debuggable.  System dies part way through booting ...
   often painful.)

Consider a bootloader that leaves an MMC/SD regulator active
when it loads Linux from an SD card.  It may take some time
before the MMC/SD driver gets loaded, if ever ... to save
power, that (LDO) regulator should be disabled ASAP.  Then
later when the MMC driver starts up, the Linux MMC stack will
need to start from a "power off" state.  It can't just

	if (regulator_is_enabled(r))
		regulator_disable(r);

unless enable state and usecount are matched ... but without
this patch, they *will* be mismatched whenever the bootloader
happens to have left that regulator active!  Similar issues
can crop up with almost any regulator.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/core.c          |   47 +++++++++++++++++++++++++++++-------
 include/linux/regulator/machine.h |    3 +-
 2 files changed, 40 insertions(+), 10 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -799,18 +799,47 @@ static int set_machine_constraints(struc
 		}
 	}
 
-	/* If the constraints say the regulator should be on at this point
-	 * and we have control then make sure it is enabled.
+	/* During integration, developers may need time to sort out what
+	 * to do with this regulator; leave the bootloader's setting alone.
+	 * Regulator consumers won't get consistent behavior.
+	 *
+	 * Else the constraints say whether it should be on or off; we
+	 * don't leave it in an unknown state.
 	 */
-	if ((constraints->always_on || constraints->boot_on) && ops->enable) {
-		ret = ops->enable(rdev);
-		if (ret < 0) {
-			printk(KERN_ERR "%s: failed to enable %s\n",
-			       __func__, name);
-			rdev->constraints = NULL;
-			goto out;
+	if (constraints->devmode) {
+		char *label = "unknown";
+
+		if (ops->is_enabled) {
+			ret = ops->is_enabled(rdev);
+			if (ret == 0)
+				label = "disabled";
+			else if (ret > 0)
+				label = "enabled";
+			ret = 0;
+		}
+		pr_warning("%s: devmode regulator '%s' state is '%s'\n",
+			       __func__, name, label);
+	} else if (constraints->always_on || constraints->boot_on) {
+		if (ops->enable) {
+			ret = ops->enable(rdev);
+			if (ret < 0) {
+				pr_err("%s: failed enabling %s\n",
+				       __func__, name);
+				rdev->constraints = NULL;
+				goto out;
+			}
 		}
 		rdev->use_count = 1;
+	} else {
+		if (ops->disable) {
+			ret = ops->disable(rdev);
+			if (ret < 0) {
+				pr_err("%s: failed disabling %s\n",
+				       __func__, name);
+				rdev->constraints = NULL;
+				goto out;
+			}
+		}
 	}
 
 	print_constraints(rdev);
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -117,7 +117,8 @@ struct regulation_constraints {
 	/* mode to set on startup */
 	unsigned int initial_mode;
 
-	/* constriant flags */
+	/* constraint flags */
+	unsigned devmode:1;	/* state after setup is indeterminate */
 	unsigned always_on:1;	/* regulator never off when system is on */
 	unsigned boot_on:1;	/* bootloader/firmware enabled regulator */
 	unsigned apply_uV:1;	/* apply uV constraint iff min == max */

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[Mark Brown]

On Wed, Mar 11, 2009 at 04:43:34PM -0800, David Brownell wrote:

> Buggy consumers could notice different bug symptoms.  The main
> example would be refcounting bugs; also, any (out-of-tree) users
> of the experimental regulator_set_optimum_mode() stuff which
> don't call it when they're done using a regulator.

I'm OK with this from a code point of view so

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

However any consumers that take advantage of this won't be able to
safely share a regulator without extra work since they have no way of
telling why a regulator is in the state that it's in without extra
stuff.  We should probably have something along the lines of a
regulator_get_exclusive() for them.  Previously the consumer counting
would have stopped them interfering with enables done by other
consumers.

There will be other consumers that can't safely share a regulator anyway
(eg, requiring additional code to notice and handle voltage changes) so
it'd be a good thing to have.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[Liam Girdwood]

On Wed, 2009-03-11 at 16:43 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Fix some refcounting issues in the regulator framework, supporting
> regulator_disable() for regulators that were enabled at boot time
> via machine constraints:
> 
>  - Update those regulators' usecounts after enabling, so they
>    can cleanly be disabled at that level.
> 
>  - Remove the problematic per-consumer usecount, so there's
>    only one level of enable/disable.
> 
> Buggy consumers could notice different bug symptoms.  The main
> example would be refcounting bugs; also, any (out-of-tree) users
> of the experimental regulator_set_optimum_mode() stuff which
> don't call it when they're done using a regulator.
> 
> This is a net minor codeshrink.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---

Applied.

Thanks

Liam

Re: [patch 2.6.29-rc7 regulator-next] regulator: init fixes

[Mark Brown]

On Wed, Mar 11, 2009 at 06:32:15PM -0800, David Brownell wrote:

> Make the regulator setup code cope more consistently with
> regulators undesirably left enabled by the bootloader.

First up I'd just like to make it absolutely clear that I agree that
this is a feature we should have - it's obviously useful.

>  * Unless the "boot_on" or "always_on" machine constraints
>    were set, disable() the regulator.  This gives drivers
>    a clean start state:  enable state matches usecount,
>    regardless of boot_on/always_on flag state.

At the minute the regulator constraints have the property that if you
pass a zero-initialised set of constraints the regulator API will not do
anything other than allow you to read the state of the regulator - any
action taken by the regulator API must be explicitly permitted by code.
This change will mean that users will have to explicitly mark any
regulators that are needed as enabled or we'll do unfortunate things.  

It is a particular problem for multi-function devices like pcf50633
which not only register all their regulators by default but also embed
constraints within the general pcf50633 platform data.  If the user
simply turns on the regulator driver in their config they'll get this
behavior if they don't edit the code.  Even with regulator code I'd not
be surprised if people were bitten by this for things like the memory or
a CPU core without regulator based DVFS.

You've addressed some of the use cases for this by providing devmode but
it's still a big incompatible change, especially at this point in the
development cycle where we've got at most a couple of weeks to the merge
window.  We could do this without introducing the incompatibility by
adding a new flag (eg, boot_off) which machine constraints need to
explicitly set to enforce this behaviour or by having something machine
drivers can call to say "now power off any regulators that look idle".

I'm not 100% agaist doing things the way you suggest since there is 
appeal in having boot_on be more of a boolean but I do feel that if
we're going to go this way we should do it more gently.  For example, we
could merge a patch now which warns loudly that this will happen and
then after the 2.6.30 merge window actually implement it.  This would
reduce merge issues for machine support coming in via other trees; we're
rather late in the development cycle to be putting this in right now.
Not everyone will read a warning but there is some chance they might and
there's also a chance they'll test in -next.  We should also provide a
temporary Kconfig which actually enables the behaviour if people request
it.

>  * To help make some integration stages easier, add a new
>    "devmode" machine constraint where state the bootloader
>    left isn't touched, but enable state and usecount may
>    not match.  (System boots but some drivers act odd ...
>    debuggable.  System dies part way through booting ...
>    often painful.)

This also addresses things like the reverse engineering use case where
people genuinely don't know what's going on and want to use the API to
inspect the state of the regulators.

I do wonder if we can't come up with a different way of expressing
devmode - a different name like dont_disable that more directly
expresses what the constraint does might be more obvious.  Something
that can enable this globally may also be convenient.

The code itself is OK.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[David Brownell]

On Thursday 12 March 2009, Mark Brown wrote:
> On Wed, Mar 11, 2009 at 04:43:34PM -0800, David Brownell wrote:
> 
> > Buggy consumers could notice different bug symptoms.  The main
> > example would be refcounting bugs; also, any (out-of-tree) users
> > of the experimental regulator_set_optimum_mode() stuff which
> > don't call it when they're done using a regulator.
> 
> I'm OK with this from a code point of view so
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> However any consumers that take advantage of this won't be able to
> safely share a regulator without extra work since they have no way of
> telling why a regulator is in the state that it's in without extra
> stuff.

Depends what you mean by "safely".  If they weren't buggy
already, I don't see how they'd notice any difference.
Having buggy consumers become non-buggy isn't exactly a
job for the framework itself.


> We should probably have something along the lines of a 
> regulator_get_exclusive() for them.  Previously the consumer counting
> would have stopped them interfering with enables done by other
> consumers.

I'd like to see get()/put() match the design pattern used
elsewhere in the kernel:  those calls signify refcount
operations.

Agreed that the "consumer" access model probably needs a few
interface updates.  I'm not sure what they would be though;
one notion would be to focus on the constraints they apply
(including "enabled") instead of what they do now.

 
> There will be other consumers that can't safely share a regulator anyway
> (eg, requiring additional code to notice and handle voltage changes) so
> it'd be a good thing to have.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[Mark Brown]

On Thu, Mar 12, 2009 at 12:35:24PM -0800, David Brownell wrote:
> On Thursday 12 March 2009, Mark Brown wrote:

> > safely share a regulator without extra work since they have no way of
> > telling why a regulator is in the state that it's in without extra
> > stuff.

> Depends what you mean by "safely".  If they weren't buggy
> already, I don't see how they'd notice any difference.
> Having buggy consumers become non-buggy isn't exactly a
> job for the framework itself.

Previously the per-consumer reference count would've meant that they
couldn't interfere with other consumers - they could set their own
state but not reverse an enable something else had done.  Now there is
only one reference count but there's no way for a consumer to exclude
other consumers and nothing which protects against breakage.

> > We should probably have something along the lines of a 
> > regulator_get_exclusive() for them.  Previously the consumer counting
> > would have stopped them interfering with enables done by other
> > consumers.

> I'd like to see get()/put() match the design pattern used
> elsewhere in the kernel:  those calls signify refcount
> operations.

Aquiring a reference is obviously what we want in the rather common case
where the supply is shared.  We could name an operation that enforces
non shared supplies something else but at the end of the day it's going
to be the same operation.  The major purpose of adding an explicit call
for this is to document the requirement the consumer has for direct
control of what it's doing.

> Agreed that the "consumer" access model probably needs a few
> interface updates.  I'm not sure what they would be though;
> one notion would be to focus on the constraints they apply
> (including "enabled") instead of what they do now.

I'm not at all sure what you mean by this - constraint narrowing by the
consumers is pretty much exactly the model the existing code has.  We
need to do things like re-add the voltage handling that was removed pre
merge but that's already the programming model we have.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[David Brownell]

On Thursday 12 March 2009, Mark Brown wrote:
> On Thu, Mar 12, 2009 at 12:35:24PM -0800, David Brownell wrote:
> > On Thursday 12 March 2009, Mark Brown wrote:
> 
> > > safely share a regulator without extra work since they have no way of
> > > telling why a regulator is in the state that it's in without extra
> > > stuff.
> 
> > Depends what you mean by "safely".  If they weren't buggy
> > already, I don't see how they'd notice any difference.
> > Having buggy consumers become non-buggy isn't exactly a
> > job for the framework itself.
> 
> Previously the per-consumer reference count would've meant that they
> couldn't interfere with other consumers - they could set their own
> state but not reverse an enable something else had done.

They still can't ... *unless* they're already buggy.


> Now there is 
> only one reference count but there's no way for a consumer to exclude
> other consumers and nothing which protects against breakage.
> 
> > > We should probably have something along the lines of a 
> > > regulator_get_exclusive() for them.  Previously the consumer counting
> > > would have stopped them interfering with enables done by other
> > > consumers.
> 
> > I'd like to see get()/put() match the design pattern used
> > elsewhere in the kernel:  those calls signify refcount
> > operations.
> 
> Aquiring a reference is obviously what we want in the rather common case
> where the supply is shared.  We could name an operation that enforces
> non shared supplies something else but at the end of the day it's going
> to be the same operation.  The major purpose of adding an explicit call
> for this is to document the requirement the consumer has for direct
> control of what it's doing.

Step back from the current interface for a moment though.

There seem to be two things going on:

  * getting a handle on the regulator;

  * adding consumer-specific constraints to it.

Currently "handle" and "regulator" are two different objects;
while "handle" and "consumer-specific constraints" are one.

And, as a new thing not currently addressed well in code, you
are talking about "non-shared" handles.

One could as easily have "handle" and "regulator" be the
same ... so the get/put idioms could work like they do
elsewhere in the kernel.

Doing that would imply making the "constraints" be separate
(as they are for machine constraints) and possibly having the
ability to control sharing (like IRQF_SHARED does).


> > Agreed that the "consumer" access model probably needs a few
> > interface updates.  I'm not sure what they would be though;
> > one notion would be to focus on the constraints they apply
> > (including "enabled") instead of what they do now.
> 
> I'm not at all sure what you mean by this - constraint narrowing by the
> consumers is pretty much exactly the model the existing code has.  We
> need to do things like re-add the voltage handling that was removed pre
> merge but that's already the programming model we have.

See above.  Currently constraints are hidden for "consumers",
behind functional accessors like regulator_set_voltage().
There are no explicit constraint objects, as there are for
the machines.

That's all I'm saying:  the approached you sketched isn't
the only one, and may not be the best one.  If you want to
change things, there may be better approaches.  Even ones
that don't change the overall interface structure much.

- Dave

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[Mark Brown]

On Thu, Mar 12, 2009 at 03:02:55PM -0800, David Brownell wrote:
> On Thursday 12 March 2009, Mark Brown wrote:

> > Previously the per-consumer reference count would've meant that they
> > couldn't interfere with other consumers - they could set their own
> > state but not reverse an enable something else had done.

> They still can't ... *unless* they're already buggy.

As previously noted they could've worked happily via bouncing their
state to match the physical state first; it's not nice or a good idea
(which is why I am happy with your patch) but, well, not everyone pays
attention to warnings and errors :(

> And, as a new thing not currently addressed well in code, you
> are talking about "non-shared" handles.

You are missing my point here; this is mostly about documentation.  The
exclusive access issue is with devices that can't tolerate any
arbitration and need the regulator to go into the state they're
requesting - if the consumer is finding itself doing something like turn
off a regulator which it did not enable itself then it's clearly not
able to play nicely with other drivers sharing the same resource without
extra communication.  This may be because the driver is doing things
that aren't really appropriate and perhaps ought to be done via
constraints if at all; it may also be because the hardware that's being
controlled demands it.

If everyone is nice and careful about what they're doing it'll not make
any difference at all either way.  Especially in the hardware
requirements case I'd hope it never even comes up that it'd make a
difference.

> One could as easily have "handle" and "regulator" be the
> same ... so the get/put idioms could work like they do
> elsewhere in the kernel.

I really don't see that there is any meaningful difference here; from
the point of view of the consumer the fact that the thing it gets back
is a handle to a structure the core uses to keep track of the consumer
rather than the underlying hardware object is an implementation detail
that shouldn't make any difference to them.  In terms of the programming
model it seems like a layering violation to know the difference between
one opaque structure and another.

> See above.  Currently constraints are hidden for "consumers",
> behind functional accessors like regulator_set_voltage().
> There are no explicit constraint objects, as there are for
> the machines.

The current interface has been driven by the needs of the users: the
majority of consumers want to do one operation on a regular basis -
normally that's enable/disable, most devices are just powering
themselves up and down, though for some things voltage changes are much
more common (DVFS being the prime example).  Overall it's been fairly
similar to the clock API in terms of usage pattern.

In terms of looking at redesigning the API I feel we're getting ahead of
ourselves here and should be working more for stability until we run
into problems that force us to make big changes.  Right now it seems
better to focus on improving the support for real systems in mainline
and addressing any issues that they see so that we've got something to
learn from when doing any redesign and can minimise the amount of
integration churn that is created.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[David Brownell]

On Thursday 12 March 2009, Mark Brown wrote:
> On Thu, Mar 12, 2009 at 03:02:55PM -0800, David Brownell wrote:
>
> > One could as easily have "handle" and "regulator" be the
> > same ... so the get/put idioms could work like they do
> > elsewhere in the kernel.
> 
> I really don't see that there is any meaningful difference here; from
> the point of view of the consumer the fact that the thing it gets back
> is a handle to a structure the core uses to keep track of the consumer
> rather than the underlying hardware object is an implementation detail
> that shouldn't make any difference to them.  In terms of the programming
> model it seems like a layering violation to know the difference between
> one opaque structure and another.

You're not stepping back from the current interface, which is
a prerequisite to understanding the points I was making:

 * Almost everywhere else in the kernel, there's only one
   handle (no per-client facet idiom), for which get/put
   works.

   Having the handle alloc/free methods be called get/put
   is a kind of problem.  We want models and idioms to
   converge, not diverge, in almost all cases ... using
   the same names to mean different things isn't good.

 * The thing that *is* per-client is basically a constraint
   set ... but it's called a "regulator", which again is
   confusing.

In the regulator-next tree you've now moved regulator_dev
into the public interface ... that's the handle to the
real hardware.  Sort of a hint that it can't really be
hidden in the way you originally thought.


> > See above.  Currently constraints are hidden for "consumers",
> > behind functional accessors like regulator_set_voltage().
> > There are no explicit constraint objects, as there are for
> > the machines.
> 
> The current interface has been driven by the needs of the users: the
> majority of consumers want to do one operation on a regular basis -
> normally that's enable/disable, most devices are just powering
> themselves up and down, though for some things voltage changes are much
> more common (DVFS being the prime example).  Overall it's been fairly
> similar to the clock API in terms of usage pattern.

Except that the clock interface uses put/get in the normal way;
they are not alloc/free calls, just lookup/refcount calls.


> In terms of looking at redesigning the API

You were the one suggesting the need for a new call, formalizing
a model that didn't previously exist ... not me!  :)

Which is why I suggested taht if you were going to add calls,
it'd be worth thinking a bit more about some existing glitches.

[patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Make the regulator setup code cope more consistently with
regulators left enabled by the bootloader ... making it act
as if the "boot_on" flag were being set by the boot loader
(as needed) instead of by Linux board init code (always).

This eliminates the other half of the bug whereby regulators
that were enabled during the boot sequence would be enabled
with enable count of zero ... preventing regulator_disable()
from working when called from drivers.  (The first half was
fixed by the "regulator: refcount fixes" patch, and related
specifically to the "boot_on" flag.)

One open issue involves systems that need to force a regulator
off at boot time -- the converse of today's "boot_on" flag. 
There can be arbitrarily long delays between system startup
and when a driver can be loaded to turn off the regulator,
during which power is being wasted.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -815,6 +815,14 @@ static int set_machine_constraints(struc
 			goto out;
 		}
 		rdev->use_count = 1;
+	} else if (ops->is_enabled) {
+		/* ... if the bootloader left it on, drivers need a
+		 * nonzero enable count else it can't be disabled.
+		 */
+		ret = ops->is_enabled(rdev);
+		if (ret > 0)
+			rdev->use_count = 1;
+		ret = 0;
 	}
 
 	print_constraints(rdev);

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[Mark Brown]

On Sat, Mar 14, 2009 at 02:29:24PM -0700, David Brownell wrote:

> You're not stepping back from the current interface, which is
> a prerequisite to understanding the points I was making:

I'm pretty sure I understand exactly what you're saying but we just
disagree on this one.  Probably best that we agree to disagree.

>  * Almost everywhere else in the kernel, there's only one
>    handle (no per-client facet idiom), for which get/put
>    works.

Looking at things from the point of view of the consumer I just don't
find that it makes any difference since as far as the consumer is
concerned it's all opaque objects manipulated via an API.  I certainly
don't experience any conceptual jar switching between this and things
like the clock API, the patterns in clients the same especially for a
basic consumer doing something along the lines of:

	foo = foo_get(dev, name);
	foo_enable(foo);
	foo_disable(foo);
	foo_put(foo);

Even once you start setting more properties in there I'm struggling to
see the difference being visible.

I feel that you are focusing too much on the implementation here, not
the interface, but like I say I think we're just going to have to agree
to disagree on this one.

>  * The thing that *is* per-client is basically a constraint
>    set ... but it's called a "regulator", which again is
>    confusing.

You could go for regulated_supply or something.  I think that at this
point it's just not worth the trouble to try to change the name, though.

If it helps think of the per client object as representing the
particular physical supply to the physical device.  We could represent
them internally as pass through regulators but since the implementation
should still be opaque to the consumers I don't think that it's worth
doing that unless it buys us something in the implementation.

I'm not overly concerned about the implementation right now since it's
not causing any problems and the opacity we have now for the consumers
supports later refactoring.  Things like the issues with working with
struct device for I2C and SPI devices seem to be causing far more
pressing problems in actual use.

> In the regulator-next tree you've now moved regulator_dev
> into the public interface ... that's the handle to the
> real hardware.  Sort of a hint that it can't really be
> hidden in the way you originally thought.

It's only exposed to the drivers for the regulator hardware; it's not
visible to consumers unless they go peering into the driver API.  The
drivers for the regulators have always had a direct handle on themselves
for obvious reasons - the only change here is that they now know a bit
more about the implementation.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Sat, Mar 14, 2009 at 05:25:35PM -0700, David Brownell wrote:

> +	} else if (ops->is_enabled) {
> +		/* ... if the bootloader left it on, drivers need a
> +		 * nonzero enable count else it can't be disabled.
> +		 */
> +		ret = ops->is_enabled(rdev);
> +		if (ret > 0)
> +			rdev->use_count = 1;
> +		ret = 0;

This means that drivers that do balanced enables and disables will never
be able to cause the regulator to actually be disabled since there will
always be this extra reference count there.  Without this patch what'll
happen with those drivers is that they'll do an enable then later on
when the last one disables its supply the reference count will fall to
zero and the regulator will be disabled.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Saturday 14 March 2009, Mark Brown wrote:
> On Sat, Mar 14, 2009 at 05:25:35PM -0700, David Brownell wrote:
> 
> > +	} else if (ops->is_enabled) {
> > +		/* ... if the bootloader left it on, drivers need a
> > +		 * nonzero enable count else it can't be disabled.
> > +		 */
> > +		ret = ops->is_enabled(rdev);
> > +		if (ret > 0)
> > +			rdev->use_count = 1;
> > +		ret = 0;
> 
> This means that drivers that do balanced enables and disables will never
> be able to cause the regulator to actually be disabled since there will
> always be this extra reference count there. 

That's already true for every regulator for which the "boot_on"
flag was set ... nothing changes.  Except that things act the
same now regardless of whether Linux or the bootloader enabled
the regulator in the first place; win!  :)

On the other hand, every driver using a regulator for which that
flag could have be set (== ALL of them) needs to be able to cope
with the regulator having been enabled when the device probe()
was called.  It's not exactly hard to check if it was enabled, then
act accordingly, in the typical "single consumer of the regulator"
case.


> Without this patch what'll 
> happen with those drivers is that they'll do an enable then later on
> when the last one disables its supply the reference count will fall to
> zero and the regulator will be disabled.

If they're prepared to work with a regulator enabled at boot time
by either the bootloader or (as its proxy) Linux, they'll first look
to see if the regulator is enabled.

Of course, trying to share a regulator that's set up like that can
bring its own joys.  This patch doesn't change that issue, but it
does get rid of one nasty initialization problem.

Re: [patch 2.6.29-rc7 regulator-next] regulator: init fixes

[David Brownell]

On Thursday 12 March 2009, Mark Brown wrote:
> On Wed, Mar 11, 2009 at 06:32:15PM -0800, David Brownell wrote:
> 
> > Make the regulator setup code cope more consistently with
> > regulators undesirably left enabled by the bootloader.
> 
> First up I'd just like to make it absolutely clear that I agree that
> this is a feature we should have - it's obviously useful.

Feature being "consistency".  I'd say "essential", not
merely "useful" ... this is bugfix territory.

There are things that just can't work with the current
regulator framework, because its handling of this routine
scenario is so inconsistent.  It's not necessarily going
to be safe to force every (!!) driver into that dance you
had described (enable-it-then-disable-it), just to force
all regulators into the kind of self-consistent state that
framework users always expect to start with.

Especially:  RIGHT AFTER INITIALIZATION!!!  There's no
excluse to be self-inconsistent that early, even if
there were an excuse later on.

The "v4" patch I posted resolves that inconsistency in
about the simplest way I can find; but unlike this patch
it doesn't solve the "force this regulator off" problem.


> It is a particular problem for multi-function devices like pcf50633
> which not only register all their regulators by default but also embed
> constraints within the general pcf50633 platform data.

The pcf50633 driver didn't *need* to register all regulators;
and I don't see what the issue would be with platform_data.
Its MFD core could easily check the regulator init data and
skip regulators that didn't initialize a key field.


> If the user 
> simply turns on the regulator driver in their config they'll get this
> behavior if they don't edit the code.  Even with regulator code I'd not
> be surprised if people were bitten by this for things like the memory or
> a CPU core without regulator based DVFS.

That would be part of why the twl4030 regulators are only
registered on request!  Not only will a given board tend to
not use all those regulators, but a number of them really
aren't intended to be managed by Linux.  (And then there
are board options, like what various control signals do.)

Similar things could happen with a system using pcf50633.
There's no reason one of the LDOs or switching regulators
shouldn't be managed exclusively by another I2C master on
that bus ... they each have dedicated registers, and its
not uncommon to dedicate a microcontroller to managing just
one part of a system (and its resources).


> You've addressed some of the use cases for this by providing devmode but

I think it's pretty ugly myself, but you did say something about
wanting explicit support for reverse engineering.  The "v4" patch
which I just sent uses a simpler approach; such support doesn't
need to be explicit in order to work.

Re: [patch 2.6.29-rc7 regulator-next] regulator: refcount fixes

[David Brownell]

On Saturday 14 March 2009, Mark Brown wrote:
> Looking at things from the point of view of the consumer I just don't
> find that it makes any difference since as far as the consumer is
> concerned it's all opaque objects manipulated via an API.

These put()/get() calls are not refcount calls.  They're
what might be called alloc()/free() calls instead.

>         foo = foo_get(dev, name);

The normal idiom is

	bar = foo_get(foo)

which just increments a refcount.  Then if "bar" were handed
to something else ("baz") right here, and "baz" needed to keep a
copy of that reference, it would be expected to grab its own
refcount via foo_get(bar), and later release via foo_put(bar).


>         foo_enable(foo);
>         foo_disable(foo);
>         foo_put(foo);

Until "baz" called foo_put(bar), that reference would still be
usable.  But ... regulator_put(foo) does a kfree, it's not
just updating refcounts.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Sat, Mar 14, 2009 at 09:05:29PM -0700, David Brownell wrote:

> was called.  It's not exactly hard to check if it was enabled, then
> act accordingly, in the typical "single consumer of the regulator"
> case.

How typical that is depends very much on the devices you're looking at.
Devices that need to do things like set voltages are fairly likely to
own the regulator but with devices that just need to ensure that they
have their supplies enabled it's much more likely that the supplies will
be shared.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Monday 16 March 2009, Mark Brown wrote:
> On Sat, Mar 14, 2009 at 09:05:29PM -0700, David Brownell wrote:
> 
> > was called.  It's not exactly hard to check if it was enabled, then
> > act accordingly, in the typical "single consumer of the regulator"
> > case.
> 
> How typical that is depends very much on the devices you're looking at.

I'd have said "systems you're looking at" ... but so long
as the boot_on flag exists, then this issue can appear with
absolutely any regulator, used for anything.

The basic issue addressed by $SUBJECT patch just restores
internal consistency to the framework, in the face of such
flags (or equivalent actions by the boot loader).


> Devices that need to do things like set voltages are fairly likely to
> own the regulator but with devices that just need to ensure that they
> have their supplies enabled it's much more likely that the supplies will
> be shared.

Right.  Do you have a model how such shared supplies would
coexist with the "enabled at boot time" model, and still
support being disabled?

My first thought is that the system designer should avoid
such boot_on modes.  But that's not always going to work.

- Dave

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Tue, Mar 17, 2009 at 11:15:06AM -0700, David Brownell wrote:
> On Monday 16 March 2009, Mark Brown wrote:

> > Devices that need to do things like set voltages are fairly likely to
> > own the regulator but with devices that just need to ensure that they
> > have their supplies enabled it's much more likely that the supplies will
> > be shared.

> Right.  Do you have a model how such shared supplies would
> coexist with the "enabled at boot time" model, and still
> support being disabled?

The drivers can essentially ignore the physical status of the regulator
when they start, enabling when they need them and disabling when they're
done.  So long as at least one device has the regulator enabled the
regulator will remain on but when the reference count drops to zero then
the regulator will be disabled.

This works well when the driver will be enabling the regulators at
startup and then disabling them later on.  It will also work well with a
late_initcall which disables any unreferenced regulators - that should
become the default at some future point (2.6.31 might be a good candiate
here, I posted a patch the other day providing an implementation which
warns if there are affected regulators and which machines can activate
if they want).

> My first thought is that the system designer should avoid
> such boot_on modes.  But that's not always going to work.

Yes, that's not something that can realistically be achieved in the
general case.  Machines should probably avoid it but often people want
to do things like bring LCDs up in the bootloader and providing graceful
handover to the actual driver helps the user experience.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Tuesday 17 March 2009, Mark Brown wrote:
> On Tue, Mar 17, 2009 at 11:15:06AM -0700, David Brownell wrote:
> > On Monday 16 March 2009, Mark Brown wrote:
> 
> > > Devices that need to do things like set voltages are fairly likely to
> > > own the regulator but with devices that just need to ensure that they
> > > have their supplies enabled it's much more likely that the supplies will
> > > be shared.
> 
> > Right.  Do you have a model how such shared supplies would
> > coexist with the "enabled at boot time" model, and still
> > support being disabled?
> 
> The drivers can essentially ignore the physical status of the regulator
> when they start,

That is, shared supplies should adopt a different model?

That approach can't be used with drivers, as for MMC slots,
which need to ensure they start with a "power off" state as
part of a clean reset/init sequence.

Maybe "sharable" should be a regulator constraint flag, so
the regulator framework can avoid committing nastiness like
allocating multiple consumer handles for them.


> It will also work well with a 
> late_initcall which disables any unreferenced regulators -

The $SUBJECT patch will prevent such things from existing.

Also, regulator use that kicks in before that particular
late_initcall will still see self-inconsistent state in
the regulator framework ... of course, $SUBJECT patch (and
its predecessors) is all about preventing self-inconsistency.

That self-inconsistency doesn't seem to concern you much.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Wed, Mar 18, 2009 at 12:25:11PM -0700, David Brownell wrote:
> On Tuesday 17 March 2009, Mark Brown wrote:

> > The drivers can essentially ignore the physical status of the regulator
> > when they start,

> That is, shared supplies should adopt a different model?

I think that's a bit strong, once we get past init the problem pretty
much goes away AFAICT.

> That approach can't be used with drivers, as for MMC slots,
> which need to ensure they start with a "power off" state as
> part of a clean reset/init sequence.

Pretty much.  They could cope if they used the enable/disable bounce
hack but if they urgently need to have a specific power state they won't
be able to cope with shared regulators anyway so it doesn't really make
any odds.

> Maybe "sharable" should be a regulator constraint flag, so
> the regulator framework can avoid committing nastiness like
> allocating multiple consumer handles for them.

Or vice versa - it's as much a property of the consumer driver that it
requires exclusivity.  From the point of view of the regulator API
there's very little difference between the two cases.

Note that for well behaved consumers that use mapped supply names we
already know about them all in advance anyway...

> > It will also work well with a 
> > late_initcall which disables any unreferenced regulators -

> The $SUBJECT patch will prevent such things from existing.

I sent a patch backing that specific change out along with the
late_initcall() patch.

> Also, regulator use that kicks in before that particular
> late_initcall will still see self-inconsistent state in
> the regulator framework ... of course, $SUBJECT patch (and
> its predecessors) is all about preventing self-inconsistency.

A driver that can cope with sharing the regulator is not going to be
able to observe anything here: it must always enable the regulator when
it is using it even if it is already enabled (since otherwise some other
consumer could cause it to be turned off) and it should expect that the
regulator might be enabled by something else.  It can't do an unbalanced
disable since otherwise it could be reversing an enable something else
did.  It's also not possible for it to inspect the use count.

If a consumer can't play with that model then I'm reasonably happy with
it having to state any additional requirements it has in some way - if
nothing else it gives us a bit of documentation about what's going on.

> That self-inconsistency doesn't seem to concern you much.

I think it's more that I'm viewing the use count as being useful
information which the API can take advantage of ("do any consumers
actually want this on right now?").  I think we should be able to handle
this without changing the use count and that this is preferable because
otherwise we make more work with shared consumers, which should be the
simplest case.

The trick is getting the non-shared regulators into sync with the
internal status, ideally without doing things like bouncing supplies
during init.  I think either using regulator_force_disable() or saying
that the consmer wants exclusive access on get and then bumping the use
count for it if the regulator is already enabled ought to cover it.
I've not thought the latter option through fully, though.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Wednesday 18 March 2009, Mark Brown wrote:
> > The $SUBJECT patch will prevent such things from existing.
> 
> I sent a patch backing that specific change out along with the
> late_initcall() patch.

Huh?  $SUBJECT patch hasn't merged.  How could you
have backed it out??

  http://marc.info/?l=linux-kernel&m=123707714131036&w=2

I didn't see such patches.  Could you post URLs so I
know specifically what you're referring to?

In the future, when you're proposing patches you think
address bugs I've reported, please remember to CC me ...

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Wednesday 18 March 2009, Mark Brown wrote:

> > That self-inconsistency doesn't seem to concern you much.
>
> I think it's more that I'm viewing the use count as being useful
> information which the API can take advantage of ("do any consumers
> actually want this on right now?").

In that case, I don't understand why you didn't like
the previous versions of this patch ... which just
forced the regulator off (at init time) if that count
was zero.


> I think we should be able to handle 
> this without changing the use count and that this is preferable because
> otherwise we make more work with shared consumers, which should be the
> simplest case.

That's what the earlier versions of this patch did,
but you rejected that approach ... patches against
both mainline (which is where the bug hurts TODAY),
and against regulator-next.

Also, I don't see why you'd think a shared consumer
would be the "simplest", given all the special rules
that you've already noted as only needing to kick in
for those cases.


> The trick is getting the non-shared regulators into sync with the
> internal status,

I don't see why that should need any tricks.  At
init time you have that state and the regulator;
and you know there are no consumers.  Put it into
a self-consistent state at that time ... done.

There are really only two ways to make that state
self-consistent.  And you have rejected both.


> ideally without doing things like bouncing supplies 
> during init.  I think either using regulator_force_disable() or saying

The force_disable() thing looks to me like an API design
botch.  As you said at one point, it has no users.  And
since the entire point is to bypass the entire usecount
scheme ... it'd be much better to remove it.


> that the consmer wants exclusive access on get and then bumping the use
> count for it if the regulator is already enabled ought to cover it.
> I've not thought the latter option through fully, though.

The problem I have with your approach here is that you
have rejected quite a few alternative bugfixes applying
to a common (and simple!!) configuration, but you haven't
provided an alternative that can work for MMC init.

I'm really at a loss to see a way out here.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[David Brownell]

On Wednesday 18 March 2009, Mark Brown wrote:

> > That self-inconsistency doesn't seem to concern you much.
>
> I think it's more that I'm viewing the use count as being useful
> information which the API can take advantage of ("do any consumers
> actually want this on right now?").

In that case, I don't understand why you didn't like
the previous versions of this patch ... which just
forced the regulator off (at init time) if that count
was zero.


> I think we should be able to handle 
> this without changing the use count and that this is preferable because
> otherwise we make more work with shared consumers, which should be the
> simplest case.

That's what the earlier versions of this patch did,
but you rejected that approach ... patches against
both mainline (which is where the bug hurts TODAY),
and against regulator-next.

Also, I don't see why you'd think a shared consumer
would be the "simplest", given all the special rules
that you've already noted as only needing to kick in
for those cases.


> The trick is getting the non-shared regulators into sync with the
> internal status,

I don't see why that should need any tricks.  At
init time you have that state and the regulator;
and you know there are no consumers.  Put it into
a self-consistent state at that time ... done.

There are really only two ways to make that state
self-consistent.  And you have rejected both.


> ideally without doing things like bouncing supplies 
> during init.  I think either using regulator_force_disable() or saying

The force_disable() thing looks to me like an API design
botch.  As you said at one point, it has no users.  And
since the entire point is to bypass the entire usecount
scheme ... it'd be much better to remove it.


> that the consmer wants exclusive access on get and then bumping the use
> count for it if the regulator is already enabled ought to cover it.
> I've not thought the latter option through fully, though.

The problem I have with your approach here is that you
have rejected quite a few alternative bugfixes applying
to a common (and simple!!) configuration, but you haven't
provided an alternative that can work for MMC init.

I'm really at a loss to see a way out here.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Wed, Mar 18, 2009 at 02:14:14PM -0700, David Brownell wrote:
> On Wednesday 18 March 2009, Mark Brown wrote:

> > I think it's more that I'm viewing the use count as being useful
> > information which the API can take advantage of ("do any consumers
> > actually want this on right now?").

> In that case, I don't understand why you didn't like
> the previous versions of this patch ... which just
> forced the regulator off (at init time) if that count
> was zero.

There are two issues which I raised in response to that patch.  One is
that this is a fairly substantial interface change which will affect
existing constraints without warning and will therefore break people
using the current code.  At the minute you can't rely on boot_on being
set for any regulator which is enabled when Linux starts.  This is the
most serious issue - a quick survey leads me to expect that turning off
regulators without boot_on would break most existing systems somehow.

The other is that I'm fairly sure that we'll end up running into systems
where the setup at startup is not fixed and forcing the regulator state
immediately on regulator registration is likely to cause problems
dealing gracefully with such systems.  You addressed that by adding
devmode, though ideally that should have a clearer name.

> > I think we should be able to handle 
> > this without changing the use count and that this is preferable because
> > otherwise we make more work with shared consumers, which should be the
> > simplest case.

> That's what the earlier versions of this patch did,
> but you rejected that approach ... patches against
> both mainline (which is where the bug hurts TODAY),
> and against regulator-next.

I have given you two suggestions which will allow your MMC driver to use
mainline without impacting other drivers.  I've also provided some
suggestions for how we could introduce changes in the regulator core to
support this better.

> Also, I don't see why you'd think a shared consumer
> would be the "simplest", given all the special rules
> that you've already noted as only needing to kick in
> for those cases.

Simplest for the consumers - they just need to do a get followed by
basic reference counting, they don't need to (and can't, really) worry
about anything else.

> > The trick is getting the non-shared regulators into sync with the
> > internal status,

> I don't see why that should need any tricks.  At
> init time you have that state and the regulator;
> and you know there are no consumers.  Put it into

Realistically we don't have that information at the minute.  For the
most part we have the physical state and we may also have some
constraint information but we can't rely on the constraint information
right now.  The fact that we can't rely on the constraint information
means that we can't tell if a regulator is enabled because something is
using it at the minute or if it's just been left on because that was the
default or similar.

> a self-consistent state at that time ... done.

> There are really only two ways to make that state
> self-consistent.  And you have rejected both.

Both of the approaches you have suggested change the interfaces and
cause problems for existing users with no warning to allow them to
transition.  Changing the reference count does avoid the problems with
powering things off but it causes other problems in doing so, ones that
look harder to resolve.

When looking at bringing the use count in sync with the physical state
during startup we have to deal with the fact that we can't currently
rely on having information about the desired state of the hardware at
the time the regulator is registered.  We need to make an API change of
some kind to get that information.

> > during init. ?I think either using regulator_force_disable() or saying

> The force_disable() thing looks to me like an API design
> botch.  As you said at one point, it has no users.  And
> since the entire point is to bypass the entire usecount
> scheme ... it'd be much better to remove it.

As the documentation says it was originally added for error handling
cases where there is a danger of hardware damage.  In that situation
you really do want to power off immediately without reference to any
other state.

> > that the consmer wants exclusive access on get and then bumping the use
> > count for it if the regulator is already enabled ought to cover it.
> > I've not thought the latter option through fully, though.

> The problem I have with your approach here is that you
> have rejected quite a few alternative bugfixes applying
> to a common (and simple!!) configuration, but you haven't
> provided an alternative that can work for MMC init.

I have given you a number of suggestions which should work for MMC init.
These are:

 - Have the MMC style consumers use regulator_force_disable() to disable
   an enabled regulator on startup.
 - Have the MMC style consumers do an enable()/disable() pair to disable
   an enabled regulator on startup.
 - Changing the way the new behaviours are specified so that they don't
   change the behaviour of existing constraints (eg, by adding a new
   constraint flag or by having consumers request exclusive access).
 - Providing a transition method to warn about a pending change in
   behaviour, ideally along with a way to request that behaviour
   immediately (which could be deprecated and removed once the default
   is changed).

The first two should work with current mainline and continue to work if
either of the other two is implemented, though they could be removed
after that has happened.

The biggest reason for the pushback here is that everything you have
posted so far changes the way the interface works for existing users.

Re: [patch 2.6.29-rc8 regulator-next] regulator: init fixes (v4)

[Mark Brown]

On Wed, Mar 18, 2009 at 02:02:14PM -0700, David Brownell wrote:

> Huh?  $SUBJECT patch hasn't merged.  How could you
> have backed it out??

Sorry - another patch introducing a version of the same issue.  Your
patch:

http://git.kernel.org/?p=linux/kernel/git/lrg/voltage-2.6.git;a=commit;h=67130ef073299cc7299d3ffa344122959e97753c

bumped the refcount for boot_on regulators.  This patch:

http://git.kernel.org/?p=linux/kernel/git/lrg/voltage-2.6.git;a=commit;h=504e7d7e42fb3a58809946770ff5e07cc9d52dbf

backs out that change only.  This was my bad on the review.