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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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)
Christophe JAILLET <[email protected]> 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 <[email protected]>
>> ---
>> 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
At 2022-06-17 07:37:06, "Michael Ellerman" <[email protected]> wrote:
>Christophe JAILLET <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
At 2022-06-17 07:37:06, "Michael Ellerman" <[email protected]> wrote:
>Christophe JAILLET <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
"Liang He" <[email protected]> writes:
> At 2022-06-17 07:37:06, "Michael Ellerman" <[email protected]> wrote:
>>Christophe JAILLET <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
2022-06-17 12:29:02，"Michael Ellerman" <[email protected]> 写道：
>"Liang He" <[email protected]> writes:
>> At 2022-06-17 07:37:06, "Michael Ellerman" <[email protected]> wrote:
>>>Christophe JAILLET <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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.