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;
-
-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);
 
-out_err:
-	rv_remove(rv_root.root_dir);
-	printk(KERN_ERR "RV: Error while creating the RV interface\n");
-	return 1;
+	return 0;
 }
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 1485a70c1bf4..8661d1b6ede4 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -17,6 +17,8 @@ struct rv_interface {
 #define rv_create_file			tracefs_create_file
 #define rv_remove			tracefs_remove
 
+DEFINE_FREE(rv_remove, struct dentry *, rv_remove(_T));
+
 #define MAX_RV_MONITOR_NAME_SIZE	32
 #define MAX_RV_REACTOR_NAME_SIZE	32
 
@@ -30,10 +32,10 @@ bool rv_is_container_monitor(struct rv_monitor *mon);
 bool rv_is_nested_monitor(struct rv_monitor *mon);
 
 #ifdef CONFIG_RV_REACTORS
-int reactor_populate_monitor(struct rv_monitor *mon);
+int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root);
 int init_rv_reactors(struct dentry *root_dir);
 #else
-static inline int reactor_populate_monitor(struct rv_monitor *mon)
+static inline int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root)
 {
 	return 0;
 }
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 37d1d37a27a3..ce7d22cbde57 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -404,14 +404,15 @@ static const struct file_operations reacting_on_fops = {
 /**
  * reactor_populate_monitor - creates per monitor reactors file
  * @mon:	The monitor.
+ * @root:	The directory of the monitor.
  *
  * Returns 0 if successful, error otherwise.
  */
-int reactor_populate_monitor(struct rv_monitor *mon)
+int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root)
 {
 	struct dentry *tmp;
 
-	tmp = rv_create_file("reactors", RV_MODE_WRITE, mon->root_d, mon, &monitor_reactors_ops);
+	tmp = rv_create_file("reactors", RV_MODE_WRITE, root, mon, &monitor_reactors_ops);
 	if (!tmp)
 		return -ENOMEM;
 
@@ -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);
 	return 0;
-
-rm_reacting:
-	rv_remove(reacting);
-rm_available:
-	rv_remove(available);
-out_err:
-	return -ENOMEM;
 }
-- 
2.51.0

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;
>  }

Steven Rostedt <rostedt@goodmis.org> writes:
> On Sun, 16 Nov 2025 15:35:12 +0000
> Nam Cao <namcao@linutronix.de> wrote:
>> +	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().

I wasn't aware that no_free_ptr() exists. Gabriele has pointed this out,
and I fixed it up in v2.

Nam

On Sun, 2025-11-16 at 15:35 +0000, Nam Cao wrote:
> Convert to use __free to tidy up the code.
> 
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Both patches look very neat, thanks!

I have a few comments on this one:

> ---
>  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)
>  {
> ... 
> +	mon->root_d = dir;
> +	retain_and_null_ptr(dir);

I think you could just save a line by doing:

+	mon->root_d = no_free_ptr(dir);

>  	return 0;
> -
> -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 as above:

+	mon->root_d = no_free_ptr(root_dir);

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

By removing this, you are also not logging in case of failure, right?
I'm still liking to be rid of goto's, but what about moving this error reporting
to the caller tracer_init_tracefs() ?

> -	return 1;
> +	return 0;
>  }
> diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
> index 1485a70c1bf4..8661d1b6ede4 100644
> --- a/kernel/trace/rv/rv.h
> +++ b/kernel/trace/rv/rv.h
> @@ -17,6 +17,8 @@ struct rv_interface {
>  #define rv_create_file			tracefs_create_file
>  #define rv_remove			tracefs_remove
>  
> +DEFINE_FREE(rv_remove, struct dentry *, rv_remove(_T));

It's kind of idiomatic to do things like `if (_T) free(_T)` in those.
Although free (like tracefs_remove) checks for null, it's probably safer to keep
it that way here as well (as the entire system, including the return_ptr magic
relies on it):

+DEFINE_FREE(rv_remove, struct dentry *, if (_T) rv_remove(_T));

> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index 37d1d37a27a3..ce7d22cbde57 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -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);

Nothing is removing "reacting_on" in case of successive failure, is it?
Am I missing anything or couldn't we just set both variables to __free() ?

Thanks,
Gabriele

> +
> +	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);
>  	return 0;
> -
> -rm_reacting:
> -	rv_remove(reacting);
> -rm_available:
> -	rv_remove(available);
> -out_err:
> -	return -ENOMEM;
>  }

Gabriele Monaco <gmonaco@redhat.com> writes:
> On Sun, 2025-11-16 at 15:35 +0000, Nam Cao wrote:
>> -	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);
>
> Nothing is removing "reacting_on" in case of successive failure, is it?
> Am I missing anything or couldn't we just set both variables to __free() ?

We can. I overlooked this one :(

Nam