Re: [PATCH 2/2] rv: Convert to use __free

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 16 Nov 2025 15:35:12 +0000
Nam Cao <namcao@linutronix.de> wrote:

> Convert to use __free to tidy up the code.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>  kernel/trace/rv/rv.c          | 65 +++++++++++++++--------------------
>  kernel/trace/rv/rv.h          |  6 ++--
>  kernel/trace/rv/rv_reactors.c | 32 ++++++++---------
>  3 files changed, 46 insertions(+), 57 deletions(-)
> 
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index b1059a3cf4fa..e83dc9f0c7bb 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -420,35 +420,28 @@ static const struct file_operations interface_desc_fops = {
>  static int create_monitor_dir(struct rv_monitor *mon, struct rv_monitor *parent)
>  {
>  	struct dentry *root = parent ? parent->root_d : get_monitors_root();
> -	const char *name = mon->name;
> +	struct dentry *dir __free(rv_remove) = rv_create_dir(mon->name, root);
>  	struct dentry *tmp;
>  	int retval;
>  
> -	mon->root_d = rv_create_dir(name, root);
> -	if (!mon->root_d)
> +	if (!dir)
>  		return -ENOMEM;
>  
> -	tmp = rv_create_file("enable", RV_MODE_WRITE, mon->root_d, mon, &interface_enable_fops);
> -	if (!tmp) {
> -		retval = -ENOMEM;
> -		goto out_remove_root;
> -	}
> +	tmp = rv_create_file("enable", RV_MODE_WRITE, dir, mon, &interface_enable_fops);
> +	if (!tmp)
> +		return -ENOMEM;
>  
> -	tmp = rv_create_file("desc", RV_MODE_READ, mon->root_d, mon, &interface_desc_fops);
> -	if (!tmp) {
> -		retval = -ENOMEM;
> -		goto out_remove_root;
> -	}
> +	tmp = rv_create_file("desc", RV_MODE_READ, dir, mon, &interface_desc_fops);
> +	if (!tmp)
> +		return -ENOMEM;
>  
> -	retval = reactor_populate_monitor(mon);
> +	retval = reactor_populate_monitor(mon, dir);
>  	if (retval)
> -		goto out_remove_root;
> +		return retval;
>  
> +	mon->root_d = dir;
> +	retain_and_null_ptr(dir);
>  	return 0;

Why the "retain_and_null_ptr() and not just:

	mon->root-d = no_free_ptr(dir);
	return 0;

As from my understanding is that the retain_and_null_ptr() is for use of
passing the variable to a function that will consume it. But for assigning
to a variable, I usually just use the no_free_ptr().

> -
> -out_remove_root:
> -	rv_remove(mon->root_d);
> -	return retval;
>  }
>  
>  /*
> @@ -827,39 +820,37 @@ int __init rv_init_interface(void)
>  {
>  	struct dentry *tmp;
>  	int retval;
> +	struct dentry *root_dir __free(rv_remove) = rv_create_dir("rv", NULL);
>  
> -	rv_root.root_dir = rv_create_dir("rv", NULL);
> -	if (!rv_root.root_dir)
> -		goto out_err;
> +	if (!root_dir)
> +		return 1;
>  
> -	rv_root.monitors_dir = rv_create_dir("monitors", rv_root.root_dir);
> +	rv_root.monitors_dir = rv_create_dir("monitors", root_dir);
>  	if (!rv_root.monitors_dir)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("available_monitors", RV_MODE_READ, rv_root.root_dir, NULL,
> +	tmp = rv_create_file("available_monitors", RV_MODE_READ, root_dir, NULL,
>  			     &available_monitors_ops);
>  	if (!tmp)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, rv_root.root_dir, NULL,
> +	tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, root_dir, NULL,
>  			     &enabled_monitors_ops);
>  	if (!tmp)
> -		goto out_err;
> +		return 1;
>  
> -	tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, rv_root.root_dir, NULL,
> +	tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, root_dir, NULL,
>  			     &monitoring_on_fops);
>  	if (!tmp)
> -		goto out_err;
> -	retval = init_rv_reactors(rv_root.root_dir);
> +		return 1;
> +	retval = init_rv_reactors(root_dir);
>  	if (retval)
> -		goto out_err;
> +		return 1;
>  
>  	turn_monitoring_on();
>  
> -	return 0;
> +	rv_root.root_dir = root_dir;
> +	retain_and_null_ptr(root_dir);

Same here.

>  
> -out_err:
> -	rv_remove(rv_root.root_dir);
> -	printk(KERN_ERR "RV: Error while creating the RV interface\n");
> -	return 1;
> +	return 0;
>  }



> @@ -438,30 +439,25 @@ static struct rv_reactor rv_nop = {
>  
>  int init_rv_reactors(struct dentry *root_dir)
>  {
> -	struct dentry *available, *reacting;
>  	int retval;
>  
> -	available = rv_create_file("available_reactors", RV_MODE_READ, root_dir, NULL,
> -				   &available_reactors_ops);
> -	if (!available)
> -		goto out_err;
> +	struct dentry *available __free(rv_remove) =
> +		rv_create_file("available_reactors", RV_MODE_READ, root_dir,
> +				NULL, &available_reactors_ops);
>  
> -	reacting = rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
> -	if (!reacting)
> -		goto rm_available;
> +	struct dentry *reacting =
> +		rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
> +
> +	if (!reacting || !available)
> +		return -ENOMEM;
>  
>  	retval = __rv_register_reactor(&rv_nop);
>  	if (retval)
> -		goto rm_reacting;
> +		return retval;
>  
>  	turn_reacting_on();
>  
> +	retain_and_null_ptr(available);
> +	retain_and_null_ptr(reacting);

Here it makes sense to use retain_and_null_ptr() as the two variables were
passed to another function. But the other two locations should simply use
the no_free_ptr() wrapper.

-- Steve


>  	return 0;
> -
> -rm_reacting:
> -	rv_remove(reacting);
> -rm_available:
> -	rv_remove(available);
> -out_err:
> -	return -ENOMEM;
>  }