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

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

[Liang He]

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@126.com>
---
 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;
+	}
 
 	err = gpio_request(gpio, "gpio-halt");
 	if (err) {
 		printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
 		       gpio);
-		halt_node = NULL;
-		return err;
+		goto err_put;
 	}
 
 	trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device *pdev)
 	gpio_direction_output(gpio, !trigger);
 
 	/* Now get the IRQ which tells us when the power button is hit */
-	irq = irq_of_parse_and_map(halt_node, 0);
+	irq = irq_of_parse_and_map(child_node, 0);
 	err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
-			  IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
+			  IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
 	if (err) {
 		printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
 		       "GPIO %d.\n", irq, gpio);
 		gpio_free(gpio);
-		halt_node = NULL;
-		return err;
+		goto err_put;
 	}
 
 	/* Register our halt function */
@@ -123,7 +126,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);
 
+	halt_node = child_node;
 	return 0;
+
+err_put:
+	of_node_put(child_node);
+	return err;
 }
 
 static int gpio_halt_remove(struct platform_device *pdev)
@@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
 
 		gpio_free(gpio);
 
+		of_node_put(halt_node);
 		halt_node = NULL;
 	}
 
-- 
2.25.1

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

[Christophe Leroy]

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@126.com>
> ---
>   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 ?

> +	}
>   
>   	err = gpio_request(gpio, "gpio-halt");
>   	if (err) {
>   		printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
>   		       gpio);
> -		halt_node = NULL;
> -		return err;
> +		goto err_put;
>   	}
>   
>   	trigger = (flags == OF_GPIO_ACTIVE_LOW);
> @@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device *pdev)
>   	gpio_direction_output(gpio, !trigger);
>   
>   	/* Now get the IRQ which tells us when the power button is hit */
> -	irq = irq_of_parse_and_map(halt_node, 0);
> +	irq = irq_of_parse_and_map(child_node, 0);
>   	err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
> -			  IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
> +			  IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
>   	if (err) {
>   		printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>   		       "GPIO %d.\n", irq, gpio);
>   		gpio_free(gpio);
> -		halt_node = NULL;
> -		return err;
> +		goto err_put;
>   	}
>   
>   	/* Register our halt function */
> @@ -123,7 +126,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);
>   
> +	halt_node = child_node;
>   	return 0;
> +
> +err_put:
> +	of_node_put(child_node);
> +	return err;
>   }
>   
>   static int gpio_halt_remove(struct platform_device *pdev)
> @@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
>   
>   		gpio_free(gpio);
>   
> +		of_node_put(halt_node);
>   		halt_node = NULL;
>   	}
>   

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

[Liang He]

At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>> ---
>>   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.


>
>> +	}
>>   
>>   	err = gpio_request(gpio, "gpio-halt");
>>   	if (err) {
>>   		printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
>>   		       gpio);
>> -		halt_node = NULL;
>> -		return err;
>> +		goto err_put;
>>   	}
>>   
>>   	trigger = (flags == OF_GPIO_ACTIVE_LOW);
>> @@ -105,15 +109,14 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>   	gpio_direction_output(gpio, !trigger);
>>   
>>   	/* Now get the IRQ which tells us when the power button is hit */
>> -	irq = irq_of_parse_and_map(halt_node, 0);
>> +	irq = irq_of_parse_and_map(child_node, 0);
>>   	err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
>> -			  IRQF_TRIGGER_FALLING, "gpio-halt", halt_node);
>> +			  IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
>>   	if (err) {
>>   		printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>>   		       "GPIO %d.\n", irq, gpio);
>>   		gpio_free(gpio);
>> -		halt_node = NULL;
>> -		return err;
>> +		goto err_put;
>>   	}
>>   
>>   	/* Register our halt function */
>> @@ -123,7 +126,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);
>>   
>> +	halt_node = child_node;
>>   	return 0;
>> +
>> +err_put:
>> +	of_node_put(child_node);
>> +	return err;
>>   }
>>   
>>   static int gpio_halt_remove(struct platform_device *pdev)
>> @@ -139,6 +147,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
>>   
>>   		gpio_free(gpio);
>>   
>> +		of_node_put(halt_node);
>>   		halt_node = NULL;
>>   	}
>>   

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

[Christophe Leroy]

Le 17/06/2022 à 08:45, Liang He a écrit :
> 
> 
> 
> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>> ---
>>>    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

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@csgroup.eu> wrote:
>
>
>Le 17/06/2022 à 08:45, Liang He a écrit :
>> 
>> 
>> 
>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>> ---
>>>>    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

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@csgroup.eu> wrote:
>>
>>
>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>
>>>
>>>
>>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>>> ---
>>>>>     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@intel.com/


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

Hope that helps,
Conor.

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

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@microchip.com wrote:
>On 17/06/2022 09:17, Liang He wrote:
>> 
>> 
>> 
>> At 2022-06-17 14:53:13, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>>>> ---
>>>>>>     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@intel.com/
>
>
>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.

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@microchip.com wrote:
>On 17/06/2022 09:17, Liang He wrote:
>> 
>> 
>> 
>> At 2022-06-17 14:53:13, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>>>
>>>
>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>>>> ---
>>>>>>     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@intel.com/
>
>
>In this case, your bug seems obvious? You typed "gotot" instead of "goto".
>
>Hope that helps,
>Conor.
>
>> 
>> Thanks again, Christophe and Conor.
>> 
>> Liang

Thanks, Conor and Christophe.

I finally figure out an efficient way in which I can use cross-compiler to check my single patched file as follow:

powerpc64le-linux-gnu-gcc -Wp,-MMD,arch/powerpc/kernel/.io.o.d  -nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated  -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -I ./arch/powerpc -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 -mcpu=powerpc -mcpu=powerpc -m32 -msoft-float -pipe -ffixed-r2 -mno-readonly-in-sdata -mno-altivec -mno-vsx -fno-asynchronous-unwind-tables -mno-string -mbig-endian -mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -fno-delete-null-pointer-checks -Wno-frame-address -Wno-format-truncation -Wno-format-overflow -Wno-address-of-packed-member -O2 --param=allow-store-data-races=0 -Wframe-larger-than=1024 -fstack-protector-strong -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wno-stringop-truncation -Wno-stringop-overflow -Wno-restrict -Wno-maybe-uninitialized -Wno-alloc-size-larger-than -fno-strict-overflow -fno-stack-check -fconserve-stack -Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init -Wno-packed-not-aligned -mstack-protector-guard-offset=576 -Werror -DKBUILD_MODNAME='"85xx"' -DKBUILD_MODFILE='"arch/powerpc/platforms/85xx/sgy_cts1000.c"' -fsyntax-only ./arch/powerpc/platforms/85xx/sgy_cts1000.c

Thanks again.

Liang

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

[Christophe Leroy]

Le 17/06/2022 à 12:47, Liang He a écrit :
> 
> 
> 
> At 2022-06-17 16:27:03, Conor.Dooley@microchip.com wrote:
>> On 17/06/2022 09:17, Liang He wrote:
>>>
>>>
>>>
>>> At 2022-06-17 14:53:13, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>>>>
>>>>
>>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>>
>>>>>
>>>>>
>>>>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>>>>> ---
>>>>>>>      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@intel.com/
>>
>>
>> In this case, your bug seems obvious? You typed "gotot" instead of "goto".
>>
>> Hope that helps,
>> Conor.
>>
>>>
>>> Thanks again, Christophe and Conor.
>>>
>>> Liang
> 
> Thanks, Conor and Christophe.
> 
> I finally figure out an efficient way in which I can use cross-compiler to check my single patched file as follow:
> 
> powerpc64le-linux-gnu-gcc -Wp,-MMD,arch/powerpc/kernel/.io.o.d  -nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated  -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -I ./arch/powerpc -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 -mcpu=powerpc -mcpu=powerpc -m32 -msoft-float -pipe -ffixed-r2 -mno-readonly-in-sdata -mno-altivec -mno-vsx -fno-asynchronous-unwind-tables -mno-string -mbig-endian -mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -fno-delete-null-pointer-checks -Wno-frame-address -Wno-format-truncation -Wno-format-overflow -Wno-address-of-packed-member -O2 --param=allow-store-data-races=0 -Wframe-larger-than=1024 -fstack-protector-strong -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wno-stringop-truncation -Wno-stringop-overflow -Wno-restrict -Wno-maybe-uninitialized -Wno-alloc-size-larger-than -fno-strict-overflow -fno-stack-check -fconserve-stack -Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init -Wno-packed-not-aligned -mstack-protector-guard-offset=576 -Werror -DKBUILD_MODNAME='"85xx"' -DKBUILD_MODFILE='"arch/powerpc/platforms/85xx/sgy_cts1000.c"' -fsyntax-only ./arch/powerpc/platforms/85xx/sgy_cts1000.c
> 

Easiest way is:
- Download cross compiler and extract it somewhere on your PC and ensure 
it is in the PATH.
- Then:

run "make CROSS_COMPILE=powerpc64-linux-gnu ARCH=powerpc 
corenet32_smp_defconfig"

modify .config to activate CONFIG_SGY_CTS1000

run "make CROSS_COMPILE=powerpc64-linux-gnu ARCH=powerpc 
arch/powerpc/platforms/85xx/sgy_cts1000.o"

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

[Liang He]

在 2022-06-17 18:57:36，"Christophe Leroy" <christophe.leroy@csgroup.eu> 写道：
>
>
>Le 17/06/2022 à 12:47, Liang He a écrit :
>> 
>> 
>> 
>> At 2022-06-17 16:27:03, Conor.Dooley@microchip.com wrote:
>>> On 17/06/2022 09:17, Liang He wrote:
>>>>
>>>>
>>>>
>>>> At 2022-06-17 14:53:13, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>>>>>
>>>>>
>>>>> Le 17/06/2022 à 08:45, Liang He a écrit :
>>>>>>
>>>>>>
>>>>>>
>>>>>> At 2022-06-17 14:28:56, "Christophe Leroy" <christophe.leroy@csgroup.eu> 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@126.com>
>>>>>>>> ---
>>>>>>>>      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@intel.com/
>>>
>>>
>>> In this case, your bug seems obvious? You typed "gotot" instead of "goto".
>>>
>>> Hope that helps,
>>> Conor.
>>>
>>>>
>>>> Thanks again, Christophe and Conor.
>>>>
>>>> Liang
>> 
>> Thanks, Conor and Christophe.
>> 
>> I finally figure out an efficient way in which I can use cross-compiler to check my single patched file as follow:
>> 
>> powerpc64le-linux-gnu-gcc -Wp,-MMD,arch/powerpc/kernel/.io.o.d  -nostdinc -I./arch/powerpc/include -I./arch/powerpc/include/generated  -I./include -I./arch/powerpc/include/uapi -I./arch/powerpc/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -I ./arch/powerpc -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 -mcpu=powerpc -mcpu=powerpc -m32 -msoft-float -pipe -ffixed-r2 -mno-readonly-in-sdata -mno-altivec -mno-vsx -fno-asynchronous-unwind-tables -mno-string -mbig-endian -mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -fno-delete-null-pointer-checks -Wno-frame-address -Wno-format-truncation -Wno-format-overflow -Wno-address-of-packed-member -O2 --param=allow-store-data-races=0 -Wframe-larger-than=1024 -fstack-protector-strong -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wno-stringop-truncation -Wno-stringop-overflow -Wno-restrict -Wno-maybe-uninitialized -Wno-alloc-size-larger-than -fno-strict-overflow -fno-stack-check -fconserve-stack -Werror=date-time -Werror=incompatible-pointer-types -Werror=designated-init -Wno-packed-not-aligned -mstack-protector-guard-offset=576 -Werror -DKBUILD_MODNAME='"85xx"' -DKBUILD_MODFILE='"arch/powerpc/platforms/85xx/sgy_cts1000.c"' -fsyntax-only ./arch/powerpc/platforms/85xx/sgy_cts1000.c
>> 
>
>Easiest way is:
>- Download cross compiler and extract it somewhere on your PC and ensure 
>it is in the PATH.
>- Then:
>
>run "make CROSS_COMPILE=powerpc64-linux-gnu ARCH=powerpc 
>corenet32_smp_defconfig"
>
>modify .config to activate CONFIG_SGY_CTS1000
>
>run "make CROSS_COMPILE=powerpc64-linux-gnu ARCH=powerpc 
>arch/powerpc/platforms/85xx/sgy_cts1000.o"
>

Thanks for your valuable experience, Christophe.

It is great and I have used it to successfully compile my patched code without
any error: 
"
CC      arch/powerpc/platforms/85xx/sgy_cts1000.o
"

BYW, it should be 'make  CROSS_COMPILE=powerpc64-linux-gnu- ...'

Thanks again!

Liang