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
