[PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Currently when of_get_parent() or syscon_node_to_regmap() fail 
'kfree(sreg->name)' is not called, which is incorrect.

Fix this by jumping to 'anatop_probe_end' instead.

While at it, make 'anatop_probe_end' to be executed only on the error path, so
that the code can be a bit easier to follow.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Fix the error path for of_get_parent() or syscon_node_to_regmap().

 drivers/regulator/anatop-regulator.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index c734d09..88d95cd 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -132,12 +132,16 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->owner = THIS_MODULE;
 
 	anatop_np = of_get_parent(np);
-	if (!anatop_np)
-		return -ENODEV;
+	if (!anatop_np) {
+		ret = -ENODEV;
+		goto anatop_probe_end;
+	}
 	sreg->anatop = syscon_node_to_regmap(anatop_np);
 	of_node_put(anatop_np);
-	if (IS_ERR(sreg->anatop))
-		return PTR_ERR(sreg->anatop);
+	if (IS_ERR(sreg->anatop)) {
+		ret = PTR_ERR(sreg->anatop);
+		goto anatop_probe_end;
+	}
 
 	ret = of_property_read_u32(np, "anatop-reg-offset",
 				   &sreg->control_reg);
@@ -210,9 +214,10 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rdev);
 
+	return 0;
+
 anatop_probe_end:
-	if (ret)
-		kfree(sreg->name);
+	kfree(sreg->name);
 
 	return ret;
 }
-- 
1.8.1.2

[PATCH v3 2/2] regulator: anatop-regulator: Remove unneeded variable

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

There is no need to create a local variable for accessing sreg->name.

Access it directly instead.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- None

 drivers/regulator/anatop-regulator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 88d95cd..cc545a0 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -226,9 +226,8 @@ static int anatop_regulator_remove(struct platform_device *pdev)
 {
 	struct regulator_dev *rdev = platform_get_drvdata(pdev);
 	struct anatop_regulator *sreg = rdev_get_drvdata(rdev);
-	const char *name = sreg->name;
 
-	kfree(name);
+	kfree(sreg->name);
 
 	return 0;
 }
-- 
1.8.1.2

Re: [PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Shawn Guo]

On Mon, Dec 23, 2013 at 12:44:40PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Currently when of_get_parent() or syscon_node_to_regmap() fail 
> 'kfree(sreg->name)' is not called, which is incorrect.
> 
> Fix this by jumping to 'anatop_probe_end' instead.
> 
> While at it, make 'anatop_probe_end' to be executed only on the error path, so
> that the code can be a bit easier to follow.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Mark Brown]

On Mon, Dec 23, 2013 at 12:44:40PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Currently when of_get_parent() or syscon_node_to_regmap() fail 
> 'kfree(sreg->name)' is not called, which is incorrect.

Applied both, thanks.

Re: [PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Dmitry Torokhov]

On Tue, Dec 24, 2013 at 01:03:57PM +0000, Mark Brown wrote:
> On Mon, Dec 23, 2013 at 12:44:40PM -0200, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Currently when of_get_parent() or syscon_node_to_regmap() fail 
> > 'kfree(sreg->name)' is not called, which is incorrect.
> 
> Applied both, thanks.

Sorry for being too later, but why are we making copy to begin with? It
is not like DT data will disappear later on.

Thanks.

-- 
Dmitry

Re: [PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Fabio Estevam]

Hi Dmitry,

On Fri, Jan 3, 2014 at 10:02 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Dec 24, 2013 at 01:03:57PM +0000, Mark Brown wrote:
>> On Mon, Dec 23, 2013 at 12:44:40PM -0200, Fabio Estevam wrote:
>> > From: Fabio Estevam <fabio.estevam@freescale.com>
>> >
>> > Currently when of_get_parent() or syscon_node_to_regmap() fail
>> > 'kfree(sreg->name)' is not called, which is incorrect.
>>
>> Applied both, thanks.
>
> Sorry for being too later, but why are we making copy to begin with? It
> is not like DT data will disappear later on.

Which lines of code are you referring to, please?

Regards,

Fabio Estevam

Re: [PATCH v3 1/2] regulator: anatop-regulator: Fix the error handling on probe

[Dmitry Torokhov]

On Sat, Jan 04, 2014 at 02:00:52PM -0200, Fabio Estevam wrote:
> Hi Dmitry,
> 
> On Fri, Jan 3, 2014 at 10:02 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Dec 24, 2013 at 01:03:57PM +0000, Mark Brown wrote:
> >> On Mon, Dec 23, 2013 at 12:44:40PM -0200, Fabio Estevam wrote:
> >> > From: Fabio Estevam <fabio.estevam@freescale.com>
> >> >
> >> > Currently when of_get_parent() or syscon_node_to_regmap() fail
> >> > 'kfree(sreg->name)' is not called, which is incorrect.
> >>
> >> Applied both, thanks.
> >
> > Sorry for being too later, but why are we making copy to begin with? It
> > is not like DT data will disappear later on.
> 
> Which lines of code are you referring to, please?

I am talking about this:

	sreg->name = kstrdup(of_get_property(np, "regulator-name", NULL),
			     GFP_KERNEL);

Whatis the reason for kstrdup? Can we simply say:

	sreg->name = of_get_property(np, "regulator-name", NULL);

Thanks.

-- 
Dmitry