Re: [PATCH] IB: mlx5: no need to check return value of debugfs_create functions

[Mark Bloch]

On 11/3/19 11:41 PM, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/infiniband/hw/mlx5/main.c    | 62 +++++++---------------------
>  drivers/infiniband/hw/mlx5/mlx5_ib.h |  9 +---
>  2 files changed, 16 insertions(+), 55 deletions(-)
> 
> Note, I kind of need to take this through my tree now as I broke the
> build due to me changing the use of debugfs_create_atomic_t() in my
> tree and not noticing this being used here.  Sorry about that, any
> objections?
> 
> And 0-day seems really broken to have missed this for the past months,
> ugh, I need to stop relying on it...
> 
> 
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 831539419c30..059db0610445 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -5710,11 +5710,10 @@ static int mlx5_ib_rn_get_params(struct ib_device *device, u8 port_num,
>  
>  static void delay_drop_debugfs_cleanup(struct mlx5_ib_dev *dev)
>  {
> -	if (!dev->delay_drop.dbg)
> +	if (!dev->delay_drop.dir_debugfs)

Shouldn't this be:
if (IS_ERR(dev->delay_drop.dir_debugfs))
	return;
?
>  		return;
> -	debugfs_remove_recursive(dev->delay_drop.dbg->dir_debugfs);
> -	kfree(dev->delay_drop.dbg);
> -	dev->delay_drop.dbg = NULL;
> +	debugfs_remove_recursive(dev->delay_drop.dir_debugfs);
> +	dev->delay_drop.dir_debugfs = NULL;

Thinking about this more, we already do something like this:
if (IS_ERR_OR_NULL(dentry))
		return;
inside debugfs_remove_recursive(), so this entire function can be reduced
to just calling debugfs_remove_recursive().

Mark

>  }
>  
>  static void cancel_delay_drop(struct mlx5_ib_dev *dev)
> @@ -5765,52 +5764,22 @@ static const struct file_operations fops_delay_drop_timeout = {
>  	.read	= delay_drop_timeout_read,
>  };
>  
> -static int delay_drop_debugfs_init(struct mlx5_ib_dev *dev)
> +static void delay_drop_debugfs_init(struct mlx5_ib_dev *dev)
>  {
> -	struct mlx5_ib_dbg_delay_drop *dbg;
> +	struct dentry *root;
>  
>  	if (!mlx5_debugfs_root)
> -		return 0;
> -
> -	dbg = kzalloc(sizeof(*dbg), GFP_KERNEL);
> -	if (!dbg)
> -		return -ENOMEM;
> -
> -	dev->delay_drop.dbg = dbg;
> -
> -	dbg->dir_debugfs =
> -		debugfs_create_dir("delay_drop",
> -				   dev->mdev->priv.dbg_root);
> -	if (!dbg->dir_debugfs)
> -		goto out_debugfs;
> -
> -	dbg->events_cnt_debugfs =
> -		debugfs_create_atomic_t("num_timeout_events", 0400,
> -					dbg->dir_debugfs,
> -					&dev->delay_drop.events_cnt);
> -	if (!dbg->events_cnt_debugfs)
> -		goto out_debugfs;
> -
> -	dbg->rqs_cnt_debugfs =
> -		debugfs_create_atomic_t("num_rqs", 0400,
> -					dbg->dir_debugfs,
> -					&dev->delay_drop.rqs_cnt);
> -	if (!dbg->rqs_cnt_debugfs)
> -		goto out_debugfs;
> -
> -	dbg->timeout_debugfs =
> -		debugfs_create_file("timeout", 0600,
> -				    dbg->dir_debugfs,
> -				    &dev->delay_drop,
> -				    &fops_delay_drop_timeout);
> -	if (!dbg->timeout_debugfs)
> -		goto out_debugfs;
> +		return;
>  
> -	return 0;
> +	root = debugfs_create_dir("delay_drop", dev->mdev->priv.dbg_root);
> +	dev->delay_drop.dir_debugfs = root;
>  
> -out_debugfs:
> -	delay_drop_debugfs_cleanup(dev);
> -	return -ENOMEM;
> +	debugfs_create_atomic_t("num_timeout_events", 0400, root,
> +				&dev->delay_drop.events_cnt);
> +	debugfs_create_atomic_t("num_rqs", 0400, root,
> +				&dev->delay_drop.rqs_cnt);
> +	debugfs_create_file("timeout", 0600, root, &dev->delay_drop,
> +			    &fops_delay_drop_timeout);
>  }
>  
>  static void init_delay_drop(struct mlx5_ib_dev *dev)
> @@ -5826,8 +5795,7 @@ static void init_delay_drop(struct mlx5_ib_dev *dev)
>  	atomic_set(&dev->delay_drop.rqs_cnt, 0);
>  	atomic_set(&dev->delay_drop.events_cnt, 0);
>  
> -	if (delay_drop_debugfs_init(dev))
> -		mlx5_ib_warn(dev, "Failed to init delay drop debugfs\n");
> +	delay_drop_debugfs_init(dev);
>  }
>  
>  static void mlx5_ib_unbind_slave_port(struct mlx5_ib_dev *ibdev,
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 1a98ee2e01c4..55ce599db803 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -792,13 +792,6 @@ enum {
>  	MLX5_MAX_DELAY_DROP_TIMEOUT_MS = 100,
>  };
>  
> -struct mlx5_ib_dbg_delay_drop {
> -	struct dentry		*dir_debugfs;
> -	struct dentry		*rqs_cnt_debugfs;
> -	struct dentry		*events_cnt_debugfs;
> -	struct dentry		*timeout_debugfs;
> -};
> -
>  struct mlx5_ib_delay_drop {
>  	struct mlx5_ib_dev     *dev;
>  	struct work_struct	delay_drop_work;
> @@ -808,7 +801,7 @@ struct mlx5_ib_delay_drop {
>  	bool			activate;
>  	atomic_t		events_cnt;
>  	atomic_t		rqs_cnt;
> -	struct mlx5_ib_dbg_delay_drop *dbg;
> +	struct dentry		*dir_debugfs;
>  };
>  
>  enum mlx5_ib_stages {
>