[PATCH] regulator: core: Get and put regulator of_node

[PATCH] regulator: core: Get and put regulator of_node

[Charles Keepax]

We should be incrementing the reference count of the of_node for the
regulator when we take a copy of it. This patch does so.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index afca1bc..e26372a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3433,7 +3433,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	/* register with sysfs */
 	rdev->dev.class = &regulator_class;
-	rdev->dev.of_node = config->of_node;
+	rdev->dev.of_node = of_node_get(config->of_node);
 	rdev->dev.parent = dev;
 	dev_set_name(&rdev->dev, "regulator.%d",
 		     atomic_inc_return(&regulator_no) - 1);
@@ -3575,6 +3575,7 @@ void regulator_unregister(struct regulator_dev *rdev)
 	list_del(&rdev->list);
 	kfree(rdev->constraints);
 	regulator_ena_gpio_free(rdev);
+	of_node_put(rdev->dev.of_node);
 	device_unregister(&rdev->dev);
 	mutex_unlock(&regulator_list_mutex);
 }
-- 
1.7.2.5

Re: [PATCH] regulator: core: Get and put regulator of_node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Wed, Apr 02, 2014 at 03:06:54PM +0100, Charles Keepax wrote:

> We should be incrementing the reference count of the of_node for the
> regulator when we take a copy of it. This patch does so.

Why?

Note that in any case this is going to have absolutely no impact on
practical systems since everything using regulators uses FDT at the
minute.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: core: Get and put regulator of_node

[Charles Keepax]

On Wed, Apr 02, 2014 at 03:23:56PM +0100, Mark Brown wrote:
> On Wed, Apr 02, 2014 at 03:06:54PM +0100, Charles Keepax wrote:
> 
> > We should be incrementing the reference count of the of_node for the
> > regulator when we take a copy of it. This patch does so.
> 
> Why?
> 
> Note that in any case this is going to have absolutely no impact on
> practical systems since everything using regulators uses FDT at the
> minute.

Apologies I must be missing something, take for example the fixed
regulator driver (fixed.c). We get an of_node from
pdev->dev->of_node, pull the init_data from it, then copy the
of_node into the regulator_config and call regulator_register.
regulator_register will copy the of_node from the regulator_config
and put it into the new device, but as far as I can see no one ever
incremented the reference count?

	of_node = pdev->dev.of_node;

	drvdata->dev = regulator_register(&drvdata->desc, &cfg);

Looks like there are a couple of other regulator drivers in the
same boat. Just seems easier to let the core do the reference
stuff rather than needing to do it in the drivers.

Thanks,
Charles

Re: [PATCH] regulator: core: Get and put regulator of_node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]

On Wed, Apr 02, 2014 at 05:04:38PM +0100, Charles Keepax wrote:
> On Wed, Apr 02, 2014 at 03:23:56PM +0100, Mark Brown wrote:
> > On Wed, Apr 02, 2014 at 03:06:54PM +0100, Charles Keepax wrote:

> > > We should be incrementing the reference count of the of_node for the
> > > regulator when we take a copy of it. This patch does so.

> > Why?

> Apologies I must be missing something, take for example the fixed

The immediate thing you're missing is this explanation - it's not
sufficiently obvious why the reference needs to be held.

> regulator driver (fixed.c). We get an of_node from
> pdev->dev->of_node, pull the init_data from it, then copy the
> of_node into the regulator_config and call regulator_register.
> regulator_register will copy the of_node from the regulator_config
> and put it into the new device, but as far as I can see no one ever
> incremented the reference count?

> 	of_node = pdev->dev.of_node;

> 	drvdata->dev = regulator_register(&drvdata->desc, &cfg);

> Looks like there are a couple of other regulator drivers in the
> same boat. Just seems easier to let the core do the reference
> stuff rather than needing to do it in the drivers.

Two things here.  One is that we're not copying the node, we're copying
a pointer to it (duplicating the node would be a problem since we need
to look for a phandle to the original node to do consumer lookups).  The
other is that if there is an issue with things never getting referenced
at all here then taking a reference within the regulator code is too
late - if a reference isn't already held at the point where we pass the
node into the regulator core then the pointer that's getting passed
about is already potentially invalid.  

To make this correct we need to at least ensure that the node passed
into the regulator API is valid and referenced at that time so there
should only be an issue for the core if the reference is dropped after
that.  In the above case the device model is holding a reference since
this is the of_node for the device itself so taking the reference won't
hurt but is redundant.  In cases where we have more than one regulator
and are using of_regulator_match() then things are more tricky.
Something needs to drop the references it returns (which isn't happening
at all at the minute).  Doing it while doing the match and register
seems simple and neat from an error handling point of view so having the
core take an additional reference during the registration would join up
with that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: core: Get and put regulator of_node

[Charles Keepax]

On Wed, Apr 02, 2014 at 05:53:54PM +0100, Mark Brown wrote:
> To make this correct we need to at least ensure that the node passed
> into the regulator API is valid and referenced at that time so there
> should only be an issue for the core if the reference is dropped after
> that.  In the above case the device model is holding a reference since
> this is the of_node for the device itself so taking the reference won't
> hurt but is redundant.  In cases where we have more than one regulator
> and are using of_regulator_match() then things are more tricky.
> Something needs to drop the references it returns (which isn't happening
> at all at the minute).

>From what I can see of_regulator_match isn't taking any
references at the minute? for_each_child_of_node will get a
reference but it will also put that when we process the next
child. We copy the pointer to the child into match->of_node
but don't manually increment the reference at all. So
of_regulator_match has no effect on the reference count of the
of_node.

>  Doing it while doing the match and register
> seems simple and neat from an error handling point of view so having the
> core take an additional reference during the registration would join up
> with that.

The main issue I have is that devm_regualtor_register is a bit
awkward. With regulator_register you will always be calling
regulator_unregister so you can put the of_node there but with
devm there isn't really a good place to put the of_node.

Would perhaps a sensible thing here be to add an of_node_get to
of_regulator_match, since we seem to be expecting that to
increase the ref count. And then just add an of_node_put to
regulator_unregister. And for anything directly using
regulator_register/devm_regulator_register they should add a
manual of_node_get?

Thanks,
Charles

Re: [PATCH] regulator: core: Get and put regulator of_node

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On Thu, Apr 03, 2014 at 11:58:04AM +0100, Charles Keepax wrote:
> On Wed, Apr 02, 2014 at 05:53:54PM +0100, Mark Brown wrote:

> > To make this correct we need to at least ensure that the node passed
> > into the regulator API is valid and referenced at that time so there
> > should only be an issue for the core if the reference is dropped after
> > that.  In the above case the device model is holding a reference since
> > this is the of_node for the device itself so taking the reference won't
> > hurt but is redundant.  In cases where we have more than one regulator
> > and are using of_regulator_match() then things are more tricky.
> > Something needs to drop the references it returns (which isn't happening
> > at all at the minute).

> From what I can see of_regulator_match isn't taking any
> references at the minute? for_each_child_of_node will get a
> reference but it will also put that when we process the next
> child. We copy the pointer to the child into match->of_node
> but don't manually increment the reference at all. So
> of_regulator_match has no effect on the reference count of the
> of_node.

Right, so that needs fixing - like I say if there's no reference being
taken we already have a problem and taking another reference later on
isn't going to fix it.

> >  Doing it while doing the match and register
> > seems simple and neat from an error handling point of view so having the
> > core take an additional reference during the registration would join up
> > with that.

> The main issue I have is that devm_regualtor_register is a bit
> awkward. With regulator_register you will always be calling
> regulator_unregister so you can put the of_node there but with
> devm there isn't really a good place to put the of_node.

That's why I suggested it might be OK to take a reference in the core -
this would allow the device probe to safely drop its reference before it
returns.

> Would perhaps a sensible thing here be to add an of_node_get to
> of_regulator_match, since we seem to be expecting that to
> increase the ref count. And then just add an of_node_put to
> regulator_unregister. And for anything directly using
> regulator_register/devm_regulator_register they should add a
> manual of_node_get?

That seems very ugly.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: core: Get and put regulator of_node

[Charles Keepax]

On Thu, Apr 03, 2014 at 12:14:31PM +0100, Mark Brown wrote:
> On Thu, Apr 03, 2014 at 11:58:04AM +0100, Charles Keepax wrote:
> > The main issue I have is that devm_regualtor_register is a bit
> > awkward. With regulator_register you will always be calling
> > regulator_unregister so you can put the of_node there but with
> > devm there isn't really a good place to put the of_node.
> 
> That's why I suggested it might be OK to take a reference in the core -
> this would allow the device probe to safely drop its reference before it
> returns.
> 
> > Would perhaps a sensible thing here be to add an of_node_get to
> > of_regulator_match, since we seem to be expecting that to
> > increase the ref count. And then just add an of_node_put to
> > regulator_unregister. And for anything directly using
> > regulator_register/devm_regulator_register they should add a
> > manual of_node_get?
> 
> That seems very ugly.

Agreed, it is not exactly made of clean interface success. So I
guess the sensible thing is to add a helper to clean up the
of_regulator_match results and add the node get in the regulator
core as per my original patch. I will fix up the commit message
for it and do patches for the other bits.

Thanks,
Charles