Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1640886 > unrolled thread
| Started by | Christophe JAILLET <christophe.jaillet@wanadoo.fr> |
|---|---|
| First post | 2017-05-13 13:50 +0200 |
| Last post | 2017-05-17 12:10 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-05-13 13:50 +0200
Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' walter harms <wharms@bfs.de> - 2017-05-13 15:30 +0200
Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-05-13 18:50 +0200
Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' Sekhar Nori <nsekhar@ti.com> - 2017-05-17 12:10 +0200
| From | Christophe JAILLET <christophe.jaillet@wanadoo.fr> |
|---|---|
| Date | 2017-05-13 13:50 +0200 |
| Subject | [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' |
| Message-ID | <tGDIC-35B-15@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | walter harms <wharms@bfs.de> |
|---|---|
| Date | 2017-05-13 15:30 +0200 |
| Subject | Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' |
| Message-ID | <tGFhn-4d4-7@gated-at.bofh.it> |
| In reply to | #1640886 |
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
[toc] | [prev] | [next] | [standalone]
| From | Christophe JAILLET <christophe.jaillet@wanadoo.fr> |
|---|---|
| Date | 2017-05-13 18:50 +0200 |
| Subject | Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' |
| Message-ID | <tGIoW-6bU-13@gated-at.bofh.it> |
| In reply to | #1640915 |
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
[toc] | [prev] | [next] | [standalone]
| From | Sekhar Nori <nsekhar@ti.com> |
|---|---|
| Date | 2017-05-17 12:10 +0200 |
| Subject | Re: [PATCH 2/2] ARM: davinci: PM: Do not free useful resources in normal path in 'davinci_pm_init' |
| Message-ID | <tI441-1Iw-11@gated-at.bofh.it> |
| In reply to | #1640886 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web