From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751320AbcFKC14 (ORCPT ); Fri, 10 Jun 2016 22:27:56 -0400 Received: from mail-yw0-f196.google.com ([209.85.161.196]:36784 "EHLO mail-yw0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750820AbcFKC1y (ORCPT ); Fri, 10 Jun 2016 22:27:54 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Allen Hubbe Date: Fri, 10 Jun 2016 22:27:53 -0400 Message-ID: Subject: Re: [PATCH 6/8] ntb_tool: Add link status file to debugfs To: Logan Gunthorpe Cc: Jon Mason , Dave Jiang , Shuah Khan , Sudip Mukherjee , Arnd Bergmann , linux-kernel@vger.kernel.org, linux-ntb@googlegroups.com, linux-kselftest@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 10, 2016 at 6:54 PM, Logan Gunthorpe 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 > --- > 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 . 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);