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

[Christophe JAILLET]

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@xxxxxxx>
> ---
>   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)