During regulator registration, any error after device_register() will
cause a double-free on the struct regulator_dev 'rdev'. The bug is in
drivers/regulator/core.c:regulator_register():
...
scrub:
device_unregister(&rdev->dev);
clean:
kfree(rdev); <---
rdev = ERR_PTR(ret);
goto out;
...
device_unregister() calls regulator_dev_release() which frees rdev. The
subsequent kfree corrupts memory and causes some OMAP3 systems to oops on
boot in regulator_get().
Applies against 2.6.30-rc3.
Signed-off-by: Paul Walmsley <[email protected]>
---
drivers/regulator/core.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 01f7702..fabd2e0 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2080,6 +2080,10 @@ out:
scrub:
device_unregister(&rdev->dev);
+ /* device core frees rdev */
+ rdev = ERR_PTR(ret);
+ goto out;
+
clean:
kfree(rdev);
rdev = ERR_PTR(ret);
--
1.6.3.rc1.51.gea0b7
On Saturday 25 April 2009, Paul Walmsley wrote:
>
> During regulator registration, any error after device_register() will
> cause a double-free on the struct regulator_dev 'rdev'. The bug is in
> drivers/regulator/core.c:regulator_register():
>
> ...
> scrub:
> device_unregister(&rdev->dev);
> clean:
> kfree(rdev); <---
> rdev = ERR_PTR(ret);
> goto out;
> ...
>
> device_unregister() calls regulator_dev_release() which frees rdev. The
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on
> boot in regulator_get().
>
> Applies against 2.6.30-rc3.
>
> Signed-off-by: Paul Walmsley <[email protected]>
This looks like it would address the oopsing I mentioned a
while back, since affects cleanup paths after errors during
driver probe().
> ---
> drivers/regulator/core.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 01f7702..fabd2e0 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2080,6 +2080,10 @@ out:
>
> scrub:
> device_unregister(&rdev->dev);
> + /* device core frees rdev */
> + rdev = ERR_PTR(ret);
> + goto out;
> +
> clean:
> kfree(rdev);
> rdev = ERR_PTR(ret);
> --
> 1.6.3.rc1.51.gea0b7
>
>
On Sat, 25 Apr 2009, Paul Walmsley wrote:
> During regulator registration, any error after device_register() will
> cause a double-free on the struct regulator_dev 'rdev'. The bug is in
> drivers/regulator/core.c:regulator_register():
>
> ...
>
> device_unregister() calls regulator_dev_release() which frees rdev. The
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on
> boot in regulator_get().
For the 3430SDP users out there, this patch also fixes the boot hang after
"regulator_init_complete: incomplete constraints, leaving VAUX3 on"
on that device.
- Paul
On Sat, Apr 25, 2009 at 05:28:36AM -0600, Paul Walmsley wrote:
> During regulator registration, any error after device_register() will
> cause a double-free on the struct regulator_dev 'rdev'. The bug is in
> drivers/regulator/core.c:regulator_register():
Looks good:
Acked-by: Mark Brown <[email protected]>
On Sat, 2009-04-25 at 05:28 -0600, Paul Walmsley wrote:
> During regulator registration, any error after device_register() will
> cause a double-free on the struct regulator_dev 'rdev'. The bug is in
> drivers/regulator/core.c:regulator_register():
>
> ...
> scrub:
> device_unregister(&rdev->dev);
> clean:
> kfree(rdev); <---
> rdev = ERR_PTR(ret);
> goto out;
> ...
>
> device_unregister() calls regulator_dev_release() which frees rdev. The
> subsequent kfree corrupts memory and causes some OMAP3 systems to oops on
> boot in regulator_get().
>
> Applies against 2.6.30-rc3.
>
> Signed-off-by: Paul Walmsley <[email protected]>
> ---
> drivers/regulator/core.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
Applied.
Thanks
Liam
On Saturday 25 April 2009, Paul Walmsley wrote:
>
> > device_unregister() calls regulator_dev_release() which frees rdev. ?The
> > subsequent kfree corrupts memory and causes some OMAP3 systems to oops on
> > boot in regulator_get().
>
> For the 3430SDP users out there, this patch also fixes the boot hang after
> "regulator_init_complete: incomplete constraints, leaving VAUX3 on"
> on that device.
For the record, that "incomplete constraints" message is bogus.
On that board, VAUX3 has a complete set of constraints: it may
only emit 2.8V.
What it lacks is something entirely different: driver support
for the LCD which uses the regulator framework, instead of just
bypassing it and talking directly to the PMIC. By the time it
gets there, the LCD has probably been turned on.
Mark and/or Liam ... you might want to fix that diagnostic, to
avoid leading more developers astray!
On Mon, Apr 27, 2009 at 08:08:50PM -0700, David Brownell wrote:
> On Saturday 25 April 2009, Paul Walmsley wrote:
> > For the 3430SDP users out there, this patch also fixes the boot hang after
> > "regulator_init_complete: incomplete constraints, leaving VAUX3 on"
> > on that device.
> For the record, that "incomplete constraints" message is bogus.
> On that board, VAUX3 has a complete set of constraints: it may
> only emit 2.8V.
It's not VAUX3 that it's saying has incomplete constraints, it's the
board as a whole - if the constraints for the board were fully specified
(and the core had been told about it) then it would power off VAUX3 at
that point.
> Mark and/or Liam ... you might want to fix that diagnostic, to
> avoid leading more developers astray!
Probably shove a "board has" in there or something I guess.
On Tuesday 28 April 2009, Mark Brown wrote:
> > For the record, that "incomplete constraints" message is bogus.
> > On that board, VAUX3 has a complete set of constraints: ?it may
> > only emit 2.8V.
>
> It's not VAUX3 that it's saying has incomplete constraints, it's the
> board as a whole - if the constraints for the board were fully specified
No; driver support != constraint. Only one of the
issues is packaged as a "constraint".
> (and the core had been told about it) then it would power off VAUX3 at
> that point.
>
> > Mark and/or Liam ... you might want to fix that diagnostic, to
> > avoid leading more developers astray!
>
> Probably shove a "board has" in there or something I guess.
How about: "VAUX3 board support is incomplete".
That's accurate.
A comment there might suggest constraints and
driver support as two common issues ... I'd not
be surprised to see a few more idioms pop up.
- Dave
On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:
> On Tuesday 28 April 2009, Mark Brown wrote:
> > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > board as a whole - if the constraints for the board were fully specified
> No; driver support != constraint. Only one of the
> issues is packaged as a "constraint".
Driver support isn't particularly relevant here. If there are devices
that don't have drivers or don't have drivers with regulator support but
need a regulator enabling then the board should mark the regulator as
always_on when setting full constraints; the always_on flag can be
removed when the required consumers are added.
> > > Mark and/or Liam ... you might want to fix that diagnostic, to
> > > avoid leading more developers astray!
> > Probably shove a "board has" in there or something I guess.
> How about: "VAUX3 board support is incomplete".
> That's accurate.
No. The constraints being complete is a property of the board as a
whole and not the particular regulator.
On Tuesday 28 April 2009, Mark Brown wrote:
> On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:
> > On Tuesday 28 April 2009, Mark Brown wrote:
> > > >
> > > > For the record, that "incomplete constraints" message is bogus.
> > > > On that board, VAUX3 has a complete set of constraints: ?it may
> > > > only emit 2.8V.
> > > >
> > > > What it lacks is something entirely different: ?driver support
> > > > for the LCD which uses the regulator framework,
> > >
> > > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > > board as a whole - if the constraints for the board were fully specified
> >
> > No; driver support != constraint. Only one of the
> > issues is packaged as a "constraint".
>
> Driver support isn't particularly relevant here.
It's the *entire* point. The driver is talking directly
to the regulator, bypassing this framework. The constraints
on that regulator are fully defined ... and then bypassed.
> > > > Mark and/or Liam ... you might want to fix that diagnostic, to
> > > > avoid leading more developers astray!
>
> > > Probably shove a "board has" in there or something I guess.
>
> > How about: "VAUX3 board support is incomplete".
> > That's accurate.
>
> No. The constraints being complete is a property of the board as a
> whole and not the particular regulator.
Except ... that "constraint" isn't the issue, it's
unexpected driver behavior. And "board" is exactly
what I said, so I don't know why you're arguing.
(For the "fun" of it?)
Board support includes full driver support, as well
as board setup (constraints). That's the common way
to factor it, at any rate -- a "board support package"
addresses both, and they need to work together.
On Tue, Apr 28, 2009 at 04:47:52AM -0700, David Brownell wrote:
> On Tuesday 28 April 2009, Mark Brown wrote:
> > On Tue, Apr 28, 2009 at 02:32:56AM -0700, David Brownell wrote:
> > > > > What it lacks is something entirely different: ?driver support
> > > > > for the LCD which uses the regulator framework,
> > > > It's not VAUX3 that it's saying has incomplete constraints, it's the
> > > > board as a whole - if the constraints for the board were fully specified
> > > No; driver support != constraint. Only one of the
> > > issues is packaged as a "constraint".
> > Driver support isn't particularly relevant here.
> It's the *entire* point. The driver is talking directly
> to the regulator, bypassing this framework. The constraints
> on that regulator are fully defined ... and then bypassed.
Ah, I see - I didn't realise that there was driver support that doesn't
use the regulator API, that had been dropped out of context by that
point in the discussion. I guess the LCD driver will be converted to
use the API in due course, hopefully there aren't too many other such
drivers.
This is the sort of use case that made me not want to have the API
disable unused regulators by default - there's stuff going on that the
regulator API and the code using it doesn't know about (since it's going
through some other interface to do the job in this case) so the API
can't safely do anything to that regulator.
> > > How about: "VAUX3 board support is incomplete".
> > > That's accurate.
> > No. The constraints being complete is a property of the board as a
> > whole and not the particular regulator.
> Except ... that "constraint" isn't the issue, it's
> unexpected driver behavior. And "board" is exactly
This is sufficiently unexpected that it's the first time I've seen
anything bypassing the regulator API; in any case, it's just a question
of where you see the problem coming from.
My terminology comes from the fact that as far as the core is concerned
the issue is that the board didn't say it had given the core complete
constraints allowing it to fully control the regulators that are visible
to it. The reason the board didn't do that is that is that the LCD
driver is bypassing the regulator API and the regulator API doesn't have
any way to express that possibility.
> what I said, so I don't know why you're arguing.
> (For the "fun" of it?)
David, that is really not necessary or constructive. This sort of
disparaging remark has been a constant feature of your interactions on
regulator API issues and it is not achieving anything useful.
> Board support includes full driver support, as well
> as board setup (constraints). That's the common way
> to factor it, at any rate -- a "board support package"
> addresses both, and they need to work together.
s/board/machine driver/; in an ideal world for Linux there is no BSP,
it's all mainline.