regulator: core: Get and put regulator of_node

PATCH In review Charles Keepax 2 participants 7 messages Apr 2, 2014 – Apr 3, 2014 Voltage and current regulator framework

lore

Share & tools

lore

Charles Keepax

author

2014-04-02 14:16 UTC #0 lore raw

PATCH 1/1 regulator: core: Get and put regulator of_node

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 <[email protected]>

---
 drivers/regulator/core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/regulator/core.c +2 -1

index afca1bc..e26372a 100644
@@ -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);
 }

Signature · 1 line

1.7.2.5

Mark Brown

maintainer

2014-04-02 14:24 UTC #1 lore raw

Quoted 2 lines · 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.

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.

Charles Keepax

author

2014-04-02 16:04 UTC #2 lore raw

Quoted 10 lines · Mark Brown

Quoted 2 lines · 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.
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

Mark Brown

maintainer

2014-04-02 16:54 UTC #3 lore raw

Quoted 9 lines · Charles Keepax

Quoted 6 lines · Mark Brown
Quoted 2 lines · 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.
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.

Quoted 7 lines

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?

Quoted 7 lines

	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.

Charles Keepax

author

2014-04-03 10:58 UTC #4 lore raw

Quoted 11 lines · Mark Brown

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.

Quoted 4 lines

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

Mark Brown

maintainer

2014-04-03 11:14 UTC #5 lore raw

Quoted 12 lines · Charles Keepax

Quoted 9 lines · Mark Brown
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).

Quoted 7 lines

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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 6 lines

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.

Charles Keepax

author

2014-04-03 11:48 UTC #6 lore raw

Quoted 10 lines · Mark Brown

Quoted 4 lines · Charles Keepax
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.

Quoted 8 lines

Quoted 6 lines
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