| Message ID | 20170513114020.23668-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(+)