diff mbox

[1/4] regulator: qcom_spmi: Fix warning Bad of_node_put()

Message ID 20180716113525.9335-2-niklas.cassel@linaro.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Niklas Cassel July 16, 2018, 11:35 a.m. UTC
For of_find_node_by_name(), you typically pass what the previous call
returned. Therefore, of_find_node_by_name() increases the refcount of
the returned node, and decreases the refcount of the node passed as the
first argument.

However, in this case we don't pass what the previous call returned,
so we have to increase the refcount of the first argument to compensate.

Also add a missing of_node_put() for the returned value, since this was
previously being leaked.

OF: ERROR: Bad of_node_put() on /soc/qcom,spmi@400f000/pmic@3/regulators
CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W         4.18.0-rc4-00223-gefd7b360b70e #12
Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
Call trace:
 dump_backtrace+0x0/0x1a8
 show_stack+0x14/0x20
 dump_stack+0x90/0xb4
 of_node_release+0x74/0x78
 kobject_put+0x90/0x1f0
 of_node_put+0x14/0x20
 of_find_node_by_name+0x80/0xd8
 qcom_spmi_regulator_probe+0x30c/0x508

Fixes: 0caecaa87202 ("regulator: qcom_spmi: Add support for SAW")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 42 ++++++++++++++++++-------
 1 file changed, 30 insertions(+), 12 deletions(-)

Comments

Johan Hovold July 16, 2018, 12:01 p.m. UTC | #1
Hi Niklas,

On Mon, Jul 16, 2018 at 01:35:22PM +0200, Niklas Cassel wrote:
> For of_find_node_by_name(), you typically pass what the previous call
> returned. Therefore, of_find_node_by_name() increases the refcount of
> the returned node, and decreases the refcount of the node passed as the
> first argument.
> 
> However, in this case we don't pass what the previous call returned,
> so we have to increase the refcount of the first argument to compensate.

I don't think this is the right fix. of_find_node_by_name() should
generally not be used by drivers in the first place as it searches the
entire tree and can end up matching an entirely unrelated node.

I haven't looked at the device-tree binding in question, but you
probably want to use something like of_get_child_by_name() instead.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Niklas Cassel July 16, 2018, 1:29 p.m. UTC | #2
On Mon, Jul 16, 2018 at 02:01:34PM +0200, Johan Hovold wrote:
> Hi Niklas,
> 
> On Mon, Jul 16, 2018 at 01:35:22PM +0200, Niklas Cassel wrote:
> > For of_find_node_by_name(), you typically pass what the previous call
> > returned. Therefore, of_find_node_by_name() increases the refcount of
> > the returned node, and decreases the refcount of the node passed as the
> > first argument.
> > 
> > However, in this case we don't pass what the previous call returned,
> > so we have to increase the refcount of the first argument to compensate.
> 
> I don't think this is the right fix. of_find_node_by_name() should
> generally not be used by drivers in the first place as it searches the
> entire tree and can end up matching an entirely unrelated node.
> 
> I haven't looked at the device-tree binding in question, but you
> probably want to use something like of_get_child_by_name() instead.
> 

Hello Johan,

of_find_node_by_name() will only search the whole tree if the
first argument is NULL, which isn't the case here.

However, of_get_child_by_name() is indeed better suited here.
Will send out a v2.

Thank you for your feedback, it is much appreciated :)

Kind regards,
Niklas
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold July 16, 2018, 2:23 p.m. UTC | #3
On Mon, Jul 16, 2018 at 03:29:31PM +0200, Niklas Cassel wrote:
> On Mon, Jul 16, 2018 at 02:01:34PM +0200, Johan Hovold wrote:
> > Hi Niklas,
> > 
> > On Mon, Jul 16, 2018 at 01:35:22PM +0200, Niklas Cassel wrote:
> > > For of_find_node_by_name(), you typically pass what the previous call
> > > returned. Therefore, of_find_node_by_name() increases the refcount of
> > > the returned node, and decreases the refcount of the node passed as the
> > > first argument.
> > > 
> > > However, in this case we don't pass what the previous call returned,
> > > so we have to increase the refcount of the first argument to compensate.
> > 
> > I don't think this is the right fix. of_find_node_by_name() should
> > generally not be used by drivers in the first place as it searches the
> > entire tree and can end up matching an entirely unrelated node.
> > 
> > I haven't looked at the device-tree binding in question, but you
> > probably want to use something like of_get_child_by_name() instead.
> > 
> 
> Hello Johan,
> 
> of_find_node_by_name() will only search the whole tree if the
> first argument is NULL, which isn't the case here.

It's searching the entire tree *starting* at its first argument, which
means you may end up matching a completely unrelated node (i.e. not a
child or even descendant) elsewhere in the tree.

> However, of_get_child_by_name() is indeed better suited here.
> Will send out a v2.

Unless you are doing a tree-wide search, using of_get_child_by_name() is
simply wrong.

I fixed up most of these bugs a few releases ago, but they keep on
creeping in.

> Thank you for your feedback, it is much appreciated :)

No worries.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 9817f1a75342..8b921b1b1df5 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1752,7 +1752,8 @@  static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 	const char *name;
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
-	struct device_node *syscon;
+	struct device_node *syscon, *reg_node;
+	struct property *reg_prop;
 	int ret, lenp;
 	struct list_head *vreg_list;
 
@@ -1780,10 +1781,18 @@  static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 
 	for (reg = match->data; reg->name; reg++) {
 
-		if (saw_regmap && \
-		    of_find_property(of_find_node_by_name(node, reg->name), \
-				     "qcom,saw-slave", &lenp)) {
-			continue;
+		if (saw_regmap) {
+			/*
+			 * Compensate for of_node_put() in
+			 * of_find_node_by_name()
+			 */
+			of_node_get(node);
+			reg_node = of_find_node_by_name(node, reg->name);
+			reg_prop = of_find_property(reg_node, "qcom,saw-slave",
+						    &lenp);
+			of_node_put(reg_node);
+			if (reg_prop)
+				continue;
 		}
 
 		vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
@@ -1816,13 +1825,22 @@  static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 		if (ret)
 			continue;
 
-		if (saw_regmap && \
-		    of_find_property(of_find_node_by_name(node, reg->name), \
-				     "qcom,saw-leader", &lenp)) {
-			spmi_saw_ops = *(vreg->desc.ops);
-			spmi_saw_ops.set_voltage_sel = \
-				spmi_regulator_saw_set_voltage;
-			vreg->desc.ops = &spmi_saw_ops;
+		if (saw_regmap) {
+			/*
+			 * Compensate for of_node_put() in
+			 * of_find_node_by_name()
+			 */
+			of_node_get(node);
+			reg_node = of_find_node_by_name(node, reg->name);
+			reg_prop = of_find_property(reg_node, "qcom,saw-leader",
+						    &lenp);
+			of_node_put(reg_node);
+			if (reg_prop) {
+				spmi_saw_ops = *(vreg->desc.ops);
+				spmi_saw_ops.set_voltage_sel =
+					spmi_regulator_saw_set_voltage;
+				vreg->desc.ops = &spmi_saw_ops;
+			}
 		}
 
 		config.dev = dev;