From: "gregkh@linuxfoundation•org" <gregkh@linuxfoundation•org>
To: Saeed Mahameed <saeedm@mellanox•com>
Cc: "netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"leon@kernel•org" <leon@kernel•org>
Subject: Re: [PATCH 03/17] mlx5: no need to check return value of debugfs_create functions
Date: Tue, 6 Aug 2019 21:47:47 +0200 [thread overview]
Message-ID: <20190806194747.GA12000@kroah.com> (raw)
In-Reply-To: <d681be03ea2c1997004c8144c3a6062f895817a4.camel@mellanox.com>
On Tue, Aug 06, 2019 at 07:41:57PM +0000, Saeed Mahameed wrote:
> On Tue, 2019-08-06 at 18:11 +0200, 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.
> >
> > This cleans up a lot of unneeded code and logic around the debugfs
> > files, making all of this much simpler and easier to understand as we
> > don't need to keep the dentries saved anymore.
> >
>
> Hi Greg,
>
> Basically i am ok with this patch and i like it very much.., but i am
> concerned about some of the driver internal flows that are dependent on
> these debug fs entries being valid.
That's never good, that's a bug in the driver :)
> for example mlx5_debug_eq_add if failed, it will fail the whole flow. I
> know it is wrong even before your patch.. but maybe we should deal with
> it now ? or let me know if you want me to follow up with my own patch.
Your own patch would be good as I do not know this part of the codebase
at all, thanks.
> All we need to improve in this patch is to void out add_res_tree()
> implemented in
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
> as i will comment below.
>
>
> > Cc: Saeed Mahameed <saeedm@mellanox•com>
> > Cc: Leon Romanovsky <leon@kernel•org>
> > Cc: netdev@vger•kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation•org>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 51 ++-------
> > .../net/ethernet/mellanox/mlx5/core/debugfs.c | 102 ++------------
> > ----
> > drivers/net/ethernet/mellanox/mlx5/core/eq.c | 11 +-
> > .../net/ethernet/mellanox/mlx5/core/lib/eq.h | 2 +-
> > .../net/ethernet/mellanox/mlx5/core/main.c | 7 +-
> > .../ethernet/mellanox/mlx5/core/mlx5_core.h | 2 +-
> > include/linux/mlx5/driver.h | 12 +--
> > 7 files changed, 24 insertions(+), 163 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> > index 8cdd7e66f8df..973f90888b1f 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> > @@ -1368,49 +1368,19 @@ static void clean_debug_files(struct
> > mlx5_core_dev *dev)
> > debugfs_remove_recursive(dbg->dbg_root);
> > }
> >
>
> [...]
>
> > void mlx5_cq_debugfs_cleanup(struct mlx5_core_dev *dev)
> > {
> > - if (!mlx5_debugfs_root)
> > - return;
> > -
> > debugfs_remove_recursive(dev->priv.cq_debugfs);
> > }
> >
> > @@ -484,7 +418,6 @@ static int add_res_tree(struct mlx5_core_dev
>
> Basically this function is a debugfs wrapper that should behave the
> same as debug_fs_*, we should fix it to return void as well, and
> improve all the its callers to ignore the return value.
But mlx5_cq_debugfs_cleanup() is a void function.
> callers are:
> mlx5_debug_qp_add()
> mlx5_debug_eq_add()
> mlx5_debug_cq_add()
Ah, you mean add_res_tree(). Yes, make that void as well.
thanks,
greg k-h
next prev parent reply other threads:[~2019-08-06 19:47 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-06 16:11 [PATCH 00/17] Networking driver debugfs cleanups Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 01/17] wimax: no need to check return value of debugfs_create functions Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 02/17] bonding: no need to print a message if debugfs_create_dir() fails Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 03/17] mlx5: no need to check return value of debugfs_create functions Greg Kroah-Hartman
2019-08-06 19:41 ` Saeed Mahameed
2019-08-06 19:47 ` gregkh [this message]
2019-08-06 20:37 ` Saeed Mahameed
2019-08-06 16:11 ` [PATCH 04/17] xgbe: " Greg Kroah-Hartman
2019-08-06 16:28 ` Lendacky, Thomas
2019-08-06 16:11 ` [PATCH 05/17] bnxt: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 06/17] cxgb4: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 07/17] hns3: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 08/17] nfp: " Greg Kroah-Hartman
2019-08-06 16:50 ` Jakub Kicinski
2019-08-06 17:00 ` Greg Kroah-Hartman
2019-08-06 17:30 ` Jakub Kicinski
2019-08-06 16:11 ` [PATCH 09/17] stmmac: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 10/17] dpaa2: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 11/17] qca: " Greg Kroah-Hartman
2019-08-08 6:30 ` Michael Heimpold
2019-08-06 16:11 ` [PATCH 12/17] skge: " Greg Kroah-Hartman
2019-08-06 17:44 ` Stephen Hemminger
2019-08-06 16:11 ` [PATCH 13/17] mvpp2: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 14/17] fm10k: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 15/17] i40e: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 16/17] ixgbe: " Greg Kroah-Hartman
2019-08-06 16:11 ` [PATCH 17/17] ieee802154: " Greg Kroah-Hartman
2019-08-06 19:22 ` Stefan Schmidt
2019-08-06 19:48 ` Greg Kroah-Hartman
2019-08-09 1:37 ` [PATCH 00/17] Networking driver debugfs cleanups David Miller
2019-08-09 1:42 ` David Miller
2019-08-09 12:30 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190806194747.GA12000@kroah.com \
--to=gregkh@linuxfoundation$(echo .)org \
--cc=leon@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=saeedm@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox