"Liang He" <win...@126.com> writes: > At 2022-06-17 07:37:06, "Michael Ellerman" <m...@ellerman.id.au> wrote: >>Christophe JAILLET <christophe.jail...@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 <win...@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 > > 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