[PATCH v2 0/8] NTB Selftest Script

[PATCH v2 0/8] NTB Selftest Script

[Logan Gunthorpe]

Changes since v1:

1) Add a comment to explain the *15 in the buf size calculation,
as per Allen's feedback.

2) Clean up the changes to the pingpong client as there were some
sloppy copying mistakes.

3) Rework the 'link' file in ntb_tool as per Allen's suggestions.
I've added a 'link_event' file the works essentially how he's asked.
Though, I found no need to use a completion as suggested and the flow
is maybe slightly simpler than he's suggested. Just write a boolean
to the event file then read to wait for the link to be either up or
down. There's still some discussion on the best interface and it's
not much work to make additional minor functional changes.

4) Update the selftest script to use the new 'link_event' file.

5) Minor change to the way the selftest script lists devices thanks to
Allen's observation.

---

I've written a ntb_test.sh script that would probably be useful if it
were included in the kernel. This series ends with that script and
includes some useful interface improvements and fixes to the existing ntb
test modules. Please see each individual commit for more information.
They are mostly independent.

The series is based off of v4.6 plus the patches I've submitted that
have been accepted into ntb-next. They've been run through checkpatch
with --strict this time.

As always, I'm happy to incorporate any feedback.

Thanks,

Logan

---

Logan Gunthorpe (8):
  ntb_perf: Schedule based on time not on performance
  ntb_perf: Improve thread handling to increase robustness
  ntb_perf: Return results by reading the run file
  ntb_perf: Wait for link before running test
  ntb_tool: BUG: Ensure the buffer size is large enough to return all
    spads
  ntb_tool: Add link status and files to debugfs
  ntb_pingpong: Add a debugfs file to get the ping count
  ntb_test: Add a selftest script for the NTB subsystem

 MAINTAINERS                             |   1 +
 drivers/ntb/test/ntb_perf.c             | 196 +++++++++++-----
 drivers/ntb/test/ntb_pingpong.c         |  62 +++++-
 drivers/ntb/test/ntb_tool.c             | 121 +++++++++-
 tools/testing/selftests/ntb/ntb_test.sh | 384 ++++++++++++++++++++++++++++++++
 5 files changed, 700 insertions(+), 64 deletions(-)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh

--
2.1.4

[PATCH v2 1/8] ntb_perf: Schedule based on time not on performance

[Logan Gunthorpe]

When debugging performance problems, if some issue causes the ntb
hardware to be significantly slower than expected, ntb_perf will
hang requiring a reboot because it only schedules once every 4GB.

Instead, schedule based on jiffies so it will not hang the CPU if
the transfer is slow.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/ntb/test/ntb_perf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 4368519..5008ccf 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -273,6 +273,7 @@ static int perf_move_data(struct pthr_ctx *pctx, char __iomem *dst, char *src,
 	char __iomem *tmp = dst;
 	u64 perf, diff_us;
 	ktime_t kstart, kstop, kdiff;
+	unsigned long last_sleep = jiffies;
 
 	chunks = div64_u64(win_size, buf_size);
 	total_chunks = div64_u64(total, buf_size);
@@ -288,8 +289,9 @@ static int perf_move_data(struct pthr_ctx *pctx, char __iomem *dst, char *src,
 		} else
 			tmp += buf_size;
 
-		/* Probably should schedule every 4GB to prevent soft hang. */
-		if (((copied % SZ_4G) == 0) && !use_dma) {
+		/* Probably should schedule every 5s to prevent soft hang. */
+		if (unlikely((jiffies - last_sleep) > 5 * HZ)) {
+			last_sleep = jiffies;
 			set_current_state(TASK_INTERRUPTIBLE);
 			schedule_timeout(1);
 		}
-- 
2.1.4

[PATCH v2 2/8] ntb_perf: Improve thread handling to increase robustness

[Logan Gunthorpe]

This commit accomplishes a few things:

1) Properly prevent multiple sets of threads from running at once using
a mutex. Lots of race issues existed with the thread_cleanup.

2) The mutex allows us to ensure that threads are finished before
tearing down the device or module.

3) Don't use kthread_stop when the threads can exit by themselves, as
this is counter-indicated by the kthread_create documentation. Threads
now wait for kthread_stop to occur.

4) Writing to the run file now blocks until the threads are complete.
The test can then be safely interrupted by a SIGINT.

Also, while I was at it:

5) debugfs_run_write shouldn't return 0 in the early check cases as this
could cause debugfs_run_write to loop undesirably.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/ntb/test/ntb_perf.c | 124 +++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 48 deletions(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 5008ccf..db4dc61 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -58,6 +58,7 @@
 #include <linux/delay.h>
 #include <linux/sizes.h>
 #include <linux/ntb.h>
+#include <linux/mutex.h>
 
 #define DRIVER_NAME		"ntb_perf"
 #define DRIVER_DESCRIPTION	"PCIe NTB Performance Measurement Tool"
@@ -121,6 +122,7 @@ struct pthr_ctx {
 	int			dma_prep_err;
 	int			src_idx;
 	void			*srcs[MAX_SRCS];
+	wait_queue_head_t       *wq;
 };
 
 struct perf_ctx {
@@ -134,9 +136,11 @@ struct perf_ctx {
 	struct dentry		*debugfs_run;
 	struct dentry		*debugfs_threads;
 	u8			perf_threads;
-	bool			run;
+	/* mutex ensures only one set of threads run at once */
+	struct mutex		run_mutex;
 	struct pthr_ctx		pthr_ctx[MAX_THREADS];
 	atomic_t		tsync;
+	atomic_t                tdone;
 };
 
 enum {
@@ -295,12 +299,18 @@ static int perf_move_data(struct pthr_ctx *pctx, char __iomem *dst, char *src,
 			set_current_state(TASK_INTERRUPTIBLE);
 			schedule_timeout(1);
 		}
+
+		if (unlikely(kthread_should_stop()))
+			break;
 	}
 
 	if (use_dma) {
 		pr_info("%s: All DMA descriptors submitted\n", current->comm);
-		while (atomic_read(&pctx->dma_sync) != 0)
+		while (atomic_read(&pctx->dma_sync) != 0) {
+			if (kthread_should_stop())
+				break;
 			msleep(20);
+		}
 	}
 
 	kstop = ktime_get();
@@ -393,7 +403,10 @@ static int ntb_perf_thread(void *data)
 		pctx->srcs[i] = NULL;
 	}
 
-	return 0;
+	atomic_inc(&perf->tdone);
+	wake_up(pctx->wq);
+	rc = 0;
+	goto done;
 
 err:
 	for (i = 0; i < MAX_SRCS; i++) {
@@ -406,6 +419,16 @@ err:
 		pctx->dma_chan = NULL;
 	}
 
+done:
+	/* Wait until we are told to stop */
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (kthread_should_stop())
+			break;
+		schedule();
+	}
+	__set_current_state(TASK_RUNNING);
+
 	return rc;
 }
 
@@ -553,6 +576,7 @@ static ssize_t debugfs_run_read(struct file *filp, char __user *ubuf,
 	struct perf_ctx *perf = filp->private_data;
 	char *buf;
 	ssize_t ret, out_offset;
+	int running;
 
 	if (!perf)
 		return 0;
@@ -560,7 +584,9 @@ static ssize_t debugfs_run_read(struct file *filp, char __user *ubuf,
 	buf = kmalloc(64, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
-	out_offset = snprintf(buf, 64, "%d\n", perf->run);
+
+	running = mutex_is_locked(&perf->run_mutex);
+	out_offset = snprintf(buf, 64, "%d\n", running);
 	ret = simple_read_from_buffer(ubuf, count, offp, buf, out_offset);
 	kfree(buf);
 
@@ -572,7 +598,6 @@ static void threads_cleanup(struct perf_ctx *perf)
 	struct pthr_ctx *pctx;
 	int i;
 
-	perf->run = false;
 	for (i = 0; i < MAX_THREADS; i++) {
 		pctx = &perf->pthr_ctx[i];
 		if (pctx->thread) {
@@ -587,65 +612,66 @@ static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
 {
 	struct perf_ctx *perf = filp->private_data;
 	int node, i;
+	DECLARE_WAIT_QUEUE_HEAD(wq);
 
 	if (!perf->link_is_up)
-		return 0;
+		return -ENOLINK;
 
 	if (perf->perf_threads == 0)
-		return 0;
+		return -EINVAL;
 
-	if (atomic_read(&perf->tsync) == 0)
-		perf->run = false;
+	if (!mutex_trylock(&perf->run_mutex))
+		return -EBUSY;
 
-	if (perf->run)
-		threads_cleanup(perf);
-	else {
-		perf->run = true;
+	if (perf->perf_threads > MAX_THREADS) {
+		perf->perf_threads = MAX_THREADS;
+		pr_info("Reset total threads to: %u\n", MAX_THREADS);
+	}
 
-		if (perf->perf_threads > MAX_THREADS) {
-			perf->perf_threads = MAX_THREADS;
-			pr_info("Reset total threads to: %u\n", MAX_THREADS);
-		}
+	/* no greater than 1M */
+	if (seg_order > MAX_SEG_ORDER) {
+		seg_order = MAX_SEG_ORDER;
+		pr_info("Fix seg_order to %u\n", seg_order);
+	}
 
-		/* no greater than 1M */
-		if (seg_order > MAX_SEG_ORDER) {
-			seg_order = MAX_SEG_ORDER;
-			pr_info("Fix seg_order to %u\n", seg_order);
-		}
+	if (run_order < seg_order) {
+		run_order = seg_order;
+		pr_info("Fix run_order to %u\n", run_order);
+	}
 
-		if (run_order < seg_order) {
-			run_order = seg_order;
-			pr_info("Fix run_order to %u\n", run_order);
-		}
+	node = dev_to_node(&perf->ntb->pdev->dev);
+	atomic_set(&perf->tdone, 0);
 
-		node = dev_to_node(&perf->ntb->pdev->dev);
-		/* launch kernel thread */
-		for (i = 0; i < perf->perf_threads; i++) {
-			struct pthr_ctx *pctx;
-
-			pctx = &perf->pthr_ctx[i];
-			atomic_set(&pctx->dma_sync, 0);
-			pctx->perf = perf;
-			pctx->thread =
-				kthread_create_on_node(ntb_perf_thread,
-						       (void *)pctx,
-						       node, "ntb_perf %d", i);
-			if (IS_ERR(pctx->thread)) {
-				pctx->thread = NULL;
-				goto err;
-			} else
-				wake_up_process(pctx->thread);
-
-			if (perf->run == false)
-				return -ENXIO;
-		}
+	/* launch kernel thread */
+	for (i = 0; i < perf->perf_threads; i++) {
+		struct pthr_ctx *pctx;
 
+		pctx = &perf->pthr_ctx[i];
+		atomic_set(&pctx->dma_sync, 0);
+		pctx->perf = perf;
+		pctx->wq = &wq;
+		pctx->thread =
+			kthread_create_on_node(ntb_perf_thread,
+					       (void *)pctx,
+					       node, "ntb_perf %d", i);
+		if (IS_ERR(pctx->thread)) {
+			pctx->thread = NULL;
+			goto err;
+		} else {
+			wake_up_process(pctx->thread);
+		}
 	}
 
+	wait_event_interruptible(wq,
+		atomic_read(&perf->tdone) == perf->perf_threads);
+
+	threads_cleanup(perf);
+	mutex_unlock(&perf->run_mutex);
 	return count;
 
 err:
 	threads_cleanup(perf);
+	mutex_unlock(&perf->run_mutex);
 	return -ENXIO;
 }
 
@@ -713,7 +739,7 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 	perf->ntb = ntb;
 	perf->perf_threads = 1;
 	atomic_set(&perf->tsync, 0);
-	perf->run = false;
+	mutex_init(&perf->run_mutex);
 	spin_lock_init(&perf->db_lock);
 	perf_setup_mw(ntb, perf);
 	INIT_DELAYED_WORK(&perf->link_work, perf_link_work);
@@ -748,6 +774,8 @@ static void perf_remove(struct ntb_client *client, struct ntb_dev *ntb)
 
 	dev_dbg(&perf->ntb->dev, "%s called\n", __func__);
 
+	mutex_lock(&perf->run_mutex);
+
 	cancel_delayed_work_sync(&perf->link_work);
 	cancel_work_sync(&perf->link_cleanup);
 
-- 
2.1.4

[PATCH v2 3/8] ntb_perf: Return results by reading the run file

[Logan Gunthorpe]

Instead of having to watch logs, allow the results to be retrieved
by reading back the run file. This file will return "running" when
the test is running and nothing if no tests have been run yet.
It returns 1 line per thread, and will display an error message if the
corresponding thread returns an error.

With the above change, the pr_info calls that returned the results are
then changed to pr_debug calls.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/ntb/test/ntb_perf.c | 67 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index db4dc61..05a8705 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -123,6 +123,9 @@ struct pthr_ctx {
 	int			src_idx;
 	void			*srcs[MAX_SRCS];
 	wait_queue_head_t       *wq;
+	int			status;
+	u64			copied;
+	u64			diff_us;
 };
 
 struct perf_ctx {
@@ -305,7 +308,7 @@ static int perf_move_data(struct pthr_ctx *pctx, char __iomem *dst, char *src,
 	}
 
 	if (use_dma) {
-		pr_info("%s: All DMA descriptors submitted\n", current->comm);
+		pr_debug("%s: All DMA descriptors submitted\n", current->comm);
 		while (atomic_read(&pctx->dma_sync) != 0) {
 			if (kthread_should_stop())
 				break;
@@ -317,13 +320,16 @@ static int perf_move_data(struct pthr_ctx *pctx, char __iomem *dst, char *src,
 	kdiff = ktime_sub(kstop, kstart);
 	diff_us = ktime_to_us(kdiff);
 
-	pr_info("%s: copied %llu bytes\n", current->comm, copied);
+	pr_debug("%s: copied %llu bytes\n", current->comm, copied);
 
-	pr_info("%s: lasted %llu usecs\n", current->comm, diff_us);
+	pr_debug("%s: lasted %llu usecs\n", current->comm, diff_us);
 
 	perf = div64_u64(copied, diff_us);
 
-	pr_info("%s: MBytes/s: %llu\n", current->comm, perf);
+	pr_debug("%s: MBytes/s: %llu\n", current->comm, perf);
+
+	pctx->copied = copied;
+	pctx->diff_us = diff_us;
 
 	return 0;
 }
@@ -345,7 +351,7 @@ static int ntb_perf_thread(void *data)
 	int rc, node, i;
 	struct dma_chan *dma_chan = NULL;
 
-	pr_info("kthread %s starting...\n", current->comm);
+	pr_debug("kthread %s starting...\n", current->comm);
 
 	node = dev_to_node(&pdev->dev);
 
@@ -575,19 +581,44 @@ static ssize_t debugfs_run_read(struct file *filp, char __user *ubuf,
 {
 	struct perf_ctx *perf = filp->private_data;
 	char *buf;
-	ssize_t ret, out_offset;
-	int running;
+	ssize_t ret, out_off = 0;
+	struct pthr_ctx *pctx;
+	int i;
+	u64 rate;
 
 	if (!perf)
 		return 0;
 
-	buf = kmalloc(64, GFP_KERNEL);
+	buf = kmalloc(1024, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	running = mutex_is_locked(&perf->run_mutex);
-	out_offset = snprintf(buf, 64, "%d\n", running);
-	ret = simple_read_from_buffer(ubuf, count, offp, buf, out_offset);
+	if (mutex_is_locked(&perf->run_mutex)) {
+		out_off = snprintf(buf, 64, "running\n");
+		goto read_from_buf;
+	}
+
+	for (i = 0; i < MAX_THREADS; i++) {
+		pctx = &perf->pthr_ctx[i];
+
+		if (pctx->status == -ENODATA)
+			break;
+
+		if (pctx->status) {
+			out_off += snprintf(buf + out_off, 1024 - out_off,
+					    "%d: error %d\n", i,
+					    pctx->status);
+			continue;
+		}
+
+		rate = div64_u64(pctx->copied, pctx->diff_us);
+		out_off += snprintf(buf + out_off, 1024 - out_off,
+			"%d: copied %llu bytes in %llu usecs, %llu MBytes/s\n",
+			i, pctx->copied, pctx->diff_us, rate);
+	}
+
+read_from_buf:
+	ret = simple_read_from_buffer(ubuf, count, offp, buf, out_off);
 	kfree(buf);
 
 	return ret;
@@ -601,12 +632,20 @@ static void threads_cleanup(struct perf_ctx *perf)
 	for (i = 0; i < MAX_THREADS; i++) {
 		pctx = &perf->pthr_ctx[i];
 		if (pctx->thread) {
-			kthread_stop(pctx->thread);
+			pctx->status = kthread_stop(pctx->thread);
 			pctx->thread = NULL;
 		}
 	}
 }
 
+static void perf_clear_thread_status(struct perf_ctx *perf)
+{
+	int i;
+
+	for (i = 0; i < MAX_THREADS; i++)
+		perf->pthr_ctx[i].status = -ENODATA;
+}
+
 static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
 				 size_t count, loff_t *offp)
 {
@@ -623,6 +662,8 @@ static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
 	if (!mutex_trylock(&perf->run_mutex))
 		return -EBUSY;
 
+	perf_clear_thread_status(perf);
+
 	if (perf->perf_threads > MAX_THREADS) {
 		perf->perf_threads = MAX_THREADS;
 		pr_info("Reset total threads to: %u\n", MAX_THREADS);
@@ -757,6 +798,8 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 	if (rc)
 		goto err_ctx;
 
+	perf_clear_thread_status(perf);
+
 	return 0;
 
 err_ctx:
-- 
2.1.4

[PATCH v2 4/8] ntb_perf: Wait for link before running test

[Logan Gunthorpe]

Instead of returning immediately with an error when the link is
down, wait for the link to come up (or the user sends a SIGINT).

This is to make scripting ntb_perf easier.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/ntb/test/ntb_perf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 05a8705..f0784e5 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -135,6 +135,7 @@ struct perf_ctx {
 	bool			link_is_up;
 	struct work_struct	link_cleanup;
 	struct delayed_work	link_work;
+	wait_queue_head_t	link_wq;
 	struct dentry		*debugfs_node_dir;
 	struct dentry		*debugfs_run;
 	struct dentry		*debugfs_threads;
@@ -533,6 +534,7 @@ static void perf_link_work(struct work_struct *work)
 		goto out1;
 
 	perf->link_is_up = true;
+	wake_up(&perf->link_wq);
 
 	return;
 
@@ -653,7 +655,7 @@ static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
 	int node, i;
 	DECLARE_WAIT_QUEUE_HEAD(wq);
 
-	if (!perf->link_is_up)
+	if (wait_event_interruptible(perf->link_wq, perf->link_is_up))
 		return -ENOLINK;
 
 	if (perf->perf_threads == 0)
@@ -783,6 +785,7 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 	mutex_init(&perf->run_mutex);
 	spin_lock_init(&perf->db_lock);
 	perf_setup_mw(ntb, perf);
+	init_waitqueue_head(&perf->link_wq);
 	INIT_DELAYED_WORK(&perf->link_work, perf_link_work);
 	INIT_WORK(&perf->link_cleanup, perf_link_cleanup);
 
-- 
2.1.4

[PATCH v2 5/8] ntb_tool: BUG: Ensure the buffer size is large enough to return all spads

[Logan Gunthorpe]

On hardware with 32 scratchpad registers the spad field in ntb tool
could chop off the end. The maximum buffer size is increased from
256 to 15 times the number or scratchpads.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>
---
 drivers/ntb/test/ntb_tool.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
index 4c01057..cba31fd 100644
--- a/drivers/ntb/test/ntb_tool.c
+++ b/drivers/ntb/test/ntb_tool.c
@@ -368,7 +368,14 @@ static ssize_t tool_spadfn_read(struct tool_ctx *tc, char __user *ubuf,
 	if (!spad_read_fn)
 		return -EINVAL;
 
-	buf_size = min_t(size_t, size, 0x100);
+	spad_count = ntb_spad_count(tc->ntb);
+
+	/*
+	 * We multiply the number of spads by 15 to get the buffer size
+	 * this is from 3 for the %d, 10 for the largest hex value
+	 * (0x00000000) and 2 for the tab and line feed.
+	 */
+	buf_size = min_t(size_t, size, spad_count * 15);
 
 	buf = kmalloc(buf_size, GFP_KERNEL);
 	if (!buf)
@@ -376,7 +383,6 @@ static ssize_t tool_spadfn_read(struct tool_ctx *tc, char __user *ubuf,
 
 	pos = 0;
 
-	spad_count = ntb_spad_count(tc->ntb);
 	for (i = 0; i < spad_count; ++i) {
 		pos += scnprintf(buf + pos, buf_size - pos, "%d\t%#x\n",
 				 i, spad_read_fn(tc->ntb, i));
-- 
2.1.4

[PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Logan Gunthorpe]

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 a
boolean (Y or N) depending on the link status. Writing to the file will
change the link state using ntb_link_enable or ntb_link_disable.

A 'link_event' file is also provided so an application can block until
the link changes to a desired state. This file is primed by writing a
boolean. If the user writes a 1, the next read of link_event will
block until the link is up. If the user writes a 0, the next read
will block until the link is down. Besides blocking, reads return the
same value as the 'link' file.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/ntb/test/ntb_tool.c | 111 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
index cba31fd..9bebd0d 100644
--- a/drivers/ntb/test/ntb_tool.c
+++ b/drivers/ntb/test/ntb_tool.c
@@ -59,6 +59,13 @@
  *
  * 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 Y > $DBG_DIR/link_event
+ * root@self# cat $DBG_DIR/link_event
+ *
  * # Set the doorbell mask
  * root@self# echo 's 1' > $DBG_DIR/mask
  *
@@ -126,7 +133,9 @@ struct tool_ctx {
 	struct dentry *dbgfs;
 	struct work_struct link_cleanup;
 	bool link_is_up;
+	bool link_event;
 	struct delayed_work link_work;
+	wait_queue_head_t link_wq;
 	int mw_count;
 	struct tool_mw mws[MAX_MWS];
 };
@@ -237,6 +246,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)
@@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
 
 	if (!tc->link_is_up)
 		cancel_delayed_work_sync(&tc->link_work);
+
+	tc->link_is_up = false;
+	wake_up(&tc->link_wq);
 }
 
 static void tool_link_event(void *ctx)
@@ -578,6 +591,95 @@ 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[3];
+
+	buf[0] = tc->link_is_up ? 'Y' : 'N';
+	buf[1] = '\n';
+	buf[2] = '\0';
+
+	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
+}
+
+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;
+	char buf[32];
+	size_t buf_size;
+	bool val;
+	int rc;
+
+	buf_size = min(size, (sizeof(buf) - 1));
+	if (copy_from_user(buf, ubuf, buf_size))
+		return -EFAULT;
+
+	buf[buf_size] = '\0';
+
+	rc = strtobool(buf, &val);
+	if (rc)
+		return rc;
+
+	if (val)
+		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
+	else
+		ntb_link_disable(tc->ntb);
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_link_fops,
+		      tool_link_read,
+		      tool_link_write);
+
+static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
+				    size_t size, loff_t *offp)
+{
+	struct tool_ctx *tc = filep->private_data;
+	char buf[3];
+
+	if (wait_event_interruptible(tc->link_wq,
+				     tc->link_is_up == tc->link_event))
+		return -ERESTART;
+
+	buf[0] = tc->link_is_up ? 'Y' : 'N';
+	buf[1] = '\n';
+	buf[2] = '\0';
+
+	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
+}
+
+static ssize_t tool_link_event_write(struct file *filep,
+				     const char __user *ubuf,
+				     size_t size, loff_t *offp)
+{
+	struct tool_ctx *tc = filep->private_data;
+	char buf[32];
+	size_t buf_size;
+	bool val;
+	int rc;
+
+	buf_size = min(size, (sizeof(buf) - 1));
+	if (copy_from_user(buf, ubuf, buf_size))
+		return -EFAULT;
+
+	buf[buf_size] = '\0';
+
+	rc = strtobool(buf, &val);
+	if (rc)
+		return rc;
+
+	tc->link_event = val;
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_link_event_fops,
+		      tool_link_event_read,
+		      tool_link_event_write);
 
 static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
 			    size_t size, loff_t *offp)
@@ -658,7 +760,6 @@ static TOOL_FOPS_RDWR(tool_mw_fops,
 		      tool_mw_read,
 		      tool_mw_write);
 
-
 static ssize_t tool_peer_mw_read(struct file *filep, char __user *ubuf,
 				   size_t size, loff_t *offp)
 {
@@ -713,6 +814,12 @@ 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);
+
+	debugfs_create_file("link_event", S_IRUSR | S_IWUSR, tc->dbgfs,
+			    tc, &tool_link_event_fops);
+
 	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
 	for (i = 0; i < mw_count; i++) {
 		char buf[30];
@@ -746,8 +853,10 @@ 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);
+	tc->link_event = true;
 
 	tool_setup_dbgfs(tc);
 
-- 
2.1.4

[PATCH v2 7/8] ntb_pingpong: Add a debugfs file to get the ping count

[Logan Gunthorpe]

This commit adds a debugfs 'count' file to ntb_pingpong. This is so
testing with ntb_pingpong can be automated beyond just checking the
logs for pong messages.

The count file returns a number which increments every pong. The
counter can be cleared by writing a zero.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/ntb/test/ntb_pingpong.c | 62 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/test/ntb_pingpong.c b/drivers/ntb/test/ntb_pingpong.c
index fe16005..7d31179 100644
--- a/drivers/ntb/test/ntb_pingpong.c
+++ b/drivers/ntb/test/ntb_pingpong.c
@@ -61,6 +61,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/debugfs.h>
 
 #include <linux/ntb.h>
 
@@ -96,8 +97,13 @@ struct pp_ctx {
 	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;
 };
 
+static struct dentry *pp_debugfs_dir;
+
 static void pp_ping(unsigned long ctx)
 {
 	struct pp_ctx *pp = (void *)ctx;
@@ -171,10 +177,32 @@ static void pp_db_event(void *ctx, int vec)
 		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);
 }
 
+static int pp_debugfs_setup(struct pp_ctx *pp)
+{
+	struct pci_dev *pdev = pp->ntb->pdev;
+
+	if (!pp_debugfs_dir)
+		return -ENODEV;
+
+	pp->debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
+						  pp_debugfs_dir);
+	if (!pp->debugfs_node_dir)
+		return -ENODEV;
+
+	pp->debugfs_count = debugfs_create_atomic_t("count", S_IRUSR | S_IWUSR,
+						    pp->debugfs_node_dir,
+						    &pp->count);
+	if (!pp->debugfs_count)
+		return -ENODEV;
+
+	return 0;
+}
+
 static const struct ntb_ctx_ops pp_ops = {
 	.link_event = pp_link_event,
 	.db_event = pp_db_event,
@@ -210,6 +238,7 @@ static int pp_probe(struct ntb_client *client,
 
 	pp->ntb = ntb;
 	pp->db_bits = 0;
+	atomic_set(&pp->count, 0);
 	spin_lock_init(&pp->db_lock);
 	setup_timer(&pp->db_timer, pp_ping, (unsigned long)pp);
 	pp->db_delay = msecs_to_jiffies(delay_ms);
@@ -218,6 +247,10 @@ static int pp_probe(struct ntb_client *client,
 	if (rc)
 		goto err_ctx;
 
+	rc = pp_debugfs_setup(pp);
+	if (rc)
+		goto err_ctx;
+
 	ntb_link_enable(ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
 	ntb_link_event(ntb);
 
@@ -234,6 +267,8 @@ static void pp_remove(struct ntb_client *client,
 {
 	struct pp_ctx *pp = ntb->ctx;
 
+	debugfs_remove_recursive(pp->debugfs_node_dir);
+
 	ntb_clear_ctx(ntb);
 	del_timer_sync(&pp->db_timer);
 	ntb_link_disable(ntb);
@@ -247,4 +282,29 @@ static struct ntb_client pp_client = {
 		.remove = pp_remove,
 	},
 };
-module_ntb_client(pp_client);
+
+static int __init pp_init(void)
+{
+	int rc;
+
+	if (debugfs_initialized())
+		pp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
+
+	rc = ntb_register_client(&pp_client);
+	if (rc)
+		goto err_client;
+
+	return 0;
+
+err_client:
+	debugfs_remove_recursive(pp_debugfs_dir);
+	return rc;
+}
+module_init(pp_init);
+
+static void __exit pp_exit(void)
+{
+	ntb_unregister_client(&pp_client);
+	debugfs_remove_recursive(pp_debugfs_dir);
+}
+module_exit(pp_exit);
-- 
2.1.4

[PATCH v2 8/8] ntb_test: Add a selftest script for the NTB subsystem

[Logan Gunthorpe]

This script automates testing doorbells, scratchpads and memory windows
for an NTB device. It can be run locally, with the NTB looped
back to the same host or use SSH to remotely control the second host.

In the single host case, the script just needs to be passed two
arguments: a PCI ID for each side of the link. In the two host case
the -r option must be used to specify the remote hostname (which must
be SSH accessible and should probably have ssh-keys exchanged).

A sample run looks like this:

$ sudo ./ntb_test.sh 0000:03:00.1 0000:83:00.1 -p 29
Starting ntb_tool tests...
Running db tests on: 0000:03:00.1 / 0000:83:00.1
  Passed
Running db tests on: 0000:83:00.1 / 0000:03:00.1
  Passed
Running spad tests on: 0000:03:00.1 / 0000:83:00.1
  Passed
Running spad tests on: 0000:83:00.1 / 0000:03:00.1
  Passed
Running mw0 tests on: 0000:03:00.1 / 0000:83:00.1
  Passed
Running mw0 tests on: 0000:83:00.1 / 0000:03:00.1
  Passed
Running mw1 tests on: 0000:03:00.1 / 0000:83:00.1
  Passed
Running mw1 tests on: 0000:83:00.1 / 0000:03:00.1
  Passed

Starting ntb_pingpong tests...
Running ping pong tests on: 0000:03:00.1 / 0000:83:00.1
  Passed

Starting ntb_perf tests...
Running local perf test without DMA
  0: copied 536870912 bytes in 238205 usecs, 2253 MBytes/s
  Passed
Running remote perf test without DMA
  0: copied 536870912 bytes in 238205 usecs, 2253 MBytes/s
  Passed

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 MAINTAINERS                             |   1 +
 tools/testing/selftests/ntb/ntb_test.sh | 384 ++++++++++++++++++++++++++++++++
 2 files changed, 385 insertions(+)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 9c567a4..f178e7e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7846,6 +7846,7 @@ F:	drivers/ntb/
 F:	drivers/net/ntb_netdev.c
 F:	include/linux/ntb.h
 F:	include/linux/ntb_transport.h
+F:	tools/testing/selftests/ntb/
 
 NTB INTEL DRIVER
 M:	Jon Mason <jdmason@kudzu.us>
diff --git a/tools/testing/selftests/ntb/ntb_test.sh b/tools/testing/selftests/ntb/ntb_test.sh
new file mode 100755
index 0000000..4422d6c
--- /dev/null
+++ b/tools/testing/selftests/ntb/ntb_test.sh
@@ -0,0 +1,384 @@
+#!/bin/bash
+# Copyright (c) 2016 Microsemi. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# Author: Logan Gunthorpe <logang@deltatee.com>
+
+REMOTE_HOST=
+LIST_DEVS=FALSE
+
+DEBUGFS=${DEBUGFS-/sys/kernel/debug}
+
+PERF_RUN_ORDER=32
+MAX_MW_SIZE=0
+RUN_DMA_TESTS=
+DONT_CLEANUP=
+
+function show_help()
+{
+	echo "Usage: $0 [OPTIONS] LOCAL_DEV REMOTE_DEV"
+	echo "Run tests on a pair of NTB endpoints."
+	echo
+	echo "If the NTB device loops back to the same host then,"
+	echo "just specifying the two PCI ids on the command line is"
+	echo "sufficient. Otherwise, if the NTB link spans two hosts"
+	echo "use the -r option to specify the hostname for the remote"
+	echo "device. SSH will then be used to test the remote side."
+	echo "An SSH key between the root users of the host would then"
+	echo "be highly recommended."
+	echo
+	echo "Options:"
+	echo "  -C              don't cleanup ntb modules on exit"
+	echo "  -d              run dma tests"
+	echo "  -h              show this help message"
+	echo "  -l              list available local and remote PCI ids"
+	echo "  -r REMOTE_HOST  specify the remote's hostname to connect"
+        echo "                  to for the test (using ssh)"
+	echo "  -p NUM          ntb_perf run order (default: $PERF_RUN_ORDER)"
+	echo "  -w max_mw_size  maxmium memory window size"
+	echo
+}
+
+function parse_args()
+{
+	OPTIND=0
+	while getopts "Cdhlr:p:w:" opt; do
+		case "$opt" in
+		C)  DONT_CLEANUP=1 ;;
+		d)  RUN_DMA_TESTS=1 ;;
+		h)  show_help; exit 0 ;;
+		l)  LIST_DEVS=TRUE ;;
+		r)  REMOTE_HOST=${OPTARG} ;;
+		p)  PERF_RUN_ORDER=${OPTARG} ;;
+		w)  MAX_MW_SIZE=${OPTARG} ;;
+		\?)
+		    echo "Invalid option: -$OPTARG" >&2
+		    exit 1
+		    ;;
+		esac
+	done
+}
+
+parse_args "$@"
+shift $((OPTIND-1))
+LOCAL_DEV=$1
+shift
+parse_args "$@"
+shift $((OPTIND-1))
+REMOTE_DEV=$1
+shift
+parse_args "$@"
+
+set -e
+
+function _modprobe()
+{
+        modprobe "$@"
+}
+
+function split_remote()
+{
+	VPATH=$1
+	REMOTE=
+
+	if [[ "$VPATH" == *":/"* ]]; then
+		REMOTE=${VPATH%%:*}
+		VPATH=${VPATH#*:}
+	fi
+}
+
+function read_file()
+{
+	split_remote $1
+	if [[ "$REMOTE" != "" ]]; then
+		ssh "$REMOTE" cat "$VPATH"
+	else
+		cat "$VPATH"
+	fi
+}
+
+function write_file()
+{
+	split_remote $2
+	VALUE=$1
+
+	if [[ "$REMOTE" != "" ]]; then
+		ssh "$REMOTE" "echo \"$VALUE\" > \"$VPATH\""
+	else
+		echo "$VALUE" > "$VPATH"
+	fi
+}
+
+function doorbell_test()
+{
+	LOC=$1
+	REM=$2
+	EXP=0
+
+	echo "Running db tests on: $(basename $LOC) / $(basename $REM)"
+
+	write_file "c 0xFFFFFFFF" "$REM/db"
+
+	for ((i=1; i <= 8; i++)); do
+		let DB=$(read_file "$REM/db") || true
+		if [[ "$DB" != "$EXP" ]]; then
+			echo "Doorbell doesn't match expected value $EXP " \
+			     "in $REM/db" >&2
+			exit -1
+		fi
+
+		let "MASK=1 << ($i-1)" || true
+		let "EXP=$EXP | $MASK" || true
+		write_file "s $MASK" "$LOC/peer_db"
+	done
+
+	echo "  Passed"
+}
+
+function read_spad()
+{
+       VPATH=$1
+       IDX=$2
+
+       ROW=($(read_file "$VPATH" | grep -e "^$IDX"))
+       let VAL=${ROW[1]} || true
+       echo $VAL
+}
+
+function scratchpad_test()
+{
+	LOC=$1
+	REM=$2
+	CNT=$(read_file "$LOC/spad" | wc -l)
+
+	echo "Running spad tests on: $(basename $LOC) / $(basename $REM)"
+
+	for ((i = 0; i < $CNT; i++)); do
+		VAL=$RANDOM
+		write_file "$i $VAL" "$LOC/peer_spad"
+		RVAL=$(read_spad "$REM/spad" $i)
+
+		if [[ "$VAL" != "$RVAL" ]]; then
+			echo "Scratchpad doesn't match expected value $VAL " \
+			     "in $REM/spad, got $RVAL" >&2
+			exit -1
+		fi
+
+	done
+
+	echo "  Passed"
+}
+
+function write_mw()
+{
+	split_remote $2
+
+	if [[ "$REMOTE" != "" ]]; then
+		ssh "$REMOTE" \
+			dd if=/dev/urandom "of=$VPATH" 2> /dev/null || true
+	else
+		dd if=/dev/urandom "of=$VPATH" 2> /dev/null || true
+	fi
+}
+
+function mw_test()
+{
+	IDX=$1
+	LOC=$2
+	REM=$3
+
+	echo "Running $IDX tests on: $(basename $LOC) / $(basename $REM)"
+
+	write_mw "$LOC/$IDX"
+
+	split_remote "$LOC/$IDX"
+	if [[ "$REMOTE" == "" ]]; then
+		A=$VPATH
+	else
+		A=/tmp/ntb_test.$$.A
+		ssh "$REMOTE" cat "$VPATH" > "$A"
+	fi
+
+	split_remote "$REM/peer_$IDX"
+	if [[ "$REMOTE" == "" ]]; then
+		B=$VPATH
+	else
+		B=/tmp/ntb_test.$$.B
+		ssh "$REMOTE" cat "$VPATH" > "$B"
+	fi
+
+	cmp "$A" "$B"
+	if [[ $? != 0 ]]; then
+		echo "Memory window $MW did not match!" >&2
+	fi
+
+	if [[ "$A" == "/tmp/*" ]]; then
+		rm "$A"
+	fi
+
+	if [[ "$B" == "/tmp/*" ]]; then
+		rm "$B"
+	fi
+
+	echo "  Passed"
+}
+
+function pingpong_test()
+{
+	LOC=$1
+	REM=$2
+
+	echo "Running ping pong tests on: $(basename $LOC) / $(basename $REM)"
+
+	LOC_START=$(read_file $LOC/count)
+	REM_START=$(read_file $REM/count)
+
+	sleep 7
+
+	LOC_END=$(read_file $LOC/count)
+	REM_END=$(read_file $REM/count)
+
+	if [[ $LOC_START == $LOC_END ]] || [[ $REM_START == $REM_END ]]; then
+		echo "Ping pong counter not incrementing!" >&2
+		exit 1
+	fi
+
+	echo "  Passed"
+}
+
+function perf_test()
+{
+	USE_DMA=$1
+
+	if [[ $USE_DMA == "1" ]]; then
+		WITH="with"
+	else
+		WITH="without"
+	fi
+
+	_modprobe ntb_perf run_order=$PERF_RUN_ORDER \
+		max_mw_size=$MAX_MW_SIZE use_dma=$USE_DMA
+
+	echo "Running local perf test $WITH DMA"
+	write_file "" $LOCAL_PERF/run
+	echo -n "  "
+	read_file $LOCAL_PERF/run
+	echo "  Passed"
+
+	echo "Running remote perf test $WITH DMA"
+	write_file "" $REMOTE_PERF/run
+	echo -n "  "
+	read_file $LOCAL_PERF/run
+	echo "  Passed"
+
+	_modprobe -r ntb_perf
+}
+
+function ntb_tool_tests()
+{
+	LOCAL_TOOL=$DEBUGFS/ntb_tool/$LOCAL_DEV
+	REMOTE_TOOL=$REMOTE_HOST:$DEBUGFS/ntb_tool/$REMOTE_DEV
+
+	echo "Starting ntb_tool tests..."
+
+	_modprobe ntb_tool
+
+	echo Y > $LOCAL_TOOL/link_event
+	cat $LOCAL_TOOL/link_event > /dev/null
+
+	doorbell_test $LOCAL_TOOL $REMOTE_TOOL
+	doorbell_test $REMOTE_TOOL $LOCAL_TOOL
+	scratchpad_test $LOCAL_TOOL $REMOTE_TOOL
+	scratchpad_test $REMOTE_TOOL $LOCAL_TOOL
+
+	for MW in $(ls $LOCAL_TOOL/mw*); do
+		MW=$(basename $MW)
+		mw_test $MW $LOCAL_TOOL $REMOTE_TOOL
+		mw_test $MW $REMOTE_TOOL $LOCAL_TOOL
+	done
+
+	_modprobe -r ntb_tool
+}
+
+function ntb_pingpong_tests()
+{
+	LOCAL_PP=$DEBUGFS/ntb_pingpong/$LOCAL_DEV
+	REMOTE_PP=$REMOTE_HOST:$DEBUGFS/ntb_pingpong/$REMOTE_DEV
+
+	echo "Starting ntb_pingpong tests..."
+
+	_modprobe ntb_pingpong
+
+	pingpong_test $LOCAL_PP $REMOTE_PP
+
+	_modprobe -r ntb_pingpong
+}
+
+function ntb_perf_tests()
+{
+	LOCAL_PERF=$DEBUGFS/ntb_perf/$LOCAL_DEV
+	REMOTE_PERF=$REMOTE_HOST:$DEBUGFS/ntb_perf/$REMOTE_DEV
+
+	echo "Starting ntb_perf tests..."
+
+	perf_test 0
+
+	if [[ $RUN_DMA_TESTS ]]; then
+		perf_test 1
+	fi
+}
+
+function cleanup()
+{
+	set +e
+	_modprobe -r ntb_tool 2> /dev/null
+	_modprobe -r ntb_perf 2> /dev/null
+	_modprobe -r ntb_pingpong 2> /dev/null
+	_modprobe -r ntb_transport 2> /dev/null
+	set -e
+}
+
+cleanup
+
+if ! [[ $$DONT_CLEANUP ]]; then
+	trap cleanup EXIT
+fi
+
+if [ "$(id -u)" != "0" ]; then
+	echo "This script must be run as root" 1>&2
+	exit 1
+fi
+
+if [[ "$LIST_DEVS" == TRUE ]]; then
+	echo "Local Devices:"
+	ls -1 /sys/bus/ntb/devices
+	echo
+
+	if [[ "$REMOTE_HOST" != "" ]]; then
+		echo "Remote Devices:"
+		ssh $REMOTE_HOST ls -1 /sys/bus/ntb/devices
+	fi
+
+	exit 0
+fi
+
+if [[ "$LOCAL_DEV" == $"" ]] || [[ "$REMOTE_DEV" == $"" ]]; then
+	show_help
+	exit 1
+fi
+
+ntb_tool_tests
+echo
+ntb_pingpong_tests
+echo
+ntb_perf_tests
+echo
-- 
2.1.4

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> 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 a
> boolean (Y or N) depending on the link status. Writing to the file will
> change the link state using ntb_link_enable or ntb_link_disable.
> 
> A 'link_event' file is also provided so an application can block until
> the link changes to a desired state. This file is primed by writing a
> boolean. If the user writes a 1, the next read of link_event will
> block until the link is up. If the user writes a 0, the next read
> will block until the link is down. Besides blocking, reads return the
> same value as the 'link' file.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/ntb/test/ntb_tool.c | 111 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index cba31fd..9bebd0d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -59,6 +59,13 @@
>   *
>   * 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 Y > $DBG_DIR/link_event
> + * root@self# cat $DBG_DIR/link_event
> + *
>   * # Set the doorbell mask
>   * root@self# echo 's 1' > $DBG_DIR/mask
>   *
> @@ -126,7 +133,9 @@ struct tool_ctx {
>  	struct dentry *dbgfs;
>  	struct work_struct link_cleanup;
>  	bool link_is_up;

Really, link_is_up means "memory windows are configured."  This comes from your earlier patch that introduced memory windows to ntb_tool.

> +	bool link_event;
>  	struct delayed_work link_work;
> +	wait_queue_head_t link_wq;
>  	int mw_count;
>  	struct tool_mw mws[MAX_MWS];
>  };
> @@ -237,6 +246,7 @@ static void tool_link_work(struct work_struct *work)
>  			"Error setting up memory windows: %d\n", rc);
> 
>  	tc->link_is_up = true;

In other words, "memory windows are configured" = true.

> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_link_cleanup(struct work_struct *work)
> @@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
> 
>  	if (!tc->link_is_up)
>  		cancel_delayed_work_sync(&tc->link_work);
> +
> +	tc->link_is_up = false;

If this was never set false anywhere in the patch that added memory windows, I wonder if there is a bug.

> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_link_event(void *ctx)
> @@ -578,6 +591,95 @@ 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[3];
> +
> +	buf[0] = tc->link_is_up ? 'Y' : 'N';

I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> +}
> +
> +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;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (val)
> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> +	else
> +		ntb_link_disable(tc->ntb);
> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_fops,
> +		      tool_link_read,
> +		      tool_link_write);
> +
> +static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
> +				    size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[3];
> +
> +	if (wait_event_interruptible(tc->link_wq,
> +				     tc->link_is_up == tc->link_event))

I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

> +		return -ERESTART;
> +
> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> +}
> +
> +static ssize_t tool_link_event_write(struct file *filep,
> +				     const char __user *ubuf,
> +				     size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	tc->link_event = val;

All writing the event file does is set the value of tc->link_event, so we have the same value that was set when reading the file.  It's rather inefficient, and oops, what if some other script comes along and writes a different value?  If script-A wants to wait for link up, and the link is already up, really it should not wait.  But if script-B changes tc->link_event to wait for link down before script-A reads the file, then script-A will incorrectly wait.

Really, I think the best thing after all would be just to wait here in the write function.

> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_event_fops,
> +		      tool_link_event_read,
> +		      tool_link_event_write);
> 
>  static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>  			    size_t size, loff_t *offp)
> @@ -658,7 +760,6 @@ static TOOL_FOPS_RDWR(tool_mw_fops,
>  		      tool_mw_read,
>  		      tool_mw_write);
> 
> -
>  static ssize_t tool_peer_mw_read(struct file *filep, char __user *ubuf,
>  				   size_t size, loff_t *offp)
>  {
> @@ -713,6 +814,12 @@ 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);
> +
> +	debugfs_create_file("link_event", S_IRUSR | S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_event_fops);
> +
>  	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
>  	for (i = 0; i < mw_count; i++) {
>  		char buf[30];
> @@ -746,8 +853,10 @@ 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);
> +	tc->link_event = true;
> 
>  	tool_setup_dbgfs(tc);
> 
> --
> 2.1.4

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> 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 a
> boolean (Y or N) depending on the link status. Writing to the file will
> change the link state using ntb_link_enable or ntb_link_disable.
> 
> A 'link_event' file is also provided so an application can block until
> the link changes to a desired state. This file is primed by writing a
> boolean. If the user writes a 1, the next read of link_event will
> block until the link is up. If the user writes a 0, the next read
> will block until the link is down. Besides blocking, reads return the
> same value as the 'link' file.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/ntb/test/ntb_tool.c | 111 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index cba31fd..9bebd0d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -59,6 +59,13 @@
>   *
>   * 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 Y > $DBG_DIR/link_event
> + * root@self# cat $DBG_DIR/link_event
> + *
>   * # Set the doorbell mask
>   * root@self# echo 's 1' > $DBG_DIR/mask
>   *
> @@ -126,7 +133,9 @@ struct tool_ctx {
>  	struct dentry *dbgfs;
>  	struct work_struct link_cleanup;
>  	bool link_is_up;

Really, link_is_up means "memory windows are configured."  This comes from your earlier patch that introduced memory windows to ntb_tool.

> +	bool link_event;
>  	struct delayed_work link_work;
> +	wait_queue_head_t link_wq;
>  	int mw_count;
>  	struct tool_mw mws[MAX_MWS];
>  };
> @@ -237,6 +246,7 @@ static void tool_link_work(struct work_struct *work)
>  			"Error setting up memory windows: %d\n", rc);
> 
>  	tc->link_is_up = true;

In other words, "memory windows are configured" = true.

> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_link_cleanup(struct work_struct *work)
> @@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
> 
>  	if (!tc->link_is_up)
>  		cancel_delayed_work_sync(&tc->link_work);
> +
> +	tc->link_is_up = false;

If this was never set false anywhere in the patch that added memory windows, I wonder if there is a bug.

> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_link_event(void *ctx)
> @@ -578,6 +591,95 @@ 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[3];
> +
> +	buf[0] = tc->link_is_up ? 'Y' : 'N';

I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> +}
> +
> +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;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (val)
> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> +	else
> +		ntb_link_disable(tc->ntb);
> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_fops,
> +		      tool_link_read,
> +		      tool_link_write);
> +
> +static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
> +				    size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[3];
> +
> +	if (wait_event_interruptible(tc->link_wq,
> +				     tc->link_is_up == tc->link_event))

I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

> +		return -ERESTART;
> +
> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> +}
> +
> +static ssize_t tool_link_event_write(struct file *filep,
> +				     const char __user *ubuf,
> +				     size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	tc->link_event = val;

All writing the event file does is set the value of tc->link_event, so we have the same value that was set when reading the file.  It's rather inefficient, and oops, what if some other script comes along and writes a different value?  If script-A wants to wait for link up, and the link is already up, really it should not wait.  But if script-B changes tc->link_event to wait for link down before script-A reads the file, then script-A will incorrectly wait.

Really, I think the best thing after all would be just to wait here in the write function.

> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_event_fops,
> +		      tool_link_event_read,
> +		      tool_link_event_write);
> 
>  static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>  			    size_t size, loff_t *offp)
> @@ -658,7 +760,6 @@ static TOOL_FOPS_RDWR(tool_mw_fops,
>  		      tool_mw_read,
>  		      tool_mw_write);
> 
> -
>  static ssize_t tool_peer_mw_read(struct file *filep, char __user *ubuf,
>  				   size_t size, loff_t *offp)
>  {
> @@ -713,6 +814,12 @@ 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);
> +
> +	debugfs_create_file("link_event", S_IRUSR | S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_event_fops);
> +
>  	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
>  	for (i = 0; i < mw_count; i++) {
>  		char buf[30];
> @@ -746,8 +853,10 @@ 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);
> +	tc->link_event = true;
> 
>  	tool_setup_dbgfs(tc);
> 
> --
> 2.1.4

Re: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Logan Gunthorpe]

On 14/06/16 01:33 PM, Allen Hubbe wrote:
>> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
>> index cba31fd..9bebd0d 100644
>> --- a/drivers/ntb/test/ntb_tool.c
>> +++ b/drivers/ntb/test/ntb_tool.c
>> @@ -59,6 +59,13 @@
>>   *
>>   * 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 Y > $DBG_DIR/link_event
>> + * root@self# cat $DBG_DIR/link_event
>> + *
>>   * # Set the doorbell mask
>>   * root@self# echo 's 1' > $DBG_DIR/mask
>>   *
>> @@ -126,7 +133,9 @@ struct tool_ctx {
>>  	struct dentry *dbgfs;
>>  	struct work_struct link_cleanup;
>>  	bool link_is_up;
> 
> Really, link_is_up means "memory windows are configured."  This comes from your earlier patch that introduced memory windows to ntb_tool.

Yes, this is technically true. However, I don't think the distinction is
necessary. The user only really cares whether everything is up and
usable -- not whether the link is just physically up or not.


>> +	bool link_event;
>>  	struct delayed_work link_work;
>> +	wait_queue_head_t link_wq;
>>  	int mw_count;
>>  	struct tool_mw mws[MAX_MWS];
>>  };
>> @@ -237,6 +246,7 @@ static void tool_link_work(struct work_struct *work)
>>  			"Error setting up memory windows: %d\n", rc);
>>
>>  	tc->link_is_up = true;
> 
> In other words, "memory windows are configured" = true.

Technically, yes.

>> +	wake_up(&tc->link_wq);
>>  }
>>
>>  static void tool_link_cleanup(struct work_struct *work)
>> @@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
>>
>>  	if (!tc->link_is_up)
>>  		cancel_delayed_work_sync(&tc->link_work);
>> +
>> +	tc->link_is_up = false;
> 
> If this was never set false anywhere in the patch that added memory windows, I wonder if there is a bug.

Yup, this looks like an oversight on my part. However, I don't think it
resulted in any noticeable bug seeing, at the time, the only way to
bring the link back down was to remove the module or the device. It is
only strictly necessary now that we have the 'link' file which can
control the link.

>> +	wake_up(&tc->link_wq);
>>  }
>>
>>  static void tool_link_event(void *ctx)
>> @@ -578,6 +591,95 @@ 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[3];
>> +
>> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> 
> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

I disagree. Bad things will happen if the user waits on the event and
then immediately uses the memory windows. It will just be buggy and
racy. I can't see a situation where the user would want to wait for the
link to come up and not have everything in ntb_tool ready and usable.

>> +	buf[1] = '\n';
>> +	buf[2] = '\0';
>> +
>> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
>> +}
>> +
>> +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;
>> +	char buf[32];
>> +	size_t buf_size;
>> +	bool val;
>> +	int rc;
>> +
>> +	buf_size = min(size, (sizeof(buf) - 1));
>> +	if (copy_from_user(buf, ubuf, buf_size))
>> +		return -EFAULT;
>> +
>> +	buf[buf_size] = '\0';
>> +
>> +	rc = strtobool(buf, &val);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (val)
>> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
>> +	else
>> +		ntb_link_disable(tc->ntb);
>> +
>> +	return size;
>> +}
>> +
>> +static TOOL_FOPS_RDWR(tool_link_fops,
>> +		      tool_link_read,
>> +		      tool_link_write);
>> +
>> +static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
>> +				    size_t size, loff_t *offp)
>> +{
>> +	struct tool_ctx *tc = filep->private_data;
>> +	char buf[3];
>> +
>> +	if (wait_event_interruptible(tc->link_wq,
>> +				     tc->link_is_up == tc->link_event))
> 
> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

See above.

>> +		return -ERESTART;
>> +
>> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
>> +	buf[1] = '\n';
>> +	buf[2] = '\0';
>> +
>> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
>> +}
>> +
>> +static ssize_t tool_link_event_write(struct file *filep,
>> +				     const char __user *ubuf,
>> +				     size_t size, loff_t *offp)
>> +{
>> +	struct tool_ctx *tc = filep->private_data;
>> +	char buf[32];
>> +	size_t buf_size;
>> +	bool val;
>> +	int rc;
>> +
>> +	buf_size = min(size, (sizeof(buf) - 1));
>> +	if (copy_from_user(buf, ubuf, buf_size))
>> +		return -EFAULT;
>> +
>> +	buf[buf_size] = '\0';
>> +
>> +	rc = strtobool(buf, &val);
>> +	if (rc)
>> +		return rc;
>> +
>> +	tc->link_event = val;
> 
> All writing the event file does is set the value of tc->link_event, so we have the same value that was set when reading the file.  It's rather inefficient, and oops, what if some other script comes along and writes a different value?  If script-A wants to wait for link up, and the link is already up, really it should not wait.  But if script-B changes tc->link_event to wait for link down before script-A reads the file, then script-A will incorrectly wait.
> 
> Really, I think the best thing after all would be just to wait here in the write function.

Yeah, I agree. It makes everything much simpler to block on the write. I
was going on your comment that it was more natural to block on the read.
I'll change this for v3. Are we happy to stick with the 'link' and
'link_event' files? Or do you like the 'link_wait' name better?


Logan

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> On 14/06/16 01:33 PM, Allen Hubbe wrote:
> >> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> >> index cba31fd..9bebd0d 100644
> >> --- a/drivers/ntb/test/ntb_tool.c
> >> +++ b/drivers/ntb/test/ntb_tool.c
> >> @@ -59,6 +59,13 @@
> >>   *
> >>   * 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 Y > $DBG_DIR/link_event
> >> + * root@self# cat $DBG_DIR/link_event
> >> + *
> >>   * # Set the doorbell mask
> >>   * root@self# echo 's 1' > $DBG_DIR/mask
> >>   *
> >> @@ -126,7 +133,9 @@ struct tool_ctx {
> >>  	struct dentry *dbgfs;
> >>  	struct work_struct link_cleanup;
> >>  	bool link_is_up;
> >
> > Really, link_is_up means "memory windows are configured."  This comes from your earlier
> patch that introduced memory windows to ntb_tool.
> 
> Yes, this is technically true. However, I don't think the distinction is
> necessary. The user only really cares whether everything is up and
> usable -- not whether the link is just physically up or not.
> 

The ntb_tool is intended to be a simple low level access to the ntb.h api.  As much as possible, I think ntb_tool should directly expose the ntb.h api through debugfs, and not invent higher level concepts.

> 
> >> +	bool link_event;
> >>  	struct delayed_work link_work;
> >> +	wait_queue_head_t link_wq;
> >>  	int mw_count;
> >>  	struct tool_mw mws[MAX_MWS];
> >>  };
> >> @@ -237,6 +246,7 @@ static void tool_link_work(struct work_struct *work)
> >>  			"Error setting up memory windows: %d\n", rc);
> >>
> >>  	tc->link_is_up = true;
> >
> > In other words, "memory windows are configured" = true.
> 
> Technically, yes.
> 
> >> +	wake_up(&tc->link_wq);
> >>  }
> >>
> >>  static void tool_link_cleanup(struct work_struct *work)
> >> @@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
> >>
> >>  	if (!tc->link_is_up)
> >>  		cancel_delayed_work_sync(&tc->link_work);
> >> +
> >> +	tc->link_is_up = false;
> >
> > If this was never set false anywhere in the patch that added memory windows, I wonder if
> there is a bug.
> 
> Yup, this looks like an oversight on my part. However, I don't think it
> resulted in any noticeable bug seeing, at the time, the only way to
> bring the link back down was to remove the module or the device. It is
> only strictly necessary now that we have the 'link' file which can
> control the link.

Even without a file to control the link, any one side could be unloaded and reloaded.  That also affects the link state on the side that stays loaded.  The side that stays loaded still needs to be sane when the link comes back up.

> 
> >> +	wake_up(&tc->link_wq);
> >>  }
> >>
> >>  static void tool_link_event(void *ctx)
> >> @@ -578,6 +591,95 @@ 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[3];
> >> +
> >> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> >
> > I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> 
> I disagree. Bad things will happen if the user waits on the event and
> then immediately uses the memory windows. It will just be buggy and
> racy. I can't see a situation where the user would want to wait for the
> link to come up and not have everything in ntb_tool ready and usable.

The memory windows can be configured prior to link up.  They can be configured when probing the device instead of waiting for link up.  Doing memory window configuration in probe would simplify the driver, and there would be no race.

> 
> >> +	buf[1] = '\n';
> >> +	buf[2] = '\0';
> >> +
> >> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> >> +}
> >> +
> >> +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;
> >> +	char buf[32];
> >> +	size_t buf_size;
> >> +	bool val;
> >> +	int rc;
> >> +
> >> +	buf_size = min(size, (sizeof(buf) - 1));
> >> +	if (copy_from_user(buf, ubuf, buf_size))
> >> +		return -EFAULT;
> >> +
> >> +	buf[buf_size] = '\0';
> >> +
> >> +	rc = strtobool(buf, &val);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	if (val)
> >> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> >> +	else
> >> +		ntb_link_disable(tc->ntb);
> >> +
> >> +	return size;
> >> +}
> >> +
> >> +static TOOL_FOPS_RDWR(tool_link_fops,
> >> +		      tool_link_read,
> >> +		      tool_link_write);
> >> +
> >> +static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
> >> +				    size_t size, loff_t *offp)
> >> +{
> >> +	struct tool_ctx *tc = filep->private_data;
> >> +	char buf[3];
> >> +
> >> +	if (wait_event_interruptible(tc->link_wq,
> >> +				     tc->link_is_up == tc->link_event))
> >
> > I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> 
> See above.
> 
> >> +		return -ERESTART;
> >> +
> >> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> >> +	buf[1] = '\n';
> >> +	buf[2] = '\0';
> >> +
> >> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> >> +}
> >> +
> >> +static ssize_t tool_link_event_write(struct file *filep,
> >> +				     const char __user *ubuf,
> >> +				     size_t size, loff_t *offp)
> >> +{
> >> +	struct tool_ctx *tc = filep->private_data;
> >> +	char buf[32];
> >> +	size_t buf_size;
> >> +	bool val;
> >> +	int rc;
> >> +
> >> +	buf_size = min(size, (sizeof(buf) - 1));
> >> +	if (copy_from_user(buf, ubuf, buf_size))
> >> +		return -EFAULT;
> >> +
> >> +	buf[buf_size] = '\0';
> >> +
> >> +	rc = strtobool(buf, &val);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	tc->link_event = val;
> >
> > All writing the event file does is set the value of tc->link_event, so we have the same
> value that was set when reading the file.  It's rather inefficient, and oops, what if some
> other script comes along and writes a different value?  If script-A wants to wait for link
> up, and the link is already up, really it should not wait.  But if script-B changes tc-
> >link_event to wait for link down before script-A reads the file, then script-A will
> incorrectly wait.
> >
> > Really, I think the best thing after all would be just to wait here in the write
> function.
> 
> Yeah, I agree. It makes everything much simpler to block on the write. I
> was going on your comment that it was more natural to block on the read.
> I'll change this for v3. Are we happy to stick with the 'link' and
> 'link_event' files? Or do you like the 'link_wait' name better?

The name link_event is ok.

> 
> 
> Logan
> 
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb"
> group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-
> ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-
> ntb/576070AE.10500%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> On 14/06/16 01:33 PM, Allen Hubbe wrote:
> >> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> >> index cba31fd..9bebd0d 100644
> >> --- a/drivers/ntb/test/ntb_tool.c
> >> +++ b/drivers/ntb/test/ntb_tool.c
> >> @@ -59,6 +59,13 @@
> >>   *
> >>   * 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 Y > $DBG_DIR/link_event
> >> + * root@self# cat $DBG_DIR/link_event
> >> + *
> >>   * # Set the doorbell mask
> >>   * root@self# echo 's 1' > $DBG_DIR/mask
> >>   *
> >> @@ -126,7 +133,9 @@ struct tool_ctx {
> >>  	struct dentry *dbgfs;
> >>  	struct work_struct link_cleanup;
> >>  	bool link_is_up;
> >
> > Really, link_is_up means "memory windows are configured."  This comes from your earlier
> patch that introduced memory windows to ntb_tool.
> 
> Yes, this is technically true. However, I don't think the distinction is
> necessary. The user only really cares whether everything is up and
> usable -- not whether the link is just physically up or not.
> 

The ntb_tool is intended to be a simple low level access to the ntb.h api.  As much as possible, I think ntb_tool should directly expose the ntb.h api through debugfs, and not invent higher level concepts.

> 
> >> +	bool link_event;
> >>  	struct delayed_work link_work;
> >> +	wait_queue_head_t link_wq;
> >>  	int mw_count;
> >>  	struct tool_mw mws[MAX_MWS];
> >>  };
> >> @@ -237,6 +246,7 @@ static void tool_link_work(struct work_struct *work)
> >>  			"Error setting up memory windows: %d\n", rc);
> >>
> >>  	tc->link_is_up = true;
> >
> > In other words, "memory windows are configured" = true.
> 
> Technically, yes.
> 
> >> +	wake_up(&tc->link_wq);
> >>  }
> >>
> >>  static void tool_link_cleanup(struct work_struct *work)
> >> @@ -246,6 +256,9 @@ static void tool_link_cleanup(struct work_struct *work)
> >>
> >>  	if (!tc->link_is_up)
> >>  		cancel_delayed_work_sync(&tc->link_work);
> >> +
> >> +	tc->link_is_up = false;
> >
> > If this was never set false anywhere in the patch that added memory windows, I wonder if
> there is a bug.
> 
> Yup, this looks like an oversight on my part. However, I don't think it
> resulted in any noticeable bug seeing, at the time, the only way to
> bring the link back down was to remove the module or the device. It is
> only strictly necessary now that we have the 'link' file which can
> control the link.

Even without a file to control the link, any one side could be unloaded and reloaded.  That also affects the link state on the side that stays loaded.  The side that stays loaded still needs to be sane when the link comes back up.

> 
> >> +	wake_up(&tc->link_wq);
> >>  }
> >>
> >>  static void tool_link_event(void *ctx)
> >> @@ -578,6 +591,95 @@ 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[3];
> >> +
> >> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> >
> > I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> 
> I disagree. Bad things will happen if the user waits on the event and
> then immediately uses the memory windows. It will just be buggy and
> racy. I can't see a situation where the user would want to wait for the
> link to come up and not have everything in ntb_tool ready and usable.

The memory windows can be configured prior to link up.  They can be configured when probing the device instead of waiting for link up.  Doing memory window configuration in probe would simplify the driver, and there would be no race.

> 
> >> +	buf[1] = '\n';
> >> +	buf[2] = '\0';
> >> +
> >> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> >> +}
> >> +
> >> +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;
> >> +	char buf[32];
> >> +	size_t buf_size;
> >> +	bool val;
> >> +	int rc;
> >> +
> >> +	buf_size = min(size, (sizeof(buf) - 1));
> >> +	if (copy_from_user(buf, ubuf, buf_size))
> >> +		return -EFAULT;
> >> +
> >> +	buf[buf_size] = '\0';
> >> +
> >> +	rc = strtobool(buf, &val);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	if (val)
> >> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> >> +	else
> >> +		ntb_link_disable(tc->ntb);
> >> +
> >> +	return size;
> >> +}
> >> +
> >> +static TOOL_FOPS_RDWR(tool_link_fops,
> >> +		      tool_link_read,
> >> +		      tool_link_write);
> >> +
> >> +static ssize_t tool_link_event_read(struct file *filep, char __user *ubuf,
> >> +				    size_t size, loff_t *offp)
> >> +{
> >> +	struct tool_ctx *tc = filep->private_data;
> >> +	char buf[3];
> >> +
> >> +	if (wait_event_interruptible(tc->link_wq,
> >> +				     tc->link_is_up == tc->link_event))
> >
> > I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> 
> See above.
> 
> >> +		return -ERESTART;
> >> +
> >> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> >> +	buf[1] = '\n';
> >> +	buf[2] = '\0';
> >> +
> >> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> >> +}
> >> +
> >> +static ssize_t tool_link_event_write(struct file *filep,
> >> +				     const char __user *ubuf,
> >> +				     size_t size, loff_t *offp)
> >> +{
> >> +	struct tool_ctx *tc = filep->private_data;
> >> +	char buf[32];
> >> +	size_t buf_size;
> >> +	bool val;
> >> +	int rc;
> >> +
> >> +	buf_size = min(size, (sizeof(buf) - 1));
> >> +	if (copy_from_user(buf, ubuf, buf_size))
> >> +		return -EFAULT;
> >> +
> >> +	buf[buf_size] = '\0';
> >> +
> >> +	rc = strtobool(buf, &val);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	tc->link_event = val;
> >
> > All writing the event file does is set the value of tc->link_event, so we have the same
> value that was set when reading the file.  It's rather inefficient, and oops, what if some
> other script comes along and writes a different value?  If script-A wants to wait for link
> up, and the link is already up, really it should not wait.  But if script-B changes tc-
> >link_event to wait for link down before script-A reads the file, then script-A will
> incorrectly wait.
> >
> > Really, I think the best thing after all would be just to wait here in the write
> function.
> 
> Yeah, I agree. It makes everything much simpler to block on the write. I
> was going on your comment that it was more natural to block on the read.
> I'll change this for v3. Are we happy to stick with the 'link' and
> 'link_event' files? Or do you like the 'link_wait' name better?

The name link_event is ok.

> 
> 
> Logan
> 
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb"
> group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-
> ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-
> ntb/576070AE.10500%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Logan Gunthorpe]

On 14/06/16 03:46 PM, Allen Hubbe wrote:
> The ntb_tool is intended to be a simple low level access to the ntb.h api.  As much as possible, I think ntb_tool should directly expose the ntb.h api through debugfs, and not invent higher level concepts.

I really think practical concerns should override this. If we do it that
way then my ntb_test script wouldn't necessarily work reliably and we'd
just be asking for race conditions. (Especially if I moved the memory
window tests earlier.) Anyone else trying to script with ntb_tool would
run into the same problem.

Additionally, the link is up _and_ the hardware is configured/usable
isn't really that high level a concept or anything a user wouldn't
expect already.

My understanding is that ntb_tool is really just a test client to verify
the API and the hardware. I personally would not recommend it for any
real applications. As such, I don't think this philosophical argument
really matches that goal.


>>> If this was never set false anywhere in the patch that added memory windows, I wonder if
>> there is a bug.
>>
>> Yup, this looks like an oversight on my part. However, I don't think it
>> resulted in any noticeable bug seeing, at the time, the only way to
>> bring the link back down was to remove the module or the device. It is
>> only strictly necessary now that we have the 'link' file which can
>> control the link.
> 
> Even without a file to control the link, any one side could be unloaded and reloaded.  That also affects the link state on the side that stays loaded.  The side that stays loaded still needs to be sane when the link comes back up.

Yup, you're correct. If the other side of link goes down then
tc->link_is_up would be incorrect. So, yes, there may be a corner case
bug there. Though, seeing tc-link_is_up was only previously used to
cancel potentially queued delayed work it's probably pretty minor.

This was copied from ntb_perf which looks like it has the same issue.
I'll make a patch for that in v3.

>>> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
>>
>> I disagree. Bad things will happen if the user waits on the event and
>> then immediately uses the memory windows. It will just be buggy and
>> racy. I can't see a situation where the user would want to wait for the
>> link to come up and not have everything in ntb_tool ready and usable.
> 
> The memory windows can be configured prior to link up.  They can be configured when probing the device instead of waiting for link up.  Doing memory window configuration in probe would simplify the driver, and there would be no race.

I'm not sure this is true, especially considering all possible hardware.
It's certainly not true with the hardware I'm working with and I'd
assume that all the existing NTB clients configured their memory windows
on link up and not in probe for a good reason.


Logan

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> On 14/06/16 03:46 PM, Allen Hubbe wrote:
> > The ntb_tool is intended to be a simple low level access to the ntb.h api.  As much as
> possible, I think ntb_tool should directly expose the ntb.h api through debugfs, and not
> invent higher level concepts.
> 
> I really think practical concerns should override this. If we do it that
> way then my ntb_test script wouldn't necessarily work reliably and we'd
> just be asking for race conditions. (Especially if I moved the memory
> window tests earlier.) Anyone else trying to script with ntb_tool would
> run into the same problem.
> 
> Additionally, the link is up _and_ the hardware is configured/usable
> isn't really that high level a concept or anything a user wouldn't
> expect already.

If the user is debugging some issue in their hardware or driver, they may care to know that the link is reported up by the driver, even if some other configuration didn't work as expected.  Debugging the api-level behaviors of hardware and hardware drivers is the primary purpose of ntb_tool.  As you note below, ntb_tool is not intended to support real applications.

> 
> My understanding is that ntb_tool is really just a test client to verify
> the API and the hardware. I personally would not recommend it for any
> real applications. As such, I don't think this philosophical argument
> really matches that goal.

The purpose is to "verify the API and the hardware", not to support "real applications."

The link status reported by the tool should be the link status reported by "the API and the hardware," and not something else that might be convenient for "my ntb_test script" or "anyone else trying to script with ntb_tool."  The primary purpose of ntb_tool is api-level debugging of hardware and drivers, not scripting.

The problem with races in ntb_tool is due to auto-configuration of memory windows in ntb_tool.  Instead of having ntb_tool setup the memory windows automatically, maybe instead it should provide a file to control the memory windows via debugfs.  Reading the file can format what is returned by ntb_mw_get_range(), and writing the file can allocate a buffer and call ntb_mw_set_trans(), or ntb_mw_clear_trans() and free the buffer.  Then, the test script can wait for link up, then setup the memory windows, and then finally proceed with the rest of the tests, and there would be no race.  There would be no confusion about what "link up" means, and ntb_tool would more closely resemble the ntb.h api for memory windows.

> 
> 
> >>> If this was never set false anywhere in the patch that added memory windows, I wonder
> if
> >> there is a bug.
> >>
> >> Yup, this looks like an oversight on my part. However, I don't think it
> >> resulted in any noticeable bug seeing, at the time, the only way to
> >> bring the link back down was to remove the module or the device. It is
> >> only strictly necessary now that we have the 'link' file which can
> >> control the link.
> >
> > Even without a file to control the link, any one side could be unloaded and reloaded.
> That also affects the link state on the side that stays loaded.  The side that stays
> loaded still needs to be sane when the link comes back up.
> 
> Yup, you're correct. If the other side of link goes down then
> tc->link_is_up would be incorrect. So, yes, there may be a corner case
> bug there. Though, seeing tc-link_is_up was only previously used to
> cancel potentially queued delayed work it's probably pretty minor.
> 
> This was copied from ntb_perf which looks like it has the same issue.
> I'll make a patch for that in v3.
> 
> >>> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> >>
> >> I disagree. Bad things will happen if the user waits on the event and
> >> then immediately uses the memory windows. It will just be buggy and
> >> racy. I can't see a situation where the user would want to wait for the
> >> link to come up and not have everything in ntb_tool ready and usable.
> >
> > The memory windows can be configured prior to link up.  They can be configured when
> probing the device instead of waiting for link up.  Doing memory window configuration in
> probe would simplify the driver, and there would be no race.
> 
> I'm not sure this is true, especially considering all possible hardware.
> It's certainly not true with the hardware I'm working with and I'd
> assume that all the existing NTB clients configured their memory windows
> on link up and not in probe for a good reason.

That's interesting about the hardware.  Maybe the driver for that particular hardware should make sure that any translation register programming happens before reporting link up to its clients.  Otherwise, ntb_transport will be broken on that hardware.  The ntb_transport driver configures memory windows the first time the link comes up, and only ever again if a different memory window size is negotiated (unlikely).

There are two reasons for doing the configuration after link up in ntb_transport.  First, it avoids consuming memory resources if the link never comes up.  Second, ntb_transport negotiates with its peer how much of the memory window will actually be used.  The ntb_perf tool is similar.

> 
> 
> Logan

RE: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> On 14/06/16 03:46 PM, Allen Hubbe wrote:
> > The ntb_tool is intended to be a simple low level access to the ntb.h api.  As much as
> possible, I think ntb_tool should directly expose the ntb.h api through debugfs, and not
> invent higher level concepts.
> 
> I really think practical concerns should override this. If we do it that
> way then my ntb_test script wouldn't necessarily work reliably and we'd
> just be asking for race conditions. (Especially if I moved the memory
> window tests earlier.) Anyone else trying to script with ntb_tool would
> run into the same problem.
> 
> Additionally, the link is up _and_ the hardware is configured/usable
> isn't really that high level a concept or anything a user wouldn't
> expect already.

If the user is debugging some issue in their hardware or driver, they may care to know that the link is reported up by the driver, even if some other configuration didn't work as expected.  Debugging the api-level behaviors of hardware and hardware drivers is the primary purpose of ntb_tool.  As you note below, ntb_tool is not intended to support real applications.

> 
> My understanding is that ntb_tool is really just a test client to verify
> the API and the hardware. I personally would not recommend it for any
> real applications. As such, I don't think this philosophical argument
> really matches that goal.

The purpose is to "verify the API and the hardware", not to support "real applications."

The link status reported by the tool should be the link status reported by "the API and the hardware," and not something else that might be convenient for "my ntb_test script" or "anyone else trying to script with ntb_tool."  The primary purpose of ntb_tool is api-level debugging of hardware and drivers, not scripting.

The problem with races in ntb_tool is due to auto-configuration of memory windows in ntb_tool.  Instead of having ntb_tool setup the memory windows automatically, maybe instead it should provide a file to control the memory windows via debugfs.  Reading the file can format what is returned by ntb_mw_get_range(), and writing the file can allocate a buffer and call ntb_mw_set_trans(), or ntb_mw_clear_trans() and free the buffer.  Then, the test script can wait for link up, then setup the memory windows, and then finally proceed with the rest of the tests, and there would be no race.  There would be no confusion about what "link up" means, and ntb_tool would more closely resemble the ntb.h api for memory windows.

> 
> 
> >>> If this was never set false anywhere in the patch that added memory windows, I wonder
> if
> >> there is a bug.
> >>
> >> Yup, this looks like an oversight on my part. However, I don't think it
> >> resulted in any noticeable bug seeing, at the time, the only way to
> >> bring the link back down was to remove the module or the device. It is
> >> only strictly necessary now that we have the 'link' file which can
> >> control the link.
> >
> > Even without a file to control the link, any one side could be unloaded and reloaded.
> That also affects the link state on the side that stays loaded.  The side that stays
> loaded still needs to be sane when the link comes back up.
> 
> Yup, you're correct. If the other side of link goes down then
> tc->link_is_up would be incorrect. So, yes, there may be a corner case
> bug there. Though, seeing tc-link_is_up was only previously used to
> cancel potentially queued delayed work it's probably pretty minor.
> 
> This was copied from ntb_perf which looks like it has the same issue.
> I'll make a patch for that in v3.
> 
> >>> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> >>
> >> I disagree. Bad things will happen if the user waits on the event and
> >> then immediately uses the memory windows. It will just be buggy and
> >> racy. I can't see a situation where the user would want to wait for the
> >> link to come up and not have everything in ntb_tool ready and usable.
> >
> > The memory windows can be configured prior to link up.  They can be configured when
> probing the device instead of waiting for link up.  Doing memory window configuration in
> probe would simplify the driver, and there would be no race.
> 
> I'm not sure this is true, especially considering all possible hardware.
> It's certainly not true with the hardware I'm working with and I'd
> assume that all the existing NTB clients configured their memory windows
> on link up and not in probe for a good reason.

That's interesting about the hardware.  Maybe the driver for that particular hardware should make sure that any translation register programming happens before reporting link up to its clients.  Otherwise, ntb_transport will be broken on that hardware.  The ntb_transport driver configures memory windows the first time the link comes up, and only ever again if a different memory window size is negotiated (unlikely).

There are two reasons for doing the configuration after link up in ntb_transport.  First, it avoids consuming memory resources if the link never comes up.  Second, ntb_transport negotiates with its peer how much of the memory window will actually be used.  The ntb_perf tool is similar.

> 
> 
> Logan

Re: [PATCH v2 6/8] ntb_tool: Add link status and files to debugfs

[Logan Gunthorpe]

On 15/06/16 10:02 AM, Allen Hubbe wrote:
>> My understanding is that ntb_tool is really just a test client to verify
>> the API and the hardware. I personally would not recommend it for any
>> real applications. As such, I don't think this philosophical argument
>> really matches that goal.
> 
> The purpose is to "verify the API and the hardware", not to support "real applications."
> 
> The link status reported by the tool should be the link status reported by "the API and the hardware," and not something else that might be convenient for "my ntb_test script" or "anyone else trying to script with ntb_tool."  The primary purpose of ntb_tool is api-level debugging of hardware and drivers, not scripting.
> 
> The problem with races in ntb_tool is due to auto-configuration of memory windows in ntb_tool.  Instead of having ntb_tool setup the memory windows automatically, maybe instead it should provide a file to control the memory windows via debugfs.  Reading the file can format what is returned by ntb_mw_get_range(), and writing the file can allocate a buffer and call ntb_mw_set_trans(), or ntb_mw_clear_trans() and free the buffer.  Then, the test script can wait for link up, then setup the memory windows, and then finally proceed with the rest of the tests, and there would be no race.  There would be no confusion about what "link up" means, and ntb_tool would more closely resemble the ntb.h api for memory windows.

Ok, well this is a good deal more complicated than it is now but I can
live with it. I'll work something up shortly.


> That's interesting about the hardware.  Maybe the driver for that particular hardware should make sure that any translation register programming happens before reporting link up to its clients.  Otherwise, ntb_transport will be broken on that hardware.  The ntb_transport driver configures memory windows the first time the link comes up, and only ever again if a different memory window size is negotiated (unlikely).
> 
> There are two reasons for doing the configuration after link up in ntb_transport.  First, it avoids consuming memory resources if the link never comes up.  Second, ntb_transport negotiates with its peer how much of the memory window will actually be used.  The ntb_perf tool is similar.

Hmm, yes I didn't notice that in ntb_transport. We'll have to make some
considerations for that in our driver.

Logan