[patch 2.6.30-rc3] regulator: regression fix

[patch 2.6.30-rc3] regulator: regression fix

[David Brownell]

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Revert the "Don't warn on omitted voltage constraints" patch
(3e2b9abda554e9f6105996dca77cca9ef98de17a), which doesn't do
quite what its comments said it was doing.

By removing the "else", it breaks the handling of fixed-voltage
regulators ... turning a non-error/non-warning situation into
a complete init failure, which can then prevent system startup.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
You might want to provide a different patch, but ignoring
this regression doesn't seem practical...

 drivers/regulator/core.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -709,12 +709,8 @@ static int set_machine_constraints(struc
 			cmax = INT_MAX;
 		}
 
-		/* voltage constraints are optional */
-		if ((cmin == 0) && (cmax == 0))
-			goto out;
-
 		/* else require explicit machine-level constraints */
-		if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
+		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
 			pr_err("%s: %s '%s' voltage constraints\n",
 				       __func__, "invalid", name);
 			ret = -EINVAL;

Re: [patch 2.6.30-rc3] regulator: regression fix

[Mark Brown]

On Mon, Apr 27, 2009 at 07:59:40PM -0700, David Brownell wrote:

> By removing the "else", it breaks the handling of fixed-voltage
> regulators ... turning a non-error/non-warning situation into
> a complete init failure, which can then prevent system startup.

The change you're making isn't relevant to what I suspect the actual
problem is (you didn't specify, I may be wrong here).

For fixed voltage regulators either the user will have specified a
voltage constraint (in which case we'll fall into your else case since
cmin ought to be non-zero) or they won't (in which case it's the default
constraint code you added will fill it in).  The problem I think you're
seeing is that the code you added to fill in a default constraint for
fixed voltage regulators uses INT_MIN as the minimum contraint value.
This is a negative value and so fails the correctness check further
down.

> You might want to provide a different patch, but ignoring
> this regression doesn't seem practical...

The code that was being fixed was only even in -next for a relatively
brief period of time.

>  		/* else require explicit machine-level constraints */
> -		if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
> +		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {

Yeah, a different patch I think.  I'll send one shortly.

Re: [patch 2.6.30-rc3] regulator: regression fix

[David Brownell]

On Tuesday 28 April 2009, Mark Brown wrote:
> On Mon, Apr 27, 2009 at 07:59:40PM -0700, David Brownell wrote:
> 
> > By removing the "else", it breaks the handling of fixed-voltage
> > regulators ... turning a non-error/non-warning situation into
> > a complete init failure, which can then prevent system startup.
> 
> The change you're making isn't relevant to what I suspect the actual
> problem is (you didn't specify, I may be wrong here).

Restoring the "else" fixed the logic flaw ...


> For fixed voltage regulators either the user will have specified a
> voltage constraint (in which case we'll fall into your else case since
> cmin ought to be non-zero) or they won't (in which case it's the default
> constraint code you added will fill it in).  The problem I think you're
> seeing is that the code you added to fill in a default constraint for
> fixed voltage regulators uses INT_MIN as the minimum contraint value.
> This is a negative value and so fails the correctness check further
> down.

That was my conclusion too.  I forget all the relevant history,
except that you disliked the notion that boards be able to accept
whatever constraint the regulator allows ... so that some of the
logic there is left over.


> > You might want to provide a different patch, but ignoring
> > this regression doesn't seem practical...
> 
> The code that was being fixed was only even in -next for a relatively
> brief period of time.

This is the first time I've seen the "fix" though.  Recall that
the code in question has been in use for several months now, while
waiting to wend its way into mainline.  It might be useful to CC
a few more folk on such "fix" patches.


> >  		/* else require explicit machine-level constraints */
> > -		if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
> > +		else if (cmin <= 0 || cmax <= 0 || cmax < cmin) {
> 
> Yeah, a different patch I think.  I'll send one shortly.
> 
> 

Re: [patch 2.6.30-rc3] regulator: regression fix

[Mark Brown]

On Tue, Apr 28, 2009 at 02:43:42AM -0700, David Brownell wrote:
> On Tuesday 28 April 2009, Mark Brown wrote:

> > The change you're making isn't relevant to what I suspect the actual
> > problem is (you didn't specify, I may be wrong here).

> Restoring the "else" fixed the logic flaw ...

I'm not sure the code ever worked for whatever it is you're doing with
it - it'd never have set a maximum voltage constraint.

> > The code that was being fixed was only even in -next for a relatively
> > brief period of time.

> This is the first time I've seen the "fix" though.  Recall that
> the code in question has been in use for several months now, while
> waiting to wend its way into mainline.  It might be useful to CC
> a few more folk on such "fix" patches.

The fix was nothing to do with the behaviour of fixed voltage regulators
- it was about restoring support for regulators without voltage
constraints.  The code looked like it was just one of the small
optimisations you're fond of, skipping the validity check since we just
set constraints which ought to be valid.

I should've pushed back harder on the obscure code in the first place.