* [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init'
@ 2017-05-13 11:40 Christophe JAILLET
2017-05-13 13:22 ` walter harms
2017-05-17 10:03 ` Sekhar Nori
0 siblings, 2 replies; 4+ messages in thread
From: Christophe JAILLET @ 2017-05-13 11:40 UTC (permalink / raw)
To: nsekhar, khilman, linux
Cc: linux-arm-kernel, linux-kernel, kernel-janitors, Christophe JAILLET
This looks spurious to iounmap resources in the normal path of this init
function.
The 3 ioremap'ed fields of 'pm_config' can be accessed later on in other
functions, so it is likely that we should return 'success' before unrolling
everything.
Fixes: aa9aa1ec2df6 ("ARM: davinci: PM: rework init, remove platform device")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is just a *guess*. The end of the function looks more like a
error handling code rather than a normal path.
---
arch/arm/mach-davinci/pm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-davinci/pm.c b/arch/arm/mach-davinci/pm.c
index d282b0783ecf..163d865abbf9 100644
--- a/arch/arm/mach-davinci/pm.c
+++ b/arch/arm/mach-davinci/pm.c
@@ -161,6 +161,7 @@ int __init davinci_pm_init(void)
davinci_cpu_suspend_sz);
suspend_set_ops(&davinci_pm_ops);
+ return 0;
no_sram_mem:
iounmap(pm_config.ddrpsc_reg_base);
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init'
2017-05-13 11:40 [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Christophe JAILLET
@ 2017-05-13 13:22 ` walter harms
2017-05-13 16:44 ` Christophe JAILLET
2017-05-17 10:03 ` Sekhar Nori
1 sibling, 1 reply; 4+ messages in thread
From: walter harms @ 2017-05-13 13:22 UTC (permalink / raw)
To: Christophe JAILLET
Cc: nsekhar, khilman, linux, linux-arm-kernel, linux-kernel, kernel-janitors
Am 13.05.2017 13:40, schrieb Christophe JAILLET:
> This looks spurious to iounmap resources in the normal path of this init
> function.
> The 3 ioremap'ed fields of 'pm_config' can be accessed later on in other
> functions, so it is likely that we should return 'success' before unrolling
> everything.
>
> Fixes: aa9aa1ec2df6 ("ARM: davinci: PM: rework init, remove platform device")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is just a *guess*. The end of the function looks more like a
> error handling code rather than a normal path.
> ---
> arch/arm/mach-davinci/pm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-davinci/pm.c b/arch/arm/mach-davinci/pm.c
> index d282b0783ecf..163d865abbf9 100644
> --- a/arch/arm/mach-davinci/pm.c
> +++ b/arch/arm/mach-davinci/pm.c
> @@ -161,6 +161,7 @@ int __init davinci_pm_init(void)
> davinci_cpu_suspend_sz);
>
> suspend_set_ops(&davinci_pm_ops);
> + return 0;
>
> no_sram_mem:
> iounmap(pm_config.ddrpsc_reg_base);
looks like, but that would mean that is wrong also:
davinci_sram_suspend = sram_alloc(davinci_cpu_suspend_sz, NULL);
if (!davinci_sram_suspend) {
pr_err("PM: cannot allocate SRAM memory\n");
return -ENOMEM;
}
what means 1 iounmap missing.
re,
wh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init'
2017-05-13 13:22 ` walter harms
@ 2017-05-13 16:44 ` Christophe JAILLET
0 siblings, 0 replies; 4+ messages in thread
From: Christophe JAILLET @ 2017-05-13 16:44 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, kernel-janitors
Le 13/05/2017 à 15:22, walter harms a écrit :
>
> Am 13.05.2017 13:40, schrieb Christophe JAILLET:
>> This looks spurious to iounmap resources in the normal path of this init
>> function.
>> The 3 ioremap'ed fields of 'pm_config' can be accessed later on in other
>> functions, so it is likely that we should return 'success' before unrolling
>> everything.
>>
>> Fixes: aa9aa1ec2df6 ("ARM: davinci: PM: rework init, remove platform device")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This patch is just a *guess*. The end of the function looks more like a
>> error handling code rather than a normal path.
>> ---
>> arch/arm/mach-davinci/pm.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/mach-davinci/pm.c b/arch/arm/mach-davinci/pm.c
>> index d282b0783ecf..163d865abbf9 100644
>> --- a/arch/arm/mach-davinci/pm.c
>> +++ b/arch/arm/mach-davinci/pm.c
>> @@ -161,6 +161,7 @@ int __init davinci_pm_init(void)
>> davinci_cpu_suspend_sz);
>>
>> suspend_set_ops(&davinci_pm_ops);
>> + return 0;
>>
>> no_sram_mem:
>> iounmap(pm_config.ddrpsc_reg_base);
>
> looks like, but that would mean that is wrong also:
>
> davinci_sram_suspend = sram_alloc(davinci_cpu_suspend_sz, NULL);
> if (!davinci_sram_suspend) {
> pr_err("PM: cannot allocate SRAM memory\n");
> return -ENOMEM;
> }
>
> what means 1 iounmap missing.
>
> re,
> wh
>
This is what I try to fix in the [1/2] patch.
CJ
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init'
2017-05-13 11:40 [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Christophe JAILLET
2017-05-13 13:22 ` walter harms
@ 2017-05-17 10:03 ` Sekhar Nori
1 sibling, 0 replies; 4+ messages in thread
From: Sekhar Nori @ 2017-05-17 10:03 UTC (permalink / raw)
To: Christophe JAILLET, khilman, linux
Cc: linux-arm-kernel, linux-kernel, kernel-janitors
On Saturday 13 May 2017 05:10 PM, Christophe JAILLET wrote:
> This looks spurious to iounmap resources in the normal path of this init
> function.
> The 3 ioremap'ed fields of 'pm_config' can be accessed later on in other
> functions, so it is likely that we should return 'success' before unrolling
> everything.
>
> Fixes: aa9aa1ec2df6 ("ARM: davinci: PM: rework init, remove platform device")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is just a *guess*. The end of the function looks more like a
> error handling code rather than a normal path.
The patch is correct.
> ---
> arch/arm/mach-davinci/pm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-davinci/pm.c b/arch/arm/mach-davinci/pm.c
> index d282b0783ecf..163d865abbf9 100644
> --- a/arch/arm/mach-davinci/pm.c
> +++ b/arch/arm/mach-davinci/pm.c
> @@ -161,6 +161,7 @@ int __init davinci_pm_init(void)
> davinci_cpu_suspend_sz);
>
> suspend_set_ops(&davinci_pm_ops);
> + return 0;
We normally have an empty line before return. I added that while
applying. Also, I modified the patch description to be more "sure" in
its language. Here is the final description:
ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init'
It is wrong to iounmap resources in the normal path of davinci_pm_init()
The 3 ioremap'ed fields of 'pm_config' can be accessed later on in other
functions, so we should return 'success' instead of unrolling everything.
Fixes: aa9aa1ec2df6 ("ARM: davinci: PM: rework init, remove platform device")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
[nsekhar@ti.com: commit message and minor style fixes]
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
I will let this go through some testing before sending for inclusion in
v4.12 kernel. Thanks for catching these.
For future, when sending more than one patch, please add a cover
letter. And have the patches appear as replies to the cover letter.
This happens if you send the patches together with cover letter using
git-send-email and have chainreplyto set to false in your .gitconfig
Thanks,
Sekhar
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-17 10:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-13 11:40 [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Christophe JAILLET
2017-05-13 13:22 ` walter harms
2017-05-13 16:44 ` Christophe JAILLET
2017-05-17 10:03 ` Sekhar Nori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).