[PATCH 0/4] regulator: qcom_spmi: fix Bad of_node_put() splat

[PATCH 0/4] regulator: qcom_spmi: fix Bad of_node_put() splat

[Niklas Cassel]

Fix Bad_of_node_put() splat, and some minor checkpatch issues.

Niklas Cassel (4):
  regulator: qcom_spmi: Fix warning Bad of_node_put()
  regulator: qcom_spmi: Use correct regmap when checking for error
  regulator: qcom_spmi: Do not initialise static to NULL
  regulator: qcom_spmi: Indent with tabs instead of spaces

 drivers/regulator/qcom_spmi-regulator.c | 48 +++++++++++++++++--------
 1 file changed, 33 insertions(+), 15 deletions(-)

-- 
2.17.1

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

[Niklas Cassel]

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(-)

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;
-- 
2.17.1

[PATCH 2/4] regulator: qcom_spmi: Use correct regmap when checking for error

[Niklas Cassel]

Since we have just assigned saw_regmap, and since the error message
refers to saw_regmap, it feels safe to assume that it is saw_regmap,
and not regmap, that should be checked for errors.

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

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 8b921b1b1df5..054c0245aec1 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1775,7 +1775,7 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 		syscon = of_parse_phandle(node, "qcom,saw-reg", 0);
 		saw_regmap = syscon_node_to_regmap(syscon);
 		of_node_put(syscon);
-		if (IS_ERR(regmap))
+		if (IS_ERR(saw_regmap))
 			dev_err(dev, "ERROR reading SAW regmap\n");
 	}
 
-- 
2.17.1

[PATCH 3/4] regulator: qcom_spmi: Do not initialise static to NULL

[Niklas Cassel]

Fix the following checkpatch error:

ERROR: do not initialise statics to NULL
+static struct regmap *saw_regmap = NULL;

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

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 054c0245aec1..dbe6ecc34859 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1060,7 +1060,7 @@ static irqreturn_t spmi_regulator_vs_ocp_isr(int irq, void *data)
 #define SAW3_AVS_CTL_TGGL_MASK	0x8000000
 #define SAW3_AVS_CTL_CLEAR_MASK	0x7efc00
 
-static struct regmap *saw_regmap = NULL;
+static struct regmap *saw_regmap;
 
 static void spmi_saw_set_vdd(void *data)
 {
-- 
2.17.1

[PATCH 4/4] regulator: qcom_spmi: Indent with tabs instead of spaces

[Niklas Cassel]

Fix the following checkpatch error:

ERROR: code indent should use tabs where possible
+        { }$

Fixes: ca5cd8c9400c ("regulator: qcom_spmi: Add support for pmi8994")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index dbe6ecc34859..a4a6c1711bc4 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1728,7 +1728,7 @@ static const struct spmi_regulator_data pmi8994_regulators[] = {
 	{ "s2", 0x1700, "vdd_s2", },
 	{ "s3", 0x1a00, "vdd_s3", },
 	{ "l1", 0x4000, "vdd_l1", },
-        { }
+	{ }
 };
 
 static const struct of_device_id qcom_spmi_regulator_match[] = {
-- 
2.17.1

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

[Johan Hovold]

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

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

[Niklas Cassel]

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

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

[Johan Hovold]

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

Applied "regulator: qcom_spmi: Indent with tabs instead of spaces" to the regulator tree

[Mark Brown]

The patch

   regulator: qcom_spmi: Indent with tabs instead of spaces

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 37164571fb25adef80ae2aaf55840b0105c15b22 Mon Sep 17 00:00:00 2001
From: Niklas Cassel <niklas.cassel@linaro.org>
Date: Mon, 16 Jul 2018 15:32:54 +0200
Subject: [PATCH] regulator: qcom_spmi: Indent with tabs instead of spaces

Fix the following checkpatch error:

ERROR: code indent should use tabs where possible
+        { }$

Fixes: ca5cd8c9400c ("regulator: qcom_spmi: Add support for pmi8994")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 5246f5b870c2..53a61fb65642 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1728,7 +1728,7 @@ static const struct spmi_regulator_data pmi8994_regulators[] = {
 	{ "s2", 0x1700, "vdd_s2", },
 	{ "s3", 0x1a00, "vdd_s3", },
 	{ "l1", 0x4000, "vdd_l1", },
-        { }
+	{ }
 };
 
 static const struct of_device_id qcom_spmi_regulator_match[] = {
-- 
2.18.0

Applied "regulator: qcom_spmi: Do not initialise static to NULL" to the regulator tree

[Mark Brown]

The patch

   regulator: qcom_spmi: Do not initialise static to NULL

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 9689ca0af345b82d06a02e45cc214c1b8bad9e8d Mon Sep 17 00:00:00 2001
From: Niklas Cassel <niklas.cassel@linaro.org>
Date: Mon, 16 Jul 2018 15:32:53 +0200
Subject: [PATCH] regulator: qcom_spmi: Do not initialise static to NULL

Fix the following checkpatch error:

ERROR: do not initialise statics to NULL
+static struct regmap *saw_regmap = NULL;

Fixes: 0caecaa87202 ("regulator: qcom_spmi: Add support for SAW")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index ba3d5e63ada6..5246f5b870c2 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1060,7 +1060,7 @@ static irqreturn_t spmi_regulator_vs_ocp_isr(int irq, void *data)
 #define SAW3_AVS_CTL_TGGL_MASK	0x8000000
 #define SAW3_AVS_CTL_CLEAR_MASK	0x7efc00
 
-static struct regmap *saw_regmap = NULL;
+static struct regmap *saw_regmap;
 
 static void spmi_saw_set_vdd(void *data)
 {
-- 
2.18.0

Applied "regulator: qcom_spmi: Use correct regmap when checking for error" to the regulator tree

[Mark Brown]

The patch

   regulator: qcom_spmi: Use correct regmap when checking for error

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 85046a15529606466bc778e1205f4cab8e3724d1 Mon Sep 17 00:00:00 2001
From: Niklas Cassel <niklas.cassel@linaro.org>
Date: Mon, 16 Jul 2018 15:32:52 +0200
Subject: [PATCH] regulator: qcom_spmi: Use correct regmap when checking for
 error

Since we have just assigned saw_regmap, and since the error message
refers to saw_regmap, it feels safe to assume that it is saw_regmap,
and not regmap, that should be checked for errors.

Fixes: 0caecaa87202 ("regulator: qcom_spmi: Add support for SAW")
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 4cc14c65d86b..ba3d5e63ada6 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1775,7 +1775,7 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 		syscon = of_parse_phandle(node, "qcom,saw-reg", 0);
 		saw_regmap = syscon_node_to_regmap(syscon);
 		of_node_put(syscon);
-		if (IS_ERR(regmap))
+		if (IS_ERR(saw_regmap))
 			dev_err(dev, "ERROR reading SAW regmap\n");
 	}
 
-- 
2.18.0