[v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

[PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Liang He 3 years, 10 months ago

In gpio_halt_probe(), of_find_matching_node() will return a node pointer with
refcount incremented. We should use of_node_put() in each fail path or when it
is not used anymore.

Signed-off-by: Liang He <windhl@126.com>
---
 changelog:

 v2: use goto-label patch style advised by Christophe.
 v1: add of_node_put() before each exit.

 arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..e280f963d88c 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
 	struct device_node *node = pdev->dev.of_node;
 	int gpio, err, irq;
 	int trigger;
+	int ret;
 
 	if (!node)
 		return -ENODEV;
@@ -84,20 +85,24 @@ static int gpio_halt_probe(struct platform_device *pdev)
 
 	/* 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(halt_node) != 1) {
+		ret = -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;
+	if (!gpio_is_valid(gpio)) {
+		ret = -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;
+		ret = err;
+		goto err_put;
 	}
 
 	trigger = (flags == OF_GPIO_ACTIVE_LOW);
@@ -112,8 +117,8 @@ static int gpio_halt_probe(struct platform_device *pdev)
 		printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
 		       "GPIO %d.\n", irq, gpio);
 		gpio_free(gpio);
-		halt_node = NULL;
-		return err;
+		ret = err;
+		goto err_put;
 	}
 
 	/* Register our halt function */
@@ -122,8 +127,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);
+	ret = 0;
 
-	return 0;
+err_put:
+	of_node_put(halt_node);
+	halt_node = NULL;
+	return ret;
 }
 
 static int gpio_halt_remove(struct platform_device *pdev)
-- 
2.25.1

Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Christophe JAILLET 3 years, 10 months ago

Le 16/06/2022 à 17:19, 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 each fail path or when it
> is not used anymore.
> 
> Signed-off-by: Liang He <windhl@126.com>
> ---
>   changelog:
> 
>   v2: use goto-label patch style advised by Christophe.
>   v1: add of_node_put() before each exit.
> 
>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
> index 98ae64075193..e280f963d88c 100644
> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>   	struct device_node *node = pdev->dev.of_node;
>   	int gpio, err, irq;
>   	int trigger;
> +	int ret;
>   
>   	if (!node)
>   		return -ENODEV;
> @@ -84,20 +85,24 @@ static int gpio_halt_probe(struct platform_device *pdev)
>   
>   	/* 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(halt_node) != 1) {
> +		ret = -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;
> +	if (!gpio_is_valid(gpio)) {
> +		ret = -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;
> +		ret = err;
> +		goto err_put;
>   	}
>   
>   	trigger = (flags == OF_GPIO_ACTIVE_LOW);
> @@ -112,8 +117,8 @@ static int gpio_halt_probe(struct platform_device *pdev)
>   		printk(KERN_ERR "gpio-halt: error requesting IRQ %d for "
>   		       "GPIO %d.\n", irq, gpio);
>   		gpio_free(gpio);
> -		halt_node = NULL;
> -		return err;
> +		ret = err;
> +		goto err_put;
>   	}
>   
>   	/* Register our halt function */
> @@ -122,8 +127,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);
> +	ret = 0;
>   
> -	return 0;
> +err_put:
> +	of_node_put(halt_node);
> +	halt_node = NULL;

Hi,
so now we set 'halt_node' to NULL even in the normal case.
This is really spurious.

Look at gpio_halt_cb(), but I think that this is just wrong and badly 
breaks this driver.

CJ


> +	return ret;
>   }
>   
>   static int gpio_halt_remove(struct platform_device *pdev)

Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Michael Ellerman 3 years, 10 months ago

Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> Le 16/06/2022 à 17:19, 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 each fail path or when it
>> is not used anymore.
>> 
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>   changelog:
>> 
>>   v2: use goto-label patch style advised by Christophe.
>>   v1: add of_node_put() before each exit.
>> 
>>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>>   1 file changed, 18 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> index 98ae64075193..e280f963d88c 100644
>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
...
>> @@ -122,8 +127,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);
>> +	ret = 0;
>>   
>> -	return 0;
>> +err_put:
>> +	of_node_put(halt_node);
>> +	halt_node = NULL;
>
> Hi,
> so now we set 'halt_node' to NULL even in the normal case.
> This is really spurious.
>
> Look at gpio_halt_cb(), but I think that this is just wrong and badly 
> breaks this driver.

I agree, thanks for reviewing.

I think the cleanest solution is to use a local variable for the node in
the body of gpio_halt_probe(), and only assign to halt_node once all the
checks have passed.

So something like:

        struct device_node *child_node;

	child_node = of_find_matching_node(node, child_match);
        ...

	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
	       " irq).\n", gpio, trigger, irq);
        ret = 0;
        halt_node = of_node_get(child_node);

out_put:
        of_node_put(child_node);
        
	return ret;
}


cheers

Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Liang He 3 years, 10 months ago



At 2022-06-17 07:37:06, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>> Le 16/06/2022 à 17:19, 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 each fail path or when it
>>> is not used anymore.
>>> 
>>> Signed-off-by: Liang He <windhl@126.com>
>>> ---
>>>   changelog:
>>> 
>>>   v2: use goto-label patch style advised by Christophe.
>>>   v1: add of_node_put() before each exit.
>>> 
>>>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> index 98ae64075193..e280f963d88c 100644
>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>...
>>> @@ -122,8 +127,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);
>>> +	ret = 0;
>>>   
>>> -	return 0;
>>> +err_put:
>>> +	of_node_put(halt_node);
>>> +	halt_node = NULL;
>>
>> Hi,
>> so now we set 'halt_node' to NULL even in the normal case.
>> This is really spurious.
>>
>> Look at gpio_halt_cb(), but I think that this is just wrong and badly 
>> breaks this driver.
>
>I agree, thanks for reviewing.
>
>I think the cleanest solution is to use a local variable for the node in
>the body of gpio_halt_probe(), and only assign to halt_node once all the
>checks have passed.
>
>So something like:
>
>        struct device_node *child_node;
>
>	child_node = of_find_matching_node(node, child_match);
>        ...
>
>	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>	       " irq).\n", gpio, trigger, irq);
>        ret = 0;
>        halt_node = of_node_get(child_node);
>
>out_put:
>        of_node_put(child_node);
>        
>	return ret;
>}
>
>
>cheers

Hi, Michael and Christophe,

I am writing the new patch based on Michael's advice. However, I wonder if there is
any place to call of_node_put(halt_node)?  As I do not exactly know if gpio_halt_remove()
or anyother place can correctly release this global reference？
If not, it is correct that I add a of_node_put(halt_node) in gpio_halt_remove(), right?

Thanks and wait for your replies.

Liang

Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Michael Ellerman 3 years, 10 months ago

"Liang He" <windhl@126.com> writes:
> At 2022-06-17 07:37:06, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>>Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>>> Le 16/06/2022 à 17:19, 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 each fail path or when it
>>>> is not used anymore.
>>>> 
>>>> Signed-off-by: Liang He <windhl@126.com>
>>>> ---
>>>>   changelog:
>>>> 
>>>>   v2: use goto-label patch style advised by Christophe.
>>>>   v1: add of_node_put() before each exit.
>>>> 
>>>>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>>> 
>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> index 98ae64075193..e280f963d88c 100644
>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>...
>>>> @@ -122,8 +127,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);
>>>> +	ret = 0;
>>>>   
>>>> -	return 0;
>>>> +err_put:
>>>> +	of_node_put(halt_node);
>>>> +	halt_node = NULL;
>>>
>>> Hi,
>>> so now we set 'halt_node' to NULL even in the normal case.
>>> This is really spurious.
>>>
>>> Look at gpio_halt_cb(), but I think that this is just wrong and badly 
>>> breaks this driver.
>>
>>I agree, thanks for reviewing.
>>
>>I think the cleanest solution is to use a local variable for the node in
>>the body of gpio_halt_probe(), and only assign to halt_node once all the
>>checks have passed.
>>
>>So something like:
>>
>>        struct device_node *child_node;
>>
>>	child_node = of_find_matching_node(node, child_match);
>>        ...
>>
>>	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>>	       " irq).\n", gpio, trigger, irq);
>>        ret = 0;
>>        halt_node = of_node_get(child_node);
>>
>>out_put:
>>        of_node_put(child_node);
>>        
>>	return ret;
>>}
>>
>>
>>cheers
>
> Hi, Michael and Christophe,
>
> I am writing the new patch based on Michael's advice. However, I wonder if there is
> any place to call of_node_put(halt_node)?  As I do not exactly know if gpio_halt_remove()
> or anyother place can correctly release this global reference？
> If not, it is correct that I add a of_node_put(halt_node) in gpio_halt_remove(), right?

Yes I think so, just before it's set to NULL, eg:

diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index 98ae64075193..7beb3cd420ba 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -139,6 +139,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
 
 		gpio_free(gpio);
 
+		of_node_put(halt_node);
 		halt_node = NULL;
 	}
 

cheers

Re:Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Liang He 3 years, 10 months ago


 2022-06-17 12:29:02，"Michael Ellerman" <mpe@ellerman.id.au> 写道：
>"Liang He" <windhl@126.com> writes:
>> At 2022-06-17 07:37:06, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>>>Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>>>> Le 16/06/2022 à 17:19, 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 each fail path or when it
>>>>> is not used anymore.
>>>>> 
>>>>> Signed-off-by: Liang He <windhl@126.com>
>>>>> ---
>>>>>   changelog:
>>>>> 
>>>>>   v2: use goto-label patch style advised by Christophe.
>>>>>   v1: add of_node_put() before each exit.
>>>>> 
>>>>>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>>>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>>>> 
>>>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>> index 98ae64075193..e280f963d88c 100644
>>>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>>>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>>>...
>>>>> @@ -122,8 +127,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);
>>>>> +	ret = 0;
>>>>>   
>>>>> -	return 0;
>>>>> +err_put:
>>>>> +	of_node_put(halt_node);
>>>>> +	halt_node = NULL;
>>>>
>>>> Hi,
>>>> so now we set 'halt_node' to NULL even in the normal case.
>>>> This is really spurious.
>>>>
>>>> Look at gpio_halt_cb(), but I think that this is just wrong and badly 
>>>> breaks this driver.
>>>
>>>I agree, thanks for reviewing.
>>>
>>>I think the cleanest solution is to use a local variable for the node in
>>>the body of gpio_halt_probe(), and only assign to halt_node once all the
>>>checks have passed.
>>>
>>>So something like:
>>>
>>>        struct device_node *child_node;
>>>
>>>	child_node = of_find_matching_node(node, child_match);
>>>        ...
>>>
>>>	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>>>	       " irq).\n", gpio, trigger, irq);
>>>        ret = 0;
>>>        halt_node = of_node_get(child_node);
>>>
>>>out_put:
>>>        of_node_put(child_node);
>>>        
>>>	return ret;
>>>}
>>>
>>>
>>>cheers
>>
>> Hi, Michael and Christophe,
>>
>> I am writing the new patch based on Michael's advice. However, I wonder if there is
>> any place to call of_node_put(halt_node)?  As I do not exactly know if gpio_halt_remove()
>> or anyother place can correctly release this global reference？
>> If not, it is correct that I add a of_node_put(halt_node) in gpio_halt_remove(), right?
>
>Yes I think so, just before it's set to NULL, eg:
>
>diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>index 98ae64075193..7beb3cd420ba 100644
>--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>@@ -139,6 +139,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
> 
> 		gpio_free(gpio);
> 
>+		of_node_put(halt_node);
> 		halt_node = NULL;
> 	}
> 
>
>cheers



Ok, I will make the new patch soon.

Re:Re: [PATCH v2] arch: powerpc: platforms: 85xx: Add missing of_node_put in sgy_cts1000.c

Posted by Liang He 3 years, 10 months ago



At 2022-06-17 07:37:06, "Michael Ellerman" <mpe@ellerman.id.au> wrote:
>Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>> Le 16/06/2022 à 17:19, 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 each fail path or when it
>>> is not used anymore.
>>> 
>>> Signed-off-by: Liang He <windhl@126.com>
>>> ---
>>>   changelog:
>>> 
>>>   v2: use goto-label patch style advised by Christophe.
>>>   v1: add of_node_put() before each exit.
>>> 
>>>   arch/powerpc/platforms/85xx/sgy_cts1000.c | 27 +++++++++++++++--------
>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> index 98ae64075193..e280f963d88c 100644
>>> --- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> +++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
>>> @@ -73,6 +73,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
>...
>>> @@ -122,8 +127,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);
>>> +	ret = 0;
>>>   
>>> -	return 0;
>>> +err_put:
>>> +	of_node_put(halt_node);
>>> +	halt_node = NULL;
>>
>> Hi,
>> so now we set 'halt_node' to NULL even in the normal case.
>> This is really spurious.
>>
>> Look at gpio_halt_cb(), but I think that this is just wrong and badly 
>> breaks this driver.
>
>I agree, thanks for reviewing.
>
>I think the cleanest solution is to use a local variable for the node in
>the body of gpio_halt_probe(), and only assign to halt_node once all the
>checks have passed.
>
>So something like:
>
>        struct device_node *child_node;
>
>	child_node = of_find_matching_node(node, child_match);
>        ...
>
>	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
>	       " irq).\n", gpio, trigger, irq);
>        ret = 0;
>        halt_node = of_node_get(child_node);
>
>out_put:
>        of_node_put(child_node);
>        
>	return ret;
>}
>
>
>cheers

Thanks, Michael and Christophe.

I will send a patch based on your reviews.

Liang