linux-kernel - RE: [PATCH] leds: lm3530: fix handling of already enabled regulators

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: <DD6B9D658496A2488636C8C75B9C0CB601DA3984@EXMAIL04.scwf.nsc.com>
Date:	Mon, 23 Apr 2012 23:27:26 -0700
From:	"Kim, Milo" <Milo.Kim@...com>
To:	"axel.lin@...il.com" <axel.lin@...il.com>
CC:	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Shreshtha Kumar Sahu <shreshthakumar.sahu@...ricsson.com>,
	Richard Purdie <rpurdie@...ys.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Girdwood, Liam" <lrg@...com>
Subject: RE: [PATCH] leds: lm3530: fix handling of already enabled regulators

> 
> > On Sun, Apr 22, 2012 at 11:11:48PM +0800, Axel Lin wrote:
> >
> >>               drvdata->regulator = NULL;
> >>               goto err_regulator_get;
> >>       }
> >> +     drvdata->enable = regulator_is_enabled(drvdata->regulator);
> >>
> >
> > This isn't sufficient for what the driver is doing - it also needs to
> > enable the regulator.  Regulator enables are reference counted (since
> > the regulator can be shared by many device supplies) so if this
> driver
> > doesn't reference the regulator when it's needed then it could get
> > powered off.
> 
> The regulator_enable() is called in lm3530_init_registers().

I think this patch has a mismatched condition between the value of variable and actual status of the regulator.

Let's suppose that the regulator (named "vin") is disabled after the lm3530 driver is probed.

Step 1. The 'vin' regulator is on

Step 2. lm3530 driver is probed
lm3530_probe()
{
	/* drvdata->enabled is true at this moment */
	drvdata->enable = regulator_is_enabled(drvdata->regulator);
	...
}

Step 3. lm3530_init_registers() is called
lm3530_init_registers()
{
	/* skip enabling the regulator because the condition is true */
	if (!drvdata->enable) {
		ret = regulator_enable(drvdata->regulator);
		...
	}
}

Step 4. The regulator is disabled by another driver.
	(It is possible because the regulator reference count is not incremented yet by the lm3530 driver)

Step 5. If the brightness mode is changed from the ALS to manual, lm3530_init_registers() is called again.

lm3530_init_registers()
{
	/* In this case, regulator should be on,
	   but it will skip again because the variable is still true */
	if (!drvdata->enable) {
		ret = regulator_enable(drvdata->regulator);
		...
	}
}

The lm3530_probe() is called once.
But lm3530_init_registers() can be called whenever the brightness mode is changed.
(e.g from ALS to manual or manual to PWM, and so on)
So the status of regulator can be mismatched.

In my opinion, current source code is the simplest way.

	1. The variable 'drvdata->enabled' is initialized as false.
	2. The regulator is enabled and the variable is updated when the driver is probed.
	   (If the regulator is already on, it's ok - keep going ...)
	3. The variable is updated when regulator_enable()/regulator_disable() is called by the lm3530 driver.
	   (If another driver tries to turn off the regulator, it can't be allowed
	    because the reference count was incremented by the lm3530 driver.)

Thanks & BR
Milo -


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