[PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Liang He]

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(-)

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();
-- 
2.25.1

Re: [PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Krzysztof Kozlowski]

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

Re: [PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Tony Lindgren]

* 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

Re:Re: [PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Liang He]

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

Re: [PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] arm/mach: Hold reference returned by of_find_xxx APIs

[Tony Lindgren]

* 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