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

["Liang He" <windhl@126.com>]

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