mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* RE: [PATCH v5 06/10] ntb_tool: Postpone memory window initialization for the user
       [not found] ` <cab9ece5eee72425b93c22294d55f2ceab34840b.1466449562.git.logang@deltatee.com>
@ 2016-06-20 21:30   ` Allen Hubbe
  0 siblings, 0 replies; 3+ messages in thread
From: Allen Hubbe @ 2016-06-20 21:30 UTC (permalink / raw)
  To: 'Logan Gunthorpe', 'Jon Mason', 'Dave Jiang'
  Cc: 'Shuah Khan', 'Sudip Mukherjee',
	'Arnd Bergmann',
	linux-kernel, linux-ntb, linux-kselftest

From: 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 <logang@deltatee.com>

Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

> ---
>  drivers/ntb/test/ntb_tool.c | 366 +++++++++++++++++++++++++++-----------------
>  1 file changed, 228 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index cba31fd..1509b4c 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -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);
> --
> 2.1.4

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v5 00/10] NTB Selftest Script
       [not found] <cover.1466449562.git.logang@deltatee.com>
       [not found] ` <cab9ece5eee72425b93c22294d55f2ceab34840b.1466449562.git.logang@deltatee.com>
@ 2016-06-26 23:29 ` Jon Mason
  2016-06-27 16:08   ` Logan Gunthorpe
  1 sibling, 1 reply; 3+ messages in thread
From: Jon Mason @ 2016-06-26 23:29 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Dave Jiang, Allen Hubbe, Shuah Khan, Sudip Mukherjee,
	Arnd Bergmann, linux-kernel, linux-ntb, linux-kselftest

On Mon, Jun 20, 2016 at 01:15:03PM -0600, Logan Gunthorpe wrote:
> 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

> 
> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v5 00/10] NTB Selftest Script
  2016-06-26 23:29 ` [PATCH v5 00/10] NTB Selftest Script Jon Mason
@ 2016-06-27 16:08   ` Logan Gunthorpe
  0 siblings, 0 replies; 3+ messages in thread
From: Logan Gunthorpe @ 2016-06-27 16:08 UTC (permalink / raw)
  To: Jon Mason
  Cc: Dave Jiang, Allen Hubbe, Shuah Khan, Sudip Mukherjee,
	Arnd Bergmann, linux-kernel, linux-ntb, linux-kselftest

Great! Thanks Jon.

Logan

On 26/06/16 05:29 PM, Jon Mason wrote:
> On Mon, Jun 20, 2016 at 01:15:03PM -0600, Logan Gunthorpe wrote:
>> 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
> 
>>
>> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-06-27 16:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1466449562.git.logang@deltatee.com>
     [not found] ` <cab9ece5eee72425b93c22294d55f2ceab34840b.1466449562.git.logang@deltatee.com>
2016-06-20 21:30   ` [PATCH v5 06/10] ntb_tool: Postpone memory window initialization for the user Allen Hubbe
2016-06-26 23:29 ` [PATCH v5 00/10] NTB Selftest Script Jon Mason
2016-06-27 16:08   ` Logan Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome