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.
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.
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.
callers are:
mlx5_debug_qp_add()
mlx5_debug_eq_add()
mlx5_debug_cq_add()
> *dev, enum dbg_rsc_type type,
> {
> struct mlx5_rsc_debug *d;
> char resn[32];
> - int err;
> int i;
>
> d = kzalloc(struct_size(d, fields, nfile), GFP_KERNEL);
> @@ -496,30 +429,15 @@ static int add_res_tree(struct mlx5_core_dev
> *dev, enum dbg_rsc_type type,
> d->type = type;
> sprintf(resn, "0x%x", rsn);
> d->root = debugfs_create_dir(resn, root);
> - if (!d->root) {
> - err = -ENOMEM;
> - goto out_free;
> - }
>
> for (i = 0; i < nfile; i++) {
> d->fields[i].i = i;
> - d->fields[i].dent = debugfs_create_file(field[i], 0400,
> - d->root, &d-
> >fields[i],
> - &fops);
> - if (!d->fields[i].dent) {
> - err = -ENOMEM;
> - goto out_rem;
> - }
> + debugfs_create_file(field[i], 0400, d->root, &d-
> >fields[i],
> + &fops);
> }
> *dbg = d;
>
> return 0;
> -out_rem:
> - debugfs_remove_recursive(d->root);
> -
> -out_free:
> - kfree(d);
> - return err;
> }
