| Message ID | 20220702013714.263614-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] powerpc: kernel: legacy_serial: Fix missing of_node_put() in add_legacy_soc_port() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
Liang He <windhl@126.com> 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 <windhl@126.com> > --- > 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-windhl@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
At 2022-09-05 11:25:38, "Michael Ellerman" <mpe@ellerman.id.au> wrote: >Liang He <windhl@126.com> 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 <windhl@126.com> >> --- >> 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-windhl@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
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,
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 <windhl@126.com> --- 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-windhl@126.com/ arch/powerpc/kernel/legacy_serial.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)