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

From: Conor.Dooley
Date: Fri Jun 17 2022 - 04:27:18 EST


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