Re: [PATCH 6/8] ntb_tool: Add link status file to debugfs

[Logan Gunthorpe]

Hey Allen,

Thanks for the feedback it's a bit more complicated but I don't object 
to that. I'll work something up on Monday.

I was trying to avoid adding link controls, but if we do, would you say 
the module should still enable the link when it's installed? Or would we 
have the user explicitly have to enable the link before using it?

Thanks,

Logan

On 10/06/16 08:27 PM, Allen Hubbe wrote:
> On Fri, Jun 10, 2016 at 6:54 PM, Logan Gunthorpe <logang@xxxxxxxxxxxx> wrote:
> > In order to more successfully script with ntb_tool it's useful to
> > have a link file to check the link status so that the script
> > doesn't use the other files until the link is up.
> >
> > This commit adds a 'link' file to the debugfs directory which reads
> > 0 or 1 depending on the link status. For scripting convenience, writing
> > will block until the link is up (discarding anything that was written).
> >
> > Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> > ---
> >   drivers/ntb/test/ntb_tool.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> > index 954e1d5..116352e 100644
> > --- a/drivers/ntb/test/ntb_tool.c
> > +++ b/drivers/ntb/test/ntb_tool.c
> > @@ -59,6 +59,12 @@
> >    *
> >    * Eg: check if clearing the doorbell mask generates an interrupt.
> >    *
> > + * # Check the link status
> > + * root@self# cat $DBG_DIR/link
> > + *
> > + * # Block until the link is up
> > + * root@self# echo > $DBG_DIR/link
>
> I think a file to get and set the link status is a good idea, but the
> way it is done as proposed here is not in a similar style to other
> ntb_tool operations.  Other operations simply read a register and
> format the value, or scan a value and write a register.  Similarly, I
> think the link status could be done in the same way: use the read file
> operation to get the current status with ntb_link_is_up(), and use the
> file write operation to enable or disable the link with
> ntb_link_enable() and ntb_link_disable().
>
> Waiting for link status is an interesting concept, too.  Really, one
> might be interested in a change in link status, whether up or down.
> What about a link event file that supports write to arm the event, and
> read to block for the event.  Consider an implementation based on
> <linux/completion.h>.  It would be used in combination with the link
> status file, above, as follows.
>
> 1: Write 1 to the event file.  This arms the event.
>    - The event will be disarmed by the next tool_link_event().
>
> 2: The application may read the link status file if it is interested
> in waiting for a particular event.
>
> 3. The application may wait for an event by reading the event file
>    - The application will wait as long as the event is still armed.
>    - If the event was disarmed before waiting, the application will not block.
>
> 4. The application should read the link status again.
>
> In any case, I think it would be more expected and natural to block
> while reading a file versus writing it.
>
> > + *
> >    * # Set the doorbell mask
> >    * root@self# echo 's 1' > $DBG_DIR/mask
> >    *
> > @@ -127,6 +133,7 @@ struct tool_ctx {
> >          struct work_struct link_cleanup;
> >          bool link_is_up;
> >          struct delayed_work link_work;
> > +       wait_queue_head_t link_wq;
> >          int mw_count;
> >          struct tool_mw mws[MAX_MWS];
> >   };
> > @@ -237,6 +244,7 @@ static void tool_link_work(struct work_struct *work)
> >                          "Error setting up memory windows: %d\n", rc);
> >
> >          tc->link_is_up = true;
> > +       wake_up(&tc->link_wq);
> >   }
> >
> >   static void tool_link_cleanup(struct work_struct *work)
> > @@ -573,6 +581,39 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
> >                        tool_peer_spad_read,
> >                        tool_peer_spad_write);
> >
> > +static ssize_t tool_link_read(struct file *filep, char __user *ubuf,
> > +                             size_t size, loff_t *offp)
> > +{
> > +       struct tool_ctx *tc = filep->private_data;
> > +       char *buf;
> > +       ssize_t pos, rc;
> > +
> > +       buf = kmalloc(64, GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       pos = scnprintf(buf, 64, "%d\n", tc->link_is_up);
> > +       rc = simple_read_from_buffer(ubuf, size, offp, buf, pos);
> > +
> > +       kfree(buf);
> > +
> > +       return rc;
> > +}
> > +
> > +static ssize_t tool_link_write(struct file *filep, const char __user *ubuf,
> > +                              size_t size, loff_t *offp)
> > +{
> > +       struct tool_ctx *tc = filep->private_data;
> > +
> > +       if (wait_event_interruptible(tc->link_wq, tc->link_is_up))
> > +               return -ERESTART;
> > +
> > +       return size;
> > +}
> > +
> > +static TOOL_FOPS_RDWR(tool_link_fops,
> > +                     tool_link_read,
> > +                     tool_link_write);
> >
> >   static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
> >                              size_t size, loff_t *offp)
> > @@ -708,6 +749,9 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
> >          debugfs_create_file("peer_spad", S_IRUSR | S_IWUSR, tc->dbgfs,
> >                              tc, &tool_peer_spad_fops);
> >
> > +       debugfs_create_file("link", S_IRUSR | S_IWUSR, tc->dbgfs,
> > +                           tc, &tool_link_fops);
> > +
> >          mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
> >          for (i = 0; i < mw_count; i++) {
> >                  char buf[30];
> > @@ -741,6 +785,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
> >          }
> >
> >          tc->ntb = ntb;
> > +       init_waitqueue_head(&tc->link_wq);
> >          INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
> >          INIT_WORK(&tc->link_cleanup, tool_link_cleanup);