[PATCH] macintosh/windfarm_smu_sat: Add missing of_node_put()
We call of_node_get() in wf_sat_probe() after sat is created,
so we need the of_node_put() before *kfree(sat)*.
Fixes: ac171c46667c ("[PATCH] powerpc: Thermal control for dual core G5s")
Signed-off-by: Liang He
---
drivers/macintosh/windfarm_smu_sat.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/macintosh/windfarm_smu_sat.c
b/drivers/macintosh/windfarm_smu_sat.c
index ebc4256a9e4a..089f2743a070 100644
--- a/drivers/macintosh/windfarm_smu_sat.c
+++ b/drivers/macintosh/windfarm_smu_sat.c
@@ -171,6 +171,7 @@ static void wf_sat_release(struct kref *ref)
if (sat->nr >= 0)
sats[sat->nr] = NULL;
+ of_node_put(sat->node);
kfree(sat);
}
--
2.25.1
[PATCH] powerpc/mpc5xxx: Add missing fwnode_handle_put()
In mpc5xxx_fwnode_get_bus_frequency(), we should add
fwnode_handle_put() when break out of the iteration
fwnode_for_each_parent_node() as it will automatically
increase and decrease the refcounter.
Fixes: de06fba62af6 ("powerpc/mpc5xxx: Switch mpc5xxx_get_bus_frequency() to
use fwnode")
Signed-off-by: Liang He
---
arch/powerpc/sysdev/mpc5xxx_clocks.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c
b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index c5bf7e1b3780..58cee28e2399 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -25,8 +25,10 @@ unsigned long mpc5xxx_fwnode_get_bus_frequency(struct
fwnode_handle *fwnode)
fwnode_for_each_parent_node(fwnode, parent) {
ret = fwnode_property_read_u32(parent, "bus-frequency",
_freq);
- if (!ret)
+ if (!ret) {
+ fwnode_handle_put(parent);
return bus_freq;
+ }
}
return 0;
--
2.25.1
[PATCH] serial: ucc_uart: Add of_node_put() in ucc_uart_remove()
In ucc_uart_probe(), we have added proper of_node_put() in the failure paths. However, we miss it before we free *qe_port* in the remove() function. Signed-off-by: Liang He --- drivers/tty/serial/ucc_uart.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c index 82cf14dd3d43..461d54de6351 100644 --- a/drivers/tty/serial/ucc_uart.c +++ b/drivers/tty/serial/ucc_uart.c @@ -1469,6 +1469,8 @@ static int ucc_uart_remove(struct platform_device *ofdev) uart_remove_one_port(_uart_driver, _port->port); + of_node_put(qe_port->np); + kfree(qe_port); return 0; -- 2.25.1
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:Re: [PATCH] powerpc/powermac/pfunc_base: Fix refcount leak bug in macio_gpio_init_one()
At 2022-08-02 12:51:08, "Benjamin Herrenschmidt"
wrote:
>On Sat, 2022-07-16 at 15:31 +0800, Liang He wrote:
>> We should call of_node_put() for the reference 'gparent' escaped
>> out of the for_each_child_of_node() as it has increased the refcount.
>
>Same comment as before. That stuff happens once at boot, there's never
>any dynamic allocation/deallocation of these, they just don't matter,
>but feel free :-)
>
Thanks for your reply, this is a valuable lesson for me.
I will now begin to learn the detailed difference of dynamic and static
allocation.
Thanks,
Liang
>> Fixes: 5b9ca526917b ("[PATCH] 3/5 powerpc: Add platform functions
>> interpreter")
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/platforms/powermac/pfunc_base.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powermac/pfunc_base.c
>> b/arch/powerpc/platforms/powermac/pfunc_base.c
>> index 9c2947a3edd5..085e0ad20eba 100644
>> --- a/arch/powerpc/platforms/powermac/pfunc_base.c
>> +++ b/arch/powerpc/platforms/powermac/pfunc_base.c
>> @@ -136,6 +136,8 @@ static void __init macio_gpio_init_one(struct
>> macio_chip *macio)
>> for_each_child_of_node(gparent, gp)
>> pmf_do_functions(gp, NULL, 0, PMF_FLAGS_ON_INIT, NULL);
>>
>> +of_node_put(gparent);
>> +
>> /* Note: We do not at this point implement the "at sleep" or
>> "at wake"
>> * functions. I yet to find any for GPIOs anyway
>> */
[PATCH] powerpc/fsl_pci: Remove of_node_put() when reference escaped out
In fsl_pci_assign_primary(), we should remove the of_node_put()
when breaking out of the for_each_matching_node() as the 'np'
is escaped out by global 'fsl_pci_primary'.
Fixes: 905e75c46dba ("powerpc/fsl-pci: Unify pci/pcie initialization code")
Signed-off-by: Liang He
---
arch/powerpc/sysdev/fsl_pci.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index e8d072e98b66..aa7faa19c6ef 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1134,7 +1134,6 @@ void __init fsl_pci_assign_primary(void)
for_each_matching_node(np, pci_ids) {
if (of_device_is_available(np)) {
fsl_pci_primary = np;
- of_node_put(np);
return;
}
}
--
2.25.1
[PATCH] powerpc/powermac/udbg_scc: Fix refcount leak bug in udbg_scc_init()
During the iteration of for_each_child_of_node(), we need to call
of_node_put() for the old references stored in to 'ch_def' and 'ch_a'
as their refcounters have been increased in last iteration.
Fixes: 51d3082fe6e5 ("[PATCH] powerpc: Unify udbg (#2)")
Signed-off-by: Liang He
---
arch/powerpc/platforms/powermac/udbg_scc.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powermac/udbg_scc.c
b/arch/powerpc/platforms/powermac/udbg_scc.c
index 734df5a32f99..1b7c39e841ee 100644
--- a/arch/powerpc/platforms/powermac/udbg_scc.c
+++ b/arch/powerpc/platforms/powermac/udbg_scc.c
@@ -81,10 +81,14 @@ void __init udbg_scc_init(int force_scc)
if (path != NULL)
stdout = of_find_node_by_path(path);
for_each_child_of_node(escc, ch) {
- if (ch == stdout)
+ if (ch == stdout) {
+ of_node_put(ch_def);
ch_def = of_node_get(ch);
- if (of_node_name_eq(ch, "ch-a"))
+ }
+ if (of_node_name_eq(ch, "ch-a")) {
+ of_node_put(ch_a);
ch_a = of_node_get(ch);
+ }
}
if (ch_def == NULL && !force_scc)
goto bail;
--
2.25.1
[PATCH] powerpc/powermac/pfunc_base: Fix refcount leak bug in macio_gpio_init_one()
We should call of_node_put() for the reference 'gparent' escaped
out of the for_each_child_of_node() as it has increased the refcount.
Fixes: 5b9ca526917b ("[PATCH] 3/5 powerpc: Add platform functions interpreter")
Signed-off-by: Liang He
---
arch/powerpc/platforms/powermac/pfunc_base.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/powermac/pfunc_base.c
b/arch/powerpc/platforms/powermac/pfunc_base.c
index 9c2947a3edd5..085e0ad20eba 100644
--- a/arch/powerpc/platforms/powermac/pfunc_base.c
+++ b/arch/powerpc/platforms/powermac/pfunc_base.c
@@ -136,6 +136,8 @@ static void __init macio_gpio_init_one(struct macio_chip
*macio)
for_each_child_of_node(gparent, gp)
pmf_do_functions(gp, NULL, 0, PMF_FLAGS_ON_INIT, NULL);
+ of_node_put(gparent);
+
/* Note: We do not at this point implement the "at sleep" or "at wake"
* functions. I yet to find any for GPIOs anyway
*/
--
2.25.1
[PATCH] powerpc/powermac/low_i2c: Fix refcount leak bug in kw_i2c_probe()
We should call of_node_put() for the reference 'parent' returned by
of_get_parent() which has increased the refcount.
Fixes: 730745a5c450 ("[PATCH] 1/5 powerpc: Rework PowerMac i2c part 1")
Signed-off-by: Liang He
---
arch/powerpc/platforms/powermac/low_i2c.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/powermac/low_i2c.c
b/arch/powerpc/platforms/powermac/low_i2c.c
index c1c430c66dc9..40f3aa432fba 100644
--- a/arch/powerpc/platforms/powermac/low_i2c.c
+++ b/arch/powerpc/platforms/powermac/low_i2c.c
@@ -627,6 +627,7 @@ static void __init kw_i2c_probe(void)
if (parent == NULL)
continue;
chans = parent->name[0] == 'u' ? 2 : 1;
+ of_node_put(parent);
for (i = 0; i < chans; i++)
kw_i2c_add(host, np, np, i);
} else {
--
2.25.1
[PATCH] powerpc/powermac/feature: Fix refcount leak bug
In probe_one_macio(), we should call of_node_put() for the refernece
'node' escaped out of the for_each_node_by_name() which has increased
its refcount. While the 'node' will finally escaped into a global
reference, we should still call of_node_put() in fail path which will
stop global reference creation.
Fixes: 51d3082fe6e5 ("[PATCH] powerpc: Unify udbg (#2)")
Signed-off-by: Liang He
---
commit-51d3082fe6e5 first introduces the 'of_find_node_by_name' which
involved the refcounting bug.
arch/powerpc/platforms/powermac/feature.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/powermac/feature.c
b/arch/powerpc/platforms/powermac/feature.c
index 5cc958adba13..0382d20b5619 100644
--- a/arch/powerpc/platforms/powermac/feature.c
+++ b/arch/powerpc/platforms/powermac/feature.c
@@ -2632,31 +2632,31 @@ static void __init probe_one_macio(const char *name,
const char *compat, int typ
if (!macio_chips[i].of_node)
break;
if (macio_chips[i].of_node == node)
- return;
+ goto out_put;
}
if (i >= MAX_MACIO_CHIPS) {
printk(KERN_ERR "pmac_feature: Please increase MAX_MACIO_CHIPS
!\n");
printk(KERN_ERR "pmac_feature: %pOF skipped\n", node);
- return;
+ goto out_put;
}
addrp = of_get_pci_address(node, 0, , NULL);
if (addrp == NULL) {
printk(KERN_ERR "pmac_feature: %pOF: can't find base !\n",
node);
- return;
+ goto out_put;
}
addr = of_translate_address(node, addrp);
if (addr == 0) {
printk(KERN_ERR "pmac_feature: %pOF, can't translate base !\n",
node);
- return;
+ goto out_put;
}
base = ioremap(addr, (unsigned long)size);
if (!base) {
printk(KERN_ERR "pmac_feature: %pOF, can't map mac-io chip !\n",
node);
- return;
+ goto out_put;
}
if (type == macio_keylargo || type == macio_keylargo2) {
const u32 *did = of_get_property(node, "device-id", NULL);
@@ -2677,6 +2677,11 @@ static void __init probe_one_macio(const char *name,
const char *compat, int typ
macio_chips[i].rev = *revp;
printk(KERN_INFO "Found a %s mac-io controller, rev: %d, mapped at
0x%p\n",
macio_names[type], macio_chips[i].rev, macio_chips[i].base);
+
+ return;
+
+out_put:
+ of_node_put(node);
}
static int __init
--
2.25.1
[PATCH] powerpc: fsl: pci: Remove of_node_put() in fsl_pci_assign_primary()
for_each_matching_node() will automatically increase and decrease
the refcount. As there is a reference escaped out into global
'fsl_pci_primary', we should not use of_node_put() anymore.
Fixes: 905e75c46dba ("powerpc/fsl-pci: Unify pci/pcie initialization code")
Signed-off-by: Liang He
---
arch/powerpc/sysdev/fsl_pci.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 4c986c955951..d23a56276f42 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1137,7 +1137,6 @@ void __init fsl_pci_assign_primary(void)
for_each_matching_node(np, pci_ids) {
if (of_device_is_available(np)) {
fsl_pci_primary = np;
- of_node_put(np);
return;
}
}
--
2.25.1
[PATCH] powerpc: sysdev: fsl_msi: Add missing of_node_put() for of_parse_phandle()
In fsl_setup_msi_irqs(), we should use of_node_put() for the
refernece 'np' returned by of_parse_phandle() which increases
the refcount.
Fixes: 895d603f945ba ("powerpc/fsl_msi: add support for the fsl, msi property
in PCI nodes")
Co-authored-by: Miaoqian Lin
Signed-off-by: Liang He
---
There is an incomplete fix:
https://lore.kernel.org/all/20220526010935.32138-1-zhengyongj...@huawei.com/
We should call of_node_put() both in fail path and normal path.
arch/powerpc/sysdev/fsl_msi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index ef9a5999fa93..73c2d70706c0 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -209,8 +209,10 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev, int
nvec, int type)
dev_err(>dev,
"node %pOF has an invalid fsl,msi phandle %u\n",
hose->dn, np->phandle);
+ of_node_put(np);
return -EINVAL;
}
+ of_node_put(np);
}
msi_for_each_desc(entry, >dev, MSI_DESC_NOTASSOCIATED) {
--
2.25.1
[PATCH] powerpc: fsl: gtm: Remove of_node_get() in fsl_gtm_init()
for_each_compatible_node() will automaitically increase and decrease the refcount of the device_node object. There is no need to call additional of_node_get(). It is better to keep the original meaning of refcounting as there is no any new reference created. Signed-off-by: Liang He --- I do not understand the whole story of the gtm, so maybe we want to keep the object always alive by using additional refcounting. Please check it carefully. arch/powerpc/sysdev/fsl_gtm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c index 39186ad6b3c3..e13ebd2be416 100644 --- a/arch/powerpc/sysdev/fsl_gtm.c +++ b/arch/powerpc/sysdev/fsl_gtm.c @@ -423,7 +423,6 @@ static int __init fsl_gtm_init(void) /* We don't want to lose the node and its ->data */ np->data = gtm; - of_node_get(np); continue; err: -- 2.25.1
[PATCH] soc: fsl: qbman: Fix missing of_node_put() in qbman_init_private_mem()
We should call of_node_put() for the reference returned by
of_parse_phandle() which will increase the refcount.
Fixes: 42d0349784c7 ("soc/fsl/qbman: Add common routine for QBMan private
allocations")
Co-authored-by: Miaoqian Lin
Signed-off-by: Liang He
---
drivers/soc/fsl/qbman/dpaa_sys.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/soc/fsl/qbman/dpaa_sys.c b/drivers/soc/fsl/qbman/dpaa_sys.c
index 9dd8bb571dbc..6138a68ea699 100644
--- a/drivers/soc/fsl/qbman/dpaa_sys.c
+++ b/drivers/soc/fsl/qbman/dpaa_sys.c
@@ -52,7 +52,8 @@ int qbman_init_private_mem(struct device *dev, int idx,
dma_addr_t *addr,
rmem = of_reserved_mem_lookup(mem_node);
if (!rmem) {
dev_err(dev, "of_reserved_mem_lookup() returned NULL\n");
- return -ENODEV;
+ err = -ENODEV;
+ goto out_of_put;
}
*addr = rmem->base;
*size = rmem->size;
@@ -66,24 +67,35 @@ int qbman_init_private_mem(struct device *dev, int idx,
dma_addr_t *addr,
prop = of_find_property(mem_node, "reg", );
if (!prop) {
prop = devm_kzalloc(dev, sizeof(*prop), GFP_KERNEL);
- if (!prop)
- return -ENOMEM;
+ if (!prop) {
+ err = -ENOMEM;
+ goto out_of_put;
+ }
prop->value = res_array = devm_kzalloc(dev, sizeof(__be32) * 4,
GFP_KERNEL);
- if (!prop->value)
- return -ENOMEM;
+ if (!prop->value) {
+ err = -ENOMEM;
+ goto out_of_put;
+ }
res_array[0] = cpu_to_be32(upper_32_bits(*addr));
res_array[1] = cpu_to_be32(lower_32_bits(*addr));
res_array[2] = cpu_to_be32(upper_32_bits(*size));
res_array[3] = cpu_to_be32(lower_32_bits(*size));
prop->length = sizeof(__be32) * 4;
prop->name = devm_kstrdup(dev, "reg", GFP_KERNEL);
- if (!prop->name)
- return -ENOMEM;
+ if (!prop->name) {
+ err = -ENOMEM;
+ goto out_of_put;
+ }
err = of_add_property(mem_node, prop);
if (err)
- return err;
+ goto out_of_put;
}
+ of_node_put(mem_node);
return 0;
+
+out_of_put:
+ of_node_put(mem_node);
+ return err;
}
--
2.25.1
[PATCH] powerpc: kernel: pci-common: Fix refcount bug for 'phb->dn'
In pcibios_alloc_controller(), 'phb' is allocated and escaped into
global 'hose_list'. So we should call of_node_get() when a new reference
created into 'phb->dn'. And when phb is freed, we should call
of_node_put() on it.
NOTE: This function is called in the iteration of for_each_xx in
chrp_find_bridges() function. If there is no of_node_get(), the object
maybe prematurely freed.
Signed-off-by: Liang He
---
I do not know if we should insert the of_node_put() in or out of the
spin_lock/spin_unlock. Please check it carefully.
arch/powerpc/kernel/pci-common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 068410cd54a3..f58dcf3a92bb 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -117,7 +117,7 @@ struct pci_controller *pcibios_alloc_controller(struct
device_node *dev)
phb->global_number = get_phb_number(dev);
list_add_tail(>list_node, _list);
spin_unlock(_spinlock);
- phb->dn = dev;
+ phb->dn = of_node_get(dev);
phb->is_dynamic = slab_is_available();
#ifdef CONFIG_PPC64
if (dev) {
@@ -140,7 +140,7 @@ void pcibios_free_controller(struct pci_controller *phb)
/* Clear bit of phb_bitmap to allow reuse of this PHB number. */
if (phb->global_number < MAX_PHBS)
clear_bit(phb->global_number, phb_bitmap);
-
+ of_node_put(phb->dn);
list_del(>list_node);
spin_unlock(_spinlock);
--
2.25.1
[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
Re:Re: [PATCH] powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API
At 2022-07-02 03:47:22, "Tyrel Datwyler" wrote:
>On 7/1/22 06:17, Liang He wrote:
>> In pci_add_device_node_info(), we should use of_node_put() for the
>> reference 'parent' returned by of_get_parent() to keep refcount
>> balance.
>>
>> Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn")
>> Co-authored-by: Miaoqian Lin
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/kernel/pci_dn.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index 938ab8838ab5..aa221958007e 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct
>> pci_controller *hose,
>> INIT_LIST_HEAD(>list);
>> parent = of_get_parent(dn);
>> pdn->parent = parent ? PCI_DN(parent) : NULL;
>NACK
>
>pdn->parent is now a long term reference so we should not do a put on parent
>until we pdn->parent is no longer valid.
>
>-Tyrel
Hi, Tyrel
Thanks for reviewing this code.
But I think there is some confusion about the of_get_parent() and I can confirm
my point when I check the 'pci_remove_device_node_info' function, which may be
a second bug.
In pci_remove_device_node_info(), I notice the comment, 'Drop the parent
pci_dn's ref ...'.
However, of_get_parent() will increase the refcount of the parent, and then the
following of_node_put() will decrease the refcount, so, finally, there is no
any change.
Back to this case, as the 'pdn->parent' is not the reference of parent
device_node, it is
device_node's data, so I think it is better to keep the original meaning of
refcounting, i.e,
add a of_node_put() to keep the refcount balance.
If I have some mis-understanding, please correct me.
Thanks,
Liang
>
>> +of_node_put(parent);
>> if (pdn->parent)
>> list_add_tail(>list, >parent->child_list);
>>
[PATCH v2 2/2] powerpc: cell: iommu: Hold reference returned by of_find_node_by_name()
In cell_iommu_init_disabled(), we should hold the reference returned by of_find_node_by_name() and use it to call of_node_put() for reference balance. Signed-off-by: Liang He --- changelog: v2: (1) split v1's two files in to two commits (2) using 'check-then-put' coding style v1: hold the reference returned by of_find_xxx OF APIs v1-link: https://lore.kernel.org/all/20220621075333.4081413-1-win...@126.com/ arch/powerpc/platforms/cell/iommu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 0ca3efeef293..8c7133039566 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -720,8 +720,10 @@ static int __init cell_iommu_init_disabled(void) cell_disable_iommus(); /* If we have no Axon, we set up the spider DMA magic offset */ - if (of_find_node_by_name(NULL, "axon") == NULL) + np = of_find_node_by_name(NULL, "axon"); + if (!np) cell_dma_nommu_offset = SPIDER_DMA_OFFSET; + of_node_put(np); /* Now we need to check to see where the memory is mapped * in PCI space. We assume that all busses use the same dma -- 2.25.1
[PATCH v2 1/2] powerpc: cell: cbe_regs: Fix refcount bugs
There are several bugs as following:
(1) In cbe_get_be_node(), we should hold the reference returned by
of_find_xxx and of_get_xxx OF APIs and use it to call of_node_put
(2) In cbe_fill_regs_map(), we should same as above
(3) In cbe_regs_init(), during the iteration of for_each_node_by_type(),
the refcount of 'cpu' will be automatically increased and decreased.
However, there is a reference escaped out into 'map->cpu_node' and
we should properly handle it.
Signed-off-by: Liang He
---
chagelog:
v2: (1) split v1's two files in to two commits
(2) merge all bugs for cbe_regs.c
(3) using 'check-then-put' coding style
v1: only detect bug (1)
v1-link: https://lore.kernel.org/all/20220621075333.4081413-1-win...@126.com/
arch/powerpc/platforms/cell/cbe_regs.c | 40 +++---
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/cell/cbe_regs.c
b/arch/powerpc/platforms/cell/cbe_regs.c
index 316e533afc00..3fd3634aa515 100644
--- a/arch/powerpc/platforms/cell/cbe_regs.c
+++ b/arch/powerpc/platforms/cell/cbe_regs.c
@@ -182,9 +182,19 @@ static struct device_node *__init cbe_get_be_node(int
cpu_id)
if (WARN_ON_ONCE(!cpu_handle))
return np;
- for (i=0; ibe_node) {
- struct device_node *be, *np;
+ struct device_node *be, *np, *parent_np;
be = map->be_node;
- for_each_node_by_type(np, "pervasive")
- if (of_get_parent(np) == be)
+ for_each_node_by_type(np, "pervasive") {
+ parent_np = of_get_parent(np);
+ if (parent_np == be)
map->pmd_regs = of_iomap(np, 0);
+ of_node_put(parent_np);
+ }
- for_each_node_by_type(np, "CBEA-Internal-Interrupt-Controller")
- if (of_get_parent(np) == be)
+ for_each_node_by_type(np, "CBEA-Internal-Interrupt-Controller")
{
+ parent_np = of_get_parent(np);
+ if (parent_np == be)
map->iic_regs = of_iomap(np, 2);
+ of_node_put(parent_np);
+ }
- for_each_node_by_type(np, "mic-tm")
- if (of_get_parent(np) == be)
+ for_each_node_by_type(np, "mic-tm") {
+ parent_np = of_get_parent(np);
+ if (parent_np == be)
map->mic_tm_regs = of_iomap(np, 0);
+ of_node_put(parent_np);
+ }
} else {
struct device_node *cpu;
/* That hack must die die die ! */
@@ -261,7 +280,8 @@ void __init cbe_regs_init(void)
of_node_put(cpu);
return;
}
- map->cpu_node = cpu;
+ of_node_put(map->cpu_node);
+ map->cpu_node = of_node_get(cpu);
for_each_possible_cpu(i) {
struct cbe_thread_map *thread = _thread_map[i];
--
2.25.1
[PATCH] powerpc: 85xx: Fix refcount bugs in ge_imp3a_pci_assign_primary()
for_each_node_by_type() will automatically increase and decrease
the refcount during the iteration. However, there is a reference
escaped into global 'fsl_pci_primary' and we need to handle it.
Signed-off-by: Liang He
---
arch/powerpc/platforms/85xx/ge_imp3a.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/ge_imp3a.c
b/arch/powerpc/platforms/85xx/ge_imp3a.c
index 8e827376d97b..e3e8f18825a1 100644
--- a/arch/powerpc/platforms/85xx/ge_imp3a.c
+++ b/arch/powerpc/platforms/85xx/ge_imp3a.c
@@ -89,8 +89,10 @@ static void __init ge_imp3a_pci_assign_primary(void)
of_device_is_compatible(np, "fsl,mpc8548-pcie") ||
of_device_is_compatible(np, "fsl,p2020-pcie")) {
of_address_to_resource(np, 0, );
- if ((rsrc.start & 0xf) == 0x9000)
- fsl_pci_primary = np;
+ if ((rsrc.start & 0xf) == 0x9000) {
+ of_node_put(fsl_pci_primary);
+ fsl_pci_primary = of_node_get(np);
+ }
}
}
#endif
--
2.25.1
[PATCH] powerpc: 44x: Add of_node_put() when break out from for_each
In ppc47x_init_irq(), we need to call of_node_put() when there is
a break during the iteration of for_each_node_with_property() which
will automatically increase and decrease the refcount.
Signed-off-by: Liang He
---
arch/powerpc/platforms/44x/ppc476.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/44x/ppc476.c
b/arch/powerpc/platforms/44x/ppc476.c
index 20cc8f80b086..7d9075fb20c9 100644
--- a/arch/powerpc/platforms/44x/ppc476.c
+++ b/arch/powerpc/platforms/44x/ppc476.c
@@ -140,6 +140,7 @@ static void __init ppc47x_init_irq(void)
ppc_md.get_irq = mpic_get_irq;
} else
panic("Unrecognized top level interrupt controller");
+ of_node_put(np);
}
#ifdef CONFIG_SMP
--
2.25.1
[PATCH] powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API
In pci_add_device_node_info(), we should use of_node_put() for the
reference 'parent' returned by of_get_parent() to keep refcount
balance.
Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn")
Co-authored-by: Miaoqian Lin
Signed-off-by: Liang He
---
arch/powerpc/kernel/pci_dn.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 938ab8838ab5..aa221958007e 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct
pci_controller *hose,
INIT_LIST_HEAD(>list);
parent = of_get_parent(dn);
pdn->parent = parent ? PCI_DN(parent) : NULL;
+ of_node_put(parent);
if (pdn->parent)
list_add_tail(>list, >parent->child_list);
--
2.25.1
[PATCH] powerpc: kernel: 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
---
Inserting of_node_put() in the 'if' condition is learned from Orsan:
https://lore.kernel.org/all/CA+H2tpH1hN1AJ=6vvgqxw6bz7xqdbzxdaev_oqwmnw+uxqk...@mail.gmail.com/
I move the of_get_parent() just before its first usage, otherwise, we
need lots of of_node_put() before the 'return'.
arch/powerpc/kernel/legacy_serial.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/legacy_serial.c
b/arch/powerpc/kernel/legacy_serial.c
index f048c424c525..c7e9d7b5df8c 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,7 +194,8 @@ 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), 0))
return add_legacy_port(np, -1, UPIO_TSI, addr, addr,
0, legacy_port_flags, 0);
else
--
2.25.1
[PATCH] macintosh: Add missing of_node_get() in do_attach()
We need a of_node_get() for of_find_compatible_node() to keep refcount
balance.
Signed-off-by: Liang He
---
drivers/macintosh/therm_windtunnel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/macintosh/therm_windtunnel.c
b/drivers/macintosh/therm_windtunnel.c
index 9226b74fa08f..bee0510ab1df 100644
--- a/drivers/macintosh/therm_windtunnel.c
+++ b/drivers/macintosh/therm_windtunnel.c
@@ -317,6 +317,7 @@ static void do_attach(struct i2c_adapter *adapter)
if (x.running || strncmp(adapter->name, "uni-n", 5))
return;
+ of_node_get(adapter->dev.of_node);
np = of_find_compatible_node(adapter->dev.of_node, NULL, "MAC,ds1775");
if (np) {
of_node_put(np);
@@ -325,6 +326,7 @@ static void do_attach(struct i2c_adapter *adapter)
i2c_new_scanned_device(adapter, , scan_ds1775, NULL);
}
+ of_node_get(adapter->dev.of_node);
np = of_find_compatible_node(adapter->dev.of_node, NULL, "MAC,adm1030");
if (np) {
of_node_put(np);
--
2.25.1
[PATCH] powerpc/pseries: Hold reference and fix refcount leak bugs
In pseries_cpuhp_cache_use_count() and pseries_cpuhp_detach_nodes(),
we need carefully hold the reference returned by
of_find_next_cache_node() and use it to call of_node_put() to keep
refcount balance.
Signed-off-by: Liang He
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 0f8cd8b06432..e0a7ac5db15d 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -619,17 +619,21 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
static unsigned int pseries_cpuhp_cache_use_count(const struct device_node
*cachedn)
{
unsigned int use_count = 0;
- struct device_node *dn;
+ struct device_node *dn, *tn;
WARN_ON(!of_node_is_type(cachedn, "cache"));
for_each_of_cpu_node(dn) {
- if (of_find_next_cache_node(dn) == cachedn)
+ tn = of_find_next_cache_node(dn);
+ of_node_put(tn);
+ if (tn == cachedn)
use_count++;
}
for_each_node_by_type(dn, "cache") {
- if (of_find_next_cache_node(dn) == cachedn)
+ tn = of_find_next_cache_node(dn);
+ of_node_put(tn);
+ if (tn == cachedn)
use_count++;
}
@@ -649,10 +653,13 @@ static int pseries_cpuhp_detach_nodes(struct device_node
*cpudn)
dn = cpudn;
while ((dn = of_find_next_cache_node(dn))) {
- if (pseries_cpuhp_cache_use_count(dn) > 1)
+ if (pseries_cpuhp_cache_use_count(dn) > 1) {
+ of_node_put(dn);
break;
+ }
ret = of_changeset_detach_node(, dn);
+ of_node_put(dn);
if (ret)
goto out;
}
--
2.25.1
[PATCH] powerpc/83xx: Hold the reference returned by of_find_compatible_node
In mpc832x_spi_init(), we should hold the reference returned by
of_find_compatible_node() and use it to call of_node_put() for
refcount balance.
Signed-off-by: Liang He
---
arch/powerpc/platforms/83xx/mpc832x_rdb.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index bb8caa5071f8..e12cb44e717f 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
@@ -162,6 +162,8 @@ static struct spi_board_info mpc832x_spi_boardinfo = {
static int __init mpc832x_spi_init(void)
{
+ struct device_node *np;
+
par_io_config_pin(3, 0, 3, 0, 1, 0); /* SPI1 MOSI, I/O */
par_io_config_pin(3, 1, 3, 0, 1, 0); /* SPI1 MISO, I/O */
par_io_config_pin(3, 2, 3, 0, 1, 0); /* SPI1 CLK, I/O */
@@ -175,7 +177,9 @@ static int __init mpc832x_spi_init(void)
* Don't bother with legacy stuff when device tree contains
* mmc-spi-slot node.
*/
- if (of_find_compatible_node(NULL, NULL, "mmc-spi-slot"))
+ np = of_find_compatible_node(NULL, NULL, "mmc-spi-slot");
+ of_node_put(np);
+ if (np)
return 0;
return fsl_spi_init(_spi_boardinfo, 1, mpc83xx_spi_cs_control);
}
--
2.25.1
[PATCH] powerpc/512x: Hold the reference returned by of_find_compatible_node
In mpc5121_clk_provide_migration_support(), we need to hold the
reference returned by of_find_compatible_node() and use it to call
of_node_put for refcount balance.
Signed-off-by: Liang He
---
arch/powerpc/platforms/512x/clock-commonclk.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c
b/arch/powerpc/platforms/512x/clock-commonclk.c
index ca475462e95b..42abeba4f698 100644
--- a/arch/powerpc/platforms/512x/clock-commonclk.c
+++ b/arch/powerpc/platforms/512x/clock-commonclk.c
@@ -950,7 +950,7 @@ static void __init mpc5121_clk_register_of_provider(struct
device_node *np)
*/
static void __init mpc5121_clk_provide_migration_support(void)
{
-
+ struct device_node *np;
/*
* pre-enable those clock items which are not yet appropriately
* acquired by their peripheral driver
@@ -970,7 +970,9 @@ static void __init
mpc5121_clk_provide_migration_support(void)
* unused and so it gets disabled
*/
clk_prepare_enable(clks[MPC512x_CLK_PSC3_MCLK]);/* serial console */
- if (of_find_compatible_node(NULL, "pci", "fsl,mpc5121-pci"))
+ np = of_find_compatible_node(NULL, "pci", "fsl,mpc5121-pci");
+ of_node_put(np);
+ if (np)
clk_prepare_enable(clks[MPC512x_CLK_PCI]);
}
--
2.25.1
[PATCH] powerpc/cell: Hold reference returned by of_find_node_by_xxx APIs
In cell_iommu_init_disabled() and cbe_get_be_node(), we need to hold the reference returned by of_find_node_by_xxx APIs and use it to call of_node_put() for refcount balance. Signed-off-by: Liang He --- arch/powerpc/platforms/cell/cbe_regs.c | 10 -- arch/powerpc/platforms/cell/iommu.c| 5 - 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/platforms/cell/cbe_regs.c b/arch/powerpc/platforms/cell/cbe_regs.c index 316e533afc00..09cbdfc070b3 100644 --- a/arch/powerpc/platforms/cell/cbe_regs.c +++ b/arch/powerpc/platforms/cell/cbe_regs.c @@ -182,9 +182,15 @@ static struct device_node *__init cbe_get_be_node(int cpu_id) if (WARN_ON_ONCE(!cpu_handle)) return np; - for (i=0; i
[PATCH] powerpc/pasemi: Fix refcount leak bug
In pas_pci_init(), we need one of_node_put() for of_find_node_by_path()
to keep refcount balance.
Signed-off-by: Liang He
---
arch/powerpc/platforms/pasemi/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/pasemi/pci.c
b/arch/powerpc/platforms/pasemi/pci.c
index 55f0160910bf..4b371fb0d9aa 100644
--- a/arch/powerpc/platforms/pasemi/pci.c
+++ b/arch/powerpc/platforms/pasemi/pci.c
@@ -282,6 +282,7 @@ void __init pas_pci_init(void)
pci_set_flags(PCI_SCAN_ALL_PCIE_DEVS);
np = of_find_compatible_node(root, NULL, "pasemi,rootbus");
+ of_node_put(root);
if (np) {
res = pas_add_bridge(np);
of_node_put(np);
--
2.25.1
[PATCH] powerpc/powermac: Fix refcount leak bug
In smp_core99_setup(), we need to add of_node_put() to keep refcount balance. Signed-off-by: Liang He --- arch/powerpc/platforms/powermac/smp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c index d9df45741ece..5b26a9012d2e 100644 --- a/arch/powerpc/platforms/powermac/smp.c +++ b/arch/powerpc/platforms/powermac/smp.c @@ -711,6 +711,7 @@ static void __init smp_core99_setup(int ncpus) printk(KERN_INFO "Processor timebase sync using" " platform function\n"); } + of_node_put(cpus); } #else /* CONFIG_PPC64 */ -- 2.25.1
[PATCH v3] powerpc/powernv: Fix refcount leak bugs
In these driver init functions, there are two kinds of errors:
(1) missing of_put_node() for of_find_compatible_node()'s returned
pointer (refcount incremented) in fail path or when it is not
used anymore.
(2) missing of_put_node() for 'for_each_xxx' loop's break
These bugs are similar with the ones reported by commit-09700c504d.
Signed-off-by: Liang He
---
changelog:
v3: merge more refcount bugs into one commit
v2: merge opal-*.c
v1: send mising of_node_put patch for each file
arch/powerpc/platforms/powernv/idle.c | 1 +
arch/powerpc/platforms/powernv/opal-core.c | 2 ++
arch/powerpc/platforms/powernv/opal-powercap.c | 6 +-
arch/powerpc/platforms/powernv/opal-psr.c | 6 +-
arch/powerpc/platforms/powernv/opal-sensor-groups.c | 6 +-
arch/powerpc/platforms/powernv/opal.c | 1 +
6 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/idle.c
b/arch/powerpc/platforms/powernv/idle.c
index 6f94b808dd39..c1b369c7f507 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -1419,6 +1419,7 @@ static int __init pnv_parse_cpuidle_dt(void)
kfree(temp_u32);
kfree(temp_u64);
kfree(temp_string);
+ of_node_put(np);
return rc;
}
diff --git a/arch/powerpc/platforms/powernv/opal-core.c
b/arch/powerpc/platforms/powernv/opal-core.c
index adcb1a1a2bfe..bb7657115f1d 100644
--- a/arch/powerpc/platforms/powernv/opal-core.c
+++ b/arch/powerpc/platforms/powernv/opal-core.c
@@ -348,6 +348,8 @@ static int __init create_opalcore(void)
if (!dn || ret)
pr_warn("WARNING: Failed to read OPAL base & entry values\n");
+ of_node_put(dn);
+
/* Use count to keep track of the program headers */
count = 0;
diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
b/arch/powerpc/platforms/powernv/opal-powercap.c
index 64506b46e77b..78c359c90093 100644
--- a/arch/powerpc/platforms/powernv/opal-powercap.c
+++ b/arch/powerpc/platforms/powernv/opal-powercap.c
@@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
GFP_KERNEL);
if (!pcaps)
- return;
+ goto out_powercap;
powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
if (!powercap_kobj) {
@@ -226,6 +226,7 @@ void __init opal_powercap_init(void)
}
i++;
}
+ of_node_put(powercap);
return;
@@ -236,6 +237,9 @@ void __init opal_powercap_init(void)
kfree(pcaps[i].pg.name);
}
kobject_put(powercap_kobj);
+ of_node_put(node);
out_pcaps:
kfree(pcaps);
+out_powercap:
+ of_node_put(powercap);
}
diff --git a/arch/powerpc/platforms/powernv/opal-psr.c
b/arch/powerpc/platforms/powernv/opal-psr.c
index 69d7e75950d1..ec32e0a93f08 100644
--- a/arch/powerpc/platforms/powernv/opal-psr.c
+++ b/arch/powerpc/platforms/powernv/opal-psr.c
@@ -135,7 +135,7 @@ void __init opal_psr_init(void)
psr_attrs = kcalloc(of_get_child_count(psr), sizeof(*psr_attrs),
GFP_KERNEL);
if (!psr_attrs)
- return;
+ goto out_psr;
psr_kobj = kobject_create_and_add("psr", opal_kobj);
if (!psr_kobj) {
@@ -162,10 +162,14 @@ void __init opal_psr_init(void)
}
i++;
}
+ of_node_put(psr);
return;
out_kobj:
+ of_node_put(node);
kobject_put(psr_kobj);
out:
kfree(psr_attrs);
+out_psr:
+ of_node_put(psr);
}
diff --git a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
index 8fba7d25ae56..9944376b115c 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
@@ -170,7 +170,7 @@ void __init opal_sensor_groups_init(void)
sgs = kcalloc(of_get_child_count(sg), sizeof(*sgs), GFP_KERNEL);
if (!sgs)
- return;
+ goto out_sg_put;
sg_kobj = kobject_create_and_add("sensor_groups", opal_kobj);
if (!sg_kobj) {
@@ -222,6 +222,7 @@ void __init opal_sensor_groups_init(void)
}
i++;
}
+ of_node_put(sg);
return;
@@ -231,6 +232,9 @@ void __init opal_sensor_groups_init(void)
kfree(sgs[i].sg.attrs);
}
kobject_put(sg_kobj);
+ of_node_put(node);
out_sgs:
kfree(sgs);
+out_sg_put:
+ of_node_put(sg);
}
diff --git a/arch/powerpc/platforms/powernv/opal.c
b/arch/powerpc/platforms/powernv/opal.c
index 55a8fbfdb5b2..d86cc48a10aa 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -952,6 +952,7 @@ static vo
[PATCH v2] powerpc/sysdev: Fix refcount leak bugs
We need add corresponding of_node_put() to keep refcount balance
in sysdev.
Signed-off-by: Liang He
---
changelog:
v2: (1) merge all sysdev related bug into one commit
(2) find new bugs in spapr.c and mpic_msgr.c
v1: find missing of_node_put() in each file
arch/powerpc/sysdev/fsl_pci.c | 6 +-
arch/powerpc/sysdev/mpic_msgr.c | 10 +-
arch/powerpc/sysdev/xive/native.c | 15 ++-
arch/powerpc/sysdev/xive/spapr.c | 1 +
4 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 1011cfea2e32..4c986c955951 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -180,6 +180,7 @@ static int setup_one_atmu(struct ccsr_pci __iomem *pci,
static bool is_kdump(void)
{
struct device_node *node;
+ bool ret;
node = of_find_node_by_type(NULL, "memory");
if (!node) {
@@ -187,7 +188,10 @@ static bool is_kdump(void)
return false;
}
- return of_property_read_bool(node, "linux,usable-memory");
+ ret = of_property_read_bool(node, "linux,usable-memory");
+ of_node_put(node);
+
+ return ret;
}
/* atmu setup for fsl pci/pcie controller */
diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
index 698fefaaa6dd..2ff3a0a0cf3f 100644
--- a/arch/powerpc/sysdev/mpic_msgr.c
+++ b/arch/powerpc/sysdev/mpic_msgr.c
@@ -121,11 +121,13 @@ static unsigned int mpic_msgr_number_of_blocks(void)
count += 1;
}
+ of_node_put(aliases);
}
return count;
}
+
static unsigned int mpic_msgr_number_of_registers(void)
{
return mpic_msgr_number_of_blocks() * MPIC_MSGR_REGISTERS_PER_BLOCK;
@@ -144,12 +146,18 @@ static int mpic_msgr_block_number(struct device_node
*node)
for (index = 0; index < number_of_blocks; ++index) {
struct property *prop;
+ struct device_node *tn;
snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
prop = of_find_property(aliases, buf, NULL);
- if (node == of_find_node_by_path(prop->value))
+ tn = of_find_node_by_path(prop->value);
+ if (node == tn) {
+ of_node_put(tn);
break;
+ }
+ of_node_put(tn);
}
+ of_node_put(aliases);
return index == number_of_blocks ? -1 : index;
}
diff --git a/arch/powerpc/sysdev/xive/native.c
b/arch/powerpc/sysdev/xive/native.c
index d25d8c692909..3925825954bc 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -579,12 +579,12 @@ bool __init xive_native_init(void)
/* Resource 1 is HV window */
if (of_address_to_resource(np, 1, )) {
pr_err("Failed to get thread mgmnt area resource\n");
- return false;
+ goto err_put;
}
tima = ioremap(r.start, resource_size());
if (!tima) {
pr_err("Failed to map thread mgmnt area\n");
- return false;
+ goto err_put;
}
/* Read number of priorities */
@@ -612,7 +612,7 @@ bool __init xive_native_init(void)
/* Resource 2 is OS window */
if (of_address_to_resource(np, 2, )) {
pr_err("Failed to get thread mgmnt area resource\n");
- return false;
+ goto err_put;
}
xive_tima_os = r.start;
@@ -624,7 +624,7 @@ bool __init xive_native_init(void)
rc = opal_xive_reset(OPAL_XIVE_MODE_EXPL);
if (rc) {
pr_err("Switch to exploitation mode failed with error %lld\n",
rc);
- return false;
+ goto err_put;
}
/* Setup some dummy HV pool VPs */
@@ -634,10 +634,15 @@ bool __init xive_native_init(void)
if (!xive_core_init(np, _native_ops, tima, TM_QW3_HV_PHYS,
max_prio)) {
opal_xive_reset(OPAL_XIVE_MODE_EMU);
- return false;
+ goto err_put;
}
+ of_node_put(np);
pr_info("Using %dkB queues\n", 1 << (xive_queue_shift - 10));
return true;
+
+err_put:
+ of_node_put(np);
+ return false;
}
static bool xive_native_provision_pages(void)
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index 7d5128676e83..d398823d138e 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -717,6 +717,7 @@ static bool __init xive_get_max_prio(u8 *max_prio)
}
reg = of_get_property(rootdn, "ibm,plat-res-int-priorities", );
+ of_node_put(rootdn);
if (!reg) {
pr_err("Failed to read 'ibm,plat-res-int-priorities'
property\n");
return false;
--
2.25.1
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
At 2022-06-20 19:11:33, "Christophe Leroy" wrote: >Hi, > >Le 20/06/2022 à 11:23, Liang He a écrit : >> >> Hi, Christophe. >> >> Sorry to trobule you again. >> >> Now I have found other bugs in same directories (i.e., arch/powerpc/sysdev), >> with the ones I have sent but not recieved acked-by or confirmed email. >> >> So I need to merge the old ones into the new ones as a PATCH-v2 and then >> resend the >> old ones ? >> or just use a new PATCH to send only new ones? >> >> I am afraid to make new trouble for maintainers, so can you share your >> valuable >> experience? >> > >Here is the list of your patches : >https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=84258 > > From my point of view, for all the patches that are still in status >"new" it is better than you send a v2 with more things into a single >patch. When the patch is in "under review" state, it is better to not >update it anymore. > >So in the list there are for instance several patches for powernv, so it >would be good if you can regroup all of them in a single v2 patch. > >Christophe Thanks, Christophe. I will follow your rules and try to group the 'new' state ones.
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
At 2022-06-18 16:48:26, "Christophe Leroy" wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>>
>>
>>
>>
>>
>> 在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>>>
>>>
>>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>>> In add_pcspkr(), it is better to call of_node_put() after the
>>>> 'if(!np)' check.
>>>
>>> Why is it better ?
>>>
>>>
>>>
>>> /**
>>> * of_node_put() - Decrement refcount of a node
>>> * @node: Node to dec refcount, NULL is supported to simplify writing of
>>> * callers
>>> */
>>> void of_node_put(struct device_node *node)
>>> {
>>> if (node)
>>> kobject_put(>kobj);
>>> }
>>> EXPORT_SYMBOL(of_node_put);
>>>
>>>
>>>
>>> Christophe
>>
>> Hi, Christophe.
>>
>> Thanks for your reply and I want to have a discussion.
>>
>> In my thought, xxx_put(pointer)'s semantic usually means
>> this reference has been used done and will not be used
>> anymore. Is this semantic more reasonable, right?
>>
>> Besides, if the np is NULL, we can just return and save a cpu
>> time for the xxx_put() call.
>>
>> Otherwise, I prefer to call it 'use(check)-after-put'.
>>
>> In fact, I have meet many other 'use(check)-after-put' instances
>> after I send this patch-commit, so I am waiting for this
>> discussion.
>>
>> This is just my thought, it may be wrong.
>>
>> Anyway, thanks for your reply.
>
>Well in principle you are right, in an ideal world it should be like
>that. However, you have to wonder if it is worth the churn. The CPU
>cycle argument is valid only if that function is used in a hot path. But
>as we are talking about error handling, it can't be a hot path.
>
>Taking into account the comment associated of of_node_put : "NULL is
>supported to simplify writing of callers", it means that usage is valid,
>just like it is with function kfree() after a kmalloc().
>
>So in a new developpement, or when doing real modifications to a driver,
>that kind of change can be done ideally. However for drivers that have
>been there for years without any change, ask yourself if it is worth the
>churn. You spend time on it, you require other people to spend time on
>it for reviewing and applying your patches and during that time they
>don't do other things that could have been more usefull.
>
>So unless this change is part of a more global patch, I think it is not
>worth the effort.
>
>By the way, also for all your other patches, I think you should start
>doing all the changes locally on your side, and when you are finished
>try to group things together in bigger patches per area instead of
>sending one by one. I see you have already started doing that for
>opal/powernv for instance, but there are still individual powernv/opal
>in the queue. I think you should group all together in a single patch.
>And same for other areas, please try to minimise the number of patches.
>We don't link huge bombs that modify all the kernel at once, but you can
>group things together, one patch for powerpc core parts, one patch for
>each platform in arch/powerpc/platforms/ etc ...
>
>
>Christophe
Hi, Christophe.
Sorry to trobule you again.
Now I have found other bugs in same directories (i.e., arch/powerpc/sysdev),
with the ones I have sent but not recieved acked-by or confirmed email.
So I need to merge the old ones into the new ones as a PATCH-v2 and then resend
the
old ones ?
or just use a new PATCH to send only new ones?
I am afraid to make new trouble for maintainers, so can you share your valuable
experience?
Thanks very much.
Liang
[PATCH] powerpc/embedded6xx/ls_uart: Add missing of_node_put()
In ls_uarts_init(), we need to add a of_node_put() to keep refcount balance. Signed-off-by: Liang He --- arch/powerpc/platforms/embedded6xx/ls_uart.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/powerpc/platforms/embedded6xx/ls_uart.c b/arch/powerpc/platforms/embedded6xx/ls_uart.c index 0133e175a0fc..4ecbc55b37c0 100644 --- a/arch/powerpc/platforms/embedded6xx/ls_uart.c +++ b/arch/powerpc/platforms/embedded6xx/ls_uart.c @@ -124,6 +124,8 @@ static int __init ls_uarts_init(void) avr_clock = *(u32*)of_get_property(avr, "clock-frequency", ); phys_addr = ((u32*)of_get_property(avr, "reg", ))[0]; + of_node_put(avr); + if (!avr_clock || !phys_addr) return -EINVAL; -- 2.25.1
[PATCH] powerpc/powernv: Fix refcount leak bug in idle.c
In pnv_parse_cpuidle_dt(), of_find_node_by_path() will return a node pointer with refcount incremented. We should use of_node_put() in fail path or when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/platforms/powernv/idle.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 6f94b808dd39..c1b369c7f507 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -1419,6 +1419,7 @@ static int __init pnv_parse_cpuidle_dt(void) kfree(temp_u32); kfree(temp_u64); kfree(temp_string); + of_node_put(np); return rc; } -- 2.25.1
[PATCH] powerpc: pseries: Fix refcount bug in ibmebus
In ibmebus_match_path(), we should use of_node_put() to keep
refcount balance.
Signed-off-by: Liang He
---
arch/powerpc/platforms/pseries/ibmebus.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/ibmebus.c
b/arch/powerpc/platforms/pseries/ibmebus.c
index 7ee3ed7d6cc2..a870cada7acd 100644
--- a/arch/powerpc/platforms/pseries/ibmebus.c
+++ b/arch/powerpc/platforms/pseries/ibmebus.c
@@ -152,7 +152,11 @@ static const struct dma_map_ops ibmebus_dma_ops = {
static int ibmebus_match_path(struct device *dev, const void *data)
{
struct device_node *dn = to_platform_device(dev)->dev.of_node;
- return (of_find_node_by_path(data) == dn);
+ struct device_node *tn = of_find_node_by_path(data);
+
+ of_node_put(tn);
+
+ return (tn == dn);
}
static int ibmebus_match_node(struct device *dev, const void *data)
--
2.25.1
[PATCH] powerpc/cell: Fix refcount leak bugs
We should use of_node_put() for of_find_node_by_path() and of_find_node_by_phandle() to keep refcount balance. Signed-off-by: Liang He --- arch/powerpc/platforms/cell/setup.c | 2 ++ arch/powerpc/platforms/cell/spu_manage.c | 2 ++ arch/powerpc/platforms/cell/spufs/inode.c | 1 + 3 files changed, 5 insertions(+) diff --git a/arch/powerpc/platforms/cell/setup.c b/arch/powerpc/platforms/cell/setup.c index 52de014983c9..47eaf75349f2 100644 --- a/arch/powerpc/platforms/cell/setup.c +++ b/arch/powerpc/platforms/cell/setup.c @@ -167,6 +167,8 @@ static int __init cell_publish_devices(void) of_platform_device_create(np, NULL, NULL); } + of_node_put(root); + /* There is no device for the MIC memory controller, thus we create * a platform device for it to attach the EDAC driver to. */ diff --git a/arch/powerpc/platforms/cell/spu_manage.c b/arch/powerpc/platforms/cell/spu_manage.c index ae09c5a91b40..f1ac4c742069 100644 --- a/arch/powerpc/platforms/cell/spu_manage.c +++ b/arch/powerpc/platforms/cell/spu_manage.c @@ -488,6 +488,8 @@ static void __init init_affinity_node(int cbe) avoid_ph = vic_dn->phandle; } + of_node_put(vic_dn); + list_add_tail(>aff_list, _spu->aff_list); last_spu = spu; break; diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index 34334c32b7f5..320008528edd 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -660,6 +660,7 @@ spufs_init_isolated_loader(void) return; loader = of_get_property(dn, "loader", ); + of_node_put(dn); if (!loader) return; -- 2.25.1
[PATCH] powerpc: kernel: Fix refcount bug in legacy_serial.c
In find_legacy_serial_ports(), of_find_node_by_path() will return
a node pointer with refcount incremented. We should use of_node_put()
when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/kernel/legacy_serial.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/legacy_serial.c
b/arch/powerpc/kernel/legacy_serial.c
index 5c58460b269a..f048c424c525 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -471,6 +471,8 @@ void __init find_legacy_serial_ports(void)
}
#endif
+ of_node_put(stdout);
+
DBG("legacy_serial_console = %d\n", legacy_serial_console);
if (legacy_serial_console >= 0)
setup_legacy_serial_console(legacy_serial_console);
--
2.25.1
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
At 2022-06-18 16:48:26, "Christophe Leroy" wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>>
>>
>>
>>
>>
>> 在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>>>
>>>
>>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>>> In add_pcspkr(), it is better to call of_node_put() after the
>>>> 'if(!np)' check.
>>>
>>> Why is it better ?
>>>
>>>
>>>
>>> /**
>>> * of_node_put() - Decrement refcount of a node
>>> * @node: Node to dec refcount, NULL is supported to simplify writing of
>>> * callers
>>> */
>>> void of_node_put(struct device_node *node)
>>> {
>>> if (node)
>>> kobject_put(>kobj);
>>> }
>>> EXPORT_SYMBOL(of_node_put);
>>>
>>>
>>>
>>> Christophe
>>
>> Hi, Christophe.
>>
>> Thanks for your reply and I want to have a discussion.
>>
>> In my thought, xxx_put(pointer)'s semantic usually means
>> this reference has been used done and will not be used
>> anymore. Is this semantic more reasonable, right?
>>
>> Besides, if the np is NULL, we can just return and save a cpu
>> time for the xxx_put() call.
>>
>> Otherwise, I prefer to call it 'use(check)-after-put'.
>>
>> In fact, I have meet many other 'use(check)-after-put' instances
>> after I send this patch-commit, so I am waiting for this
>> discussion.
>>
>> This is just my thought, it may be wrong.
>>
>> Anyway, thanks for your reply.
>
>Well in principle you are right, in an ideal world it should be like
>that. However, you have to wonder if it is worth the churn. The CPU
>cycle argument is valid only if that function is used in a hot path. But
>as we are talking about error handling, it can't be a hot path.
>
Thanks very much for this valuable lesson.
>Taking into account the comment associated of of_node_put : "NULL is
>supported to simplify writing of callers", it means that usage is valid,
>just like it is with function kfree() after a kmalloc().
>
>So in a new developpement, or when doing real modifications to a driver,
>that kind of change can be done ideally. However for drivers that have
>been there for years without any change, ask yourself if it is worth the
>churn. You spend time on it, you require other people to spend time on
>it for reviewing and applying your patches and during that time they
>don't do other things that could have been more usefull.
>
Thanks for you advice, I will keep it in my mind before I send a new patch.
>So unless this change is part of a more global patch, I think it is not
>worth the effort.
>
>By the way, also for all your other patches, I think you should start
>doing all the changes locally on your side, and when you are finished
>try to group things together in bigger patches per area instead of
>sending one by one. I see you have already started doing that for
>opal/powernv for instance, but there are still individual powernv/opal
>in the queue. I think you should group all together in a single patch.
>And same for other areas, please try to minimise the number of patches.
>We don't link huge bombs that modify all the kernel at once, but you can
>group things together, one patch for powerpc core parts, one patch for
>each platform in arch/powerpc/platforms/ etc ...
>
You are right and I will follow this principle in future patching work.
While It is too exciting for me to begin the patching work , I should have
grouped my patches.
>
>Christophe
Thanks again, Christophe.
Liang
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>
>
>Le 17/06/2022 à 13:26, Liang He a écrit :
>> In add_pcspkr(), it is better to call of_node_put() after the
>> 'if(!np)' check.
>
>Why is it better ?
>
>
>
>/**
> * of_node_put() - Decrement refcount of a node
> * @node: Node to dec refcount, NULL is supported to simplify writing of
> *callers
> */
>void of_node_put(struct device_node *node)
>{
> if (node)
> kobject_put(>kobj);
>}
>EXPORT_SYMBOL(of_node_put);
>
>
>
>Christophe
Hi, Christophe.
Thanks for your reply and I want to have a discussion.
In my thought, xxx_put(pointer)'s semantic usually means
this reference has been used done and will not be used
anymore. Is this semantic more reasonable, right?
Besides, if the np is NULL, we can just return and save a cpu
time for the xxx_put() call.
Otherwise, I prefer to call it 'use(check)-after-put'.
In fact, I have meet many other 'use(check)-after-put' instances
after I send this patch-commit, so I am waiting for this
discussion.
This is just my thought, it may be wrong.
Anyway, thanks for your reply.
Liang
>
>
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/kernel/setup-common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/setup-common.c
>> b/arch/powerpc/kernel/setup-common.c
>> index eb0077b302e2..761817d1f4db 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
>> int ret;
>>
>> np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
>> -of_node_put(np);
>> if (!np)
>> return -ENODEV;
>> +of_node_put(np);
>>
>> pd = platform_device_alloc("pcspkr", -1);
>> if (!pd)
[PATCH] powerpc: perf: Fix refcount leak bug in imc-pmu.c
In update_events_in_group(), of_find_node_by_phandle() will return
a node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/perf/imc-pmu.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index d7976ab40d38..d517aba94d1b 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -240,8 +240,10 @@ static int update_events_in_group(struct device_node
*node, struct imc_pmu *pmu)
ct = of_get_child_count(pmu_events);
/* Get the event prefix */
- if (of_property_read_string(node, "events-prefix", ))
+ if (of_property_read_string(node, "events-prefix", )) {
+ of_node_put(pmu_events);
return 0;
+ }
/* Get a global unit and scale data if available */
if (of_property_read_string(node, "scale", _scale))
@@ -255,8 +257,10 @@ static int update_events_in_group(struct device_node
*node, struct imc_pmu *pmu)
/* Allocate memory for the events */
pmu->events = kcalloc(ct, sizeof(struct imc_events), GFP_KERNEL);
- if (!pmu->events)
+ if (!pmu->events) {
+ of_node_put(pmu_events);
return -ENOMEM;
+ }
ct = 0;
/* Parse the events and update the struct */
@@ -266,6 +270,8 @@ static int update_events_in_group(struct device_node *node,
struct imc_pmu *pmu)
ct++;
}
+ of_node_put(pmu_events);
+
/* Allocate memory for attribute group */
attr_group = kzalloc(sizeof(*attr_group), GFP_KERNEL);
if (!attr_group) {
--
2.25.1
[PATCH] tty: serial: Fix refcount leak bug in ucc_uart.c
In soc_info(), of_find_node_by_type() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- drivers/tty/serial/ucc_uart.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c index 6000853973c1..3cc9ef08455c 100644 --- a/drivers/tty/serial/ucc_uart.c +++ b/drivers/tty/serial/ucc_uart.c @@ -1137,6 +1137,8 @@ static unsigned int soc_info(unsigned int *rev_h, unsigned int *rev_l) /* No compatible property, so try the name. */ soc_string = np->name; + of_node_put(np); + /* Extract the SOC number from the "PowerPC," string */ if ((sscanf(soc_string, "PowerPC,%u", ) != 1) || !soc) return 0; -- 2.25.1
[PATCH v2] powerpc: embedded6xx: Fix refcount leak bugs
In xx_init_xx(), of_find_node_by_type() will return a node pointer
with refcount incremented. We should use of_node_put() when it is
not used anymore.
Signed-off-by: Liang He
---
v2: we merge all embedded6xx related bugs into one commit
v1: we only report the bug in holly_init_pci() of holly.c
arch/powerpc/platforms/embedded6xx/holly.c| 6 ++
arch/powerpc/platforms/embedded6xx/mpc7448_hpc2.c | 3 +++
2 files changed, 9 insertions(+)
diff --git a/arch/powerpc/platforms/embedded6xx/holly.c
b/arch/powerpc/platforms/embedded6xx/holly.c
index 78f2378d9223..bebc5a972694 100644
--- a/arch/powerpc/platforms/embedded6xx/holly.c
+++ b/arch/powerpc/platforms/embedded6xx/holly.c
@@ -123,6 +123,8 @@ static void __init holly_init_pci(void)
if (np)
tsi108_setup_pci(np, HOLLY_PCI_CFG_PHYS, 1);
+ of_node_put(np);
+
ppc_md.pci_exclude_device = holly_exclude_device;
if (ppc_md.progress)
ppc_md.progress("tsi108: resources set", 0x100);
@@ -184,6 +186,9 @@ static void __init holly_init_IRQ(void)
tsi108_pci_int_init(cascade_node);
irq_set_handler_data(cascade_pci_irq, mpic);
irq_set_chained_handler(cascade_pci_irq, tsi108_irq_cascade);
+
+ of_node_put(tsi_pci);
+ of_node_put(cascade_node);
#endif
/* Configure MPIC outputs to CPU0 */
tsi108_write_reg(TSI108_MPIC_OFFSET + 0x30c, 0);
@@ -210,6 +215,7 @@ static void __noreturn holly_restart(char *cmd)
if (bridge) {
prop = of_get_property(bridge, "reg", );
addr = of_translate_address(bridge, prop);
+ of_node_put(bridge);
}
addr += (TSI108_PB_OFFSET + 0x414);
diff --git a/arch/powerpc/platforms/embedded6xx/mpc7448_hpc2.c
b/arch/powerpc/platforms/embedded6xx/mpc7448_hpc2.c
index 8b2b42210356..ddf0c652af80 100644
--- a/arch/powerpc/platforms/embedded6xx/mpc7448_hpc2.c
+++ b/arch/powerpc/platforms/embedded6xx/mpc7448_hpc2.c
@@ -135,6 +135,9 @@ static void __init mpc7448_hpc2_init_IRQ(void)
tsi108_pci_int_init(cascade_node);
irq_set_handler_data(cascade_pci_irq, mpic);
irq_set_chained_handler(cascade_pci_irq, tsi108_irq_cascade);
+
+ of_node_put(tsi_pci);
+ of_node_put(cascade_node);
#endif
/* Configure MPIC outputs to CPU0 */
tsi108_write_reg(TSI108_MPIC_OFFSET + 0x30c, 0);
--
2.25.1
[PATCH] powerpc: embedded6xx: Fix refcount leak bug in holly.c
In holly_init_pci(), of_find_node_by_type() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/platforms/embedded6xx/holly.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/embedded6xx/holly.c b/arch/powerpc/platforms/embedded6xx/holly.c index 78f2378d9223..a4938505e89a 100644 --- a/arch/powerpc/platforms/embedded6xx/holly.c +++ b/arch/powerpc/platforms/embedded6xx/holly.c @@ -122,6 +122,7 @@ static void __init holly_init_pci(void) np = of_find_node_by_type(NULL, "pci"); if (np) tsi108_setup_pci(np, HOLLY_PCI_CFG_PHYS, 1); + of_node_put(np); ppc_md.pci_exclude_device = holly_exclude_device; if (ppc_md.progress) -- 2.25.1
[PATCH] powerpc: sysdev: Fix refcount leak bug in fsl_pci.c
In is_kdump(), we need a of_node_put() to dec the refcount which is
incremented by of_find_node_by_type().
Signed-off-by: Liang He
---
arch/powerpc/sysdev/fsl_pci.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 1011cfea2e32..4c986c955951 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -180,6 +180,7 @@ static int setup_one_atmu(struct ccsr_pci __iomem *pci,
static bool is_kdump(void)
{
struct device_node *node;
+ bool ret;
node = of_find_node_by_type(NULL, "memory");
if (!node) {
@@ -187,7 +188,10 @@ static bool is_kdump(void)
return false;
}
- return of_property_read_bool(node, "linux,usable-memory");
+ ret = of_property_read_bool(node, "linux,usable-memory");
+ of_node_put(node);
+
+ return ret;
}
/* atmu setup for fsl pci/pcie controller */
--
2.25.1
[PATCH] powerpc: 8xx: Fix refcount leak bug in tqm8xx_setup
In init_ioports(), of_find_node_by_name() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/platforms/8xx/tqm8xx_setup.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/powerpc/platforms/8xx/tqm8xx_setup.c b/arch/powerpc/platforms/8xx/tqm8xx_setup.c index 3725d51248df..ffcfd17a5fa3 100644 --- a/arch/powerpc/platforms/8xx/tqm8xx_setup.c +++ b/arch/powerpc/platforms/8xx/tqm8xx_setup.c @@ -105,6 +105,9 @@ static void __init init_ioports(void) if (dnode == NULL) return; prop = of_find_property(dnode, "ethernet1", ); + + of_node_put(dnode); + if (prop == NULL) return; -- 2.25.1
[PATCH] cpufreq: pmac32-cpufreq: Fix refcount leak bug
In pmac_cpufreq_init_MacRISC3(), we need to add corresponding of_node_put() for the three node pointers whose refcount have been incremented by of_find_node_by_name(). Signed-off-by: Liang He --- drivers/cpufreq/pmac32-cpufreq.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/cpufreq/pmac32-cpufreq.c b/drivers/cpufreq/pmac32-cpufreq.c index 20f64a8b0a35..4b8ee2014da6 100644 --- a/drivers/cpufreq/pmac32-cpufreq.c +++ b/drivers/cpufreq/pmac32-cpufreq.c @@ -470,6 +470,10 @@ static int pmac_cpufreq_init_MacRISC3(struct device_node *cpunode) if (slew_done_gpio_np) slew_done_gpio = read_gpio(slew_done_gpio_np); + of_node_put(volt_gpio_np); + of_node_put(freq_gpio_np); + of_node_put(slew_done_gpio_np); + /* If we use the frequency GPIOs, calculate the min/max speeds based * on the bus frequencies */ -- 2.25.1
[PATCH] powerpc: powernv: opal-core: Fix refcount leak bug
In create_opalcore(), of_find_node_by_name() will return a node
pointer with refcount incremented. We should use of_node_put()
when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/platforms/powernv/opal-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/opal-core.c
b/arch/powerpc/platforms/powernv/opal-core.c
index adcb1a1a2bfe..bb7657115f1d 100644
--- a/arch/powerpc/platforms/powernv/opal-core.c
+++ b/arch/powerpc/platforms/powernv/opal-core.c
@@ -348,6 +348,8 @@ static int __init create_opalcore(void)
if (!dn || ret)
pr_warn("WARNING: Failed to read OPAL base & entry values\n");
+ of_node_put(dn);
+
/* Use count to keep track of the program headers */
count = 0;
--
2.25.1
[PATCH] powerpc: sysdev: Fix refcount leak bug in mpic_msgr
In mpic_msgr_number_of_blocks() and mpic_msgr_block_number(), of_find_node_by_name() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/sysdev/mpic_msgr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c index 698fefaaa6dd..c2a235d9eb09 100644 --- a/arch/powerpc/sysdev/mpic_msgr.c +++ b/arch/powerpc/sysdev/mpic_msgr.c @@ -122,6 +122,7 @@ static unsigned int mpic_msgr_number_of_blocks(void) count += 1; } } + of_node_put(aliases); return count; } @@ -150,6 +151,7 @@ static int mpic_msgr_block_number(struct device_node *node) if (node == of_find_node_by_path(prop->value)) break; } + of_node_put(aliases); return index == number_of_blocks ? -1 : index; } -- 2.25.1
Re:Re: [PATCH] powerpc: powernv: Fix refcount leak bug in opal-powercap
At 2022-06-17 13:01:27, "Christophe JAILLET"
wrote:
>Le 17/06/2022 à 06:20, Liang He a écrit :
>> In opal_powercap_init(), of_find_compatible_node() will return
>> a node pointer with refcount incremented. We should use of_node_put()
>> in fail path or when it is not used anymore.
>>
>> Besides, for_each_child_of_node() will automatically *inc* and *dec*
>> refcount during iteration. However, we should add the of_node_put()
>> if there is a break.
>
>Hi,
>
>I'm not sure that your patch is right here. Because of this *inc* and
>*dec* things, do we still need to of_node_put(powercap) once we have
>entered for_each_child_of_node?
>
>I think that this reference will be released on the first iteration of
>the loop.
>
>
>Maybe of_node_put(powercap) should be duplicated everywhere it is
>relevant and removed from the error handling path?
>Or an additional reference should be taken before the loop?
>Or adding a new label with "powercap = NULL" and branching there when
>needed?
>
>CJ
>
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/platforms/powernv/opal-powercap.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
>> b/arch/powerpc/platforms/powernv/opal-powercap.c
>> index 64506b46e77b..b102477d3f95 100644
>> --- a/arch/powerpc/platforms/powernv/opal-powercap.c
>> +++ b/arch/powerpc/platforms/powernv/opal-powercap.c
>> @@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
>> pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
>> GFP_KERNEL);
>> if (!pcaps)
>> -return;
>> +goto out_powercap;
>>
>> powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
>> if (!powercap_kobj) {
>> @@ -236,6 +236,9 @@ void __init opal_powercap_init(void)
>> kfree(pcaps[i].pg.name);
>> }
>> kobject_put(powercap_kobj);
>> +of_node_put(node);
>> out_pcaps:
>> kfree(pcaps);
>> +out_powercap:
>> +of_node_put(powercap);
>> }
Hi, CJ.
I think my patch is correct based on the old commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.19-rc2=09700c504d8e63faffd2a2235074e8c5d130cb8f
Bugs and fix solutions in this 09700c504d8e63-commit are very similar with mine.
Besides, I also find similar new bugs in other two files in the same directory
'powernv',
so I have merged all three files' patches into one commit. '[PATCH v2]
powerpc: powernv: Fix refcount leak bug'.
Thanks.
Liang
[PATCH v2] powerpc: powernv: Fix refcount leak bug
In these driver init functions, there are two kinds of errors:
(1) missing of_put_node() for of_find_compatible_node()'s returned
pointer (refcount incremented) in fail path or when it is not
used anymore.
(2) missing of_put_node() for 'for_each_xxx' loop's break
These bugs are similar with the ones reported by commit-09700c504d.
Signed-off-by: Liang He
---
changelog:
v2: merge all powernv related bugs into one commit
v1: only fix bugs in opal-powercap.c
arch/powerpc/platforms/powernv/opal-powercap.c | 6 +-
arch/powerpc/platforms/powernv/opal-psr.c | 6 +-
arch/powerpc/platforms/powernv/opal-sensor-groups.c | 6 +-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
b/arch/powerpc/platforms/powernv/opal-powercap.c
index 64506b46e77b..78c359c90093 100644
--- a/arch/powerpc/platforms/powernv/opal-powercap.c
+++ b/arch/powerpc/platforms/powernv/opal-powercap.c
@@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
GFP_KERNEL);
if (!pcaps)
- return;
+ goto out_powercap;
powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
if (!powercap_kobj) {
@@ -226,6 +226,7 @@ void __init opal_powercap_init(void)
}
i++;
}
+ of_node_put(powercap);
return;
@@ -236,6 +237,9 @@ void __init opal_powercap_init(void)
kfree(pcaps[i].pg.name);
}
kobject_put(powercap_kobj);
+ of_node_put(node);
out_pcaps:
kfree(pcaps);
+out_powercap:
+ of_node_put(powercap);
}
diff --git a/arch/powerpc/platforms/powernv/opal-psr.c
b/arch/powerpc/platforms/powernv/opal-psr.c
index 69d7e75950d1..ec32e0a93f08 100644
--- a/arch/powerpc/platforms/powernv/opal-psr.c
+++ b/arch/powerpc/platforms/powernv/opal-psr.c
@@ -135,7 +135,7 @@ void __init opal_psr_init(void)
psr_attrs = kcalloc(of_get_child_count(psr), sizeof(*psr_attrs),
GFP_KERNEL);
if (!psr_attrs)
- return;
+ goto out_psr;
psr_kobj = kobject_create_and_add("psr", opal_kobj);
if (!psr_kobj) {
@@ -162,10 +162,14 @@ void __init opal_psr_init(void)
}
i++;
}
+ of_node_put(psr);
return;
out_kobj:
+ of_node_put(node);
kobject_put(psr_kobj);
out:
kfree(psr_attrs);
+out_psr:
+ of_node_put(psr);
}
diff --git a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
index 8fba7d25ae56..9944376b115c 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
@@ -170,7 +170,7 @@ void __init opal_sensor_groups_init(void)
sgs = kcalloc(of_get_child_count(sg), sizeof(*sgs), GFP_KERNEL);
if (!sgs)
- return;
+ goto out_sg_put;
sg_kobj = kobject_create_and_add("sensor_groups", opal_kobj);
if (!sg_kobj) {
@@ -222,6 +222,7 @@ void __init opal_sensor_groups_init(void)
}
i++;
}
+ of_node_put(sg);
return;
@@ -231,6 +232,9 @@ void __init opal_sensor_groups_init(void)
kfree(sgs[i].sg.attrs);
}
kobject_put(sg_kobj);
+ of_node_put(node);
out_sgs:
kfree(sgs);
+out_sg_put:
+ of_node_put(sg);
}
--
2.25.1
[PATCH] powerpc: maple: Fix refcount leak bug in time.c
In maple_get_boot_time(), of_find_compatible_node() will return
a node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/platforms/maple/time.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/maple/time.c
b/arch/powerpc/platforms/maple/time.c
index 823e219ef8ee..91606411d2e0 100644
--- a/arch/powerpc/platforms/maple/time.c
+++ b/arch/powerpc/platforms/maple/time.c
@@ -153,6 +153,7 @@ time64_t __init maple_get_boot_time(void)
maple_rtc_addr);
}
bail:
+ of_node_put(rtcs);
if (maple_rtc_addr == 0) {
maple_rtc_addr = RTC_PORT(0); /* legacy address */
printk(KERN_INFO "Maple: No device node for RTC, assuming "
--
2.25.1
Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
在 2022-06-17 18:57:36，"Christophe Leroy" 写道：
>
>
>Le 17/06/2022 à 12:47, Liang He a écrit :
>>
>>
>>
>> At 2022-06-17 16:27:03, conor.doo...@microchip.com wrote:
>>> On 17/06/2022 09:17, Liang He wrote:
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:53:13, "Christophe Leroy"
>>>> wrote:
>>>>>
>>>>>
>>>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>>>
>>>>>>
>>>>>>
>>>>>> At 2022-06-17 14:28:56, "Christophe Leroy"
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Le 17/06/2022 à 08:08, Liang He a écrit :
>>>>>>>> In gpio_halt_probe(), of_find_matching_node() will return a node
>>>>>>>> pointer with refcount incremented. We should use of_node_put() in
>>>>>>>> fail path or when it is not used anymore.
>>>>>>>>
>>>>>>>> Signed-off-by: Liang He
>>>>>>>> ---
>>>>>>>> changelog:
>>>>>>>> v4: reuse exist 'err' and use a simple code style, advised by CJ
>>>>>>>> v3: use local 'child_node' advised by Michael.
>>>>>>>> v2: use goto-label patch style advised by Christophe Leroy.
>>>>>>>> v1: add of_node_put() before each exit.
>>>>>>>>
>>>>>>>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 35
>>>>>>>> ++-
>>>>>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>>>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>>>> index 98ae64075193..e4588943fe7e 100644
>>>>>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>>>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device
>>>>>>>> *pdev)
>>>>>>>> {
>>>>>>>>enum of_gpio_flags flags;
>>>>>>>>struct device_node *node = pdev->dev.of_node;
>>>>>>>> + struct device_node *child_node;
>>>>>>>>int gpio, err, irq;
>>>>>>>>int trigger;
>>>>>>>>
>>>>>>>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device
>>>>>>>> *pdev)
>>>>>>>>return -ENODEV;
>>>>>>>>
>>>>>>>>/* If there's no matching child, this isn't really an error */
>>>>>>>> - halt_node = of_find_matching_node(node, child_match);
>>>>>>>> - if (!halt_node)
>>>>>>>> + child_node = of_find_matching_node(node, child_match);
>>>>>>>> + if (!child_node)
>>>>>>>>return 0;
>>>>>>>>
>>>>>>>>/* Technically we could just read the first one, but punish
>>>>>>>> * DT writers for invalid form. */
>>>>>>>> - if (of_gpio_count(halt_node) != 1)
>>>>>>>> - return -EINVAL;
>>>>>>>> + if (of_gpio_count(child_node) != 1) {
>>>>>>>> + err = -EINVAL;
>>>>>>>> + goto err_put;
>>>>>>>> + }
>>>>>>>>
>>>>>>>>/* Get the gpio number relative to the dynamic base. */
>>>>>>>> - gpio = of_get_gpio_flags(halt_node, 0, );
>>>>>>>> - if (!gpio_is_valid(gpio))
>>>>>>>> - return -EINVAL;
>>>>>>>> + gpio = of_get_gpio_flags(child_node, 0, );
>>>>>>>> + if (!gpio_is_valid(gpio)) {
>>>>>>>> + err = -EINVAL;
>>>>>>>> + gotot err_put;
>>>>>>>
>>>>>>> Did you test the build ?
>>>>>>
>>>>>> Sorry for this fault.
>>
[PATCH] powerpc: kernel: Change the order of of_node_put()
In add_pcspkr(), it is better to call of_node_put() after the
'if(!np)' check.
Signed-off-by: Liang He
---
arch/powerpc/kernel/setup-common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/setup-common.c
b/arch/powerpc/kernel/setup-common.c
index eb0077b302e2..761817d1f4db 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
int ret;
np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
- of_node_put(np);
if (!np)
return -ENODEV;
+ of_node_put(np);
pd = platform_device_alloc("pcspkr", -1);
if (!pd)
--
2.25.1
[PATCH v5] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
In gpio_halt_probe(), of_find_matching_node() will return a node
pointer with refcount incremented. We should use of_node_put() in
fail path or when it is not used anymore.
Signed-off-by: Liang He
---
changelog:
v5: fix 'gotot' error introduced by v4 and use cross-compiler to test
v4: reuse exist 'err' and use a simple code style, advised by CJ
v3: use local 'child_node' advised by Michael.
v2: use goto-label patch style advised by Christophe Leroy.
v1: add of_node_put() before each exit.
arch/powerpc/platforms/85xx/sgy_cts1000.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..e14d1b74d4e4 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
{
enum of_gpio_flags flags;
struct device_node *node = pdev->dev.of_node;
+ struct device_node *child_node;
int gpio, err, irq;
int trigger;
@@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device *pdev)
return -ENODEV;
/* If there's no matching child, this isn't really an error */
- halt_node = of_find_matching_node(node, child_match);
- if (!halt_node)
+ child_node = of_find_matching_node(node, child_match);
+ if (!child_node)
return 0;
/* Technically we could just read the first one, but punish
* DT writers for invalid form. */
- if (of_gpio_count(halt_node) != 1)
- return -EINVAL;
+ if (of_gpio_count(child_node) != 1) {
+ err = -EINVAL;
+ goto err_put;
+ }
/* Get the gpio number relative to the dynamic base. */
- gpio = of_get_gpio_flags(halt_node, 0, );
- if (!gpio_is_valid(gpio))
- return -EINVAL;
+ gpio = of_get_gpio_flags(child_node, 0, );
+ if (!gpio_is_valid(gpio)) {
+ err = -EINVAL;
+ goto err_put;
+ }
err = gpio_request(gpio, "gpio-halt");
if (err) {
printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
gpio);
- halt_node = NULL;
- return err;
+ goto err_put;
}
trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device *pdev)
gpio_direction_output(gpio, !trigger);
/* Now get the IRQ which tells us when the power button is hit */
- irq = irq_of_parse_and_map(halt_node, 0);
+ irq = irq_of_parse_and_map(child_node, 0);
err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
- IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
+ IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
if (err) {
printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
"GPIO %d.\n", irq, gpio);
gpio_free(gpio);
- halt_node = NULL;
- return err;
+ goto err_put;
}
/* Register our halt function */
@@ -123,7 +126,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
" irq).\n", gpio, trigger, irq);
+ halt_node = child_node;
return 0;
+
+err_put:
+ of_node_put(child_node);
+ return err;
}
static int gpio_halt_remove(struct platform_device *pdev)
@@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
gpio_free(gpio);
+ of_node_put(halt_node);
halt_node = NULL;
}
--
2.25.1
Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
At 2022-06-17 16:27:03, conor.doo...@microchip.com wrote:
>On 17/06/2022 09:17, Liang He wrote:
>>
>>
>>
>> At 2022-06-17 14:53:13, "Christophe Leroy"
>> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:28:56, "Christophe Leroy"
>>>> wrote:
>>>>>
>>>>>
>>>>> Le 17/06/2022 à 08:08, Liang He a écrit :
>>>>>> In gpio_halt_probe(), of_find_matching_node() will return a node
>>>>>> pointer with refcount incremented. We should use of_node_put() in
>>>>>> fail path or when it is not used anymore.
>>>>>>
>>>>>> Signed-off-by: Liang He
>>>>>> ---
>>>>>> changelog:
>>>>>> v4: reuse exist 'err' and use a simple code style, advised by CJ
>>>>>> v3: use local 'child_node' advised by Michael.
>>>>>> v2: use goto-label patch style advised by Christophe Leroy.
>>>>>> v1: add of_node_put() before each exit.
>>>>>>
>>>>>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 35
>>>>>> ++-
>>>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> index 98ae64075193..e4588943fe7e 100644
>>>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device
>>>>>> *pdev)
>>>>>> {
>>>>>> enum of_gpio_flags flags;
>>>>>> struct device_node *node = pdev->dev.of_node;
>>>>>> +struct device_node *child_node;
>>>>>> int gpio, err, irq;
>>>>>> int trigger;
>>>>>>
>>>>>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device
>>>>>> *pdev)
>>>>>> return -ENODEV;
>>>>>>
>>>>>> /* If there's no matching child, this isn't really an error */
>>>>>> -halt_node = of_find_matching_node(node, child_match);
>>>>>> -if (!halt_node)
>>>>>> +child_node = of_find_matching_node(node, child_match);
>>>>>> +if (!child_node)
>>>>>> return 0;
>>>>>>
>>>>>> /* Technically we could just read the first one, but punish
>>>>>> * DT writers for invalid form. */
>>>>>> -if (of_gpio_count(halt_node) != 1)
>>>>>> -return -EINVAL;
>>>>>> +if (of_gpio_count(child_node) != 1) {
>>>>>> +err = -EINVAL;
>>>>>> +goto err_put;
>>>>>> +}
>>>>>>
>>>>>> /* Get the gpio number relative to the dynamic base. */
>>>>>> -gpio = of_get_gpio_flags(halt_node, 0, );
>>>>>> -if (!gpio_is_valid(gpio))
>>>>>> -return -EINVAL;
>>>>>> +gpio = of_get_gpio_flags(child_node, 0, );
>>>>>> +if (!gpio_is_valid(gpio)) {
>>>>>> +err = -EINVAL;
>>>>>> +gotot err_put;
>>>>>
>>>>> Did you test the build ?
>>>>
>>>> Sorry for this fault.
>>>>
>>>> In fact, I am still finding an efficient way to building different arch
>>>> source code as I only have x86-64.
>>>>
>>>> Now I am try using QEMU.
>>>>
>>>> Anyway, sorry for this fault.
>>>
>>> You can find cross compilers for most architectures for x86-64 here :
>>> https://mirrors.edge.kernel.org/pub/tools/crosstool/
>>>
>>> Christophe
>>
>> Hi, Christophe and Conor.
>>
>> Sorry to trouble you again.
>>
>> Now I only know how to quickly identify the refcounting bugs, but I cannot
>> efficiently give a build test.
>>
>> For example,
Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
At 2022-06-17 16:27:03, conor.doo...@microchip.com wrote:
>On 17/06/2022 09:17, Liang He wrote:
>>
>>
>>
>> At 2022-06-17 14:53:13, "Christophe Leroy"
>> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:28:56, "Christophe Leroy"
>>>> wrote:
>>>>>
>>>>>
>>>>> Le 17/06/2022 à 08:08, Liang He a écrit :
>>>>>> In gpio_halt_probe(), of_find_matching_node() will return a node
>>>>>> pointer with refcount incremented. We should use of_node_put() in
>>>>>> fail path or when it is not used anymore.
>>>>>>
>>>>>> Signed-off-by: Liang He
>>>>>> ---
>>>>>> changelog:
>>>>>> v4: reuse exist 'err' and use a simple code style, advised by CJ
>>>>>> v3: use local 'child_node' advised by Michael.
>>>>>> v2: use goto-label patch style advised by Christophe Leroy.
>>>>>> v1: add of_node_put() before each exit.
>>>>>>
>>>>>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 35
>>>>>> ++-
>>>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> index 98ae64075193..e4588943fe7e 100644
>>>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device
>>>>>> *pdev)
>>>>>> {
>>>>>> enum of_gpio_flags flags;
>>>>>> struct device_node *node = pdev->dev.of_node;
>>>>>> +struct device_node *child_node;
>>>>>> int gpio, err, irq;
>>>>>> int trigger;
>>>>>>
>>>>>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device
>>>>>> *pdev)
>>>>>> return -ENODEV;
>>>>>>
>>>>>> /* If there's no matching child, this isn't really an error */
>>>>>> -halt_node = of_find_matching_node(node, child_match);
>>>>>> -if (!halt_node)
>>>>>> +child_node = of_find_matching_node(node, child_match);
>>>>>> +if (!child_node)
>>>>>> return 0;
>>>>>>
>>>>>> /* Technically we could just read the first one, but punish
>>>>>> * DT writers for invalid form. */
>>>>>> -if (of_gpio_count(halt_node) != 1)
>>>>>> -return -EINVAL;
>>>>>> +if (of_gpio_count(child_node) != 1) {
>>>>>> +err = -EINVAL;
>>>>>> +goto err_put;
>>>>>> +}
>>>>>>
>>>>>> /* Get the gpio number relative to the dynamic base. */
>>>>>> -gpio = of_get_gpio_flags(halt_node, 0, );
>>>>>> -if (!gpio_is_valid(gpio))
>>>>>> -return -EINVAL;
>>>>>> +gpio = of_get_gpio_flags(child_node, 0, );
>>>>>> +if (!gpio_is_valid(gpio)) {
>>>>>> +err = -EINVAL;
>>>>>> +gotot err_put;
>>>>>
>>>>> Did you test the build ?
>>>>
>>>> Sorry for this fault.
>>>>
>>>> In fact, I am still finding an efficient way to building different arch
>>>> source code as I only have x86-64.
>>>>
>>>> Now I am try using QEMU.
>>>>
>>>> Anyway, sorry for this fault.
>>>
>>> You can find cross compilers for most architectures for x86-64 here :
>>> https://mirrors.edge.kernel.org/pub/tools/crosstool/
>>>
>>> Christophe
>>
>> Hi, Christophe and Conor.
>>
>> Sorry to trouble you again.
>>
>> Now I only know how to quickly identify the refcounting bugs, but I cannot
>> efficiently give a build test.
>>
>> For example, I use the cross compilers 'powerpc-linux-gnu-gcc' to compile
>> 'arch/powerpc/platforms/85xx/sgy_cts1000.c' with -fsyntax-only flag.
>> But I meet too many header file missing errors. Even if I add some 'include'
>> pathes, e.g., ./arch/powerpc/include, ./include,
>> there are still too many other errors.
>>
>> So if there is any efficient way to check my patch code to avoid 'gotot'
>> error again.
>
>idk anything about powerpc, but what I find is a nice way to get a compiler
>for an arch I don't use is to search on lore.kernel.org for a 0day robot
>build error since it gives instructions for building on that arch.
>For example:
>https://lore.kernel.org/linuxppc-dev/202206060910.ryntfqdi-...@intel.com/
>
>
>In this case, your bug seems obvious? You typed "gotot" instead of "goto".
>
>Hope that helps,
>Conor.
>
>>
>> Thanks again, Christophe and Conor.
>>
>> Liang
Thanks so much, I will try it.
Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
At 2022-06-17 14:53:13, "Christophe Leroy" wrote:
>
>
>Le 17/06/2022 à 08:45, Liang He a écrit :
>>
>>
>>
>> At 2022-06-17 14:28:56, "Christophe Leroy"
>> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:08, Liang He a écrit :
>>>> In gpio_halt_probe(), of_find_matching_node() will return a node
>>>> pointer with refcount incremented. We should use of_node_put() in
>>>> fail path or when it is not used anymore.
>>>>
>>>> Signed-off-by: Liang He
>>>> ---
>>>>changelog:
>>>>v4: reuse exist 'err' and use a simple code style, advised by CJ
>>>>v3: use local 'child_node' advised by Michael.
>>>>v2: use goto-label patch style advised by Christophe Leroy.
>>>>v1: add of_node_put() before each exit.
>>>>
>>>>arch/powerpc/platforms/85xx/sgy_cts1000.c | 35 ++-
>>>>1 file changed, 22 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> index 98ae64075193..e4588943fe7e 100644
>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>>>{
>>>>enum of_gpio_flags flags;
>>>>struct device_node *node = pdev->dev.of_node;
>>>> + struct device_node *child_node;
>>>>int gpio, err, irq;
>>>>int trigger;
>>>>
>>>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device
>>>> *pdev)
>>>>return -ENODEV;
>>>>
>>>>/* If there's no matching child, this isn't really an error */
>>>> - halt_node = of_find_matching_node(node, child_match);
>>>> - if (!halt_node)
>>>> + child_node = of_find_matching_node(node, child_match);
>>>> + if (!child_node)
>>>>return 0;
>>>>
>>>>/* Technically we could just read the first one, but punish
>>>> * DT writers for invalid form. */
>>>> - if (of_gpio_count(halt_node) != 1)
>>>> - return -EINVAL;
>>>> + if (of_gpio_count(child_node) != 1) {
>>>> + err = -EINVAL;
>>>> + goto err_put;
>>>> + }
>>>>
>>>>/* Get the gpio number relative to the dynamic base. */
>>>> - gpio = of_get_gpio_flags(halt_node, 0, );
>>>> - if (!gpio_is_valid(gpio))
>>>> - return -EINVAL;
>>>> + gpio = of_get_gpio_flags(child_node, 0, );
>>>> + if (!gpio_is_valid(gpio)) {
>>>> + err = -EINVAL;
>>>> + gotot err_put;
>>>
>>> Did you test the build ?
>>
>> Sorry for this fault.
>>
>> In fact, I am still finding an efficient way to building different arch
>> source code as I only have x86-64.
>>
>> Now I am try using QEMU.
>>
>> Anyway, sorry for this fault.
>
>You can find cross compilers for most architectures for x86-64 here :
>https://mirrors.edge.kernel.org/pub/tools/crosstool/
>
>Christophe
Hi, Christophe and Conor.
Sorry to trouble you again.
Now I only know how to quickly identify the refcounting bugs, but I cannot
efficiently give a build test.
For example, I use the cross compilers 'powerpc-linux-gnu-gcc' to compile
'arch/powerpc/platforms/85xx/sgy_cts1000.c' with -fsyntax-only flag.
But I meet too many header file missing errors. Even if I add some 'include'
pathes, e.g., ./arch/powerpc/include, ./include,
there are still too many other errors.
So if there is any efficient way to check my patch code to avoid 'gotot' error
again.
Thanks again, Christophe and Conor.
Liang
[PATCH] powerpc: powernv: Fix refcount leak in opal
In opal_imc_init_dev(), of_find_compatible_node() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/platforms/powernv/opal.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c index 55a8fbfdb5b2..d86cc48a10aa 100644 --- a/arch/powerpc/platforms/powernv/opal.c +++ b/arch/powerpc/platforms/powernv/opal.c @@ -952,6 +952,7 @@ static void __init opal_imc_init_dev(void) np = of_find_compatible_node(NULL, NULL, IMC_DTB_COMPAT); if (np) of_platform_device_create(np, NULL, NULL); + of_node_put(np); } static int kopald(void *unused) -- 2.25.1
Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
At 2022-06-17 14:28:56, "Christophe Leroy" wrote:
>
>
>Le 17/06/2022 à 08:08, Liang He a écrit :
>> In gpio_halt_probe(), of_find_matching_node() will return a node
>> pointer with refcount incremented. We should use of_node_put() in
>> fail path or when it is not used anymore.
>>
>> Signed-off-by: Liang He
>> ---
>> changelog:
>> v4: reuse exist 'err' and use a simple code style, advised by CJ
>> v3: use local 'child_node' advised by Michael.
>> v2: use goto-label patch style advised by Christophe Leroy.
>> v1: add of_node_put() before each exit.
>>
>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 35 ++-
>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> index 98ae64075193..e4588943fe7e 100644
>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> {
>> enum of_gpio_flags flags;
>> struct device_node *node = pdev->dev.of_node;
>> +struct device_node *child_node;
>> int gpio, err, irq;
>> int trigger;
>>
>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> return -ENODEV;
>>
>> /* If there's no matching child, this isn't really an error */
>> -halt_node = of_find_matching_node(node, child_match);
>> -if (!halt_node)
>> +child_node = of_find_matching_node(node, child_match);
>> +if (!child_node)
>> return 0;
>>
>> /* Technically we could just read the first one, but punish
>> * DT writers for invalid form. */
>> -if (of_gpio_count(halt_node) != 1)
>> -return -EINVAL;
>> +if (of_gpio_count(child_node) != 1) {
>> +err = -EINVAL;
>> +goto err_put;
>> +}
>>
>> /* Get the gpio number relative to the dynamic base. */
>> -gpio = of_get_gpio_flags(halt_node, 0, );
>> -if (!gpio_is_valid(gpio))
>> -return -EINVAL;
>> +gpio = of_get_gpio_flags(child_node, 0, );
>> +if (!gpio_is_valid(gpio)) {
>> +err = -EINVAL;
>> +gotot err_put;
>
>Did you test the build ?
Sorry for this fault.
In fact, I am still finding an efficient way to building different arch source
code as I only have x86-64.
Now I am try using QEMU.
Anyway, sorry for this fault.
>
>> +}
>>
>> err = gpio_request(gpio, "gpio-halt");
>> if (err) {
>> printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
>> gpio);
>> -halt_node = NULL;
>> -return err;
>> +goto err_put;
>> }
>>
>> trigger = (flags == OF_GPIO_ACTIVE_LOW);
>> @@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device
>> *pdev)
>> gpio_direction_output(gpio, !trigger);
>>
>> /* Now get the IRQ which tells us when the power button is hit */
>> -irq = irq_of_parse_and_map(halt_node, 0);
>> +irq = irq_of_parse_and_map(child_node, 0);
>> err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
>> - IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
>> + IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
>> if (err) {
>> printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>> "GPIO %d.\n", irq, gpio);
>> gpio_free(gpio);
>> -halt_node = NULL;
>> -return err;
>> +goto err_put;
>> }
>>
>> /* Register our halt function */
>> @@ -123,7 +126,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>> " irq).\n", gpio, trigger, irq);
>>
>> +halt_node = child_node;
>> return 0;
>> +
>> +err_put:
>> +of_node_put(child_node);
>> +return err;
>> }
>>
>> static int gpio_halt_remove(struct platform_device *pdev)
>> @@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
>>
>> gpio_free(gpio);
>>
>> +of_node_put(halt_node);
>> halt_node = NULL;
>> }
>>
[PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
In gpio_halt_probe(), of_find_matching_node() will return a node
pointer with refcount incremented. We should use of_node_put() in
fail path or when it is not used anymore.
Signed-off-by: Liang He
---
changelog:
v4: reuse exist 'err' and use a simple code style, advised by CJ
v3: use local 'child_node' advised by Michael.
v2: use goto-label patch style advised by Christophe Leroy.
v1: add of_node_put() before each exit.
arch/powerpc/platforms/85xx/sgy_cts1000.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..e4588943fe7e 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
{
enum of_gpio_flags flags;
struct device_node *node = pdev->dev.of_node;
+ struct device_node *child_node;
int gpio, err, irq;
int trigger;
@@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device *pdev)
return -ENODEV;
/* If there's no matching child, this isn't really an error */
- halt_node = of_find_matching_node(node, child_match);
- if (!halt_node)
+ child_node = of_find_matching_node(node, child_match);
+ if (!child_node)
return 0;
/* Technically we could just read the first one, but punish
* DT writers for invalid form. */
- if (of_gpio_count(halt_node) != 1)
- return -EINVAL;
+ if (of_gpio_count(child_node) != 1) {
+ err = -EINVAL;
+ goto err_put;
+ }
/* Get the gpio number relative to the dynamic base. */
- gpio = of_get_gpio_flags(halt_node, 0, );
- if (!gpio_is_valid(gpio))
- return -EINVAL;
+ gpio = of_get_gpio_flags(child_node, 0, );
+ if (!gpio_is_valid(gpio)) {
+ err = -EINVAL;
+ gotot err_put;
+ }
err = gpio_request(gpio, "gpio-halt");
if (err) {
printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
gpio);
- halt_node = NULL;
- return err;
+ goto err_put;
}
trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device *pdev)
gpio_direction_output(gpio, !trigger);
/* Now get the IRQ which tells us when the power button is hit */
- irq = irq_of_parse_and_map(halt_node, 0);
+ irq = irq_of_parse_and_map(child_node, 0);
err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
- IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
+ IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
if (err) {
printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
"GPIO %d.\n", irq, gpio);
gpio_free(gpio);
- halt_node = NULL;
- return err;
+ goto err_put;
}
/* Register our halt function */
@@ -123,7 +126,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
" irq).\n", gpio, trigger, irq);
+ halt_node = child_node;
return 0;
+
+err_put:
+ of_node_put(child_node);
+ return err;
}
static int gpio_halt_remove(struct platform_device *pdev)
@@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
gpio_free(gpio);
+ of_node_put(halt_node);
halt_node = NULL;
}
--
2.25.1
Re:Re: [PATCH v3] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
At 2022-06-17 13:37:12, "Christophe JAILLET"
wrote:
>Le 17/06/2022 à 07:22, Liang He a écrit :
>> In gpio_halt_probe(), of_find_matching_node() will return a node
>> pointer with refcount incremented. We should use of_node_put() in
>> fail path or when it is not used anymore.
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 39 +++
>> 1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> index 98ae64075193..a8690fc552cf 100644
>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> @@ -71,33 +71,39 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> {
>> enum of_gpio_flags flags;
>> struct device_node *node = pdev->dev.of_node;
>> +struct device_node *child_node;
>> int gpio, err, irq;
>> int trigger;
>> +int ret;
>>
>> if (!node)
>> return -ENODEV;
>>
>> /* If there's no matching child, this isn't really an error */
>> -halt_node = of_find_matching_node(node, child_match);
>> -if (!halt_node)
>> +child_node = of_find_matching_node(node, child_match);
>> +if (!child_node)
>> return 0;
>>
>> /* Technically we could just read the first one, but punish
>> * DT writers for invalid form. */
>> -if (of_gpio_count(halt_node) != 1)
>> -return -EINVAL;
>> +if (of_gpio_count(child_node) != 1) {
>> +ret = -EINVAL;
>> +goto err_put;
>> +}
>>
>> /* Get the gpio number relative to the dynamic base. */
>> -gpio = of_get_gpio_flags(halt_node, 0, );
>> -if (!gpio_is_valid(gpio))
>> -return -EINVAL;
>> +gpio = of_get_gpio_flags(child_node, 0, );
>> +if (!gpio_is_valid(gpio)) {
>> +ret = -EINVAL;
>> +gotot err_put;
>> +}
>>
>> err = gpio_request(gpio, "gpio-halt");
>> if (err) {
>> printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
>> gpio);
>> -halt_node = NULL;
>> -return err;
>> +ret = err;
>
>Sorry for not seeing and asking before, but why do you need 'ret'?
>Can't you use the existing 'err' in place in this whole patch?
>
Thanks, CJ.
Your advice is good and I have not noticed the 'err'.
>> +goto err_put;
>> }
>>
>> trigger = (flags == OF_GPIO_ACTIVE_LOW);
>> @@ -105,15 +111,15 @@ static int gpio_halt_probe(struct platform_device
>> *pdev)
>> gpio_direction_output(gpio, !trigger);
>>
>> /* Now get the IRQ which tells us when the power button is hit */
>> -irq = irq_of_parse_and_map(halt_node, 0);
>> +irq = irq_of_parse_and_map(child_node, 0);
>> err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
>> - IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
>> + IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
>> if (err) {
>> printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>> "GPIO %d.\n", irq, gpio);
>> gpio_free(gpio);
>> -halt_node = NULL;
>> -return err;
>> +ret = err;
>> +goto err_put;
>> }
>>
>> /* Register our halt function */
>> @@ -122,8 +128,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>
>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>> " irq).\n", gpio, trigger, irq);
>> +ret = 0;
>> +halt_node = of_node_get(child_node);
>
>LGTM, but my preferred style would be:
> halt_node = child_node;
> return 0;
>I'm not a maintainer, so this is just my opinion and it is mostly a
>mater of taste.
>
>CJ
Thanks, CJ.
Now, I also prefer this style and I will use it.
>
>>
>> -return 0;
>> +err_put:
>> +of_node_put(child_node);
>> +return ret;
>> }
>>
>> static int gpio_halt_remove(struct platform_device *pdev)
>> @@ -139,6 +149,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
>>
>> gpio_free(gpio);
>>
>> +of_node_put(halt_node);
>> halt_node = NULL;
>> }
>>
Re:Re: [PATCH] powerpc: powernv: Fix refcount leak bug in opal-powercap
At 2022-06-17 13:01:27, "Christophe JAILLET"
wrote:
>Le 17/06/2022 à 06:20, Liang He a écrit :
>> In opal_powercap_init(), of_find_compatible_node() will return
>> a node pointer with refcount incremented. We should use of_node_put()
>> in fail path or when it is not used anymore.
>>
>> Besides, for_each_child_of_node() will automatically *inc* and *dec*
>> refcount during iteration. However, we should add the of_node_put()
>> if there is a break.
>
>Hi,
>
>I'm not sure that your patch is right here. Because of this *inc* and
>*dec* things, do we still need to of_node_put(powercap) once we have
>entered for_each_child_of_node?
>
>I think that this reference will be released on the first iteration of
>the loop.
>
Hi, CJ,
Thanks for your reply and I want have a discuss.
Based on my review on the src of 'of_get_next_child', there is only
*inc* for next and *dec* for pre as follow.
(|node| == powercap)
==__of_get_next_child( |node|, prev)==
...
next = prev? prev->sibling:|node|->child;
of_node_get(next);
of_node_put(prev);
...
=
However, there is no any code to release the |node| (i.e., *powercap*).
Am I right? If I am wrong, please correct me, thanks.
>
>Maybe of_node_put(powercap) should be duplicated everywhere it is
>relevant and removed from the error handling path?
>Or an additional reference should be taken before the loop?
>Or adding a new label with "powercap = NULL" and branching there when
>needed?
>
>CJ
If my understanding is right, I think current patch is right.
Otherwise, I will make a new patch to handle that, Thanks.
Liang
>
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/platforms/powernv/opal-powercap.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
>> b/arch/powerpc/platforms/powernv/opal-powercap.c
>> index 64506b46e77b..b102477d3f95 100644
>> --- a/arch/powerpc/platforms/powernv/opal-powercap.c
>> +++ b/arch/powerpc/platforms/powernv/opal-powercap.c
>> @@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
>> pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
>> GFP_KERNEL);
>> if (!pcaps)
>> -return;
>> +goto out_powercap;
>>
>> powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
>> if (!powercap_kobj) {
>> @@ -236,6 +236,9 @@ void __init opal_powercap_init(void)
>> kfree(pcaps[i].pg.name);
>> }
>> kobject_put(powercap_kobj);
>> +of_node_put(node);
>> out_pcaps:
>> kfree(pcaps);
>> +out_powercap:
>> +of_node_put(powercap);
>> }
[PATCH v3] powerpc:85xx: Add missing of_node_put() in sgy_cst1000
In gpio_halt_probe(), of_find_matching_node() will return a node
pointer with refcount incremented. We should use of_node_put() in
fail path or when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/platforms/85xx/sgy_cts1000.c | 39 +++
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..a8690fc552cf 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -71,33 +71,39 @@ static int gpio_halt_probe(struct platform_device *pdev)
{
enum of_gpio_flags flags;
struct device_node *node = pdev->dev.of_node;
+ struct device_node *child_node;
int gpio, err, irq;
int trigger;
+ int ret;
if (!node)
return -ENODEV;
/* If there's no matching child, this isn't really an error */
- halt_node = of_find_matching_node(node, child_match);
- if (!halt_node)
+ child_node = of_find_matching_node(node, child_match);
+ if (!child_node)
return 0;
/* Technically we could just read the first one, but punish
* DT writers for invalid form. */
- if (of_gpio_count(halt_node) != 1)
- return -EINVAL;
+ if (of_gpio_count(child_node) != 1) {
+ ret = -EINVAL;
+ goto err_put;
+ }
/* Get the gpio number relative to the dynamic base. */
- gpio = of_get_gpio_flags(halt_node, 0, );
- if (!gpio_is_valid(gpio))
- return -EINVAL;
+ gpio = of_get_gpio_flags(child_node, 0, );
+ if (!gpio_is_valid(gpio)) {
+ ret = -EINVAL;
+ gotot err_put;
+ }
err = gpio_request(gpio, "gpio-halt");
if (err) {
printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
gpio);
- halt_node = NULL;
- return err;
+ ret = err;
+ goto err_put;
}
trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -105,15 +111,15 @@ static int gpio_halt_probe(struct platform_device *pdev)
gpio_direction_output(gpio, !trigger);
/* Now get the IRQ which tells us when the power button is hit */
- irq = irq_of_parse_and_map(halt_node, 0);
+ irq = irq_of_parse_and_map(child_node, 0);
err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
- IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
+ IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
if (err) {
printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
"GPIO %d.\n", irq, gpio);
gpio_free(gpio);
- halt_node = NULL;
- return err;
+ ret = err;
+ goto err_put;
}
/* Register our halt function */
@@ -122,8 +128,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
" irq).\n", gpio, trigger, irq);
+ ret = 0;
+ halt_node = of_node_get(child_node);
- return 0;
+err_put:
+ of_node_put(child_node);
+ return ret;
}
static int gpio_halt_remove(struct platform_device *pdev)
@@ -139,6 +149,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
gpio_free(gpio);
+ of_node_put(halt_node);
halt_node = NULL;
}
--
2.25.1
Re:Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
2022-06-17 12:29:02，"Michael Ellerman" 写道： >"Liang He" writes: >> At 2022-06-17 07:37:06, "Michael Ellerman" wrote: >>>Christophe JAILLET writes: >>>> Le 16/06/2022 à 17:19, Liang He a écrit : >>>>> In gpio_halt_probe(), of_find_matching_node() will return a node pointer >>>>> with >>>>> refcount incremented. We should use of_node_put() in each fail path or >>>>> when it >>>>> is not used anymore. >>>>> >>>>> Signed-off-by: Liang He >>>>> --- >>>>> changelog: >>>>> >>>>> v2: use goto-label patch style advised by Christophe. >>>>> v1: add of_node_put() before each exit. >>>>> >>>>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++ >>>>> 1 file changed, 18 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>>>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>>>> index 98ae64075193..e280f963d88c 100644 >>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>>>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev) >>>... >>>>> @@ -122,8 +127,12 @@ static int gpio_halt_probe(struct platform_device >>>>> *pdev) >>>>> >>>>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" >>>>> " irq).\n", gpio, trigger, irq); >>>>> + ret = 0; >>>>> >>>>> - return 0; >>>>> +err_put: >>>>> + of_node_put(halt_node); >>>>> + halt_node = NULL; >>>> >>>> Hi, >>>> so now we set 'halt_node' to NULL even in the normal case. >>>> This is really spurious. >>>> >>>> Look at gpio_halt_cb(), but I think that this is just wrong and badly >>>> breaks this driver. >>> >>>I agree, thanks for reviewing. >>> >>>I think the cleanest solution is to use a local variable for the node in >>>the body of gpio_halt_probe(), and only assign to halt_node once all the >>>checks have passed. >>> >>>So something like: >>> >>>struct device_node *child_node; >>> >>> child_node = of_find_matching_node(node, child_match); >>>... >>> >>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" >>>" irq).\n", gpio, trigger, irq); >>>ret = 0; >>>halt_node = of_node_get(child_node); >>> >>>out_put: >>>of_node_put(child_node); >>> >>> return ret; >>>} >>> >>> >>>cheers >> >> Hi, Michael and Christophe, >> >> I am writing the new patch based on Michael's advice. However, I wonder if >> there is >> any place to call of_node_put(halt_node)? As I do not exactly know if >> gpio_halt_remove() >> or anyother place can correctly release this global reference？ >> If not, it is correct that I add a of_node_put(halt_node) in >> gpio_halt_remove(), right? > >Yes I think so, just before it's set to NULL, eg: > >diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c >b/arch/powerpc/platforms/85xx/sgy_cts1000.c >index 98ae64075193..7beb3cd420ba 100644 >--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c >+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c >@@ -139,6 +139,7 @@ static int gpio_halt_remove(struct platform_device *pdev) > > gpio_free(gpio); > >+ of_node_put(halt_node); > halt_node = NULL; > } > > >cheers Ok, I will make the new patch soon.
[PATCH] powerpc: powernv: Fix refcount leak bug in opal-powercap
In opal_powercap_init(), of_find_compatible_node() will return
a node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Besides, for_each_child_of_node() will automatically *inc* and *dec*
refcount during iteration. However, we should add the of_node_put()
if there is a break.
Signed-off-by: Liang He
---
arch/powerpc/platforms/powernv/opal-powercap.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
b/arch/powerpc/platforms/powernv/opal-powercap.c
index 64506b46e77b..b102477d3f95 100644
--- a/arch/powerpc/platforms/powernv/opal-powercap.c
+++ b/arch/powerpc/platforms/powernv/opal-powercap.c
@@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
GFP_KERNEL);
if (!pcaps)
- return;
+ goto out_powercap;
powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
if (!powercap_kobj) {
@@ -236,6 +236,9 @@ void __init opal_powercap_init(void)
kfree(pcaps[i].pg.name);
}
kobject_put(powercap_kobj);
+ of_node_put(node);
out_pcaps:
kfree(pcaps);
+out_powercap:
+ of_node_put(powercap);
}
--
2.25.1
Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
At 2022-06-17 07:37:06, "Michael Ellerman" wrote: >Christophe JAILLET writes: >> Le 16/06/2022 à 17:19, Liang He a écrit : >>> In gpio_halt_probe(), of_find_matching_node() will return a node pointer >>> with >>> refcount incremented. We should use of_node_put() in each fail path or when >>> it >>> is not used anymore. >>> >>> Signed-off-by: Liang He >>> --- >>> changelog: >>> >>> v2: use goto-label patch style advised by Christophe. >>> v1: add of_node_put() before each exit. >>> >>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++ >>> 1 file changed, 18 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> index 98ae64075193..e280f963d88c 100644 >>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev) >... >>> @@ -122,8 +127,12 @@ static int gpio_halt_probe(struct platform_device >>> *pdev) >>> >>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" >>>" irq).\n", gpio, trigger, irq); >>> + ret = 0; >>> >>> - return 0; >>> +err_put: >>> + of_node_put(halt_node); >>> + halt_node = NULL; >> >> Hi, >> so now we set 'halt_node' to NULL even in the normal case. >> This is really spurious. >> >> Look at gpio_halt_cb(), but I think that this is just wrong and badly >> breaks this driver. > >I agree, thanks for reviewing. > >I think the cleanest solution is to use a local variable for the node in >the body of gpio_halt_probe(), and only assign to halt_node once all the >checks have passed. > >So something like: > >struct device_node *child_node; > > child_node = of_find_matching_node(node, child_match); >... > > printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" > " irq).\n", gpio, trigger, irq); >ret = 0; >halt_node = of_node_get(child_node); > >out_put: >of_node_put(child_node); > > return ret; >} > > >cheers Hi, Michael and Christophe, I am writing the new patch based on Michael's advice. However, I wonder if there is any place to call of_node_put(halt_node)? As I do not exactly know if gpio_halt_remove() or anyother place can correctly release this global reference？ If not, it is correct that I add a of_node_put(halt_node) in gpio_halt_remove(), right? Thanks and wait for your replies. Liang
Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
At 2022-06-17 07:37:06, "Michael Ellerman" wrote: >Christophe JAILLET writes: >> Le 16/06/2022 à 17:19, Liang He a écrit : >>> In gpio_halt_probe(), of_find_matching_node() will return a node pointer >>> with >>> refcount incremented. We should use of_node_put() in each fail path or when >>> it >>> is not used anymore. >>> >>> Signed-off-by: Liang He >>> --- >>> changelog: >>> >>> v2: use goto-label patch style advised by Christophe. >>> v1: add of_node_put() before each exit. >>> >>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++ >>> 1 file changed, 18 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> index 98ae64075193..e280f963d88c 100644 >>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c >>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev) >... >>> @@ -122,8 +127,12 @@ static int gpio_halt_probe(struct platform_device >>> *pdev) >>> >>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" >>>" irq).\n", gpio, trigger, irq); >>> + ret = 0; >>> >>> - return 0; >>> +err_put: >>> + of_node_put(halt_node); >>> + halt_node = NULL; >> >> Hi, >> so now we set 'halt_node' to NULL even in the normal case. >> This is really spurious. >> >> Look at gpio_halt_cb(), but I think that this is just wrong and badly >> breaks this driver. > >I agree, thanks for reviewing. > >I think the cleanest solution is to use a local variable for the node in >the body of gpio_halt_probe(), and only assign to halt_node once all the >checks have passed. > >So something like: > >struct device_node *child_node; > > child_node = of_find_matching_node(node, child_match); >... > > printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d" > " irq).\n", gpio, trigger, irq); >ret = 0; >halt_node = of_node_get(child_node); > >out_put: >of_node_put(child_node); > > return ret; >} > > >cheers Thanks, Michael and Christophe. I will send a patch based on your reviews. Liang
[PATCH] powerpc: sysdev: xive: Fix refcount leak in native.c
In xive_native_init(), of_find_compatible_node() will return a
node pointer with refcount incremented. We should use of_node_put()
in each fail path or when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/sysdev/xive/native.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/native.c
b/arch/powerpc/sysdev/xive/native.c
index d25d8c692909..3925825954bc 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -579,12 +579,12 @@ bool __init xive_native_init(void)
/* Resource 1 is HV window */
if (of_address_to_resource(np, 1, )) {
pr_err("Failed to get thread mgmnt area resource\n");
- return false;
+ goto err_put;
}
tima = ioremap(r.start, resource_size());
if (!tima) {
pr_err("Failed to map thread mgmnt area\n");
- return false;
+ goto err_put;
}
/* Read number of priorities */
@@ -612,7 +612,7 @@ bool __init xive_native_init(void)
/* Resource 2 is OS window */
if (of_address_to_resource(np, 2, )) {
pr_err("Failed to get thread mgmnt area resource\n");
- return false;
+ goto err_put;
}
xive_tima_os = r.start;
@@ -624,7 +624,7 @@ bool __init xive_native_init(void)
rc = opal_xive_reset(OPAL_XIVE_MODE_EXPL);
if (rc) {
pr_err("Switch to exploitation mode failed with error %lld\n",
rc);
- return false;
+ goto err_put;
}
/* Setup some dummy HV pool VPs */
@@ -634,10 +634,15 @@ bool __init xive_native_init(void)
if (!xive_core_init(np, _native_ops, tima, TM_QW3_HV_PHYS,
max_prio)) {
opal_xive_reset(OPAL_XIVE_MODE_EMU);
- return false;
+ goto err_put;
}
+ of_node_put(np);
pr_info("Using %dkB queues\n", 1 << (xive_queue_shift - 10));
return true;
+
+err_put:
+ of_node_put(np);
+ return false;
}
static bool xive_native_provision_pages(void)
--
2.25.1
[PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
In gpio_halt_probe(), of_find_matching_node() will return a node pointer with
refcount incremented. We should use of_node_put() in each fail path or when it
is not used anymore.
Signed-off-by: Liang He
---
changelog:
v2: use goto-label patch style advised by Christophe.
v1: add of_node_put() before each exit.
arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..e280f963d88c 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
struct device_node *node = pdev->dev.of_node;
int gpio, err, irq;
int trigger;
+ int ret;
if (!node)
return -ENODEV;
@@ -84,20 +85,24 @@ static int gpio_halt_probe(struct platform_device *pdev)
/* Technically we could just read the first one, but punish
* DT writers for invalid form. */
- if (of_gpio_count(halt_node) != 1)
- return -EINVAL;
+ if (of_gpio_count(halt_node) != 1) {
+ ret = -EINVAL;
+ goto err_put;
+ }
/* Get the gpio number relative to the dynamic base. */
gpio = of_get_gpio_flags(halt_node, 0, );
- if (!gpio_is_valid(gpio))
- return -EINVAL;
+ if (!gpio_is_valid(gpio)) {
+ ret = -EINVAL;
+ gotot err_put;
+ }
err = gpio_request(gpio, "gpio-halt");
if (err) {
printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
gpio);
- halt_node = NULL;
- return err;
+ ret = err;
+ goto err_put;
}
trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -112,8 +117,8 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
"GPIO %d.\n", irq, gpio);
gpio_free(gpio);
- halt_node = NULL;
- return err;
+ ret = err;
+ goto err_put;
}
/* Register our halt function */
@@ -122,8 +127,12 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
" irq).\n", gpio, trigger, irq);
+ ret = 0;
- return 0;
+err_put:
+ of_node_put(halt_node);
+ halt_node = NULL;
+ return ret;
}
static int gpio_halt_remove(struct platform_device *pdev)
--
2.25.1
Re:Re: [PATCH] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
At 2022-06-16 22:49:36, "Christophe Leroy" wrote:
>
>
>Le 15/06/2022 à 14:07, Liang He a écrit :
>> [You don't often get email from win...@126.com. Learn why this is important
>> at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/platforms/85xx/sgy_cts1000.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> index 98ae64075193..2a45b30852b2 100644
>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> @@ -85,17 +85,24 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> /* Technically we could just read the first one, but punish
>> * DT writers for invalid form. */
>> if (of_gpio_count(halt_node) != 1)
>> + {
>> + of_node_put(halt_node);
>
>Duplicating the same code at multiple exit points is bad practice.
>
>If you can't do a simple 'return' exit, you should use 'goto' to a
>common error path exit.
Thanks for your valuable advice, I will resend a new patch for that.
>
>> return -EINVAL;
>> + }
>>
>> /* Get the gpio number relative to the dynamic base. */
>> gpio = of_get_gpio_flags(halt_node, 0, );
>> if (!gpio_is_valid(gpio))
>> + {
>> + of_node_put(halt_node);
>> return -EINVAL;
>> + }
>>
>> err = gpio_request(gpio, "gpio-halt");
>> if (err) {
>> printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
>> gpio);
>> + of_node_put(halt_node);
>> halt_node = NULL;
>> return err;
>> }
>> @@ -112,6 +119,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>> "GPIO %d.\n", irq, gpio);
>> gpio_free(gpio);
>> + of_node_put(halt_node);
>> halt_node = NULL;
>> return err;
>> }
>> @@ -123,6 +131,8 @@ static int gpio_halt_probe(struct platform_device *pdev)
>> printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>> " irq).\n", gpio, trigger, irq);
>>
>> + of_node_put(halt_node);
>> +
>> return 0;
>> }
>>
>> --
>> 2.25.1
>>
[PATCH] powerpc: platforms: 52xx: Fix refcount leak in media5200.c
In media5200_init_irq(), of_find_compatible_node() will return a
node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Don't worry about 'fpga_np==NULL' as of_node_put() can correctly
handle it.
Signed-off-by: Liang He
---
arch/powerpc/platforms/52xx/media5200.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/platforms/52xx/media5200.c
b/arch/powerpc/platforms/52xx/media5200.c
index ee367ff3ec8a..33a35fff11b5 100644
--- a/arch/powerpc/platforms/52xx/media5200.c
+++ b/arch/powerpc/platforms/52xx/media5200.c
@@ -174,6 +174,8 @@ static void __init media5200_init_irq(void)
goto out;
pr_debug("%s: allocated irqhost\n", __func__);
+ of_node_put(fpga_np);
+
irq_set_handler_data(cascade_virq, _irq);
irq_set_chained_handler(cascade_virq, media5200_irq_cascade);
@@ -181,6 +183,7 @@ static void __init media5200_init_irq(void)
out:
pr_err("Could not find Media5200 FPGA; PCI interrupts will not work\n");
+ of_node_put(fpga_np);
}
/*
--
2.25.1
[PATCH] arch: powerpc: platforms: 85xx: Fix refcount leak bug in ksi8560.c
In ksi8560_setup_arch(), of_find_compatible_node() will return a
node pointer with refcount incremented. We should use of_node_put()
when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/platforms/85xx/ksi8560.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/85xx/ksi8560.c
b/arch/powerpc/platforms/85xx/ksi8560.c
index bdf9d42f8521..a22f02b0fc77 100644
--- a/arch/powerpc/platforms/85xx/ksi8560.c
+++ b/arch/powerpc/platforms/85xx/ksi8560.c
@@ -133,6 +133,8 @@ static void __init ksi8560_setup_arch(void)
else
printk(KERN_ERR "Can't find CPLD in device tree\n");
+ of_node_put(cpld);
+
if (ppc_md.progress)
ppc_md.progress("ksi8560_setup_arch()", 0);
--
2.25.1
[PATCH] arch: powerpc: platforms: 512x: Add missing of_node_put()
In mpc5121_clk_init(), of_find_compatible_node() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- arch/powerpc/platforms/512x/clock-commonclk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c b/arch/powerpc/platforms/512x/clock-commonclk.c index 0652c7e69225..ca475462e95b 100644 --- a/arch/powerpc/platforms/512x/clock-commonclk.c +++ b/arch/powerpc/platforms/512x/clock-commonclk.c @@ -1208,6 +1208,8 @@ int __init mpc5121_clk_init(void) /* register as an OF clock provider */ mpc5121_clk_register_of_provider(clk_np); + of_node_put(clk_np); + /* * unbreak not yet adjusted peripheral drivers during migration * towards fully operational common clock support, and allow -- 2.25.1
[PATCH] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c
Signed-off-by: Liang He
---
arch/powerpc/platforms/85xx/sgy_cts1000.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..2a45b30852b2 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -85,17 +85,24 @@ static int gpio_halt_probe(struct platform_device *pdev)
/* Technically we could just read the first one, but punish
* DT writers for invalid form. */
if (of_gpio_count(halt_node) != 1)
+ {
+ of_node_put(halt_node);
return -EINVAL;
+ }
/* Get the gpio number relative to the dynamic base. */
gpio = of_get_gpio_flags(halt_node, 0, );
if (!gpio_is_valid(gpio))
+ {
+ of_node_put(halt_node);
return -EINVAL;
+ }
err = gpio_request(gpio, "gpio-halt");
if (err) {
printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
gpio);
+ of_node_put(halt_node);
halt_node = NULL;
return err;
}
@@ -112,6 +119,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
"GPIO %d.\n", irq, gpio);
gpio_free(gpio);
+ of_node_put(halt_node);
halt_node = NULL;
return err;
}
@@ -123,6 +131,8 @@ static int gpio_halt_probe(struct platform_device *pdev)
printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
" irq).\n", gpio, trigger, irq);
+ of_node_put(halt_node);
+
return 0;
}
--
2.25.1
