[PATCH 1/2] omap3: sr: fix memory leak and simplify the code

[PATCH 1/2] omap3: sr: fix memory leak and simplify the code

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

This patch fixes the following problem indicated by kmemleak:

kmemleak: unreferenced object 0xdf93c280 (size 64):
kmemleak:   backtrace:
kmemleak:     [<c00df6d4>] create_object+0x104/0x200
kmemleak:     [<c00d7638>] kmem_cache_alloc+0xe4/0xf4
kmemleak:     [<c000fc38>] omap_devinit_smartreflex+0x44/0x244
kmemleak:     [<c003a33c>] do_one_initcall+0x5c/0x1b8
kmemleak:     [<c00083fc>] kernel_init+0x94/0x110
kmemleak:     [<c003ba04>] kernel_thread_exit+0x0/0x8

The reason is that 'omap_devinit_smartreflex()' allocates 'sr_data',
then passes it to 'omap_device_build()', which 'kmemdup()'s it and
uses the copy. But 'omap_devinit_smartreflex()' never frees 'sr_data'.

This patch make 'sr_data' to be a stack variable, which eliminates
the memory leak and simplifies the code a bit.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 arch/arm/mach-omap2/sr_device.c |   27 +++++++++------------------
 1 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 24630ef..d1bb495 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -129,7 +129,7 @@ static int __init omap_devinit_smartreflex(void)
 	char *name = "smartreflex";
 
 	do {
-		struct omap_smartreflex_data *sr_data;
+		struct omap_smartreflex_data sr_data;
 		struct omap_smartreflex_dev_data *sr_dev_data;
 		struct omap_device *od;
 		struct omap_hwmod *oh;
@@ -140,15 +140,7 @@ static int __init omap_devinit_smartreflex(void)
 		if (!oh)
 			break;
 
-		sr_data = kzalloc(sizeof(struct omap_smartreflex_data),
-								GFP_KERNEL);
 		sr_dev_data = (struct omap_smartreflex_dev_data *)oh->dev_attr;
-		if (!sr_data) {
-			pr_warning("%s: could not find hwmod data for %d\n",
-					__func__, i + 1);
-			kfree(sr_data);
-			return -ENOMEM;
-		}
 
 		/*
 		 * OMAP3430 ES3.1 chips by default come with Efuse burnt
@@ -159,26 +151,25 @@ static int __init omap_devinit_smartreflex(void)
 		 */
 		if (cpu_is_omap343x()) {
 			if (omap_rev() == OMAP3430_REV_ES3_1)
-				sr_data->enable_on_init = true;
+				sr_data.enable_on_init = true;
 			else
-				sr_data->enable_on_init = false;
+				sr_data.enable_on_init = false;
 		} else {
-			sr_data->enable_on_init = false;
+			sr_data.enable_on_init = false;
 		}
-		sr_data->device_enable = omap_device_enable;
-		sr_data->device_shutdown = omap_device_shutdown;
-		sr_data->device_idle = omap_device_idle;
+		sr_data.device_enable = omap_device_enable;
+		sr_data.device_shutdown = omap_device_shutdown;
+		sr_data.device_idle = omap_device_idle;
 		omap_get_voltage_table(i, &sr_dev_data->volt_data,
 					&sr_dev_data->volts_supported);
-		sr_set_nvalues(sr_dev_data, sr_data);
+		sr_set_nvalues(sr_dev_data, &sr_data);
 
-		od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
+		od = omap_device_build(name, i, oh, &sr_data, sizeof(sr_data),
 				       omap_sr_latency,
 				       ARRAY_SIZE(omap_sr_latency), 0);
 		if (IS_ERR(od)) {
 			pr_warning("%s: Could not build omap_device %s:%s\n",
 				__func__, name, oh->name);
-			kfree(sr_data);
 		}
 		i++;
 	} while (1);
-- 
1.7.1.1

[PATCH 2/2] omap3: sr: improve errors handling

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Do not forget to check the 'platform_device_add_data()' error code
in 'omap_device_build_ss()'.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 arch/arm/plat-omap/omap_device.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index f2bd2e2..e8da192 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -414,7 +414,9 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
 	od->pdev.num_resources = res_count;
 	od->pdev.resource = res;
 
-	platform_device_add_data(&od->pdev, pdata, pdata_len);
+	ret = platform_device_add_data(&od->pdev, pdata, pdata_len);
+	if (ret)
+		goto odbs_exit4;
 
 	od->pm_lats = pm_lats;
 	od->pm_lats_cnt = pm_lats_cnt;
-- 
1.7.1.1

Re: [PATCH 2/2] omap3: sr: improve errors handling

[Nishanth Menon]

Artem Bityutskiy had written, on 07/09/2010 08:20 AM, the following:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> Do not forget to check the 'platform_device_add_data()' error code
> in 'omap_device_build_ss()'.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Acked-by: Nishanth Menon <nm@ti.com>

> ---
>  arch/arm/plat-omap/omap_device.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index f2bd2e2..e8da192 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -414,7 +414,9 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>  	od->pdev.num_resources = res_count;
>  	od->pdev.resource = res;
>  
> -	platform_device_add_data(&od->pdev, pdata, pdata_len);
> +	ret = platform_device_add_data(&od->pdev, pdata, pdata_len);
> +	if (ret)
> +		goto odbs_exit4;
>  
>  	od->pm_lats = pm_lats;
>  	od->pm_lats_cnt = pm_lats_cnt;


-- 
Regards,
Nishanth Menon

RE: [PATCH 2/2] omap3: sr: improve errors handling

[Gopinath, Thara]

>>-----Original Message-----
>>From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
>>Sent: Friday, July 09, 2010 6:50 PM
>>To: linux-omap@vger.kernel.org
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara; Peter p2 De Schrijver
>>Subject: [PATCH 2/2] omap3: sr: improve errors handling
>>
>>From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>>
>>Do not forget to check the 'platform_device_add_data()' error code
>>in 'omap_device_build_ss()'.

Hello Artem,

Can we have a better subject and description. The subject esp has got
nothing to do with the fix.

Regards
Thara

>>
>>Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>>---
>> arch/arm/plat-omap/omap_device.c |    4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>>diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>>index f2bd2e2..e8da192 100644
>>--- a/arch/arm/plat-omap/omap_device.c
>>+++ b/arch/arm/plat-omap/omap_device.c
>>@@ -414,7 +414,9 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>> 	od->pdev.num_resources = res_count;
>> 	od->pdev.resource = res;
>>
>>-	platform_device_add_data(&od->pdev, pdata, pdata_len);
>>+	ret = platform_device_add_data(&od->pdev, pdata, pdata_len);
>>+	if (ret)
>>+		goto odbs_exit4;
>>
>> 	od->pm_lats = pm_lats;
>> 	od->pm_lats_cnt = pm_lats_cnt;
>>--
>>1.7.1.1

RE: [PATCH 2/2] omap3: sr: improve errors handling

[Artem Bityutskiy]

On Fri, 2010-07-09 at 20:35 +0530, Gopinath, Thara wrote:
> 
> >>-----Original Message-----
> >>From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
> >>Sent: Friday, July 09, 2010 6:50 PM
> >>To: linux-omap@vger.kernel.org
> >>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara; Peter p2 De Schrijver
> >>Subject: [PATCH 2/2] omap3: sr: improve errors handling
> >>
> >>From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> >>
> >>Do not forget to check the 'platform_device_add_data()' error code
> >>in 'omap_device_build_ss()'.
> 
> Hello Artem,
> 
> Can we have a better subject and description. The subject esp has got
> nothing to do with the fix.

Hmm, I do not see why you think so. I think the subject is fine - the
patch improves error handling in the SR code. Then description says how
exactly it improves it - it makes the code to not forget to check the
return code, and it tells in which function and which return code.

So the logic is:

1. The subject line is a short description which gives the idea what the
commit is about. So, my subject line says:
  1.1. It is about omap3 (which is true)
  1.2. It is about sr (which is true)
  1.3. It improves error handling (which is also true)

So the subject line is descriptive enough. It does not have to provide
all glory details.

Then the description provides the details.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] omap3: sr: improve errors handling

[Nishanth Menon]

Artem Bityutskiy had written, on 07/09/2010 10:44 AM, the following:
> On Fri, 2010-07-09 at 20:35 +0530, Gopinath, Thara wrote:
>>>> -----Original Message-----
>>>> From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
>>>> Sent: Friday, July 09, 2010 6:50 PM
>>>> To: linux-omap@vger.kernel.org
>>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara; Peter p2 De Schrijver
>>>> Subject: [PATCH 2/2] omap3: sr: improve errors handling
>>>>
>>>> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>>>>
>>>> Do not forget to check the 'platform_device_add_data()' error code
>>>> in 'omap_device_build_ss()'.
>> Hello Artem,
>>
>> Can we have a better subject and description. The subject esp has got
>> nothing to do with the fix.
> 
> Hmm, I do not see why you think so. I think the subject is fine - the
> patch improves error handling in the SR code. Then description says how
> exactly it improves it - it makes the code to not forget to check the
> return code, and it tells in which function and which return code.
> 
> So the logic is:
> 
> 1. The subject line is a short description which gives the idea what the
> commit is about. So, my subject line says:
>   1.1. It is about omap3 (which is true)
>   1.2. It is about sr (which is true)
I think Thara's contention is that this is omap_device related- yeah SR 
is one of the users of omap_device, but there are much more folks using 
omap_device in l-o today.

>   1.3. It improves error handling (which is also true)
> 
> So the subject line is descriptive enough. It does not have to provide
> all glory details.
> 
> Then the description provides the details.
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 2/2] omap3: sr: improve errors handling

[Artem Bityutskiy]

On Fri, 2010-07-09 at 10:51 -0500, Nishanth Menon wrote:
> > So the logic is:
> > 
> > 1. The subject line is a short description which gives the idea what the
> > commit is about. So, my subject line says:
> >   1.1. It is about omap3 (which is true)
> >   1.2. It is about sr (which is true)
> I think Thara's contention is that this is omap_device related- yeah SR 
> is one of the users of omap_device, but there are much more folks using 
> omap_device in l-o today.

I see. I'll try to come up with a better prefix, but if you suggest me a
good subject line, I'll be grateful :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] omap3: sr: improve errors handling

[Menon, Nishanth]

> -----Original Message-----
> From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
> Sent: Friday, July 09, 2010 10:51 AM
> 
> On Fri, 2010-07-09 at 10:51 -0500, Nishanth Menon wrote:
> > > So the logic is:
> > >
> > > 1. The subject line is a short description which gives the idea what
> the
> > > commit is about. So, my subject line says:
> > >   1.1. It is about omap3 (which is true)
> > >   1.2. It is about sr (which is true)
> > I think Thara's contention is that this is omap_device related- yeah SR
> > is one of the users of omap_device, but there are much more folks using
> > omap_device in l-o today.
> 
> I see. I'll try to come up with a better prefix, but if you suggest me a
> good subject line, I'll be grateful :-)

Subject: omap: device: improve errors handling

Do not forget to check the 'platform_device_add_data()' error code
in 'omap_device_build_ss()'.

How does that sound?

RE: [PATCH 2/2] omap3: sr: improve errors handling

[Artem Bityutskiy]

On Fri, 2010-07-09 at 11:00 -0500, Menon, Nishanth wrote:
> > -----Original Message-----
> > From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
> > Sent: Friday, July 09, 2010 10:51 AM
> > 
> > On Fri, 2010-07-09 at 10:51 -0500, Nishanth Menon wrote:
> > > > So the logic is:
> > > >
> > > > 1. The subject line is a short description which gives the idea what
> > the
> > > > commit is about. So, my subject line says:
> > > >   1.1. It is about omap3 (which is true)
> > > >   1.2. It is about sr (which is true)
> > > I think Thara's contention is that this is omap_device related- yeah SR
> > > is one of the users of omap_device, but there are much more folks using
> > > omap_device in l-o today.
> > 
> > I see. I'll try to come up with a better prefix, but if you suggest me a
> > good subject line, I'll be grateful :-)
> 
> Subject: omap: device: improve errors handling
> 
> Do not forget to check the 'platform_device_add_data()' error code
> in 'omap_device_build_ss()'.
> 
> How does that sound?

Good, thank you! I'll re-send the patch tomorrow unless you change the
subject yourself :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] omap3: sr: improve errors handling

[Artem Bityutskiy]

On Fri, 2010-07-09 at 16:20 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> Do not forget to check the 'platform_device_add_data()' error code
> in 'omap_device_build_ss()'.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Please, discard this patch, I'll send a new one with amended subject.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] omap3: sr: fix memory leak and simplify the code

[Artem Bityutskiy]

On Fri, 2010-07-09 at 16:20 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> This patch fixes the following problem indicated by kmemleak:
> 
> kmemleak: unreferenced object 0xdf93c280 (size 64):
> kmemleak:   backtrace:
> kmemleak:     [<c00df6d4>] create_object+0x104/0x200
> kmemleak:     [<c00d7638>] kmem_cache_alloc+0xe4/0xf4
> kmemleak:     [<c000fc38>] omap_devinit_smartreflex+0x44/0x244
> kmemleak:     [<c003a33c>] do_one_initcall+0x5c/0x1b8
> kmemleak:     [<c00083fc>] kernel_init+0x94/0x110
> kmemleak:     [<c003ba04>] kernel_thread_exit+0x0/0x8
> 
> The reason is that 'omap_devinit_smartreflex()' allocates 'sr_data',
> then passes it to 'omap_device_build()', which 'kmemdup()'s it and
> uses the copy. But 'omap_devinit_smartreflex()' never frees 'sr_data'.
> 
> This patch make 'sr_data' to be a stack variable, which eliminates
> the memory leak and simplifies the code a bit.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  arch/arm/mach-omap2/sr_device.c |   27 +++++++++------------------
>  1 files changed, 9 insertions(+), 18 deletions(-)

Note, I only compile-tested both of the patches, so be careful.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html