[PATCH 0/2] regulator: gpio: fix error paths in probe()

[PATCH 0/2] regulator: gpio: fix error paths in probe()

[Harald Geyer]

Currently resources aren't freed in the order in which they have been
allocated. Eg probe() frees drvdata->states even if it failed before
allocating that. I think allocating/copying the affacted data structures
isn't necessary anyway, so this is the easiest way to fix this.

Patch 1/2 is the actual fix.
Patch 2/2 is minor cleanup while looking at the code.

This series has only been compile tested yet. I will test it on
device-tree HW when I get access to my test equipment. I can't test
non-device-tree code however.

Other notes:
* This driver passes &drvdata->desc uninitialized (but zeroed) to
  of_get_gpio_regulator_config() - I don't know if this is ok.
* I'm not sure if copying config->supply_name is actually necessary,
  so I left that one in.

Harald Geyer (2):
  regulator: gpio: Avoid unnecessarily copying data structures in
    probe()
  regulator: gpio: Move last remaining memory allocation to devres

 drivers/regulator/gpio-regulator.c | 46 +++++++++-----------------------------
 1 file changed, 10 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH 1/2] regulator: gpio: Avoid unnecessarily copying data structures in probe()

[Harald Geyer]

The data structures either have been copied in
of_get_gpio_regulator_config() already or are part of platform data,
which we keep a pointer to for the life time of the device anyway.

As a side effect this fixes the cleanup pathes if probe() fails.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/regulator/gpio-regulator.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 7bba8b7..5e2e14d 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -271,33 +271,18 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	}
 
 	if (config->nr_gpios != 0) {
-		drvdata->gpios = kmemdup(config->gpios,
-					 config->nr_gpios * sizeof(struct gpio),
-					 GFP_KERNEL);
-		if (drvdata->gpios == NULL) {
-			dev_err(&pdev->dev, "Failed to allocate gpio data\n");
-			ret = -ENOMEM;
-			goto err_name;
-		}
+		drvdata->gpios = config->gpios;
 
 		drvdata->nr_gpios = config->nr_gpios;
 		ret = gpio_request_array(drvdata->gpios, drvdata->nr_gpios);
 		if (ret) {
 			dev_err(&pdev->dev,
 			"Could not obtain regulator setting GPIOs: %d\n", ret);
-			goto err_memstate;
+			goto err_name;
 		}
 	}
 
-	drvdata->states = kmemdup(config->states,
-				  config->nr_states *
-					 sizeof(struct gpio_regulator_state),
-				  GFP_KERNEL);
-	if (drvdata->states == NULL) {
-		dev_err(&pdev->dev, "Failed to allocate state data\n");
-		ret = -ENOMEM;
-		goto err_memgpio;
-	}
+	drvdata->states = config->states;
 	drvdata->nr_states = config->nr_states;
 
 	drvdata->desc.owner = THIS_MODULE;
@@ -317,7 +302,7 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	default:
 		dev_err(&pdev->dev, "No regulator type set\n");
 		ret = -EINVAL;
-		goto err_memgpio;
+		goto err_gpio;
 	}
 
 	/* build initial state from gpio init data. */
@@ -354,19 +339,15 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	if (IS_ERR(drvdata->dev)) {
 		ret = PTR_ERR(drvdata->dev);
 		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
-		goto err_stategpio;
+		goto err_gpio;
 	}
 
 	platform_set_drvdata(pdev, drvdata);
 
 	return 0;
 
-err_stategpio:
+err_gpio:
 	gpio_free_array(drvdata->gpios, drvdata->nr_gpios);
-err_memstate:
-	kfree(drvdata->states);
-err_memgpio:
-	kfree(drvdata->gpios);
 err_name:
 	kfree(drvdata->desc.name);
 err:
@@ -381,9 +362,6 @@ static int gpio_regulator_remove(struct platform_device *pdev)
 
 	gpio_free_array(drvdata->gpios, drvdata->nr_gpios);
 
-	kfree(drvdata->states);
-	kfree(drvdata->gpios);
-
 	kfree(drvdata->desc.name);
 
 	return 0;
-- 
2.1.4

Re: [PATCH 1/2] regulator: gpio: Avoid unnecessarily copying data structures in probe()

[Mark Brown]

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

On Thu, Jan 28, 2016 at 07:55:17PM +0000, Harald Geyer wrote:
> The data structures either have been copied in
> of_get_gpio_regulator_config() already or are part of platform data,
> which we keep a pointer to for the life time of the device anyway.

The point of this code is to avoid referring to platform data after
probe for robustness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: gpio: Avoid unnecessarily copying data structures in probe()

[Harald Geyer]

Mark Brown writes:
> On Thu, Jan 28, 2016 at 07:55:17PM +0000, Harald Geyer wrote:
> > The data structures either have been copied in
> > of_get_gpio_regulator_config() already or are part of platform data,
> > which we keep a pointer to for the life time of the device anyway.
> 
> The point of this code is to avoid referring to platform data after
> probe for robustness.

Well, if we can't rely on platform data staying available after probe()
then probably we shouldn't keep a pointer to it - but thats a different
issue, so lets focus on the original problem:

What's your preferred way to fix the error path of probe() then?

Harald

Re: [PATCH 1/2] regulator: gpio: Avoid unnecessarily copying data structures in probe()

[Mark Brown]

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

On Fri, Jan 29, 2016 at 05:25:28PM +0100, Harald Geyer wrote:
> Mark Brown writes:
> > On Thu, Jan 28, 2016 at 07:55:17PM +0000, Harald Geyer wrote:
> > > The data structures either have been copied in
> > > of_get_gpio_regulator_config() already or are part of platform data,
> > > which we keep a pointer to for the life time of the device anyway.

> > The point of this code is to avoid referring to platform data after
> > probe for robustness.

> Well, if we can't rely on platform data staying available after probe()
> then probably we shouldn't keep a pointer to it - but thats a different
> issue, so lets focus on the original problem:

> What's your preferred way to fix the error path of probe() then?

Add the matching frees?  I'm not sure I know what the original problem
is mind you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/2] regulator: gpio: Move last remaining memory allocation to devres

[Harald Geyer]

Simple cleanup without functional changes.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/regulator/gpio-regulator.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 5e2e14d..5fe4921 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -263,11 +263,11 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 			return PTR_ERR(config);
 	}
 
-	drvdata->desc.name = kstrdup(config->supply_name, GFP_KERNEL);
+	drvdata->desc.name = devm_kstrdup(&pdev->dev, config->supply_name,
+					  GFP_KERNEL);
 	if (drvdata->desc.name == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate supply name\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 
 	if (config->nr_gpios != 0) {
@@ -278,7 +278,7 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev,
 			"Could not obtain regulator setting GPIOs: %d\n", ret);
-			goto err_name;
+			return ret;
 		}
 	}
 
@@ -348,9 +348,7 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 
 err_gpio:
 	gpio_free_array(drvdata->gpios, drvdata->nr_gpios);
-err_name:
-	kfree(drvdata->desc.name);
-err:
+
 	return ret;
 }
 
@@ -362,8 +360,6 @@ static int gpio_regulator_remove(struct platform_device *pdev)
 
 	gpio_free_array(drvdata->gpios, drvdata->nr_gpios);
 
-	kfree(drvdata->desc.name);
-
 	return 0;
 }
 
-- 
2.1.4