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 <sae...@mellanox.com>
> > Cc: Leon Romanovsky <l...@kernel.org>
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gre...@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
