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

[Gabriele Monaco <gmonaco@redhat.com>]

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