Messages in this thread | | | From | Michael Ellerman <> | Subject | Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c | Date | Fri, 17 Jun 2022 09:37:06 +1000 |
| |
Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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 <windhl@126.com> >> --- >> 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
| |