[PATCH V3 0/2] devm_* API operation for fixed regulator

[PATCH V3 0/2] devm_* API operation for fixed regulator

[Manish Badarkhe]

Use devm_* API operations for fixed regulator driver so that
driver core will manage resources.

Also, introduce a new API "devm_kstrdup" and used it in fixed
regulator driver to manage resources.

Changes since V2:
 1. Update driver to use new API "devm_kstrdup" instead of
    "kstrdup".
 2. Added a seperate patch to introduce new API "devm_kstrdup"

Changes since V1:
 1. Updated driver to use "devm_kzalloc" instead of "kstrdup".
 2. Updated commit message.

Manish Badarkhe (2):
  devres: introduce API "devm_kstrdup"
  regulator: fixed: update to devm_* API

 drivers/base/devres.c     |   26 ++++++++++++++++++++++++++
 drivers/regulator/fixed.c |   44 ++++++++++++++------------------------------
 include/linux/device.h    |    2 ++
 3 files changed, 42 insertions(+), 30 deletions(-)

-- 
1.7.10.4

[PATCH V3 1/2] devres: introduce API "devm_kstrdup"

[Manish Badarkhe]

This patch introduces "devm_kstrdup" API so that the
device's driver can allocate memory and copy string.

Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
:100644 100644 545c4de... 6e88a3d... M	drivers/base/devres.c
:100644 100644 952b010... 8fee649... M	include/linux/device.h
 drivers/base/devres.c  |   26 ++++++++++++++++++++++++++
 include/linux/device.h |    2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 545c4de..6e88a3d 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -791,6 +791,32 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 EXPORT_SYMBOL_GPL(devm_kmalloc);
 
 /**
+ * devm_kstrdup - Allocate resource managed space and
+ *                and copy an existing string
+ * @dev: Device to allocate memory for
+ * @s: the string to duplicate
+ * @size: Allocation size
+ * @gfp: the GFP mask used in the devm_kzalloc() call when
+ *       allocating memory
+ * RETURNS:
+ * Pointer to allocated string on success, NULL on failure.
+ */
+char *devm_kstrdup(struct device *dev,
+		   const char *s, size_t size, gfp_t gfp)
+{
+	char *buf;
+
+	if (!s)
+		return NULL;
+
+	buf = devm_kzalloc(dev, size, gfp);
+	if (buf)
+		memcpy(buf, s, size);
+	return buf;
+}
+EXPORT_SYMBOL_GPL(devm_kstrdup);
+
+/**
  * devm_kfree - Resource-managed kfree
  * @dev: Device this memory belongs to
  * @p: Memory to free
diff --git a/include/linux/device.h b/include/linux/device.h
index 952b010..8fee649 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -626,6 +626,8 @@ static inline void *devm_kcalloc(struct device *dev,
 	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
 }
 extern void devm_kfree(struct device *dev, void *p);
+extern char *devm_kstrdup(struct device *dev, const char *s, size_t size,
+			gfp_t gfp);
 
 void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
 void __iomem *devm_request_and_ioremap(struct device *dev,
-- 
1.7.10.4

Re: [PATCH V3 1/2] devres: introduce API "devm_kstrdup"

[Joe Perches]

On Wed, 2014-01-29 at 09:33 +0530, Manish Badarkhe wrote:
> This patch introduces "devm_kstrdup" API so that the
> device's driver can allocate memory and copy string.
[]
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
[]
> @@ -791,6 +791,32 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
>  EXPORT_SYMBOL_GPL(devm_kmalloc);
>  
>  /**
> + * devm_kstrdup - Allocate resource managed space and
> + *                and copy an existing string
> + * @dev: Device to allocate memory for
> + * @s: the string to duplicate
> + * @size: Allocation size

Why is size necessary at all?
I think it should be calculated by strlen

> +char *devm_kstrdup(struct device *dev,
> +		   const char *s, size_t size, gfp_t gfp)
> +{
> +	char *buf;
> +
> +	if (!s)
> +		return NULL;
> +
> +	buf = devm_kzalloc(dev, size, gfp);

If this is really necessary, please use devm_kmalloc

Re: [PATCH V3 1/2] devres: introduce API "devm_kstrdup"

[Manish Badarkhe]

Hi Joe,

Thank you for your review.

On Wed, Jan 29, 2014 at 9:48 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-01-29 at 09:33 +0530, Manish Badarkhe wrote:
>> This patch introduces "devm_kstrdup" API so that the
>> device's driver can allocate memory and copy string.
> []
>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> []
>> @@ -791,6 +791,32 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
>>  EXPORT_SYMBOL_GPL(devm_kmalloc);
>>
>>  /**
>> + * devm_kstrdup - Allocate resource managed space and
>> + *                and copy an existing string
>> + * @dev: Device to allocate memory for
>> + * @s: the string to duplicate
>> + * @size: Allocation size
>
> Why is size necessary at all?
> I think it should be calculated by strlen

I thought of avoiding string length calculation in function.
But,yes its better to do it in function to avoid extra parsing
of argument to function.Will update code and
post a patch.

>> +char *devm_kstrdup(struct device *dev,
>> +                const char *s, size_t size, gfp_t gfp)
>> +{
>> +     char *buf;
>> +
>> +     if (!s)
>> +             return NULL;
>> +
>> +     buf = devm_kzalloc(dev, size, gfp);
>
> If this is really necessary, please use devm_kmalloc

devm_kzalloc is always better giving zeroed memory locations.
Is there any reason not to go for it?

Regards
Manish Badarkhe

Re: [PATCH V3 1/2] devres: introduce API "devm_kstrdup"

[Mark Brown]

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

On Wed, Jan 29, 2014 at 04:38:20PM +0530, Manish Badarkhe wrote:
> On Wed, Jan 29, 2014 at 9:48 AM, Joe Perches <joe@perches.com> wrote:

> >> +     buf = devm_kzalloc(dev, size, gfp);

> > If this is really necessary, please use devm_kmalloc

> devm_kzalloc is always better giving zeroed memory locations.
> Is there any reason not to go for it?

If the allocated memory is going to be immediately overwritten with the
string then it shouldn't make any difference if it was zeroed when
allocated.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V3 1/2] devres: introduce API "devm_kstrdup"

[Manish Badarkhe]

Hi Mark,

On Wed, Jan 29, 2014 at 5:10 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jan 29, 2014 at 04:38:20PM +0530, Manish Badarkhe wrote:
>> On Wed, Jan 29, 2014 at 9:48 AM, Joe Perches <joe@perches.com> wrote:
>
>> >> +     buf = devm_kzalloc(dev, size, gfp);
>
>> > If this is really necessary, please use devm_kmalloc
>
>> devm_kzalloc is always better giving zeroed memory locations.
>> Is there any reason not to go for it?
>
> If the allocated memory is going to be immediately overwritten with the
> string then it shouldn't make any difference if it was zeroed when
> allocated.

Thank you for clarification. I will go ahead to make it devm_kmalloc
and post a new version of patch.

Regards
Manish Badarkhe

[PATCH V3 2/2] regulator: fixed: update to devm_* API

[Manish Badarkhe]

Update the code to use devm_* API so that driver core will manage
resources.

Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
:100644 100644 5ea64b9... 1d42613... M	drivers/regulator/fixed.c
 drivers/regulator/fixed.c |   44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 5ea64b9..1d42613 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -132,15 +132,16 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 			       GFP_KERNEL);
 	if (drvdata == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate device data\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 
-	drvdata->desc.name = kstrdup(config->supply_name, GFP_KERNEL);
+	drvdata->desc.name = devm_kstrdup(&pdev->dev,
+					  config->supply_name,
+					  strlen(config->supply_name) + 1,
+					  GFP_KERNEL);
 	if (drvdata->desc.name == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate supply name\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 	drvdata->desc.type = REGULATOR_VOLTAGE;
 	drvdata->desc.owner = THIS_MODULE;
@@ -149,13 +150,14 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	drvdata->desc.enable_time = config->startup_delay;
 
 	if (config->input_supply) {
-		drvdata->desc.supply_name = kstrdup(config->input_supply,
-							GFP_KERNEL);
+		drvdata->desc.supply_name = devm_kstrdup(&pdev->dev,
+					    config->input_supply,
+					    strlen(config->input_supply) + 1,
+					    GFP_KERNEL);
 		if (!drvdata->desc.supply_name) {
 			dev_err(&pdev->dev,
 				"Failed to allocate input supply\n");
-			ret = -ENOMEM;
-			goto err_name;
+			return -ENOMEM;
 		}
 	}
 
@@ -186,11 +188,12 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	cfg.driver_data = drvdata;
 	cfg.of_node = pdev->dev.of_node;
 
-	drvdata->dev = regulator_register(&drvdata->desc, &cfg);
+	drvdata->dev = devm_regulator_register(&pdev->dev, &drvdata->desc,
+					       &cfg);
 	if (IS_ERR(drvdata->dev)) {
 		ret = PTR_ERR(drvdata->dev);
 		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
-		goto err_input;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, drvdata);
@@ -199,24 +202,6 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 		drvdata->desc.fixed_uV);
 
 	return 0;
-
-err_input:
-	kfree(drvdata->desc.supply_name);
-err_name:
-	kfree(drvdata->desc.name);
-err:
-	return ret;
-}
-
-static int reg_fixed_voltage_remove(struct platform_device *pdev)
-{
-	struct fixed_voltage_data *drvdata = platform_get_drvdata(pdev);
-
-	regulator_unregister(drvdata->dev);
-	kfree(drvdata->desc.supply_name);
-	kfree(drvdata->desc.name);
-
-	return 0;
 }
 
 #if defined(CONFIG_OF)
@@ -229,7 +214,6 @@ MODULE_DEVICE_TABLE(of, fixed_of_match);
 
 static struct platform_driver regulator_fixed_voltage_driver = {
 	.probe		= reg_fixed_voltage_probe,
-	.remove		= reg_fixed_voltage_remove,
 	.driver		= {
 		.name		= "reg-fixed-voltage",
 		.owner		= THIS_MODULE,
-- 
1.7.10.4