At 2022-06-22 06:49:52, "Bjorn Helgaas" <helgaas@xxxxxxxxxx> 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@xxxxxxx>
>> ---
>> 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
