Re: [PATCH 1/3] ntb_perf: Allow limiting the size of the memory windows

Re: [PATCH 1/3] ntb_perf: Allow limiting the size of the memory windows

[Jiang, Dave]

On Fri, 2016-06-03 at 14:50 -0600, Logan Gunthorpe wrote:
> On my system, dma_alloc_coherent won't produce memory anywhere
> near the size of the BAR. So I needed a way to limit this.
> 
> It's pretty much copied straight from ntb_transport.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/ntb/test/ntb_perf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index 8dfce9c..30635c8 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -83,6 +83,10 @@ MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
>  
>  static struct dentry *perf_debugfs_dir;
>  
> +static unsigned long max_mw_size;
> +module_param(max_mw_size, ulong, 0644);
> +MODULE_PARM_DESC(max_mw_size, "Limit size of large memory windows");
> +
>  static unsigned int seg_order = 19; /* 512K */
>  module_param(seg_order, uint, 0644);
>  MODULE_PARM_DESC(seg_order, "size order [n^2] of buffer segment for
> testing");
> @@ -472,6 +476,10 @@ static void perf_link_work(struct work_struct
> *work)
>  	dev_dbg(&perf->ntb->pdev->dev, "%s called\n", __func__);
>  
>  	size = perf->mw.phys_size;
> +
> +	if (max_mw_size && size > max_mw_size)
> +		size = max_mw_size;
> +
>  	ntb_peer_spad_write(ndev, MW_SZ_HIGH, upper_32_bits(size));
>  	ntb_peer_spad_write(ndev, MW_SZ_LOW, lower_32_bits(size));
>  	ntb_peer_spad_write(ndev, VERSION, PERF_VERSION);
> -- 
> 2.1.4
> 

RE: [PATCH 3/3] ntb_tool: Add memory window debug support

[Allen Hubbe]

From: Logan Gunthorpe 
> We allocate some memory window buffers when the link comes up, then we
> provide debugfs files to read/write each side of the link.
> 
> This is useful for debugging the mapping when writing new drivers.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Thanks! This was on my wish list.

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

> ---
>  drivers/ntb/test/ntb_tool.c | 258 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 257 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index 209ef7c..4c01057 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -89,6 +89,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
> +#include <linux/uaccess.h>
> 
>  #include <linux/ntb.h>
> 
> @@ -105,11 +106,29 @@ MODULE_VERSION(DRIVER_VERSION);
>  MODULE_AUTHOR(DRIVER_AUTHOR);
>  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 {
> +	resource_size_t size;
> +	u8 __iomem *local;
> +	u8 *peer;
> +	dma_addr_t peer_dma;
> +};
> +
>  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];
>  };
> 
>  #define SPAD_FNAME_SIZE 0x10
> @@ -124,6 +143,111 @@ 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;
> @@ -135,6 +259,11 @@ 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)
> @@ -443,8 +572,112 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
>  		      tool_peer_spad_read,
>  		      tool_peer_spad_write);
> 
> +
> +static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
> +			    size_t size, loff_t *offp)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +	ssize_t rc;
> +	loff_t pos = *offp;
> +	void *buf;
> +
> +	if (mw->local == NULL)
> +		return -EIO;
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= mw->size || !size)
> +		return 0;
> +	if (size > mw->size - pos)
> +		size = mw->size - pos;
> +
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	memcpy_fromio(buf, mw->local + pos, size);
> +	rc = copy_to_user(ubuf, buf, size);
> +	if (rc == size) {
> +		rc = -EFAULT;
> +		goto err_free;
> +	}
> +
> +	size -= rc;
> +	*offp = pos + size;
> +	rc = size;
> +
> +err_free:
> +	kfree(buf);
> +
> +	return rc;
> +}
> +
> +static ssize_t tool_mw_write(struct file *filep, const char __user *ubuf,
> +			     size_t size, loff_t *offp)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +	ssize_t rc;
> +	loff_t pos = *offp;
> +	void *buf;
> +
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= mw->size || !size)
> +		return 0;
> +	if (size > mw->size - pos)
> +		size = mw->size - pos;
> +
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	rc = copy_from_user(buf, ubuf, size);
> +	if (rc == size) {
> +		rc = -EFAULT;
> +		goto err_free;
> +	}
> +
> +	size -= rc;
> +	*offp = pos + size;
> +	rc = size;
> +
> +	memcpy_toio(mw->local + pos, buf, size);
> +
> +err_free:
> +	kfree(buf);
> +
> +	return rc;
> +}
> +
> +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)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +
> +	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)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +
> +	return simple_write_to_buffer(mw->peer, mw->size, offp, ubuf, size);
> +}
> +
> +static TOOL_FOPS_RDWR(tool_peer_mw_fops,
> +		      tool_peer_mw_read,
> +		      tool_peer_mw_write);
> +
>  static void tool_setup_dbgfs(struct tool_ctx *tc)
>  {
> +	int mw_count;
> +	int i;
> +
>  	/* This modules is useless without dbgfs... */
>  	if (!tool_dbgfs) {
>  		tc->dbgfs = NULL;
> @@ -473,6 +706,20 @@ 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++) {
> +		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);
> +		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
> +				    &tc->mws[i], &tool_peer_mw_fops);
> +
> +	}
>  }
> 
>  static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
> @@ -486,13 +733,15 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  	if (ntb_spad_is_unsafe(ntb))
>  		dev_dbg(&ntb->dev, "scratchpad is unsafe\n");
> 
> -	tc = kmalloc(sizeof(*tc), GFP_KERNEL);
> +	tc = kzalloc(sizeof(*tc), GFP_KERNEL);
>  	if (!tc) {
>  		rc = -ENOMEM;
>  		goto err_tc;
>  	}
> 
>  	tc->ntb = ntb;
> +	INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
> +	INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
> 
>  	tool_setup_dbgfs(tc);
> 
> @@ -507,6 +756,8 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
> 
>  err_ctx:
>  	debugfs_remove_recursive(tc->dbgfs);
> +	cancel_delayed_work_sync(&tc->link_work);
> +	cancel_work_sync(&tc->link_cleanup);
>  	kfree(tc);
>  err_tc:
>  	return rc;
> @@ -516,6 +767,11 @@ 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);
>  	ntb_link_disable(ntb);
> 
> --
> 2.1.4

Re: [PATCH 1/3] ntb_perf: Allow limiting the size of the memory windows

[Jon Mason]

On Fri, Jun 3, 2016 at 5:03 PM, Jiang, Dave <dave.jiang@intel.com> wrote:
> On Fri, 2016-06-03 at 14:50 -0600, Logan Gunthorpe wrote:
>> On my system, dma_alloc_coherent won't produce memory anywhere
>> near the size of the BAR. So I needed a way to limit this.
>>
>> It's pretty much copied straight from ntb_transport.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Acked-by: Dave Jiang <dave.jiang@intel.com>

Applied to my ntb-next branch

Thanks,
Jon

>> ---
>>  drivers/ntb/test/ntb_perf.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/ntb/test/ntb_perf.c
>> b/drivers/ntb/test/ntb_perf.c
>> index 8dfce9c..30635c8 100644
>> --- a/drivers/ntb/test/ntb_perf.c
>> +++ b/drivers/ntb/test/ntb_perf.c
>> @@ -83,6 +83,10 @@ MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
>>
>>  static struct dentry *perf_debugfs_dir;
>>
>> +static unsigned long max_mw_size;
>> +module_param(max_mw_size, ulong, 0644);
>> +MODULE_PARM_DESC(max_mw_size, "Limit size of large memory windows");
>> +
>>  static unsigned int seg_order = 19; /* 512K */
>>  module_param(seg_order, uint, 0644);
>>  MODULE_PARM_DESC(seg_order, "size order [n^2] of buffer segment for
>> testing");
>> @@ -472,6 +476,10 @@ static void perf_link_work(struct work_struct
>> *work)
>>       dev_dbg(&perf->ntb->pdev->dev, "%s called\n", __func__);
>>
>>       size = perf->mw.phys_size;
>> +
>> +     if (max_mw_size && size > max_mw_size)
>> +             size = max_mw_size;
>> +
>>       ntb_peer_spad_write(ndev, MW_SZ_HIGH, upper_32_bits(size));
>>       ntb_peer_spad_write(ndev, MW_SZ_LOW, lower_32_bits(size));
>>       ntb_peer_spad_write(ndev, VERSION, PERF_VERSION);
>> --
>> 2.1.4
>>

Re: [PATCH 3/3] ntb_tool: Add memory window debug support

[Jon Mason]

On Fri, Jun 3, 2016 at 5:20 PM, Allen Hubbe <Allen.Hubbe@emc.com> wrote:
> From: Logan Gunthorpe
>> We allocate some memory window buffers when the link comes up, then we
>> provide debugfs files to read/write each side of the link.
>>
>> This is useful for debugging the mapping when writing new drivers.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>
> Thanks! This was on my wish list.
>
> Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

Applied to my ntb-next branch

Thanks,
Jon

>> ---
>>  drivers/ntb/test/ntb_tool.c | 258 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 257 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
>> index 209ef7c..4c01057 100644
>> --- a/drivers/ntb/test/ntb_tool.c
>> +++ b/drivers/ntb/test/ntb_tool.c
>> @@ -89,6 +89,7 @@
>>  #include <linux/dma-mapping.h>
>>  #include <linux/pci.h>
>>  #include <linux/slab.h>
>> +#include <linux/uaccess.h>
>>
>>  #include <linux/ntb.h>
>>
>> @@ -105,11 +106,29 @@ MODULE_VERSION(DRIVER_VERSION);
>>  MODULE_AUTHOR(DRIVER_AUTHOR);
>>  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 {
>> +     resource_size_t size;
>> +     u8 __iomem *local;
>> +     u8 *peer;
>> +     dma_addr_t peer_dma;
>> +};
>> +
>>  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];
>>  };
>>
>>  #define SPAD_FNAME_SIZE 0x10
>> @@ -124,6 +143,111 @@ 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;
>> @@ -135,6 +259,11 @@ 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)
>> @@ -443,8 +572,112 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
>>                     tool_peer_spad_read,
>>                     tool_peer_spad_write);
>>
>> +
>> +static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>> +                         size_t size, loff_t *offp)
>> +{
>> +     struct tool_mw *mw = filep->private_data;
>> +     ssize_t rc;
>> +     loff_t pos = *offp;
>> +     void *buf;
>> +
>> +     if (mw->local == NULL)
>> +             return -EIO;
>> +     if (pos < 0)
>> +             return -EINVAL;
>> +     if (pos >= mw->size || !size)
>> +             return 0;
>> +     if (size > mw->size - pos)
>> +             size = mw->size - pos;
>> +
>> +     buf = kmalloc(size, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>> +
>> +     memcpy_fromio(buf, mw->local + pos, size);
>> +     rc = copy_to_user(ubuf, buf, size);
>> +     if (rc == size) {
>> +             rc = -EFAULT;
>> +             goto err_free;
>> +     }
>> +
>> +     size -= rc;
>> +     *offp = pos + size;
>> +     rc = size;
>> +
>> +err_free:
>> +     kfree(buf);
>> +
>> +     return rc;
>> +}
>> +
>> +static ssize_t tool_mw_write(struct file *filep, const char __user *ubuf,
>> +                          size_t size, loff_t *offp)
>> +{
>> +     struct tool_mw *mw = filep->private_data;
>> +     ssize_t rc;
>> +     loff_t pos = *offp;
>> +     void *buf;
>> +
>> +     if (pos < 0)
>> +             return -EINVAL;
>> +     if (pos >= mw->size || !size)
>> +             return 0;
>> +     if (size > mw->size - pos)
>> +             size = mw->size - pos;
>> +
>> +     buf = kmalloc(size, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>> +
>> +     rc = copy_from_user(buf, ubuf, size);
>> +     if (rc == size) {
>> +             rc = -EFAULT;
>> +             goto err_free;
>> +     }
>> +
>> +     size -= rc;
>> +     *offp = pos + size;
>> +     rc = size;
>> +
>> +     memcpy_toio(mw->local + pos, buf, size);
>> +
>> +err_free:
>> +     kfree(buf);
>> +
>> +     return rc;
>> +}
>> +
>> +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)
>> +{
>> +     struct tool_mw *mw = filep->private_data;
>> +
>> +     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)
>> +{
>> +     struct tool_mw *mw = filep->private_data;
>> +
>> +     return simple_write_to_buffer(mw->peer, mw->size, offp, ubuf, size);
>> +}
>> +
>> +static TOOL_FOPS_RDWR(tool_peer_mw_fops,
>> +                   tool_peer_mw_read,
>> +                   tool_peer_mw_write);
>> +
>>  static void tool_setup_dbgfs(struct tool_ctx *tc)
>>  {
>> +     int mw_count;
>> +     int i;
>> +
>>       /* This modules is useless without dbgfs... */
>>       if (!tool_dbgfs) {
>>               tc->dbgfs = NULL;
>> @@ -473,6 +706,20 @@ 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++) {
>> +             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);
>> +             debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
>> +                                 &tc->mws[i], &tool_peer_mw_fops);
>> +
>> +     }
>>  }
>>
>>  static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>> @@ -486,13 +733,15 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>>       if (ntb_spad_is_unsafe(ntb))
>>               dev_dbg(&ntb->dev, "scratchpad is unsafe\n");
>>
>> -     tc = kmalloc(sizeof(*tc), GFP_KERNEL);
>> +     tc = kzalloc(sizeof(*tc), GFP_KERNEL);
>>       if (!tc) {
>>               rc = -ENOMEM;
>>               goto err_tc;
>>       }
>>
>>       tc->ntb = ntb;
>> +     INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
>> +     INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
>>
>>       tool_setup_dbgfs(tc);
>>
>> @@ -507,6 +756,8 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>>
>>  err_ctx:
>>       debugfs_remove_recursive(tc->dbgfs);
>> +     cancel_delayed_work_sync(&tc->link_work);
>> +     cancel_work_sync(&tc->link_cleanup);
>>       kfree(tc);
>>  err_tc:
>>       return rc;
>> @@ -516,6 +767,11 @@ 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);
>>       ntb_link_disable(ntb);
>>
>> --
>> 2.1.4
>
>

Re: [PATCH 2/3] ntb_transport: Check the number of spads the hardware supports

[Jon Mason]

On Fri, Jun 03, 2016 at 02:50:32PM -0600, Logan Gunthorpe wrote:
> I'm working on hardware that currently has a limited number of
> scratchpad registers and ntb_ndev fails with no clue as to why. I
> feel it is better to fail early and provide a reasonable error message
> then to fail later on..
> 
> The same is done to ntb_perf, but it doesn't currently require enough
> spads to actually fail.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/ntb/ntb_transport.c | 9 +++++++--
>  drivers/ntb/test/ntb_perf.c | 8 ++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 2ef9d913..34e7a7a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1037,6 +1037,13 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>  	int node;
>  	int rc, i;
>  
> +	mw_count = ntb_mw_count(ndev);
> +	if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count*2)) {

Nit, please add spaces around '*' (per checkpatch)

> +		dev_err(&ndev->dev, "Not enough scratch pad registers for %s",
> +			NTB_TRANSPORT_NAME);
> +		return -EIO;
> +	}
> +
>  	if (ntb_db_is_unsafe(ndev))
>  		dev_dbg(&ndev->dev,
>  			"doorbell is unsafe, proceed anyway...\n");
> @@ -1052,8 +1059,6 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>  
>  	nt->ndev = ndev;
>  
> -	mw_count = ntb_mw_count(ndev);
> -
>  	nt->mw_count = mw_count;
>  
>  	nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec),
> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> index 30635c8..1815592 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -143,8 +143,6 @@ enum {
>  	VERSION = 0,
>  	MW_SZ_HIGH,
>  	MW_SZ_LOW,
> -	SPAD_MSG,
> -	SPAD_ACK,

Please explicitly point out that this is being modified in the commit
message.  I don't see them being used, so probably not a big deal
(unless Dave Jiang has something queued that will use it).

>  	MAX_SPAD
>  };
>  
> @@ -698,6 +696,12 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
>  
>  	node = dev_to_node(&pdev->dev);
>  
> +	if (ntb_spad_count(ntb) < MAX_SPAD) {
> +		dev_err(&ntb->dev, "Not enough scratch pad registers for %s",
> +			DRIVER_NAME);
> +		return -EIO;
> +	}

Move this check above the dev_to_node assignment above.

Thanks,
Jon

> +
>  	perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
>  	if (!perf) {
>  		rc = -ENOMEM;
> -- 
> 2.1.4
> 

Re: [PATCH 2/3] ntb_transport: Check the number of spads the hardware supports

[Logan Gunthorpe]

Hi Jon,

Thanks for the feedback. I'll send an updated patch in a moment.

On 04/06/16 09:40 AM, Jon Mason wrote:
> Nit, please add spaces around '*' (per checkpatch)

I'll change this, but I did run it through checkpatch and it did not
warn about this.

> Please explicitly point out that this is being modified in the commit
> message.  I don't see them being used, so probably not a big deal
> (unless Dave Jiang has something queued that will use it).

Done. I feel like he can always add them back in when he adds the
functionality. This way, when he does, MAX_SPAD will be updated and the
check will still be correct.

> Move this check above the dev_to_node assignment above.

Done.


Logan

Re: [PATCH v2 2/3] ntb_transport: Check the number of spads the hardware supports

[Jiang, Dave]

On Tue, 2016-06-07 at 11:20 -0600, Logan Gunthorpe wrote:
> I'm working on hardware that currently has a limited number of
> scratchpad registers and ntb_ndev fails with no clue as to why. I
> feel it is better to fail early and provide a reasonable error
> message
> then to fail later on.
> 
> The same is done to ntb_perf, but it doesn't currently require enough
> spads to actually fail. I've also removed the unused SPAD_MSG and
> SPAD_ACK enums so that MAX_SPAD accurately reflects the number of
> spads used.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/ntb/ntb_transport.c | 9 +++++++--
>  drivers/ntb/test/ntb_perf.c | 8 ++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c
> b/drivers/ntb/ntb_transport.c
> index 2ef9d913..6d9940a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1037,6 +1037,13 @@ static int ntb_transport_probe(struct
> ntb_client *self, struct ntb_dev *ndev)
>  	int node;
>  	int rc, i;
>  
> +	mw_count = ntb_mw_count(ndev);
> +	if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) {
> +		dev_err(&ndev->dev, "Not enough scratch pad
> registers for %s",
> +			NTB_TRANSPORT_NAME);
> +		return -EIO;
> +	}
> +
>  	if (ntb_db_is_unsafe(ndev))
>  		dev_dbg(&ndev->dev,
>  			"doorbell is unsafe, proceed anyway...\n");
> @@ -1052,8 +1059,6 @@ static int ntb_transport_probe(struct
> ntb_client *self, struct ntb_dev *ndev)
>  
>  	nt->ndev = ndev;
>  
> -	mw_count = ntb_mw_count(ndev);
> -
>  	nt->mw_count = mw_count;
>  
>  	nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec),
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index 30635c8..4368519 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -143,8 +143,6 @@ enum {
>  	VERSION = 0,
>  	MW_SZ_HIGH,
>  	MW_SZ_LOW,
> -	SPAD_MSG,
> -	SPAD_ACK,
>  	MAX_SPAD
>  };
>  
> @@ -696,6 +694,12 @@ static int perf_probe(struct ntb_client *client,
> struct ntb_dev *ntb)
>  	int node;
>  	int rc = 0;
>  
> +	if (ntb_spad_count(ntb) < MAX_SPAD) {
> +		dev_err(&ntb->dev, "Not enough scratch pad registers
> for %s",
> +			DRIVER_NAME);
> +		return -EIO;
> +	}
> +
>  	node = dev_to_node(&pdev->dev);
>  
>  	perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
> -- 
> 2.1.4
> 

Re: [PATCH v2 2/3] ntb_transport: Check the number of spads the hardware supports

[Jon Mason]

On Tue, Jun 7, 2016 at 1:24 PM, Jiang, Dave <dave.jiang@intel.com> wrote:
> On Tue, 2016-06-07 at 11:20 -0600, Logan Gunthorpe wrote:
>> I'm working on hardware that currently has a limited number of
>> scratchpad registers and ntb_ndev fails with no clue as to why. I
>> feel it is better to fail early and provide a reasonable error
>> message
>> then to fail later on.
>>
>> The same is done to ntb_perf, but it doesn't currently require enough
>> spads to actually fail. I've also removed the unused SPAD_MSG and
>> SPAD_ACK enums so that MAX_SPAD accurately reflects the number of
>> spads used.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>
> Acked-by: Dave Jiang <dave.jiang@intel.com>

Added to the ntb-next branch.

thanks,
Jon

>> ---
>>  drivers/ntb/ntb_transport.c | 9 +++++++--
>>  drivers/ntb/test/ntb_perf.c | 8 ++++++--
>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/ntb/ntb_transport.c
>> b/drivers/ntb/ntb_transport.c
>> index 2ef9d913..6d9940a 100644
>> --- a/drivers/ntb/ntb_transport.c
>> +++ b/drivers/ntb/ntb_transport.c
>> @@ -1037,6 +1037,13 @@ static int ntb_transport_probe(struct
>> ntb_client *self, struct ntb_dev *ndev)
>>       int node;
>>       int rc, i;
>>
>> +     mw_count = ntb_mw_count(ndev);
>> +     if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) {
>> +             dev_err(&ndev->dev, "Not enough scratch pad
>> registers for %s",
>> +                     NTB_TRANSPORT_NAME);
>> +             return -EIO;
>> +     }
>> +
>>       if (ntb_db_is_unsafe(ndev))
>>               dev_dbg(&ndev->dev,
>>                       "doorbell is unsafe, proceed anyway...\n");
>> @@ -1052,8 +1059,6 @@ static int ntb_transport_probe(struct
>> ntb_client *self, struct ntb_dev *ndev)
>>
>>       nt->ndev = ndev;
>>
>> -     mw_count = ntb_mw_count(ndev);
>> -
>>       nt->mw_count = mw_count;
>>
>>       nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec),
>> diff --git a/drivers/ntb/test/ntb_perf.c
>> b/drivers/ntb/test/ntb_perf.c
>> index 30635c8..4368519 100644
>> --- a/drivers/ntb/test/ntb_perf.c
>> +++ b/drivers/ntb/test/ntb_perf.c
>> @@ -143,8 +143,6 @@ enum {
>>       VERSION = 0,
>>       MW_SZ_HIGH,
>>       MW_SZ_LOW,
>> -     SPAD_MSG,
>> -     SPAD_ACK,
>>       MAX_SPAD
>>  };
>>
>> @@ -696,6 +694,12 @@ static int perf_probe(struct ntb_client *client,
>> struct ntb_dev *ntb)
>>       int node;
>>       int rc = 0;
>>
>> +     if (ntb_spad_count(ntb) < MAX_SPAD) {
>> +             dev_err(&ntb->dev, "Not enough scratch pad registers
>> for %s",
>> +                     DRIVER_NAME);
>> +             return -EIO;
>> +     }
>> +
>>       node = dev_to_node(&pdev->dev);
>>
>>       perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
>> --
>> 2.1.4
>>
>
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/1465320253.16234.176.camel%40intel.com.
> For more options, visit https://groups.google.com/d/optout.