Re:Re: [PATCH v2] powerpc: kernel: legacy_serial: Fix missing of_node_put() in add_legacy_soc_port()
At 2022-09-05 11:25:38, "Michael Ellerman" wrote: >Liang He writes: >> We should call of_node_put() for the reference 'tsi' returned by >> of_get_parent() which will increase the refcount. >> >> Signed-off-by: Liang He >> --- >> changelog: >> >> v2: use more conservative way to call of_node_put() >> v1: mov 'of_node_put()' into the 'if' condition >> >> v1 Link: https://lore.kernel.org/all/20220701130203.240023-1-win...@126.com/ >> >> arch/powerpc/kernel/legacy_serial.c | 11 --- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/legacy_serial.c >> b/arch/powerpc/kernel/legacy_serial.c >> index f048c424c525..cca72081b864 100644 >> --- a/arch/powerpc/kernel/legacy_serial.c >> +++ b/arch/powerpc/kernel/legacy_serial.c >> @@ -166,7 +166,7 @@ static int __init add_legacy_soc_port(struct device_node >> *np, >> { >> u64 addr; >> const __be32 *addrp; >> -struct device_node *tsi = of_get_parent(np); >> +struct device_node *tsi; >> >> /* We only support ports that have a clock frequency properly >> * encoded in the device-tree. >> @@ -194,12 +194,17 @@ static int __init add_legacy_soc_port(struct >> device_node *np, >> /* Add port, irq will be dealt with later. We passed a translated >> * IO port value. It will be fixed up later along with the irq >> */ >> -if (of_node_is_type(tsi, "tsi-bridge")) >> +tsi = of_get_parent(np); >> +if (of_node_is_type(tsi, "tsi-bridge")) { >> +of_node_put(tsi); >> return add_legacy_port(np, -1, UPIO_TSI, addr, addr, >> 0, legacy_port_flags, 0); >> -else >> +} >> +else { >> +of_node_put(tsi); >> return add_legacy_port(np, -1, UPIO_MEM, addr, addr, >> 0, legacy_port_flags, 0); >> +} >> } > >The two legs of the else end up with duplicated code except for a single >parameter to add_legacy_port(). > >Better would be: > >{ > int iotype; >... > > tsi = of_get_parent(np); > if (of_node_is_type(tsi, "tsi-bridge")) > iotype = UPIO_TSI; > else > iotype = UPIO_MEM; > > of_node_put(tsi); >return add_legacy_port(np, -1, iotype, addr, addr, 0, > legacy_port_flags, 0); >} > > >cheers Thanks, I will give another version of this patch. Liang
Re: [PATCH v2] powerpc: kernel: legacy_serial: Fix missing of_node_put() in add_legacy_soc_port()
Liang He writes: > We should call of_node_put() for the reference 'tsi' returned by > of_get_parent() which will increase the refcount. > > Signed-off-by: Liang He > --- > changelog: > > v2: use more conservative way to call of_node_put() > v1: mov 'of_node_put()' into the 'if' condition > > v1 Link: https://lore.kernel.org/all/20220701130203.240023-1-win...@126.com/ > > arch/powerpc/kernel/legacy_serial.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/legacy_serial.c > b/arch/powerpc/kernel/legacy_serial.c > index f048c424c525..cca72081b864 100644 > --- a/arch/powerpc/kernel/legacy_serial.c > +++ b/arch/powerpc/kernel/legacy_serial.c > @@ -166,7 +166,7 @@ static int __init add_legacy_soc_port(struct device_node > *np, > { > u64 addr; > const __be32 *addrp; > - struct device_node *tsi = of_get_parent(np); > + struct device_node *tsi; > > /* We only support ports that have a clock frequency properly >* encoded in the device-tree. > @@ -194,12 +194,17 @@ static int __init add_legacy_soc_port(struct > device_node *np, > /* Add port, irq will be dealt with later. We passed a translated >* IO port value. It will be fixed up later along with the irq >*/ > - if (of_node_is_type(tsi, "tsi-bridge")) > + tsi = of_get_parent(np); > + if (of_node_is_type(tsi, "tsi-bridge")) { > + of_node_put(tsi); > return add_legacy_port(np, -1, UPIO_TSI, addr, addr, > 0, legacy_port_flags, 0); > - else > + } > + else { > + of_node_put(tsi); > return add_legacy_port(np, -1, UPIO_MEM, addr, addr, > 0, legacy_port_flags, 0); > + } > } The two legs of the else end up with duplicated code except for a single parameter to add_legacy_port(). Better would be: { int iotype; ... tsi = of_get_parent(np); if (of_node_is_type(tsi, "tsi-bridge")) iotype = UPIO_TSI; else iotype = UPIO_MEM; of_node_put(tsi); return add_legacy_port(np, -1, iotype, addr, addr, 0, legacy_port_flags, 0); } cheers
[PATCH v2] powerpc: kernel: legacy_serial: Fix missing of_node_put() in add_legacy_soc_port()
We should call of_node_put() for the reference 'tsi' returned by of_get_parent() which will increase the refcount. Signed-off-by: Liang He --- changelog: v2: use more conservative way to call of_node_put() v1: mov 'of_node_put()' into the 'if' condition v1 Link: https://lore.kernel.org/all/20220701130203.240023-1-win...@126.com/ arch/powerpc/kernel/legacy_serial.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c index f048c424c525..cca72081b864 100644 --- a/arch/powerpc/kernel/legacy_serial.c +++ b/arch/powerpc/kernel/legacy_serial.c @@ -166,7 +166,7 @@ static int __init add_legacy_soc_port(struct device_node *np, { u64 addr; const __be32 *addrp; - struct device_node *tsi = of_get_parent(np); + struct device_node *tsi; /* We only support ports that have a clock frequency properly * encoded in the device-tree. @@ -194,12 +194,17 @@ static int __init add_legacy_soc_port(struct device_node *np, /* Add port, irq will be dealt with later. We passed a translated * IO port value. It will be fixed up later along with the irq */ - if (of_node_is_type(tsi, "tsi-bridge")) + tsi = of_get_parent(np); + if (of_node_is_type(tsi, "tsi-bridge")) { + of_node_put(tsi); return add_legacy_port(np, -1, UPIO_TSI, addr, addr, 0, legacy_port_flags, 0); - else + } + else { + of_node_put(tsi); return add_legacy_port(np, -1, UPIO_MEM, addr, addr, 0, legacy_port_flags, 0); + } } static int __init add_legacy_isa_port(struct device_node *np, -- 2.25.1