Re: [PATCH v2 04/15] NTB: ntb_pp: Add full multi-port NTB API support

[Serge Semin]

On Tue, Dec 05, 2017 at 12:02:13PM -0500, Jon Mason <jdmason@xxxxxxxx> wrote:
> On Sun, Dec 3, 2017 at 2:17 PM, Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> > NTB API has been updated to support multi-port devices like IDT
> > 89HPESx series or Microsemi Switchtec. Message registers
> > functionality has also been added to new API. In order to keep
> > the new hardware and corresponding capabilities well tested, NTB
> > pingpong driver is accordingly altered.
> >
> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > ---
> >
> > Changelog v1:
> > - Add Multi-port API support
> > - Ping-pong now works like cyclic ping around all the peers
> >
> > Changelog v2:
> > - Remove driver Author/Description/Version macros
> >
> >  drivers/ntb/test/ntb_pingpong.c | 450 +++++++++++++++++++++++++---------------
> >  1 file changed, 286 insertions(+), 164 deletions(-)
> >
> > diff --git a/drivers/ntb/test/ntb_pingpong.c b/drivers/ntb/test/ntb_pingpong.c
> > index 3f5a92bae6f8..3e4a35a856cc 100644
> > --- a/drivers/ntb/test/ntb_pingpong.c
> > +++ b/drivers/ntb/test/ntb_pingpong.c
> > @@ -1,10 +1,11 @@
> >  /*
> > - * This file is provided under a dual BSD/GPLv2 license.  When using or
> > + *   This file is provided under a dual BSD/GPLv2 license.  When using or
> >   *   redistributing this file, you may do so under either license.
> >   *
> >   *   GPL LICENSE SUMMARY
> >   *
> >   *   Copyright (C) 2015 EMC Corporation. All Rights Reserved.
> > + *   Copyright (C) 2017 T-Platforms. All Rights Reserved.
> >   *
> >   *   This program is free software; you can redistribute it and/or modify
> >   *   it under the terms of version 2 of the GNU General Public License as
> > @@ -18,6 +19,7 @@
> >   *   BSD LICENSE
> >   *
> >   *   Copyright (C) 2015 EMC Corporation. All Rights Reserved.
> > + *   Copyright (C) 2017 T-Platforms. All Rights Reserved.
> >   *
> >   *   Redistribution and use in source and binary forms, with or without
> >   *   modification, are permitted provided that the following conditions
> > @@ -49,34 +51,46 @@
> >   *
> >   * Contact Information:
> >   * Allen Hubbe <Allen.Hubbe@xxxxxxx>
> > + * Serge Semin <fancer.lancer@xxxxxxxxx>, <Sergey.Semin@xxxxxxxxxxxxxx>
> 
> 2 emails seems unnecessary.  Just add whichever you want kernel emails on.
> 

I'd prefer gmail one, but we agreed to completely remove the contacts info from
the drivers anyway. So these lines will be discarded in v3.

> >   */
> >
> > -/* Note: load this module with option 'dyndbg=+p' */
> > +/*
> > + * How to use this tool, by example.
> > + *
> > + * Assuming $DBG_DIR is something like:
> > + * '/sys/kernel/debug/ntb_perf/0000:00:03.0'
> > + * Suppose aside from local device there is at least one remote device
> > + * connected to NTB with index 0.
> > + *-----------------------------------------------------------------------------
> > + * Eg: install driver with specified delay between doorbell event and response
> > + *
> > + * root@self# insmod ntb_pingpong.ko delay_ms=1000
> > + *-----------------------------------------------------------------------------
> > + * Eg: get number of ping-pong cycles performed
> > + *
> > + * root@self# cat $DBG_DIR/count
> > + */
> >
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/bitops.h>
> >
> > -#include <linux/dma-mapping.h>
> >  #include <linux/pci.h>
> >  #include <linux/slab.h>
> > -#include <linux/spinlock.h>
> > +#include <linux/hrtimer.h>
> >  #include <linux/debugfs.h>
> >
> >  #include <linux/ntb.h>
> >
> > -#define DRIVER_NAME                    "ntb_pingpong"
> > -#define DRIVER_DESCRIPTION             "PCIe NTB Simple Pingpong Client"
> > -
> > -#define DRIVER_LICENSE                 "Dual BSD/GPL"
> > -#define DRIVER_VERSION                 "1.0"
> > -#define DRIVER_RELDATE                 "24 March 2015"
> > -#define DRIVER_AUTHOR                  "Allen Hubbe <Allen.Hubbe@xxxxxxx>"
> > +#define DRIVER_NAME            "ntb_pingpong"
> > +#define DRIVER_VERSION         "2.0"
> >
> > -MODULE_LICENSE(DRIVER_LICENSE);
> > +MODULE_LICENSE("Dual BSD/GPL");
> >  MODULE_VERSION(DRIVER_VERSION);
> > -MODULE_AUTHOR(DRIVER_AUTHOR);
> > -MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
> > +MODULE_AUTHOR("Allen Hubbe <Allen.Hubbe@xxxxxxx>");
> > +MODULE_DESCRIPTION("PCIe NTB Simple Pingpong Client");
> 
> The changes above seem like unrelated, unnecessary cleanup.  If so,
> break it out into a separate patch.  Otherwise, code changes might get
> lost in the mess.
> 

Is it really necessary to have all of this info defined in macro? We don't use
them anywhere in the source below. I add this change here due to the last Greg
patch, where he removed DRIVER_LICENSE from the code. He said, that the license
check tool needs it to be directly defined in the MODULE_LICENSE. The rest of 
macro aren't used by any tool, but still there might be some utility in future
to collect all the driver authors or descriptions. Still if you think it is
unnecessary and it's ok to still have the macro left here, I'll get them back
to the drivers code.

> >
> >  static unsigned int unsafe;
> >  module_param(unsafe, uint, 0644);
> > @@ -86,237 +100,345 @@ static unsigned int delay_ms = 1000;
> >  module_param(delay_ms, uint, 0644);
> >  MODULE_PARM_DESC(delay_ms, "Milliseconds to delay the response to peer");
> >
> > -static unsigned long db_init = 0x7;
> > -module_param(db_init, ulong, 0644);
> > -MODULE_PARM_DESC(db_init, "Initial doorbell bits to ring on the peer");
> > -
> > -/* Only two-ports NTB devices are supported */
> > -#define PIDX           NTB_DEF_PEER_IDX
> > -
> >  struct pp_ctx {
> > -       struct ntb_dev                  *ntb;
> > -       u64                             db_bits;
> > -       /* synchronize access to db_bits by ping and pong */
> > -       spinlock_t                      db_lock;
> > -       struct timer_list               db_timer;
> > -       unsigned long                   db_delay;
> > -       struct dentry                   *debugfs_node_dir;
> > -       struct dentry                   *debugfs_count;
> > -       atomic_t                        count;
> > +       struct ntb_dev *ntb;
> > +       struct hrtimer timer;
> > +       u64 in_db;
> > +       u64 out_db;
> > +       int out_pidx;
> > +       u64 nmask;
> > +       u64 pmask;
> > +       atomic_t count;
> > +       spinlock_t lock;
> > +       struct dentry *dbgfs_dir;
> >  };
> > +#define to_pp_timer(__timer) \
> > +       container_of(__timer, struct pp_ctx, timer)
> >
> > -static struct dentry *pp_debugfs_dir;
> > +static struct dentry *pp_dbgfs_topdir;
> >
> > -static void pp_ping(struct timer_list *t)
> > +static int pp_find_next_peer(struct pp_ctx *pp)
> >  {
> > -       struct pp_ctx *pp = from_timer(pp, t, db_timer);
> > -       unsigned long irqflags;
> > -       u64 db_bits, db_mask;
> > -       u32 spad_rd, spad_wr;
> > +       u64 link, out_db;
> > +       int pidx;
> > +
> > +       link = ntb_link_is_up(pp->ntb, NULL, NULL);
> > +
> > +       /* Find next available peer */
> > +       if (link & pp->nmask) {
> > +               pidx = __ffs64(link & pp->nmask);
> > +               out_db = BIT_ULL(pidx + 1);
> > +       } else if (link & pp->pmask) {
> > +               pidx = __ffs64(link & pp->pmask);
> > +               out_db = BIT_ULL(pidx);
> > +       } else {
> > +               return -ENODEV;
> > +       }
> >
> > -       spin_lock_irqsave(&pp->db_lock, irqflags);
> > -       {
> > -               db_mask = ntb_db_valid_mask(pp->ntb);
> > -               db_bits = ntb_db_read(pp->ntb);
> > +       spin_lock(&pp->lock);
> > +       pp->out_pidx = pidx;
> > +       pp->out_db = out_db;
> > +       spin_unlock(&pp->lock);
> >
> > -               if (db_bits) {
> > -                       dev_dbg(&pp->ntb->dev,
> > -                               "Masked pongs %#llx\n",
> > -                               db_bits);
> > -                       ntb_db_clear(pp->ntb, db_bits);
> > -               }
> > +       return 0;
> > +}
> >
> > -               db_bits = ((pp->db_bits | db_bits) << 1) & db_mask;
> > +static void pp_setup(struct pp_ctx *pp)
> > +{
> > +       int ret;
> >
> > -               if (!db_bits)
> > -                       db_bits = db_init;
> > +       ntb_db_set_mask(pp->ntb, pp->in_db);
> >
> > -               spad_rd = ntb_spad_read(pp->ntb, 0);
> > -               spad_wr = spad_rd + 1;
> > +       hrtimer_cancel(&pp->timer);
> >
> > -               dev_dbg(&pp->ntb->dev,
> > -                       "Ping bits %#llx read %#x write %#x\n",
> > -                       db_bits, spad_rd, spad_wr);
> > +       ret = pp_find_next_peer(pp);
> > +       if (ret == -ENODEV) {
> 
> I would suggest any error (thus < 0)
> 

pp_find_next_peer() doesn't return any error except -ENODEV, so technically
it is ok to leave it as is. Additionally the definite value check can be seen
as some kind of code documentation. The only problem which I see here can happen
only if somebody changes the function definition, so it would return another
errors. I'd leave it as is for now, since positive sides seem to me more
valuable.

> > +               dev_dbg(&pp->ntb->dev, "Got no peers, so cancel\n");
> > +               return;
> > +       }
> >
> > -               ntb_peer_spad_write(pp->ntb, PIDX, 0, spad_wr);
> > -               ntb_peer_db_set(pp->ntb, db_bits);
> > -               ntb_db_clear_mask(pp->ntb, db_mask);
> > +       spin_lock(&pp->lock);
> > +       dev_dbg(&pp->ntb->dev, "Ping-pong started with port %d, db %#llx\n",
> > +               ntb_peer_port_number(pp->ntb, pp->out_pidx), pp->out_db);
> > +       spin_unlock(&pp->lock);
> 
> Ugh, I don't like this.  You are adding a side effect of the lock
> being potentially grabbed by someone else just for debugging that
> might not be enabled.
> 

Fair enough. I'll discard the lock/unlock.

> >
> > -               pp->db_bits = 0;
> > -       }
> > -       spin_unlock_irqrestore(&pp->db_lock, irqflags);
> > +       hrtimer_start(&pp->timer, ms_to_ktime(delay_ms), HRTIMER_MODE_REL);
> >  }
> >
> > -static void pp_link_event(void *ctx)
> > +static void pp_clear(struct pp_ctx *pp)
> >  {
> > -       struct pp_ctx *pp = ctx;
> > +       hrtimer_cancel(&pp->timer);
> >
> > -       if (ntb_link_is_up(pp->ntb, NULL, NULL) == 1) {
> > -               dev_dbg(&pp->ntb->dev, "link is up\n");
> > -               pp_ping(&pp->db_timer);
> > -       } else {
> > -               dev_dbg(&pp->ntb->dev, "link is down\n");
> > -               del_timer(&pp->db_timer);
> > -       }
> > +       ntb_db_set_mask(pp->ntb, pp->in_db);
> > +
> > +       dev_dbg(&pp->ntb->dev, "Ping-pong cancelled\n");
> >  }
> >
> > -static void pp_db_event(void *ctx, int vec)
> > +static void pp_ping(struct pp_ctx *pp)
> >  {
> > -       struct pp_ctx *pp = ctx;
> > -       u64 db_bits, db_mask;
> > -       unsigned long irqflags;
> > +       u32 count;
> >
> > -       spin_lock_irqsave(&pp->db_lock, irqflags);
> > -       {
> > -               db_mask = ntb_db_vector_mask(pp->ntb, vec);
> > -               db_bits = db_mask & ntb_db_read(pp->ntb);
> > -               ntb_db_set_mask(pp->ntb, db_mask);
> > -               ntb_db_clear(pp->ntb, db_bits);
> > +       count = atomic_read(&pp->count);
> >
> > -               pp->db_bits |= db_bits;
> > +       spin_lock(&pp->lock);
> > +       ntb_peer_spad_write(pp->ntb, pp->out_pidx, 0, count);
> > +       ntb_peer_msg_write(pp->ntb, pp->out_pidx, 0, count);
> >
> > -               mod_timer(&pp->db_timer, jiffies + pp->db_delay);
> > +       dev_dbg(&pp->ntb->dev, "Ping port %d spad %#x, msg %#x\n",
> > +               ntb_peer_port_number(pp->ntb, pp->out_pidx), count, count);
> >
> > -               dev_dbg(&pp->ntb->dev,
> > -                       "Pong vec %d bits %#llx\n",
> > -                       vec, db_bits);
> > -               atomic_inc(&pp->count);
> > -       }
> > -       spin_unlock_irqrestore(&pp->db_lock, irqflags);
> > +       ntb_peer_db_set(pp->ntb, pp->out_db);
> > +       ntb_db_clear_mask(pp->ntb, pp->in_db);
> > +       spin_unlock(&pp->lock);
> >  }
> >
> > -static int pp_debugfs_setup(struct pp_ctx *pp)
> > +static void pp_pong(struct pp_ctx *pp)
> >  {
> > -       struct pci_dev *pdev = pp->ntb->pdev;
> > +       u32 msg_data = -1, spad_data = -1;
> > +       int pidx = 0;
> >
> > -       if (!pp_debugfs_dir)
> > -               return -ENODEV;
> > +       /* Read pong data */
> > +       spad_data = ntb_spad_read(pp->ntb, 0);
> > +       msg_data = ntb_msg_read(pp->ntb, &pidx, 0);
> > +       ntb_msg_clear_sts(pp->ntb, -1);
> >
> > -       pp->debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
> > -                                                 pp_debugfs_dir);
> > -       if (!pp->debugfs_node_dir)
> > -               return -ENODEV;
> > +       /*
> > +        * Scratchpad and message data may differ, since message register can't
> > +        * be rewritten unless status is cleared. Additionally either of them
> > +        * might be unsupported
> > +        */
> > +       dev_dbg(&pp->ntb->dev, "Pong spad %#x, msg %#x (port %d)\n",
> > +               spad_data, msg_data, ntb_peer_port_number(pp->ntb, pidx));
> >
> > -       pp->debugfs_count = debugfs_create_atomic_t("count", S_IRUSR | S_IWUSR,
> > -                                                   pp->debugfs_node_dir,
> > -                                                   &pp->count);
> > -       if (!pp->debugfs_count)
> > -               return -ENODEV;
> > +       atomic_inc(&pp->count);
> >
> > -       return 0;
> > +       ntb_db_set_mask(pp->ntb, pp->in_db);
> > +       ntb_db_clear(pp->ntb, pp->in_db);
> > +
> > +       hrtimer_start(&pp->timer, ms_to_ktime(delay_ms), HRTIMER_MODE_REL);
> > +}
> > +
> > +static enum hrtimer_restart pp_timer_func(struct hrtimer *t)
> > +{
> > +       struct pp_ctx *pp = to_pp_timer(t);
> > +
> > +       pp_ping(pp);
> > +
> > +       return HRTIMER_NORESTART;
> > +}
> > +
> > +static void pp_link_event(void *ctx)
> > +{
> > +       struct pp_ctx *pp = ctx;
> > +
> > +       pp_setup(pp);
> > +}
> > +
> > +static void pp_db_event(void *ctx, int vec)
> > +{
> > +       struct pp_ctx *pp = ctx;
> > +
> > +       pp_pong(pp);
> >  }
> >
> >  static const struct ntb_ctx_ops pp_ops = {
> >         .link_event = pp_link_event,
> > -       .db_event = pp_db_event,
> > +       .db_event = pp_db_event
> >  };
> >
> > -static int pp_probe(struct ntb_client *client,
> > -                   struct ntb_dev *ntb)
> > +static int pp_check_ntb(struct ntb_dev *ntb)
> >  {
> > -       struct pp_ctx *pp;
> > -       int rc;
> > +       u64 pmask;
> >
> >         if (ntb_db_is_unsafe(ntb)) {
> > -               dev_dbg(&ntb->dev, "doorbell is unsafe\n");
> > -               if (!unsafe) {
> > -                       rc = -EINVAL;
> > -                       goto err_pp;
> > -               }
> > -       }
> > -
> > -       if (ntb_spad_count(ntb) < 1) {
> > -               dev_dbg(&ntb->dev, "no enough scratchpads\n");
> > -               rc = -EINVAL;
> > -               goto err_pp;
> > +               dev_dbg(&ntb->dev, "Doorbell is unsafe\n");
> > +               if (!unsafe)
> > +                       return -EINVAL;
> >         }
> >
> >         if (ntb_spad_is_unsafe(ntb)) {
> > -               dev_dbg(&ntb->dev, "scratchpad is unsafe\n");
> > -               if (!unsafe) {
> > -                       rc = -EINVAL;
> > -                       goto err_pp;
> > -               }
> > +               dev_dbg(&ntb->dev, "Scratchpad is unsafe\n");
> > +               if (!unsafe)
> > +                       return -EINVAL;
> >         }
> >
> > -       if (ntb_peer_port_count(ntb) != NTB_DEF_PEER_CNT)
> > -               dev_warn(&ntb->dev, "multi-port NTB is unsupported\n");
> > +       pmask = GENMASK_ULL(ntb_peer_port_count(ntb), 0);
> > +       if ((ntb_db_valid_mask(ntb) & pmask) != pmask) {
> > +               dev_err(&ntb->dev, "Unsupported DB configuration\n");
> > +               return -EINVAL;
> > +       }
> >
> > -       pp = kmalloc(sizeof(*pp), GFP_KERNEL);
> > -       if (!pp) {
> > -               rc = -ENOMEM;
> > -               goto err_pp;
> > +       if (ntb_spad_count(ntb) < 1 && ntb_msg_count(ntb) < 1) {
> > +               dev_dbg(&ntb->dev, "Scratchpads and messages unsupported\n");
> > +               return -EINVAL;
> > +       } else if (ntb_spad_count(ntb) < 1) {
> > +               dev_dbg(&ntb->dev, "Scratchpads unsupported\n");
> > +       } else if (ntb_msg_count(ntb) < 1) {
> > +               dev_dbg(&ntb->dev, "Messages unsupported\n");
> 
> Should these be warnings or errors?
> 

The last two should be debug prints. It's ok if one of them unsupported,
some of NTB devices can provides Messages only, some work with Scratchpads.
As far as I know there is only one NTB device, which supports both of them
on hardware level. It's IDT® 89HPES8NT2, 89HPES16NT2, 89HPES16NT2, 89HPES12NT3,
89HPES24NT3 series, which isn't supported by the current IDT driver, since
I don't have it.
But it's bug if neither of the registers supported. NTB API even doesn't allow
such devices registration. In this case we should print an error. I'll replace
the first dev_dbg with dev_err then.

> >         }
> >
> > +       return 0;
> > +}
> > +
> > +static struct pp_ctx *pp_create_data(struct ntb_dev *ntb)
> > +{
> > +       struct pp_ctx *pp;
> > +
> > +       pp = devm_kzalloc(&ntb->dev, sizeof(*pp), GFP_KERNEL);
> > +       if (!pp)
> > +               return ERR_PTR(-ENOMEM);
> > +
> >         pp->ntb = ntb;
> > -       pp->db_bits = 0;
> >         atomic_set(&pp->count, 0);
> > -       spin_lock_init(&pp->db_lock);
> > -       timer_setup(&pp->db_timer, pp_ping, 0);
> > -       pp->db_delay = msecs_to_jiffies(delay_ms);
> > +       spin_lock_init(&pp->lock);
> > +       hrtimer_init(&pp->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +       pp->timer.function = pp_timer_func;
> >
> > -       rc = ntb_set_ctx(ntb, pp, &pp_ops);
> > -       if (rc)
> > -               goto err_ctx;
> > +       return pp;
> > +}
> > +
> > +static void pp_init_flds(struct pp_ctx *pp)
> > +{
> > +       int pidx, lport, pcnt;
> > +
> > +       /* Find global port index */
> > +       lport = ntb_port_number(pp->ntb);
> > +       pcnt = ntb_peer_port_count(pp->ntb);
> > +       for (pidx = 0; pidx < pcnt; pidx++) {
> > +               if (lport < ntb_peer_port_number(pp->ntb, pidx))
> > +                       break;
> > +       }
> >
> > -       rc = pp_debugfs_setup(pp);
> > -       if (rc)
> > -               goto err_ctx;
> > +       pp->in_db = BIT_ULL(pidx);
> > +       pp->pmask = GENMASK_ULL(pidx, 0) >> 1;
> > +       pp->nmask = GENMASK_ULL(pcnt - 1, pidx);
> >
> > -       ntb_link_enable(ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> > -       ntb_link_event(ntb);
> > +       dev_dbg(&pp->ntb->dev, "Inbound db %#llx, prev %#llx, next %#llx\n",
> > +               pp->in_db, pp->pmask, pp->nmask);
> > +}
> > +
> > +static int pp_mask_events(struct pp_ctx *pp)
> > +{
> > +       u64 db_mask, msg_mask;
> > +       int ret;
> > +
> > +       db_mask = ntb_db_valid_mask(pp->ntb);
> > +       ret = ntb_db_set_mask(pp->ntb, db_mask);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Skip message events masking if unsupported */
> > +       if (ntb_msg_count(pp->ntb) < 1)
> > +               return 0;
> > +
> > +       msg_mask = ntb_msg_outbits(pp->ntb) | ntb_msg_inbits(pp->ntb);
> > +       return ntb_msg_set_mask(pp->ntb, msg_mask);
> > +}
> > +
> > +static int pp_setup_ctx(struct pp_ctx *pp)
> > +{
> > +       int ret;
> > +
> > +       ret = ntb_set_ctx(pp->ntb, pp, &pp_ops);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ntb_link_enable(pp->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> > +       /* Might be not necessary */
> > +       ntb_link_event(pp->ntb);
> >
> >         return 0;
> > +}
> > +
> > +static void pp_clear_ctx(struct pp_ctx *pp)
> > +{
> > +       ntb_link_disable(pp->ntb);
> >
> > -err_ctx:
> > -       kfree(pp);
> > -err_pp:
> > -       return rc;
> > +       ntb_clear_ctx(pp->ntb);
> >  }
> >
> > -static void pp_remove(struct ntb_client *client,
> > -                     struct ntb_dev *ntb)
> > +static void pp_setup_dbgfs(struct pp_ctx *pp)
> > +{
> > +       struct pci_dev *pdev = pp->ntb->pdev;
> > +       void *ret;
> > +
> > +       pp->dbgfs_dir = debugfs_create_dir(pci_name(pdev), pp_dbgfs_topdir);
> > +
> > +       ret = debugfs_create_atomic_t("count", 0600, pp->dbgfs_dir, &pp->count);
> > +       if (!ret)
> > +               dev_warn(&pp->ntb->dev, "DebugFS unsupported\n");
> 
> Shouldn't this be fatal?
> 

If only I had the record of all the comments I have got on the DebugFS
initialization code.) Everyone had different opinion on this.
Last time I submitted a patch to driver/misc subsystem, where Grag K-H is
reviewer. He said, that DebugFS got just one purpose - for debugging. A driver
isn't supposed to fail in case if the FS isn't available in the kernel. I had to
rewrite the code in the same way he said. I assume the Ping-Pong driver shall
still work if DebugFS isn't available, but the user won't be able to see how
many pings have been made. For such circumstances I added the warning to inform
the user about this. So lets leave the code as is. Although I'd accept such the
comment on ntb_tool/ntb_perf drivers, since they get to be useless without
DebugFS support in the kernel.

> > +}
> > +
> > +static void pp_clear_dbgfs(struct pp_ctx *pp)
> > +{
> > +       debugfs_remove_recursive(pp->dbgfs_dir);
> > +}
> > +
> > +static int pp_probe(struct ntb_client *client, struct ntb_dev *ntb)
> > +{
> > +       struct pp_ctx *pp;
> > +       int ret;
> > +
> > +       ret = pp_check_ntb(ntb);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pp = pp_create_data(ntb);
> > +       if (IS_ERR(pp))
> > +               return PTR_ERR(pp);
> > +
> > +       pp_init_flds(pp);
> > +
> > +       ret = pp_mask_events(pp);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = pp_setup_ctx(pp);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pp_setup_dbgfs(pp);
> > +
> > +       return 0;
> > +}
> > +
> > +static void pp_remove(struct ntb_client *client, struct ntb_dev *ntb)
> >  {
> >         struct pp_ctx *pp = ntb->ctx;
> >
> > -       debugfs_remove_recursive(pp->debugfs_node_dir);
> > +       pp_clear_dbgfs(pp);
> >
> > -       ntb_clear_ctx(ntb);
> > -       del_timer_sync(&pp->db_timer);
> > -       ntb_link_disable(ntb);
> > +       pp_clear_ctx(pp);
> >
> > -       kfree(pp);
> > +       pp_clear(pp);
> >  }
> >
> >  static struct ntb_client pp_client = {
> >         .ops = {
> >                 .probe = pp_probe,
> > -               .remove = pp_remove,
> > -       },
> > +               .remove = pp_remove
> > +       }
> >  };
> >
> >  static int __init pp_init(void)
> >  {
> > -       int rc;
> > +       int ret;
> >
> >         if (debugfs_initialized())
> > -               pp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > +               pp_dbgfs_topdir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> >
> > -       rc = ntb_register_client(&pp_client);
> > -       if (rc)
> > -               goto err_client;
> > +       ret = ntb_register_client(&pp_client);
> > +       if (ret)
> > +               debugfs_remove_recursive(pp_dbgfs_topdir);
> >
> > -       return 0;
> > -
> > -err_client:
> > -       debugfs_remove_recursive(pp_debugfs_dir);
> > -       return rc;
> > +       return ret;
> >  }
> >  module_init(pp_init);
> >
> >  static void __exit pp_exit(void)
> >  {
> >         ntb_unregister_client(&pp_client);
> > -       debugfs_remove_recursive(pp_debugfs_dir);
> > +       debugfs_remove_recursive(pp_dbgfs_topdir);
> >  }
> >  module_exit(pp_exit);
> > +
> > --
> > 2.12.0
> >