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

[Liang He]

At 2022-06-17 16:27:03, Conor.Dooley@xxxxxxxxxxxxx wrote:
>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


Thanks so much, I will try it.