| Message ID | 20220621091937.4082422-1-windhl@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm/mach: Hold reference returned by of_find_xxx APIs | expand |
On 21/06/2022 11:19, Liang He wrote: > In highbank_init_irq(), tegra_init_irq() and omap4_cpcap_init(), > we should hold the reference returned by of_find_xxx APIs and > use it to call of_node_put() for refcount balance. > > Signed-off-by: Liang He <windhl@126.com> > --- > arch/arm/mach-highbank/highbank.c | 6 +++++- > arch/arm/mach-omap2/pmic-cpcap.c | 5 ++++- > arch/arm/mach-tegra/irq.c | 6 +++++- > 3 files changed, 14 insertions(+), 3 deletions(-) Before applying the patch please check it carefully. Previous evidence [1] suggests that not it was not even compiled. [1] https://lore.kernel.org/all/202206221602.odN70SHs-lkp@intel.com/ Best regards, Krzysztof
* Liang He <windhl@126.com> [220621 12:14]: > diff --git a/arch/arm/mach-omap2/pmic-cpcap.c b/arch/arm/mach-omap2/pmic-cpcap.c > index 668dc84fd31e..a7368d657aa8 100644 > --- a/arch/arm/mach-omap2/pmic-cpcap.c > +++ b/arch/arm/mach-omap2/pmic-cpcap.c > @@ -238,8 +238,11 @@ static struct omap_voltdm_pmic omap4_fan_iva = { > int __init omap4_cpcap_init(void) > { > struct voltagedomain *voltdm; > + struct device_node *np; > > - if (!of_find_compatible_node(NULL, NULL, "motorola,cpcap")) > + np = of_find_compatible_node(NULL, NULL, "motorola,cpcap"); > + of_node_put(np); > + if (!np) > return -ENODEV; Hmm so here you are checking for !np after of_node_put()? > diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c > index 4e1ee70b2a3f..2aeac041bcb9 100644 > --- a/arch/arm/mach-tegra/irq.c > +++ b/arch/arm/mach-tegra/irq.c > @@ -88,7 +88,11 @@ static const struct of_device_id tegra_ictlr_match[] __initconst = { > > void __init tegra_init_irq(void) > { > - if (WARN_ON(!of_find_matching_node(NULL, tegra_ictlr_match))) > + struct device_node *np; > + > + np = of_find_matching_node(NULL, tegra_ictlr_match); > + of_node_put(np); > + if (WARN_ON(!np)) > pr_warn("Outdated DT detected, suspend/resume will NOT work\n"); > > tegra114_gic_cpu_pm_registration(); Here too. Regards, Tony
At 2022-06-28 11:59:45, "Tony Lindgren" <tony@atomide.com> wrote: >* Liang He <windhl@126.com> [220621 12:14]: >> diff --git a/arch/arm/mach-omap2/pmic-cpcap.c b/arch/arm/mach-omap2/pmic-cpcap.c >> index 668dc84fd31e..a7368d657aa8 100644 >> --- a/arch/arm/mach-omap2/pmic-cpcap.c >> +++ b/arch/arm/mach-omap2/pmic-cpcap.c >> @@ -238,8 +238,11 @@ static struct omap_voltdm_pmic omap4_fan_iva = { >> int __init omap4_cpcap_init(void) >> { >> struct voltagedomain *voltdm; >> + struct device_node *np; >> >> - if (!of_find_compatible_node(NULL, NULL, "motorola,cpcap")) >> + np = of_find_compatible_node(NULL, NULL, "motorola,cpcap"); >> + of_node_put(np); >> + if (!np) >> return -ENODEV; > >Hmm so here you are checking for !np after of_node_put()? Hi, Tony. Thanks very much for your effort to review my code. I just wanted to use this 'check-after-put' coding style to keep consistent with existing style. But based on the disccussion with Greg KH, yesterday, I am now preparing 'check-then-put' coding style patch. Link: https://lore.kernel.org/all/1bed06e5.43da.181a5bac7e5.Coremail.windhl@126.com/ > >> diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c >> index 4e1ee70b2a3f..2aeac041bcb9 100644 >> --- a/arch/arm/mach-tegra/irq.c >> +++ b/arch/arm/mach-tegra/irq.c >> @@ -88,7 +88,11 @@ static const struct of_device_id tegra_ictlr_match[] __initconst = { >> >> void __init tegra_init_irq(void) >> { >> - if (WARN_ON(!of_find_matching_node(NULL, tegra_ictlr_match))) >> + struct device_node *np; >> + >> + np = of_find_matching_node(NULL, tegra_ictlr_match); >> + of_node_put(np); >> + if (WARN_ON(!np)) >> pr_warn("Outdated DT detected, suspend/resume will NOT work\n"); >> >> tegra114_gic_cpu_pm_registration(); > >Here too. > >Regards, > >Tony Thanks again, Tony. I will send a new 'check-and-put' patch soon. Liang
On Tue, Jun 28, 2022 at 06:59:45AM +0300, Tony Lindgren wrote: > * Liang He <windhl@126.com> [220621 12:14]: > > diff --git a/arch/arm/mach-omap2/pmic-cpcap.c b/arch/arm/mach-omap2/pmic-cpcap.c > > index 668dc84fd31e..a7368d657aa8 100644 > > --- a/arch/arm/mach-omap2/pmic-cpcap.c > > +++ b/arch/arm/mach-omap2/pmic-cpcap.c > > @@ -238,8 +238,11 @@ static struct omap_voltdm_pmic omap4_fan_iva = { > > int __init omap4_cpcap_init(void) > > { > > struct voltagedomain *voltdm; > > + struct device_node *np; > > > > - if (!of_find_compatible_node(NULL, NULL, "motorola,cpcap")) > > + np = of_find_compatible_node(NULL, NULL, "motorola,cpcap"); > > + of_node_put(np); > > + if (!np) > > return -ENODEV; > > Hmm so here you are checking for !np after of_node_put()? This is permissible, because the value of the _pointer_ is being checked without dereferencing the pointer. So the fact that the node may have been freed is actually immaterial.
* Russell King (Oracle) <linux@armlinux.org.uk> [220628 16:11]: > On Tue, Jun 28, 2022 at 06:59:45AM +0300, Tony Lindgren wrote: > > * Liang He <windhl@126.com> [220621 12:14]: > > > diff --git a/arch/arm/mach-omap2/pmic-cpcap.c b/arch/arm/mach-omap2/pmic-cpcap.c > > > index 668dc84fd31e..a7368d657aa8 100644 > > > --- a/arch/arm/mach-omap2/pmic-cpcap.c > > > +++ b/arch/arm/mach-omap2/pmic-cpcap.c > > > @@ -238,8 +238,11 @@ static struct omap_voltdm_pmic omap4_fan_iva = { > > > int __init omap4_cpcap_init(void) > > > { > > > struct voltagedomain *voltdm; > > > + struct device_node *np; > > > > > > - if (!of_find_compatible_node(NULL, NULL, "motorola,cpcap")) > > > + np = of_find_compatible_node(NULL, NULL, "motorola,cpcap"); > > > + of_node_put(np); > > > + if (!np) > > > return -ENODEV; > > > > Hmm so here you are checking for !np after of_node_put()? > > This is permissible, because the value of the _pointer_ is being > checked without dereferencing the pointer. So the fact that the > node may have been freed is actually immaterial. OK yeah. This is several lines of code to check if something exists. Maybe we should just add bool of_compatible_node_exists() to simplify cases like this that does not keep the kfef. Regards, Tony
diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c index db607955a7e4..4f50000377b8 100644 --- a/arch/arm/mach-highbank/highbank.c +++ b/arch/arm/mach-highbank/highbank.c @@ -50,10 +50,14 @@ static void highbank_l2c310_write_sec(unsigned long val, unsigned reg) static void __init highbank_init_irq(void) { + struct device_node *np; + irqchip_init(); - if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9")) + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9"); + if (np) highbank_scu_map_io(); + of_node_put(np); } static void highbank_power_off(void) diff --git a/arch/arm/mach-omap2/pmic-cpcap.c b/arch/arm/mach-omap2/pmic-cpcap.c index 668dc84fd31e..a7368d657aa8 100644 --- a/arch/arm/mach-omap2/pmic-cpcap.c +++ b/arch/arm/mach-omap2/pmic-cpcap.c @@ -238,8 +238,11 @@ static struct omap_voltdm_pmic omap4_fan_iva = { int __init omap4_cpcap_init(void) { struct voltagedomain *voltdm; + struct device_node *np; - if (!of_find_compatible_node(NULL, NULL, "motorola,cpcap")) + np = of_find_compatible_node(NULL, NULL, "motorola,cpcap"); + of_node_put(np); + if (!np) return -ENODEV; voltdm = voltdm_lookup("mpu"); diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c index 4e1ee70b2a3f..2aeac041bcb9 100644 --- a/arch/arm/mach-tegra/irq.c +++ b/arch/arm/mach-tegra/irq.c @@ -88,7 +88,11 @@ static const struct of_device_id tegra_ictlr_match[] __initconst = { void __init tegra_init_irq(void) { - if (WARN_ON(!of_find_matching_node(NULL, tegra_ictlr_match))) + struct device_node *np; + + np = of_find_matching_node(NULL, tegra_ictlr_match); + of_node_put(np); + if (WARN_ON(!np)) pr_warn("Outdated DT detected, suspend/resume will NOT work\n"); tegra114_gic_cpu_pm_registration();
In highbank_init_irq(), tegra_init_irq() and omap4_cpcap_init(), we should hold the reference returned by of_find_xxx APIs and use it to call of_node_put() for refcount balance. Signed-off-by: Liang He <windhl@126.com> --- arch/arm/mach-highbank/highbank.c | 6 +++++- arch/arm/mach-omap2/pmic-cpcap.c | 5 ++++- arch/arm/mach-tegra/irq.c | 6 +++++- 3 files changed, 14 insertions(+), 3 deletions(-)