[PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[Liang He]

In rcar_pcie_init(), we need to hold the reference returned by
of_find_matching_node() which is used to call of_node_put() for
refcount balance.

Signed-off-by: Liang He <windhl@126.com>
---
 drivers/pci/controller/pcie-rcar-host.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index 997c4df6a1e7..405ec3d64f30 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -1158,7 +1158,10 @@ static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst
 
 static int __init rcar_pcie_init(void)
 {
-	if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
+	struct device_node *np = of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match);
+
+	if (np) {
+		of_node_put(np);
 #ifdef CONFIG_ARM_LPAE
 		hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
 				"asynchronous external abort");
-- 
2.25.1

Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[Geert Uytterhoeven]

On Tue, Jun 21, 2022 at 9:47 AM Liang He <windhl@126.com> wrote:
> In rcar_pcie_init(), we need to hold the reference returned by
> of_find_matching_node() which is used to call of_node_put() for
> refcount balance.
>
> Signed-off-by: Liang He <windhl@126.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[Bjorn Helgaas]

On Tue, Jun 21, 2022 at 03:01:45PM +0800, Liang He wrote:
> In rcar_pcie_init(), we need to hold the reference returned by
> of_find_matching_node() which is used to call of_node_put() for
> refcount balance.
> 
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  drivers/pci/controller/pcie-rcar-host.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> index 997c4df6a1e7..405ec3d64f30 100644
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -1158,7 +1158,10 @@ static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst
>  
>  static int __init rcar_pcie_init(void)
>  {
> -	if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
> +	struct device_node *np = of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match);
> +
> +	if (np) {
> +		of_node_put(np);

I think this is correct, but it would be nicer to update the way this
driver uses of_device_get_match_data(), e.g.,

  struct rcar_variant {
    int (*phy_init_fn)(struct rcar_pcie_host *host);
    bool hook_abort;
  };

  struct rcar_pcie_host {
    ...
    const struct rcar_variant *variant;
  };

  static int rcar_pcie_probe(...)
  {
    host->variant = of_device_get_match_data(dev);
    err = host->variant->phy_init_fn(host);
    ...

  #ifdef CONFIG_ARM
    if (host->variant->hook_abort) {
  #ifdef CONFIG_ARM_LPAE
      hook_fault_code(17, ...);
  # else
      hook_fault_code(22, ...);
  #endif
    }
  #endif
  }

Or keep the hook in a separate function called from rcar_pcie_probe()
if you think that's cleaner.

I'm not sure hook_fault_code() needs to be called separately as a
device_initcall().  The pci-ixp4xx.c driver does it in
ixp4xx_pci_probe(), so I assume rcar could do it in probe as well.

>  #ifdef CONFIG_ARM_LPAE
>  		hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>  				"asynchronous external abort");
> -- 
> 2.25.1
> 

Re:Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[Liang He]

At 2022-06-22 06:49:52, "Bjorn Helgaas" <helgaas@kernel.org> wrote:
>On Tue, Jun 21, 2022 at 03:01:45PM +0800, Liang He wrote:
>> In rcar_pcie_init(), we need to hold the reference returned by
>> of_find_matching_node() which is used to call of_node_put() for
>> refcount balance.
>> 
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>  drivers/pci/controller/pcie-rcar-host.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
>> index 997c4df6a1e7..405ec3d64f30 100644
>> --- a/drivers/pci/controller/pcie-rcar-host.c
>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>> @@ -1158,7 +1158,10 @@ static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst
>>  
>>  static int __init rcar_pcie_init(void)
>>  {
>> -	if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
>> +	struct device_node *np = of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match);
>> +
>> +	if (np) {
>> +		of_node_put(np);
>
>I think this is correct, but it would be nicer to update the way this
>driver uses of_device_get_match_data(), e.g.,
>
>  struct rcar_variant {
>    int (*phy_init_fn)(struct rcar_pcie_host *host);
>    bool hook_abort;
>  };
>
>  struct rcar_pcie_host {
>    ...
>    const struct rcar_variant *variant;
>  };
>
>  static int rcar_pcie_probe(...)
>  {
>    host->variant = of_device_get_match_data(dev);
>    err = host->variant->phy_init_fn(host);
>    ...
>
>  #ifdef CONFIG_ARM
>    if (host->variant->hook_abort) {
>  #ifdef CONFIG_ARM_LPAE
>      hook_fault_code(17, ...);
>  # else
>      hook_fault_code(22, ...);
>  #endif
>    }
>  #endif
>  }
>
>Or keep the hook in a separate function called from rcar_pcie_probe()
>if you think that's cleaner.
>
>I'm not sure hook_fault_code() needs to be called separately as a
>device_initcall().  The pci-ixp4xx.c driver does it in
>ixp4xx_pci_probe(), so I assume rcar could do it in probe as well.
>
>>  #ifdef CONFIG_ARM_LPAE
>>  		hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>>  				"asynchronous external abort");
>> -- 
>> 2.25.1
>> 

Thanks very much for your effort to review my patch.

Now I am very sorry that I only know how to correctly use of_find_xxx APIs 
and when there will be a leak bug.

In another word, I have no idea the details of the drivers and I cannot do 
anyother thing than  just fix the leak bug caused by missing of_node_put().

Sorry again.

Liang

Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node

[Geert Uytterhoeven]

Hi Bjorn,

On Wed, Jun 22, 2022 at 12:57 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Jun 21, 2022 at 03:01:45PM +0800, Liang He wrote:
> > In rcar_pcie_init(), we need to hold the reference returned by
> > of_find_matching_node() which is used to call of_node_put() for
> > refcount balance.
> >
> > Signed-off-by: Liang He <windhl@126.com>

> > --- a/drivers/pci/controller/pcie-rcar-host.c
> > +++ b/drivers/pci/controller/pcie-rcar-host.c
> > @@ -1158,7 +1158,10 @@ static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst
> >
> >  static int __init rcar_pcie_init(void)
> >  {
> > -     if (of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match)) {
> > +     struct device_node *np = of_find_matching_node(NULL, rcar_pcie_abort_handler_of_match);
> > +
> > +     if (np) {
> > +             of_node_put(np);
>
> I think this is correct, but it would be nicer to update the way this
> driver uses of_device_get_match_data(), e.g.,
>
>   struct rcar_variant {
>     int (*phy_init_fn)(struct rcar_pcie_host *host);
>     bool hook_abort;
>   };
>
>   struct rcar_pcie_host {
>     ...
>     const struct rcar_variant *variant;
>   };
>
>   static int rcar_pcie_probe(...)
>   {
>     host->variant = of_device_get_match_data(dev);
>     err = host->variant->phy_init_fn(host);
>     ...
>
>   #ifdef CONFIG_ARM
>     if (host->variant->hook_abort) {
>   #ifdef CONFIG_ARM_LPAE
>       hook_fault_code(17, ...);
>   # else
>       hook_fault_code(22, ...);
>   #endif
>     }
>   #endif
>   }
>
> Or keep the hook in a separate function called from rcar_pcie_probe()
> if you think that's cleaner.
>
> I'm not sure hook_fault_code() needs to be called separately as a
> device_initcall().

Yes it doesn, as hook_fault_code() is __init.

> The pci-ixp4xx.c driver does it in
> ixp4xx_pci_probe(), so I assume rcar could do it in probe as well.

The pci-ixp4xx.c driver uses builtin_platform_driver_probe(), and
ixp4xx_pci_probe() is marked __init.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds