Skip to contentSkip to conversation

NTB Selftest Script

PATCH In review Logan Gunthorpe 3 participants 14 messages Jun 20, 2016 – Jun 27, 2016

Subsystems (2)
Kselftest NTB driver core
lore
Share & tools
lore

Logan Gunthorpe

author
#0 lore raw

v5 cover NTB Selftest Script

Sorry, I thought this was done but I found one more minor issue with these patches so I'm resubmitting them one last time. Besides this isuse, I think I have acks for all the patches and everything is working as I'd like.

Changes since v4:

1) In patch 0006, when setting a translation fails in tool_setup_mw, we now properly clean up the peer dma memory. Before that patch, it wasn't required because if tool_setup_mw failed the module would clean up all mws. After this patch, the clean up never happened, so it would return an error to the user but the DMA memory would still be allocated and the peer_trans file would report that it's ready but the debugfs file would not have been created. In v5, after an error, no DMA memory is allocated and it will still report that it's not ready to the user when reading peer_trans.

---

Changes since v3:

1) Check the error returned when setting the link in ntb_tool and pass it back to the user.

2) If an error occurs when setting the link down during a link test in ntb_test, just print unsupported and continue on. (For hardware that does not support setting the link down.)

3) Fix a race condition problem introduced by clearing the link is up flag in ntb_perf. We do this by getting rid of the link cleanup work and doing the few actions in the link event handler. Dave Jiang has already ok'd this approach.

---

Changes since v2:

1) As per Allan's suggestion, I've added a patch to postpone the peer memory window setup until the user requests it with a 'peer_trans*' file. This pushes the ntb_tool API closer to the kernel API and allows the link status file to return the raw status of the NTB link.

2) Change the link status file to return the value of ntb_link_is_up instead of the local 'memory window ready' value.

3) Change the link_event file to just block on write. As it was in v2, if multiple users attempted to use the link_event file they could corrupt the state another user had set. Reads to this file are no longer permitted.

4) Updates to the ntb_test script to accommodate the above changes.

5) Added some link tests to the ntb_test script. It will bring the link down and check that the other side agrees.

6) Added a minor bug fix (Patch #10) to ntb_perf. During discussions with Allen it was noticed that the link_is_up flag is never cleared if the link goes away.

---

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 (10):
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: Postpone memory window initialization for the user
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
ntb_perf: clear link_is_up flag when the link goes down.

 MAINTAINERS                             |   1 +
 drivers/ntb/test/ntb_perf.c             | 224 +++++++++------
 drivers/ntb/test/ntb_pingpong.c         |  62 ++++-
 drivers/ntb/test/ntb_tool.c             | 468 ++++++++++++++++++++++----------
 tools/testing/selftests/ntb/ntb_test.sh | 422 ++++++++++++++++++++++++++++
 5 files changed, 957 insertions(+), 220 deletions(-)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh

--
2.1.4

author
#1 lore raw

PATCH v5 1/10 ntb_perf: Schedule based on time not on performance

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 <[email protected]>
Acked-by Dave Jiang <[email protected]>
---
 drivers/ntb/test/ntb_perf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/ntb/test/ntb_perf.c +4 -2
index 4368519..5008ccf 100644
@@ -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);
 		}
Signature · 1 line
2.1.4
author
#2 lore raw

PATCH v5 2/10 ntb_perf: Improve thread handling to increase robustness

In reply to 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 <[email protected]>
Acked-by Dave Jiang <[email protected]>
---
 drivers/ntb/test/ntb_perf.c | 124 +++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 48 deletions(-)
drivers/ntb/test/ntb_perf.c +76 -48
index 5008ccf..db4dc61 100644
@@ -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);
Signature · 1 line
2.1.4
author
#3 lore raw

PATCH v5 3/10 ntb_perf: Return results by reading the run file

In reply to 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 <[email protected]>
Acked-by Dave Jiang <[email protected]>
---
 drivers/ntb/test/ntb_perf.c | 67 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 12 deletions(-)
drivers/ntb/test/ntb_perf.c +55 -12
index db4dc61..05a8705 100644
@@ -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:
Signature · 1 line
2.1.4
author
#4 lore raw

PATCH v5 4/10 ntb_perf: Wait for link before running test

In reply to 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 <[email protected]>
Acked-by Dave Jiang <[email protected]>
---
 drivers/ntb/test/ntb_perf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/ntb/test/ntb_perf.c +4 -1
index 05a8705..f0784e5 100644
@@ -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);
Signature · 1 line
2.1.4
author
#5 lore raw

PATCH v5 5/10 ntb_tool: BUG: Ensure the buffer size is large enough to return all spads

In reply to 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 <[email protected]>
Acked-by Allen Hubbe <[email protected]>
---
 drivers/ntb/test/ntb_tool.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/ntb/test/ntb_tool.c +8 -2
index 4c01057..cba31fd 100644
@@ -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));
Signature · 1 line
2.1.4
author
#6 lore raw

PATCH v5 6/10 ntb_tool: Postpone memory window initialization for the user

In reply to Logan Gunthorpe

In order to make the interface closer to the raw NTB API, this commit changes memory windows so they are not initialized on link up. Instead, the 'peer_trans*' debugfs files are introduced. When read, they return information provided by ntb_mw_get_range. When written, they create a buffer and initialize the memory window. The value written is taken as the requested size of the buffer (which is then rounded for alignment). Writing a value of zero frees the buffer and tears down the memory window translation. The 'peer_mw*' file is only created once the memory window translation is setup by the user.

Additionally, it was noticed that the read and write functions for the 'peer_mw*' files should have checked for a NULL pointer.

Signed-off-by Logan Gunthorpe <[email protected]>
---
 drivers/ntb/test/ntb_tool.c | 366 +++++++++++++++++++++++++++-----------------
 1 file changed, 228 insertions(+), 138 deletions(-)
drivers/ntb/test/ntb_tool.c +228 -138
index cba31fd..1509b4c 100644
@@ -79,6 +79,13 @@
  * root@self# cat $DBG_DIR/spad
  *
  * Observe that spad 0 and 1 have the values set by the peer.
+ *
+ * # Check the memory window translation info
+ * cat $DBG_DIR/peer_trans0
+ *
+ * # Setup a 16k memory window buffer
+ * echo 16384 > $DBG_DIR/peer_trans0
+ *
  */
 
 #include <linux/init.h>

@@ -108,25 +115,22 @@ MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
 
 #define MAX_MWS 16
 
-static unsigned long mw_size = 16;
-module_param(mw_size, ulong, 0644);
-MODULE_PARM_DESC(mw_size, "size order [n^2] of the memory window for testing");
-
 static struct dentry *tool_dbgfs;
 
 struct tool_mw {
+	int idx;
+	struct tool_ctx *tc;
+	resource_size_t win_size;
 	resource_size_t size;
 	u8 __iomem *local;
 	u8 *peer;
 	dma_addr_t peer_dma;
+	struct dentry *peer_dbg_file;
 };
 
 struct tool_ctx {
 	struct ntb_dev *ntb;
 	struct dentry *dbgfs;
-	struct work_struct link_cleanup;
-	bool link_is_up;
-	struct delayed_work link_work;
 	int mw_count;
 	struct tool_mw mws[MAX_MWS];
 };

@@ -143,111 +147,6 @@ struct tool_ctx {
 		.write = __write,		\
 	}
 
-static int tool_setup_mw(struct tool_ctx *tc, int idx)
-{
-	int rc;
-	struct tool_mw *mw = &tc->mws[idx];
-	phys_addr_t base;
-	resource_size_t size, align, align_size;
-
-	if (mw->local)
-		return 0;
-
-	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
-			      &align_size);
-	if (rc)
-		return rc;
-
-	mw->size = min_t(resource_size_t, 1 << mw_size, size);
-	mw->size = round_up(mw->size, align);
-	mw->size = round_up(mw->size, align_size);
-
-	mw->local = ioremap_wc(base, size);
-	if (mw->local == NULL)
-		return -EFAULT;
-
-	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
-				      &mw->peer_dma, GFP_KERNEL);
-
-	if (mw->peer == NULL)
-		return -ENOMEM;
-
-	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
-	if (rc)
-		return rc;
-
-	return 0;
-}
-
-static void tool_free_mws(struct tool_ctx *tc)
-{
-	int i;
-
-	for (i = 0; i < tc->mw_count; i++) {
-		if (tc->mws[i].peer) {
-			ntb_mw_clear_trans(tc->ntb, i);
-			dma_free_coherent(&tc->ntb->pdev->dev, tc->mws[i].size,
-					  tc->mws[i].peer,
-					  tc->mws[i].peer_dma);
-
-		}
-
-		tc->mws[i].peer = NULL;
-		tc->mws[i].peer_dma = 0;
-
-		if (tc->mws[i].local)
-			iounmap(tc->mws[i].local);
-
-		tc->mws[i].local = NULL;
-	}
-
-	tc->mw_count = 0;
-}
-
-static int tool_setup_mws(struct tool_ctx *tc)
-{
-	int i;
-	int rc;
-
-	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
-
-	for (i = 0; i < tc->mw_count; i++) {
-		rc = tool_setup_mw(tc, i);
-		if (rc)
-			goto err_out;
-	}
-
-	return 0;
-
-err_out:
-	tool_free_mws(tc);
-	return rc;
-}
-
-static void tool_link_work(struct work_struct *work)
-{
-	int rc;
-	struct tool_ctx *tc = container_of(work, struct tool_ctx,
-					   link_work.work);
-
-	tool_free_mws(tc);
-	rc = tool_setup_mws(tc);
-	if (rc)
-		dev_err(&tc->ntb->dev,
-			"Error setting up memory windows: %d\n", rc);
-
-	tc->link_is_up = true;
-}
-
-static void tool_link_cleanup(struct work_struct *work)
-{
-	struct tool_ctx *tc = container_of(work, struct tool_ctx,
-					   link_cleanup);
-
-	if (!tc->link_is_up)
-		cancel_delayed_work_sync(&tc->link_work);
-}
-
 static void tool_link_event(void *ctx)
 {
 	struct tool_ctx *tc = ctx;

@@ -260,10 +159,6 @@ static void tool_link_event(void *ctx)
 	dev_dbg(&tc->ntb->dev, "link is %s speed %d width %d\n",
 		up ? "up" : "down", speed, width);
 
-	if (up)
-		schedule_delayed_work(&tc->link_work, 2*HZ);
-	else
-		schedule_work(&tc->link_cleanup);
 }
 
 static void tool_db_event(void *ctx, int vec)

@@ -591,10 +486,10 @@ static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
 		return -EIO;
 	if (pos < 0)
 		return -EINVAL;
-	if (pos >= mw->size || !size)
+	if (pos >= mw->win_size || !size)
 		return 0;
-	if (size > mw->size - pos)
-		size = mw->size - pos;
+	if (size > mw->win_size - pos)
+		size = mw->win_size - pos;
 
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf)

@@ -627,10 +522,10 @@ static ssize_t tool_mw_write(struct file *filep, const char __user *ubuf,
 
 	if (pos < 0)
 		return -EINVAL;
-	if (pos >= mw->size || !size)
+	if (pos >= mw->win_size || !size)
 		return 0;
-	if (size > mw->size - pos)
-		size = mw->size - pos;
+	if (size > mw->win_size - pos)
+		size = mw->win_size - pos;
 
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf)

@@ -658,20 +553,25 @@ 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)
+				 size_t size, loff_t *offp)
 {
 	struct tool_mw *mw = filep->private_data;
 
+	if (!mw->peer)
+		return -ENXIO;
+
 	return simple_read_from_buffer(ubuf, size, offp, mw->peer, mw->size);
 }
 
 static ssize_t tool_peer_mw_write(struct file *filep, const char __user *ubuf,
-				    size_t size, loff_t *offp)
+				  size_t size, loff_t *offp)
 {
 	struct tool_mw *mw = filep->private_data;
 
+	if (!mw->peer)
+		return -ENXIO;
+
 	return simple_write_to_buffer(mw->peer, mw->size, offp, ubuf, size);
 }
 

@@ -679,9 +579,199 @@ static TOOL_FOPS_RDWR(tool_peer_mw_fops,
 		      tool_peer_mw_read,
 		      tool_peer_mw_write);
 
+static int tool_setup_mw(struct tool_ctx *tc, int idx, size_t req_size)
+{
+	int rc;
+	struct tool_mw *mw = &tc->mws[idx];
+	phys_addr_t base;
+	resource_size_t size, align, align_size;
+	char buf[16];
+
+	if (mw->peer)
+		return 0;
+
+	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
+			      &align_size);
+	if (rc)
+		return rc;
+
+	mw->size = min_t(resource_size_t, req_size, size);
+	mw->size = round_up(mw->size, align);
+	mw->size = round_up(mw->size, align_size);
+	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
+				      &mw->peer_dma, GFP_KERNEL);
+
+	if (!mw->peer)
+		return -ENOMEM;
+
+	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
+	if (rc)
+		goto err_free_dma;
+
+	snprintf(buf, sizeof(buf), "peer_mw%d", idx);
+	mw->peer_dbg_file = debugfs_create_file(buf, S_IRUSR | S_IWUSR,
+						mw->tc->dbgfs, mw,
+						&tool_peer_mw_fops);
+
+	return 0;
+
+err_free_dma:
+	dma_free_coherent(&tc->ntb->pdev->dev, mw->size,
+			  mw->peer,
+			  mw->peer_dma);
+	mw->peer = NULL;
+	mw->peer_dma = 0;
+	mw->size = 0;
+
+	return rc;
+}
+
+static void tool_free_mw(struct tool_ctx *tc, int idx)
+{
+	struct tool_mw *mw = &tc->mws[idx];
+
+	if (mw->peer) {
+		ntb_mw_clear_trans(tc->ntb, idx);
+		dma_free_coherent(&tc->ntb->pdev->dev, mw->size,
+				  mw->peer,
+				  mw->peer_dma);
+	}
+
+	mw->peer = NULL;
+	mw->peer_dma = 0;
+
+	debugfs_remove(mw->peer_dbg_file);
+
+	mw->peer_dbg_file = NULL;
+}
+
+static ssize_t tool_peer_mw_trans_read(struct file *filep,
+				       char __user *ubuf,
+				       size_t size, loff_t *offp)
+{
+	struct tool_mw *mw = filep->private_data;
+
+	char *buf;
+	size_t buf_size;
+	ssize_t ret, off = 0;
+
+	phys_addr_t base;
+	resource_size_t mw_size;
+	resource_size_t align;
+	resource_size_t align_size;
+
+	buf_size = min_t(size_t, size, 512);
+
+	buf = kmalloc(buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ntb_mw_get_range(mw->tc->ntb, mw->idx,
+			 &base, &mw_size, &align, &align_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Peer MW %d Information:\n", mw->idx);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Physical Address      \t%pa[p]\n",
+			 &base);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Window Size           \t%lld\n",
+			 (unsigned long long)mw_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Alignment             \t%lld\n",
+			 (unsigned long long)align);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Size Alignment        \t%lld\n",
+			 (unsigned long long)align_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Ready                 \t%c\n",
+			 (mw->peer) ? 'Y' : 'N');
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Allocated Size       \t%zd\n",
+			 (mw->peer) ? (size_t)mw->size : 0);
+
+	ret = simple_read_from_buffer(ubuf, size, offp, buf, off);
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t tool_peer_mw_trans_write(struct file *filep,
+					const char __user *ubuf,
+					size_t size, loff_t *offp)
+{
+	struct tool_mw *mw = filep->private_data;
+
+	char buf[32];
+	size_t buf_size;
+	unsigned long long 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 = kstrtoull(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	tool_free_mw(mw->tc, mw->idx);
+	if (val)
+		rc = tool_setup_mw(mw->tc, mw->idx, val);
+
+	if (rc)
+		return rc;
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_peer_mw_trans_fops,
+		      tool_peer_mw_trans_read,
+		      tool_peer_mw_trans_write);
+
+static int tool_init_mw(struct tool_ctx *tc, int idx)
+{
+	struct tool_mw *mw = &tc->mws[idx];
+	phys_addr_t base;
+	int rc;
+
+	rc = ntb_mw_get_range(tc->ntb, idx, &base, &mw->win_size,
+			      NULL, NULL);
+	if (rc)
+		return rc;
+
+	mw->tc = tc;
+	mw->idx = idx;
+	mw->local = ioremap_wc(base, mw->win_size);
+	if (!mw->local)
+		return -EFAULT;
+
+	return 0;
+}
+
+static void tool_free_mws(struct tool_ctx *tc)
+{
+	int i;
+
+	for (i = 0; i < tc->mw_count; i++) {
+		tool_free_mw(tc, i);
+
+		if (tc->mws[i].local)
+			iounmap(tc->mws[i].local);
+
+		tc->mws[i].local = NULL;
+	}
+}
+
 static void tool_setup_dbgfs(struct tool_ctx *tc)
 {
-	int mw_count;
 	int i;
 
 	/* This modules is useless without dbgfs... */

@@ -713,18 +803,16 @@ 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);
 
-	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
-	for (i = 0; i < mw_count; i++) {
+	for (i = 0; i < tc->mw_count; i++) {
 		char buf[30];
 
 		snprintf(buf, sizeof(buf), "mw%d", i);
 		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
 				    &tc->mws[i], &tool_mw_fops);
 
-		snprintf(buf, sizeof(buf), "peer_mw%d", i);
+		snprintf(buf, sizeof(buf), "peer_trans%d", i);
 		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
-				    &tc->mws[i], &tool_peer_mw_fops);
-
+				    &tc->mws[i], &tool_peer_mw_trans_fops);
 	}
 }
 

@@ -732,6 +820,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 {
 	struct tool_ctx *tc;
 	int rc;
+	int i;
 
 	if (ntb_db_is_unsafe(ntb))
 		dev_dbg(&ntb->dev, "doorbell is unsafe\n");

@@ -746,8 +835,13 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 	}
 
 	tc->ntb = ntb;
-	INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
-	INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
+
+	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
+	for (i = 0; i < tc->mw_count; i++) {
+		rc = tool_init_mw(tc, i);
+		if (rc)
+			goto err_ctx;
+	}
 
 	tool_setup_dbgfs(tc);
 

@@ -761,9 +855,8 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 	return 0;
 
 err_ctx:
+	tool_free_mws(tc);
 	debugfs_remove_recursive(tc->dbgfs);
-	cancel_delayed_work_sync(&tc->link_work);
-	cancel_work_sync(&tc->link_cleanup);
 	kfree(tc);
 err_tc:
 	return rc;

@@ -773,9 +866,6 @@ static void tool_remove(struct ntb_client *self, struct ntb_dev *ntb)
 {
 	struct tool_ctx *tc = ntb->ctx;
 
-	cancel_delayed_work_sync(&tc->link_work);
-	cancel_work_sync(&tc->link_cleanup);
-
 	tool_free_mws(tc);
 
 	ntb_clear_ctx(ntb);
Signature · 1 line
2.1.4
Acked-by on v5 6/10
#7 lore raw

RE: [PATCH v5 06/10] ntb_tool: Postpone memory window initialization for the user

From: Logan Gunthorpe

Quoted 10 lines

In order to make the interface closer to the raw NTB API, this commit changes memory windows so they are not initialized on link up. Instead, the 'peer_trans*' debugfs files are introduced. When read, they return information provided by ntb_mw_get_range. When written, they create a buffer and initialize the memory window. The value written is taken as the requested size of the buffer (which is then rounded for alignment). Writing a value of zero frees the buffer and tears down the memory window translation. The 'peer_mw*' file is only created once the memory window translation is setup by the user.

Quoted 4 lines

Additionally, it was noticed that the read and write functions for the 'peer_mw*' files should have checked for a NULL pointer.

Signed-off-by Logan Gunthorpe <[email protected]>
Acked-by Allen Hubbe <[email protected]>
Quoted 503 lines
---
 drivers/ntb/test/ntb_tool.c | 366 +++++++++++++++++++++++++++-----------------
 1 file changed, 228 insertions(+), 138 deletions(-)
drivers/ntb/test/ntb_tool.c +17 -122
index cba31fd..1509b4c 100644
@@ -79,6 +79,13 @@
  * root@self# cat $DBG_DIR/spad
  *
  * Observe that spad 0 and 1 have the values set by the peer.
+ *
+ * # Check the memory window translation info
+ * cat $DBG_DIR/peer_trans0
+ *
+ * # Setup a 16k memory window buffer
+ * echo 16384 > $DBG_DIR/peer_trans0
+ *
  */
 
 #include <linux/init.h>

@@ -108,25 +115,22 @@ MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
 
 #define MAX_MWS 16
 
-static unsigned long mw_size = 16;
-module_param(mw_size, ulong, 0644);
-MODULE_PARM_DESC(mw_size, "size order [n^2] of the memory window for testing");
-
 static struct dentry *tool_dbgfs;
 
 struct tool_mw {
+	int idx;
+	struct tool_ctx *tc;
+	resource_size_t win_size;
 	resource_size_t size;
 	u8 __iomem *local;
 	u8 *peer;
 	dma_addr_t peer_dma;
+	struct dentry *peer_dbg_file;
 };
 
 struct tool_ctx {
 	struct ntb_dev *ntb;
 	struct dentry *dbgfs;
-	struct work_struct link_cleanup;
-	bool link_is_up;
-	struct delayed_work link_work;
 	int mw_count;
 	struct tool_mw mws[MAX_MWS];
 };

@@ -143,111 +147,6 @@ struct tool_ctx {
 		.write = __write,		\
 	}
 
-static int tool_setup_mw(struct tool_ctx *tc, int idx)
-{
-	int rc;
-	struct tool_mw *mw = &tc->mws[idx];
-	phys_addr_t base;
-	resource_size_t size, align, align_size;
-
-	if (mw->local)
-		return 0;
-
-	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
-			      &align_size);
-	if (rc)
-		return rc;
-
-	mw->size = min_t(resource_size_t, 1 << mw_size, size);
-	mw->size = round_up(mw->size, align);
-	mw->size = round_up(mw->size, align_size);
-
-	mw->local = ioremap_wc(base, size);
-	if (mw->local == NULL)
-		return -EFAULT;
-
-	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
-				      &mw->peer_dma, GFP_KERNEL);
-
-	if (mw->peer == NULL)
-		return -ENOMEM;
-
-	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
-	if (rc)
-		return rc;
-
-	return 0;
-}
-
-static void tool_free_mws(struct tool_ctx *tc)
-{
-	int i;
-
-	for (i = 0; i < tc->mw_count; i++) {
-		if (tc->mws[i].peer) {
-			ntb_mw_clear_trans(tc->ntb, i);
-			dma_free_coherent(&tc->ntb->pdev->dev, tc->mws[i].size,
-					  tc->mws[i].peer,
-					  tc->mws[i].peer_dma);
-
-		}
-
-		tc->mws[i].peer = NULL;
-		tc->mws[i].peer_dma = 0;
-
-		if (tc->mws[i].local)
-			iounmap(tc->mws[i].local);
-
-		tc->mws[i].local = NULL;
-	}
-
-	tc->mw_count = 0;
-}
-
-static int tool_setup_mws(struct tool_ctx *tc)
-{
-	int i;
-	int rc;
-
-	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
-
-	for (i = 0; i < tc->mw_count; i++) {
-		rc = tool_setup_mw(tc, i);
-		if (rc)
-			goto err_out;
-	}
-
-	return 0;
-
-err_out:
-	tool_free_mws(tc);
-	return rc;
-}
-
-static void tool_link_work(struct work_struct *work)
-{
-	int rc;
-	struct tool_ctx *tc = container_of(work, struct tool_ctx,
-					   link_work.work);
-
-	tool_free_mws(tc);
-	rc = tool_setup_mws(tc);
-	if (rc)
-		dev_err(&tc->ntb->dev,
-			"Error setting up memory windows: %d\n", rc);
-
-	tc->link_is_up = true;
-}
-
-static void tool_link_cleanup(struct work_struct *work)
-{
-	struct tool_ctx *tc = container_of(work, struct tool_ctx,
-					   link_cleanup);
-
-	if (!tc->link_is_up)
-		cancel_delayed_work_sync(&tc->link_work);
-}
-
 static void tool_link_event(void *ctx)
 {
 	struct tool_ctx *tc = ctx;

@@ -260,10 +159,6 @@ static void tool_link_event(void *ctx)
 	dev_dbg(&tc->ntb->dev, "link is %s speed %d width %d\n",
 		up ? "up" : "down", speed, width);
 
-	if (up)
-		schedule_delayed_work(&tc->link_work, 2*HZ);
-	else
-		schedule_work(&tc->link_cleanup);
 }
 
 static void tool_db_event(void *ctx, int vec)

@@ -591,10 +486,10 @@ static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
 		return -EIO;
 	if (pos < 0)
 		return -EINVAL;
-	if (pos >= mw->size || !size)
+	if (pos >= mw->win_size || !size)
 		return 0;
-	if (size > mw->size - pos)
-		size = mw->size - pos;
+	if (size > mw->win_size - pos)
+		size = mw->win_size - pos;
 
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf)

@@ -627,10 +522,10 @@ static ssize_t tool_mw_write(struct file *filep, const char __user
 *ubuf,
 
 	if (pos < 0)
 		return -EINVAL;
-	if (pos >= mw->size || !size)
+	if (pos >= mw->win_size || !size)
 		return 0;
-	if (size > mw->size - pos)
-		size = mw->size - pos;
+	if (size > mw->win_size - pos)
+		size = mw->win_size - pos;
 
 	buf = kmalloc(size, GFP_KERNEL);
drivers/ntb/test/ntb_tool.c +211 -17
 	if (!buf)

@@ -658,20 +553,25 @@ 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)
+				 size_t size, loff_t *offp)
 {
 	struct tool_mw *mw = filep->private_data;
 
+	if (!mw->peer)
+		return -ENXIO;
+
 	return simple_read_from_buffer(ubuf, size, offp, mw->peer, mw->size);
 }
 
 static ssize_t tool_peer_mw_write(struct file *filep, const char __user *ubuf,
-				    size_t size, loff_t *offp)
+				  size_t size, loff_t *offp)
 {
 	struct tool_mw *mw = filep->private_data;
 
+	if (!mw->peer)
+		return -ENXIO;
+
 	return simple_write_to_buffer(mw->peer, mw->size, offp, ubuf, size);
 }
 

@@ -679,9 +579,199 @@ static TOOL_FOPS_RDWR(tool_peer_mw_fops,
 		      tool_peer_mw_read,
 		      tool_peer_mw_write);
 
+static int tool_setup_mw(struct tool_ctx *tc, int idx, size_t req_size)
+{
+	int rc;
+	struct tool_mw *mw = &tc->mws[idx];
+	phys_addr_t base;
+	resource_size_t size, align, align_size;
+	char buf[16];
+
+	if (mw->peer)
+		return 0;
+
+	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
+			      &align_size);
+	if (rc)
+		return rc;
+
+	mw->size = min_t(resource_size_t, req_size, size);
+	mw->size = round_up(mw->size, align);
+	mw->size = round_up(mw->size, align_size);
+	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
+				      &mw->peer_dma, GFP_KERNEL);
+
+	if (!mw->peer)
+		return -ENOMEM;
+
+	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
+	if (rc)
+		goto err_free_dma;
+
+	snprintf(buf, sizeof(buf), "peer_mw%d", idx);
+	mw->peer_dbg_file = debugfs_create_file(buf, S_IRUSR | S_IWUSR,
+						mw->tc->dbgfs, mw,
+						&tool_peer_mw_fops);
+
+	return 0;
+
+err_free_dma:
+	dma_free_coherent(&tc->ntb->pdev->dev, mw->size,
+			  mw->peer,
+			  mw->peer_dma);
+	mw->peer = NULL;
+	mw->peer_dma = 0;
+	mw->size = 0;
+
+	return rc;
+}
+
+static void tool_free_mw(struct tool_ctx *tc, int idx)
+{
+	struct tool_mw *mw = &tc->mws[idx];
+
+	if (mw->peer) {
+		ntb_mw_clear_trans(tc->ntb, idx);
+		dma_free_coherent(&tc->ntb->pdev->dev, mw->size,
+				  mw->peer,
+				  mw->peer_dma);
+	}
+
+	mw->peer = NULL;
+	mw->peer_dma = 0;
+
+	debugfs_remove(mw->peer_dbg_file);
+
+	mw->peer_dbg_file = NULL;
+}
+
+static ssize_t tool_peer_mw_trans_read(struct file *filep,
+				       char __user *ubuf,
+				       size_t size, loff_t *offp)
+{
+	struct tool_mw *mw = filep->private_data;
+
+	char *buf;
+	size_t buf_size;
+	ssize_t ret, off = 0;
+
+	phys_addr_t base;
+	resource_size_t mw_size;
+	resource_size_t align;
+	resource_size_t align_size;
+
+	buf_size = min_t(size_t, size, 512);
+
+	buf = kmalloc(buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ntb_mw_get_range(mw->tc->ntb, mw->idx,
+			 &base, &mw_size, &align, &align_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Peer MW %d Information:\n", mw->idx);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Physical Address      \t%pa[p]\n",
+			 &base);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Window Size           \t%lld\n",
+			 (unsigned long long)mw_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Alignment             \t%lld\n",
+			 (unsigned long long)align);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Size Alignment        \t%lld\n",
+			 (unsigned long long)align_size);
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Ready                 \t%c\n",
+			 (mw->peer) ? 'Y' : 'N');
+
+	off += scnprintf(buf + off, buf_size - off,
+			 "Allocated Size       \t%zd\n",
+			 (mw->peer) ? (size_t)mw->size : 0);
+
+	ret = simple_read_from_buffer(ubuf, size, offp, buf, off);
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t tool_peer_mw_trans_write(struct file *filep,
+					const char __user *ubuf,
+					size_t size, loff_t *offp)
+{
+	struct tool_mw *mw = filep->private_data;
+
+	char buf[32];
+	size_t buf_size;
+	unsigned long long 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 = kstrtoull(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	tool_free_mw(mw->tc, mw->idx);
+	if (val)
+		rc = tool_setup_mw(mw->tc, mw->idx, val);
+
+	if (rc)
+		return rc;
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_peer_mw_trans_fops,
+		      tool_peer_mw_trans_read,
+		      tool_peer_mw_trans_write);
+
+static int tool_init_mw(struct tool_ctx *tc, int idx)
+{
+	struct tool_mw *mw = &tc->mws[idx];
+	phys_addr_t base;
+	int rc;
+
+	rc = ntb_mw_get_range(tc->ntb, idx, &base, &mw->win_size,
+			      NULL, NULL);
+	if (rc)
+		return rc;
+
+	mw->tc = tc;
+	mw->idx = idx;
+	mw->local = ioremap_wc(base, mw->win_size);
+	if (!mw->local)
+		return -EFAULT;
+
+	return 0;
+}
+
+static void tool_free_mws(struct tool_ctx *tc)
+{
+	int i;
+
+	for (i = 0; i < tc->mw_count; i++) {
+		tool_free_mw(tc, i);
+
+		if (tc->mws[i].local)
+			iounmap(tc->mws[i].local);
+
+		tc->mws[i].local = NULL;
+	}
+}
+
 static void tool_setup_dbgfs(struct tool_ctx *tc)
 {
-	int mw_count;
 	int i;
 
 	/* This modules is useless without dbgfs... */

@@ -713,18 +803,16 @@ 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);
 
-	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
-	for (i = 0; i < mw_count; i++) {
+	for (i = 0; i < tc->mw_count; i++) {
 		char buf[30];
 
 		snprintf(buf, sizeof(buf), "mw%d", i);
 		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
 				    &tc->mws[i], &tool_mw_fops);
 
-		snprintf(buf, sizeof(buf), "peer_mw%d", i);
+		snprintf(buf, sizeof(buf), "peer_trans%d", i);
 		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
-				    &tc->mws[i], &tool_peer_mw_fops);
-
+				    &tc->mws[i], &tool_peer_mw_trans_fops);
 	}
 }
 

@@ -732,6 +820,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 {
 	struct tool_ctx *tc;
 	int rc;
+	int i;
 
 	if (ntb_db_is_unsafe(ntb))
 		dev_dbg(&ntb->dev, "doorbell is unsafe\n");

@@ -746,8 +835,13 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 	}
 
 	tc->ntb = ntb;
-	INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
-	INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
+
+	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
+	for (i = 0; i < tc->mw_count; i++) {
+		rc = tool_init_mw(tc, i);
+		if (rc)
+			goto err_ctx;
+	}
 
 	tool_setup_dbgfs(tc);
 

@@ -761,9 +855,8 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 	return 0;
 
 err_ctx:
+	tool_free_mws(tc);
 	debugfs_remove_recursive(tc->dbgfs);
-	cancel_delayed_work_sync(&tc->link_work);
-	cancel_work_sync(&tc->link_cleanup);
 	kfree(tc);
 err_tc:
 	return rc;

@@ -773,9 +866,6 @@ static void tool_remove(struct ntb_client *self, struct ntb_dev *ntb)
 {
 	struct tool_ctx *tc = ntb->ctx;
 
-	cancel_delayed_work_sync(&tc->link_work);
-	cancel_work_sync(&tc->link_cleanup);
-
 	tool_free_mws(tc);
 
 	ntb_clear_ctx(ntb);
--

2.1.4

author
#8 lore raw

PATCH v5 7/10 ntb_tool: Add link status and files to debugfs

In reply to 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 boolean (Y or N) depending on the link status. Writing to the file 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 the desired state. If the user writes a 1, it will block until the link is up. If the user writes a 0, it will block until the link is down.

Signed-off-by Logan Gunthorpe <[email protected]>
Acked-by Allen Hubbe <[email protected]>
---
 drivers/ntb/test/ntb_tool.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
drivers/ntb/test/ntb_tool.c +92 -0
index 1509b4c..61bf2ef 100644
@@ -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 Y > $DBG_DIR/link_event
+ *
  * # Set the doorbell mask
  * root@self# echo 's 1' > $DBG_DIR/mask
  *

@@ -131,6 +137,7 @@ struct tool_mw {
 struct tool_ctx {
 	struct ntb_dev *ntb;
 	struct dentry *dbgfs;
+	wait_queue_head_t link_wq;
 	int mw_count;
 	struct tool_mw mws[MAX_MWS];
 };

@@ -159,6 +166,7 @@ static void tool_link_event(void *ctx)
 	dev_dbg(&tc->ntb->dev, "link is %s speed %d width %d\n",
 		up ? "up" : "down", speed, width);
 
+	wake_up(&tc->link_wq);
 }
 
 static void tool_db_event(void *ctx, int vec)

@@ -473,6 +481,83 @@ 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] = ntb_link_is_up(tc->ntb, NULL, NULL) ? '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)
+		rc = ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
+	else
+		rc = ntb_link_disable(tc->ntb);
+
+	if (rc)
+		return rc;
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_link_fops,
+		      tool_link_read,
+		      tool_link_write);
+
+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;
+
+	if (wait_event_interruptible(tc->link_wq,
+		ntb_link_is_up(tc->ntb, NULL, NULL) == val))
+		return -ERESTART;
+
+	return size;
+}
+
+static TOOL_FOPS_RDWR(tool_link_event_fops,
+		      NULL,
+		      tool_link_event_write);
 
 static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
 			    size_t size, loff_t *offp)

@@ -803,6 +888,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_IWUSR, tc->dbgfs,
+			    tc, &tool_link_event_fops);
+
 	for (i = 0; i < tc->mw_count; i++) {
 		char buf[30];
 

@@ -835,6 +926,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
 	}
 
 	tc->ntb = ntb;
+	init_waitqueue_head(&tc->link_wq);
 
 	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
 	for (i = 0; i < tc->mw_count; i++) {
Signature · 1 line
2.1.4
author
#9 lore raw

PATCH v5 8/10 ntb_pingpong: Add a debugfs file to get the ping count

In reply to 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 <[email protected]>
Acked-by Allen Hubbe <[email protected]>
---
 drivers/ntb/test/ntb_pingpong.c | 62 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)
drivers/ntb/test/ntb_pingpong.c +61 -1
index fe16005..7d31179 100644
@@ -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);
Signature · 1 line
2.1.4
author
#10 lore raw

PATCH v5 9/10 ntb_test: Add a selftest script for the NTB subsystem

In reply to 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 link tests on: 0000:03:00.1 / 0000:83:00.1
  Passed
Running link tests on: 0000:83:00.1 / 0000:03:00.1
  Passed
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 164453 usecs, 3264 MBytes/s Passed
Running remote perf test without DMA
0: copied 536870912 bytes in 164453 usecs, 3264 MBytes/s Passed

Signed-off-by Logan Gunthorpe <[email protected]>
Acked-by Shuah Khan <[email protected]>
Acked-by Allen Hubbe <[email protected]>
---
 MAINTAINERS                             |   1 +
 tools/testing/selftests/ntb/ntb_test.sh | 422 ++++++++++++++++++++++++++++++++
 2 files changed, 423 insertions(+)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh
MAINTAINERS +1 -0
index 9c567a4..f178e7e 100644
@@ -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 <[email protected]>
tools/testing/selftests/ntb/ntb_test.sh new file +422 -0

422 lines not on this page · raw message

Signature · 1 line
2.1.4
author
#11 lore raw

PATCH v5 10/10 ntb_perf: clear link_is_up flag when the link goes down.

In reply to Logan Gunthorpe

When the link goes down, the link_is_up flag did not return to false. This could have caused some subtle corner case bugs when the link goes up and down quickly.

Once that was fixed, there was found to be a race if the link was brought down then immediately up. The link_cleanup work would occasionally be scheduled after the next link up event. This would cancel the link_work that was supposed to occur and leave ntb_perf in an unusable state.

To fix this we get rid of the link_cleanup work and put the actions directly in the link_down event.

Signed-off-by Logan Gunthorpe <[email protected]>
Acked-by Dave Jiang <[email protected]>
---
 drivers/ntb/test/ntb_perf.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/ntb/test/ntb_perf.c +9 -19
index f0784e5..6a50f20 100644
@@ -133,7 +133,6 @@ struct perf_ctx {
 	spinlock_t		db_lock;
 	struct perf_mw		mw;
 	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;

@@ -158,10 +157,16 @@ static void perf_link_event(void *ctx)
 {
 	struct perf_ctx *perf = ctx;
 
-	if (ntb_link_is_up(perf->ntb, NULL, NULL) == 1)
+	if (ntb_link_is_up(perf->ntb, NULL, NULL) == 1) {
 		schedule_delayed_work(&perf->link_work, 2*HZ);
-	else
-		schedule_work(&perf->link_cleanup);
+	} else {
+		dev_dbg(&perf->ntb->pdev->dev, "link down\n");
+
+		if (!perf->link_is_up)
+			cancel_delayed_work_sync(&perf->link_work);
+
+		perf->link_is_up = false;
+	}
 }
 
 static void perf_db_event(void *ctx, int vec)

@@ -547,18 +552,6 @@ out:
 				      msecs_to_jiffies(PERF_LINK_DOWN_TIMEOUT));
 }
 
-static void perf_link_cleanup(struct work_struct *work)
-{
-	struct perf_ctx *perf = container_of(work,
-					     struct perf_ctx,
-					     link_cleanup);
-
-	dev_dbg(&perf->ntb->pdev->dev, "%s called\n", __func__);
-
-	if (!perf->link_is_up)
-		cancel_delayed_work_sync(&perf->link_work);
-}
-
 static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
 {
 	struct perf_mw *mw;

@@ -787,7 +780,6 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 	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);
 
 	rc = ntb_set_ctx(ntb, perf, &perf_ops);
 	if (rc)

@@ -807,7 +799,6 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 
 err_ctx:
 	cancel_delayed_work_sync(&perf->link_work);
-	cancel_work_sync(&perf->link_cleanup);
 	kfree(perf);
 err_perf:
 	return rc;

@@ -823,7 +814,6 @@ static void perf_remove(struct ntb_client *client, struct ntb_dev *ntb)
 	mutex_lock(&perf->run_mutex);
 
 	cancel_delayed_work_sync(&perf->link_work);
-	cancel_work_sync(&perf->link_cleanup);
 
 	ntb_clear_ctx(ntb);
 	ntb_link_disable(ntb);
Signature · 1 line
2.1.4
maintainer
#12 lore raw
In reply to Logan Gunthorpe
Quoted 4 lines · Logan Gunthorpe

Sorry, I thought this was done but I found one more minor issue with these patches so I'm resubmitting them one last time. Besides this isuse, I think I have acks for all the patches and everything is working as I'd like.

Applying patch 05/10 to my ntb branch (as it is a bug fix and should go into 4.7)

Applying the rest to my ntb-next branch (which should go into 4.8)

Note: I'm including patch 09/10 in my tree, even though it is for the selftest subsystem. Given that Shuah Khan acked it, I assume this is the desired outcome.

Thanks,
Jon

Quoted 121 lines

Changes since v4:

1) In patch 0006, when setting a translation fails in tool_setup_mw, we now properly clean up the peer dma memory. Before that patch, it wasn't required because if tool_setup_mw failed the module would clean up all mws. After this patch, the clean up never happened, so it would return an error to the user but the DMA memory would still be allocated and the peer_trans file would report that it's ready but the debugfs file would not have been created. In v5, after an error, no DMA memory is allocated and it will still report that it's not ready to the user when reading peer_trans.

---

Changes since v3:

1) Check the error returned when setting the link in ntb_tool and pass it back to the user.

2) If an error occurs when setting the link down during a link test in ntb_test, just print unsupported and continue on. (For hardware that does not support setting the link down.)

3) Fix a race condition problem introduced by clearing the link is up flag in ntb_perf. We do this by getting rid of the link cleanup work and doing the few actions in the link event handler. Dave Jiang has already ok'd this approach.

---

Changes since v2:

1) As per Allan's suggestion, I've added a patch to postpone the peer memory window setup until the user requests it with a 'peer_trans*' file. This pushes the ntb_tool API closer to the kernel API and allows the link status file to return the raw status of the NTB link.

2) Change the link status file to return the value of ntb_link_is_up instead of the local 'memory window ready' value.

3) Change the link_event file to just block on write. As it was in v2, if multiple users attempted to use the link_event file they could corrupt the state another user had set. Reads to this file are no longer permitted.

4) Updates to the ntb_test script to accommodate the above changes.

5) Added some link tests to the ntb_test script. It will bring the link down and check that the other side agrees.

6) Added a minor bug fix (Patch #10) to ntb_perf. During discussions with Allen it was noticed that the link_is_up flag is never cleared if the link goes away.

---

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 (10):
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: Postpone memory window initialization for the user
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
ntb_perf: clear link_is_up flag when the link goes down.

 MAINTAINERS                             |   1 +
 drivers/ntb/test/ntb_perf.c             | 224 +++++++++------
 drivers/ntb/test/ntb_pingpong.c         |  62 ++++-
 drivers/ntb/test/ntb_tool.c             | 468 ++++++++++++++++++++++----------
 tools/testing/selftests/ntb/ntb_test.sh | 422 ++++++++++++++++++++++++++++
 5 files changed, 957 insertions(+), 220 deletions(-)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh

--
2.1.4

author
#13 lore raw

Great! Thanks Jon.

Logan

Quoted 139 lines · Jon Mason
Quoted 4 lines · Logan Gunthorpe

Sorry, I thought this was done but I found one more minor issue with these patches so I'm resubmitting them one last time. Besides this isuse, I think I have acks for all the patches and everything is working as I'd like.

Applying patch 05/10 to my ntb branch (as it is a bug fix and should go into 4.7)

Applying the rest to my ntb-next branch (which should go into 4.8)

Note: I'm including patch 09/10 in my tree, even though it is for the selftest subsystem. Given that Shuah Khan acked it, I assume this is the desired outcome.

Thanks,
Jon

Quoted 121 lines

Changes since v4:

1) In patch 0006, when setting a translation fails in tool_setup_mw, we now properly clean up the peer dma memory. Before that patch, it wasn't required because if tool_setup_mw failed the module would clean up all mws. After this patch, the clean up never happened, so it would return an error to the user but the DMA memory would still be allocated and the peer_trans file would report that it's ready but the debugfs file would not have been created. In v5, after an error, no DMA memory is allocated and it will still report that it's not ready to the user when reading peer_trans.

---

Changes since v3:

1) Check the error returned when setting the link in ntb_tool and pass it back to the user.

2) If an error occurs when setting the link down during a link test in ntb_test, just print unsupported and continue on. (For hardware that does not support setting the link down.)

3) Fix a race condition problem introduced by clearing the link is up flag in ntb_perf. We do this by getting rid of the link cleanup work and doing the few actions in the link event handler. Dave Jiang has already ok'd this approach.

---

Changes since v2:

1) As per Allan's suggestion, I've added a patch to postpone the peer memory window setup until the user requests it with a 'peer_trans*' file. This pushes the ntb_tool API closer to the kernel API and allows the link status file to return the raw status of the NTB link.

2) Change the link status file to return the value of ntb_link_is_up instead of the local 'memory window ready' value.

3) Change the link_event file to just block on write. As it was in v2, if multiple users attempted to use the link_event file they could corrupt the state another user had set. Reads to this file are no longer permitted.

4) Updates to the ntb_test script to accommodate the above changes.

5) Added some link tests to the ntb_test script. It will bring the link down and check that the other side agrees.

6) Added a minor bug fix (Patch #10) to ntb_perf. During discussions with Allen it was noticed that the link_is_up flag is never cleared if the link goes away.

---

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 (10):
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: Postpone memory window initialization for the user
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
ntb_perf: clear link_is_up flag when the link goes down.

 MAINTAINERS                             |   1 +
 drivers/ntb/test/ntb_perf.c             | 224 +++++++++------
 drivers/ntb/test/ntb_pingpong.c         |  62 ++++-
 drivers/ntb/test/ntb_tool.c             | 468 ++++++++++++++++++++++----------
 tools/testing/selftests/ntb/ntb_test.sh | 422 ++++++++++++++++++++++++++++
 5 files changed, 957 insertions(+), 220 deletions(-)
 create mode 100755 tools/testing/selftests/ntb/ntb_test.sh

--
2.1.4