[PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Sangjung Woo]

In order to be free automatically and make the cleanup paths more
simple, use devm_kzalloc() instead of kzalloc().

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/rtc/rtc-pl030.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
index 22bacdb..ecd57c6 100644
--- a/drivers/rtc/rtc-pl030.c
+++ b/drivers/rtc/rtc-pl030.c
@@ -106,7 +106,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret)
 		goto err_req;
 
-	rtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
+	rtc = devm_kzalloc(&dev->dev, sizeof(*rtc), GFP_KERNEL);
 	if (!rtc) {
 		ret = -ENOMEM;
 		goto err_rtc;
@@ -115,7 +115,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
 	rtc->base = ioremap(dev->res.start, resource_size(&dev->res));
 	if (!rtc->base) {
 		ret = -ENOMEM;
-		goto err_map;
+		goto err_rtc;
 	}
 
 	__raw_writel(0, rtc->base + RTC_CR);
@@ -141,8 +141,6 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
 	free_irq(dev->irq[0], rtc);
  err_irq:
 	iounmap(rtc->base);
- err_map:
-	kfree(rtc);
  err_rtc:
 	amba_release_regions(dev);
  err_req:
@@ -160,7 +158,6 @@ static int pl030_remove(struct amba_device *dev)
 	free_irq(dev->irq[0], rtc);
 	rtc_device_unregister(rtc->rtc);
 	iounmap(rtc->base);
-	kfree(rtc);
 	amba_release_regions(dev);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Joe Perches]

On Wed, 2013-10-09 at 13:00 +0900, Sangjung Woo wrote:
> In order to be free automatically and make the cleanup paths more
> simple, use devm_kzalloc() instead of kzalloc().
[]
> diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
[]
> @@ -106,7 +106,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
>  	if (ret)
>  		goto err_req;
>  
> -	rtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
> +	rtc = devm_kzalloc(&dev->dev, sizeof(*rtc), GFP_KERNEL);

You're not deleting a memset and you're
converting a kmalloc.

Why do you need the zalloc version?

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[sangjung.woo]

On 10/09/2013 01:07 PM, Joe Perches wrote:
> On Wed, 2013-10-09 at 13:00 +0900, Sangjung Woo wrote:
>> In order to be free automatically and make the cleanup paths more
>> simple, use devm_kzalloc() instead of kzalloc().
> []
>> diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
> []
>> @@ -106,7 +106,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
>>   	if (ret)
>>   		goto err_req;
>>   
>> -	rtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
>> +	rtc = devm_kzalloc(&dev->dev, sizeof(*rtc), GFP_KERNEL);

First of all, thanks for your review.

> You're not deleting a memset and you're
> converting a kmalloc.
You are right.
>
> Why do you need the zalloc version?
>
The key point of this patch is resource-managed memory allocation.
As you already know, memory space that is allocated by devm_kzalloc() 
function
is automatically freed on driver detach. That makes the code tidy and
reduces human's mistakes not to kfree().

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Joe Perches]

On Wed, 2013-10-09 at 13:36 +0900, sangjung.woo wrote:
> On 10/09/2013 01:07 PM, Joe Perches wrote:
> > On Wed, 2013-10-09 at 13:00 +0900, Sangjung Woo wrote:
> >> In order to be free automatically and make the cleanup paths more
> >> simple, use devm_kzalloc() instead of kzalloc().
> > []
> >> diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
> > []
> >> @@ -106,7 +106,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
> >>   	if (ret)
> >>   		goto err_req;
> >>   
> >> -	rtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
> >> +	rtc = devm_kzalloc(&dev->dev, sizeof(*rtc), GFP_KERNEL);
[]
> > You're not deleting a memset and you're
> > converting a kmalloc.
> You are right.
> >
> > Why do you need the zalloc version?
> >
> The key point of this patch is resource-managed memory allocation.

The commit message doesn't match the patch subject
(shows kzalloc)

I was a bit surprised to find there isn't a devm_kmalloc.

This seems fine otherwise.

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[sangjung.woo]

On 10/09/2013 01:59 PM, Joe Perches wrote:
> The commit message doesn't match the patch subject
> (shows kzalloc)
>
> I was a bit surprised to find there isn't a devm_kmalloc.
>
> This seems fine otherwise.

I just sent the second patch file after modifying the commit message.
Thank you for your opinion.

Cheers,
Sangjung Woo
>

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Andrew Morton]

On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:

> I was a bit surprised to find there isn't a devm_kmalloc.

Yes, the unconditional memset is silly.  Especially when the
function has a handy gfp_t and could be passed __GFP_ZERO.

The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
always use kzalloc".  There's no explanation for this - it looks like
some ideological thing.


--- a/drivers/base/devres.c~a
+++ a/drivers/base/devres.c
@@ -91,7 +91,8 @@ static __always_inline struct devres * a
 	if (unlikely(!dr))
 		return NULL;
 
-	memset(dr, 0, tot_size);
+	if (gfp & __GFP_ZERO)
+		memset(dr, 0, tot_size);
 	INIT_LIST_HEAD(&dr->node.entry);
 	dr->node.release = release;
 	return dr;
@@ -770,7 +771,7 @@ static int devm_kzalloc_match(struct dev
  * RETURNS:
  * Pointer to allocated memory on success, NULL on failure.
  */
-void * devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
+static void *__devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	struct devres *dr;
 
@@ -783,8 +784,19 @@ void * devm_kzalloc(struct device *dev,
 	devres_add(dev, dr->data);
 	return dr->data;
 }
+
+void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
+{
+	return __devm_kzalloc(dev, size, gfp | __GFP_ZERO);
+}
 EXPORT_SYMBOL_GPL(devm_kzalloc);
 
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
+{
+	return __devm_kzalloc(dev, size, gfp);
+}
+EXPORT_SYMBOL_GPL(devm_kmalloc);
+
 /**
  * devm_kfree - Resource-managed kfree
  * @dev: Device this memory belongs to
--- a/include/linux/device.h~a
+++ a/include/linux/device.h
@@ -602,8 +602,9 @@ extern void devres_close_group(struct de
 extern void devres_remove_group(struct device *dev, void *id);
 extern int devres_release_group(struct device *dev, void *id);
 
-/* managed kzalloc/kfree for device drivers, no kmalloc, always use kzalloc */
+/* managed kmalloc/kzalloc/kfree for device drivers */
 extern void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp);
+extern void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp);
 extern void devm_kfree(struct device *dev, void *p);
 
 void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
_

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Joe Perches]

On Thu, 2013-10-10 at 16:06 -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > I was a bit surprised to find there isn't a devm_kmalloc.
> 
> Yes, the unconditional memset is silly.  Especially when the
> function has a handy gfp_t and could be passed __GFP_ZERO.
> 
> The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> always use kzalloc".  There's no explanation for this - it looks like
> some ideological thing.

Try this patch instead:
https://lkml.org/lkml/2013/10/9/14

Yours has an unnecessary duplicate memset of the whole
block when __GFP_ZERO is passed when that's already
done by the kmalloc_track_caller allocator.

Also if __GFP_ZERO is not passed, you should still
zero the struct devres header.

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Andrew Morton]

On Thu, 10 Oct 2013 16:14:00 -0700 Joe Perches <joe@perches.com> wrote:

> On Thu, 2013-10-10 at 16:06 -0700, Andrew Morton wrote:
> > On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > I was a bit surprised to find there isn't a devm_kmalloc.
> > 
> > Yes, the unconditional memset is silly.  Especially when the
> > function has a handy gfp_t and could be passed __GFP_ZERO.
> > 
> > The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> > always use kzalloc".  There's no explanation for this - it looks like
> > some ideological thing.
> 
> Try this patch instead:
> https://lkml.org/lkml/2013/10/9/14
> 

That looks rather better.

Apart from forcing a needless memset, the current code will defeat
kmemcheck used-uninitialized checking.

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Tejun Heo]

Hello,

On Thu, Oct 10, 2013 at 04:06:03PM -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > I was a bit surprised to find there isn't a devm_kmalloc.
> 
> Yes, the unconditional memset is silly.  Especially when the
> function has a handy gfp_t and could be passed __GFP_ZERO.
> 
> The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> always use kzalloc".  There's no explanation for this - it looks like
> some ideological thing.

Skipping resetting just is highly unlikely to make any meaningful
difference in the paths devres is supposed to be used.  These don't
and shouldn't get called from hot paths.  Wouldn't it be more
idelogical to have something just because it feels weird not to have
it even if it is not necessary?  I don't necessarily object to having
non-zeroing interface but it seems rather pointless.  Do we have an
actual cases where this makes meaningful differences?

> -	memset(dr, 0, tot_size);
> +	if (gfp & __GFP_ZERO)
> +		memset(dr, 0, tot_size);

I'd much prefer to have at least the devres_node part cleared
regardless of __GFP_ZERO and Joe already has better patches doing
this.

Thanks.

-- 
tejun

Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

[Joe Perches]

On Thu, 2013-10-10 at 19:18 -0400, Tejun Heo wrote:
> Do we have an
> actual cases where this makes meaningful differences?

There are already a few array allocations
where the array is completely reinitialized.

Does it matter?  Shrug.

It's more API compatible and more symmetric.

Direct conversions of kmalloc blocks don't
involve kmalloc->kzalloc translations, so
it'll be bug-for-bug compatible.