Received-SPF: pass (google.com: domain of linux-kernel+bounces-167609-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <3823976c-13b6-4662-a9fd-7615cf69475a@gmail.com>
Date: Fri, 3 May 2024 13:54:41 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] tty: hvc: hvc_opal: eliminate uses of of_node_put()
To: Lu Dai <dai.lu@exordes.com>, npiggin@gmail.com,
 christophe.leroy@csgroup.eu, naveen.n.rao@linux.ibm.com
Cc: mpe@ellerman.id.au, gregkh@linuxfoundation.org, jirislaby@kernel.org,
 linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
 linux-serial@vger.kernel.org, shuah@kernel.org
References: <20240503114330.221764-1-dai.lu@exordes.com>
Content-Language: en-US, de-AT
From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <20240503114330.221764-1-dai.lu@exordes.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 5/3/24 13:43, Lu Dai wrote:
> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.
> 
> Removes the need for a 'goto' as an effect.
> 
> Signed-off-by: Lu Dai <dai.lu@exordes.com>
> ---
>  drivers/tty/hvc/hvc_opal.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 095c33ad10f8..67e90fa993a3 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -327,14 +327,14 @@ static void udbg_init_opal_common(void)
>  
>  void __init hvc_opal_init_early(void)
>  {
> -	struct device_node *stdout_node = of_node_get(of_stdout);
> +	struct device_node *stdout_node __free(device_node) = of_node_get(of_stdout);
>  	const __be32 *termno;
>  	const struct hv_ops *ops;
>  	u32 index;
>  
>  	/* If the console wasn't in /chosen, try /ibm,opal */
>  	if (!stdout_node) {
> -		struct device_node *opal, *np;

Generally, you should always initialize the variable where it is
declared. What would happen if the variable goes out of scope before it
gets initialized? Now it is not dangerous, but if new code is added and
it returns because of some error, we might run into trouble.

In this particular case you can solve this easily by putting together
your modification and the assignment right after the comment.


> +		struct device_node *opal __free(device_node), *np;
>  
>  		/* Current OPAL takeover doesn't provide the stdout
>  		 * path, so we hard wire it
> @@ -356,7 +356,6 @@ void __init hvc_opal_init_early(void)
>  				break;
>  			}
>  		}
> -		of_node_put(opal);
>  	}
>  	if (!stdout_node)
>  		return;
> @@ -382,13 +381,11 @@ void __init hvc_opal_init_early(void)
>  		hvsilib_establish(&hvc_opal_boot_priv.hvsi);
>  		pr_devel("hvc_opal: Found HVSI console\n");
>  	} else
> -		goto out;
> +		return;
>  	hvc_opal_boot_termno = index;
>  	udbg_init_opal_common();
>  	add_preferred_console("hvc", index, NULL);
>  	hvc_instantiate(index, index, ops);
> -out:
> -	of_node_put(stdout_node);
>  }
>  
>  #ifdef CONFIG_PPC_EARLY_DEBUG_OPAL_RAW


Best regards,
Javier Carrasco