2014-01-29 04:03:05

by Manish Badarkhe

[permalink] [raw]
Subject: [PATCH V3 0/2] devm_* API operation for fixed regulator

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


2014-01-29 04:03:15

by Manish Badarkhe

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

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

Signed-off-by: Manish Badarkhe <[email protected]>
---
: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

2014-01-29 04:03:43

by Manish Badarkhe

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

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

Signed-off-by: Manish Badarkhe <[email protected]>
---
: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

2014-01-29 04:18:49

by Joe Perches

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

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

2014-01-29 11:08:22

by Manish Badarkhe

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

Hi Joe,

Thank you for your review.

On Wed, Jan 29, 2014 at 9:48 AM, Joe Perches <[email protected]> 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

2014-01-29 11:40:35

by Mark Brown

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

On Wed, Jan 29, 2014 at 04:38:20PM +0530, Manish Badarkhe wrote:
> On Wed, Jan 29, 2014 at 9:48 AM, Joe Perches <[email protected]> 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.


Attachments:
(No filename) (499.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-01-29 11:55:43

by Manish Badarkhe

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

Hi Mark,

On Wed, Jan 29, 2014 at 5:10 PM, Mark Brown <[email protected]> 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 <[email protected]> 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