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

[Conor.Dooley]

On 17/06/2022 09:17, Liang He wrote:
> 
> 
> 
> 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.

idk anything about powerpc, but what I find is a nice way to get a compiler
for an arch I don't use is to search on lore.kernel.org for a 0day robot
build error since it gives instructions for building on that arch.
For example:
https://lore.kernel.org/linuxppc-dev/202206060910.rYNTFqdI-lkp@xxxxxxxxx/


In this case, your bug seems obvious? You typed "gotot" instead of "goto".

Hope that helps,
Conor.

> 
> Thanks again, Christophe and Conor.
> 
> Liang