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

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

[Greg Kroah-Hartman]

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@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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)
 		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;
 }
 
 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 {
-- 
2.23.0

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

[Jason Gunthorpe]

On Mon, Nov 04, 2019 at 08:41:41AM +0100, 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@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  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?

I think it is fine, I don't forsee conflicts here at this point.

To be clear, the build is broken in your tree and in linux-next?

> And 0-day seems really broken to have missed this for the past months,
> ugh, I need to stop relying on it...

Yes, I've noticed it missing a lot of stuff now too. Not sure why

Jason

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

[Greg Kroah-Hartman]

On Mon, Nov 04, 2019 at 04:59:14PM -0400, Jason Gunthorpe wrote:
> On Mon, Nov 04, 2019 at 08:41:41AM +0100, 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@kernel.org>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >  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?
> 
> I think it is fine, I don't forsee conflicts here at this point.

Thanks!

> To be clear, the build is broken in your tree and in linux-next?

Yeah, my fault :(

> > And 0-day seems really broken to have missed this for the past months,
> > ugh, I need to stop relying on it...
> 
> Yes, I've noticed it missing a lot of stuff now too. Not sure why

It is very hit-or-miss these days :(

thanks,

greg k-h

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@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  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 {
> 

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

[Greg Kroah-Hartman]

On Tue, Nov 05, 2019 at 12:48:16AM +0000, Mark Bloch wrote:
> 
> 
> 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@kernel.org>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  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;
> ?

No, really there should not be any check at all as there is no problem
taking the result of a debugfs call and feeding it back into another
call.  There is no need to check these return values at all.

So the code should just be dropped, I can do that as a follow-on if you
want me to.

> >  		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().

Very true, I was trying to keep the patch simple :)

thanks,

greg k-h

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

[Leon Romanovsky]

On Mon, Nov 04, 2019 at 08:41:41AM +0100, 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@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  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...
>

Thanks,
Acked-by: Leon Romanovsky <leonro@mellanox.com>