2022-06-17 12:29:02，"Michael Ellerman" <m...@ellerman.id.au> 写道： >"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
Ok, I will make the new patch soon.
