* [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node
@ 2022-06-21 7:01 Liang He
2022-06-21 8:36 ` Geert Uytterhoeven
2022-06-21 22:49 ` Bjorn Helgaas
0 siblings, 2 replies; 5+ messages in thread
From: Liang He @ 2022-06-21 7:01 UTC (permalink / raw)
To: marek.vasut+renesas, yoshihiro.shimoda.uh, lpieralisi, robh, kw,
bhelgaas
Cc: windhl, linux-pci, linux-renesas-soc
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node
2022-06-21 7:01 [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node Liang He
@ 2022-06-21 8:36 ` Geert Uytterhoeven
2022-06-21 22:49 ` Bjorn Helgaas
1 sibling, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2022-06-21 8:36 UTC (permalink / raw)
To: Liang He
Cc: Marek Vasut, Yoshihiro Shimoda, lpieralisi, Rob Herring,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
Linux-Renesas
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node
2022-06-21 7:01 [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node Liang He
2022-06-21 8:36 ` Geert Uytterhoeven
@ 2022-06-21 22:49 ` Bjorn Helgaas
2022-06-22 1:32 ` Liang He
2022-06-22 7:49 ` Geert Uytterhoeven
1 sibling, 2 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2022-06-21 22:49 UTC (permalink / raw)
To: Liang He
Cc: marek.vasut+renesas, yoshihiro.shimoda.uh, lpieralisi, robh, kw,
bhelgaas, linux-pci, linux-renesas-soc
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re:Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node
2022-06-21 22:49 ` Bjorn Helgaas
@ 2022-06-22 1:32 ` Liang He
2022-06-22 7:49 ` Geert Uytterhoeven
1 sibling, 0 replies; 5+ messages in thread
From: Liang He @ 2022-06-22 1:32 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: marek.vasut+renesas, yoshihiro.shimoda.uh, lpieralisi, robh, kw,
bhelgaas, linux-pci, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node
2022-06-21 22:49 ` Bjorn Helgaas
2022-06-22 1:32 ` Liang He
@ 2022-06-22 7:49 ` Geert Uytterhoeven
1 sibling, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2022-06-22 7:49 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Liang He, Marek Vasut, Yoshihiro Shimoda, Lorenzo Pieralisi,
Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
Linux-Renesas
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-22 7:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-21 7:01 [PATCH] pci/controller/pcie-rcar-host: Hold the reference returned by of_find_matching_node Liang He
2022-06-21 8:36 ` Geert Uytterhoeven
2022-06-21 22:49 ` Bjorn Helgaas
2022-06-22 1:32 ` Liang He
2022-06-22 7:49 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).