Re:Re: [PATCH v4] powerpc:85xx: Add missing of_node_put() in sgy_cst1000

[Liang He]

At 2022-06-17 14:53:13, "Christophe Leroy" <christophe.leroy@xxxxxxxxxx> wrote:
>
>
>Le 17/06/2022 à 08:45, Liang He a écrit :
>> 
>> 
>> 
>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@xxxxxxxxxx> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:08, 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
>>>> fail path or when it is not used anymore.
>>>>
>>>> Signed-off-by: Liang He <windhl@xxxxxxx>
>>>> ---
>>>>    changelog:
>>>>    v4: reuse exist 'err' and use a simple code style, advised by CJ
>>>>    v3: use local 'child_node' advised by Michael.
>>>>    v2: use goto-label patch style advised by Christophe Leroy.
>>>>    v1: add of_node_put() before each exit.
>>>>
>>>>    arch/powerpc/platforms/85xx/sgy_cts1000.c | 35 ++++++++++++++---------
>>>>    1 file changed, 22 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> index 98ae64075193..e4588943fe7e 100644
>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> @@ -71,6 +71,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>>>    {
>>>>    	enum of_gpio_flags flags;
>>>>    	struct device_node *node = pdev->dev.of_node;
>>>> +	struct device_node *child_node;
>>>>    	int gpio, err, irq;
>>>>    	int trigger;
>>>>    
>>>> @@ -78,26 +79,29 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>>>    		return -ENODEV;
>>>>    
>>>>    	/* If there's no matching child, this isn't really an error */
>>>> -	halt_node = of_find_matching_node(node, child_match);
>>>> -	if (!halt_node)
>>>> +	child_node = of_find_matching_node(node, child_match);
>>>> +	if (!child_node)
>>>>    		return 0;
>>>>    
>>>>    	/* Technically we could just read the first one, but punish
>>>>    	 * DT writers for invalid form. */
>>>> -	if (of_gpio_count(halt_node) != 1)
>>>> -		return -EINVAL;
>>>> +	if (of_gpio_count(child_node) != 1) {
>>>> +		err = -EINVAL;
>>>> +		goto err_put;
>>>> +	}
>>>>    
>>>>    	/* Get the gpio number relative to the dynamic base. */
>>>> -	gpio = of_get_gpio_flags(halt_node, 0, &flags);
>>>> -	if (!gpio_is_valid(gpio))
>>>> -		return -EINVAL;
>>>> +	gpio = of_get_gpio_flags(child_node, 0, &flags);
>>>> +	if (!gpio_is_valid(gpio)) {
>>>> +		err = -EINVAL;
>>>> +		gotot err_put;
>>>
>>> Did you test the build ?
>> 
>> Sorry for this fault.
>> 
>> In fact, I am still finding an efficient way to building different arch source code as I only have x86-64.
>> 
>> Now I am try using QEMU.
>> 
>> Anyway, sorry for this fault.
>
>You can find cross compilers for most architectures for x86-64 here : 
>https://mirrors.edge.kernel.org/pub/tools/crosstool/
>
>Christophe

Hi, Christophe and Conor.

Sorry to trouble you again.

Now I only know how to quickly identify the refcounting bugs, but I cannot efficiently give a build test.

For example, I use the cross compilers 'powerpc-linux-gnu-gcc' to compile 'arch/powerpc/platforms/85xx/sgy_cts1000.c' with -fsyntax-only flag.
But I meet too many header file missing errors. Even if I add some 'include' pathes, e.g., ./arch/powerpc/include, ./include, 
there are still too many other errors.

So if there is any efficient way to check my patch code to avoid 'gotot' error again.

Thanks again, Christophe and Conor.

Liang