| Message ID | 20220621070145.4080147-1-windhl@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node | expand |
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
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 >
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
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
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");
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(-)