Messages in this thread | | | Date | Fri, 17 Jun 2022 10:25:13 +0800 (CST) | From | "Liang He" <> | Subject | 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" <mpe@ellerman.id.au> wrote: >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
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
| |