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

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

[Allen Hubbe]

From: Logan Gunthorpe
> In order to more successfully script with ntb_tool it's useful to
> have a link file to check the link status so that the script
> doesn't use the other files until the link is up.
> 
> This commit adds a 'link' file to the debugfs directory which reads a
> boolean (Y or N) depending on the link status. Writing to the file will
> change the link state using ntb_link_enable or ntb_link_disable.
> 
> A 'link_event' file is also provided so an application can block until
> the link changes to a desired state. This file is primed by writing a
> boolean. If the user writes a 1, the next read of link_event will
> block until the link is up. If the user writes a 0, the next read
> will block until the link is down. Besides blocking, reads return the
> same value as the 'link' file.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/ntb/test/ntb_tool.c | 111 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index cba31fd..9bebd0d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -59,6 +59,13 @@
>   *
>   * Eg: check if clearing the doorbell mask generates an interrupt.
>   *
> + * # Check the link status
> + * root@self# cat $DBG_DIR/link
> + *
> + * # Block until the link is up
> + * root@self# echo Y > $DBG_DIR/link_event
> + * root@self# cat $DBG_DIR/link_event
> + *
>   * # Set the doorbell mask
>   * root@self# echo 's 1' > $DBG_DIR/mask
>   *
> @@ -126,7 +133,9 @@ struct tool_ctx {
>  	struct dentry *dbgfs;
>  	struct work_struct link_cleanup;
>  	bool link_is_up;

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

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

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

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

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

> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_link_event(void *ctx)
> @@ -578,6 +591,95 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
>  		      tool_peer_spad_read,
>  		      tool_peer_spad_write);
> 
> +static ssize_t tool_link_read(struct file *filep, char __user *ubuf,
> +			      size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[3];
> +
> +	buf[0] = tc->link_is_up ? 'Y' : 'N';

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

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

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

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

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

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

> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_event_fops,
> +		      tool_link_event_read,
> +		      tool_link_event_write);
> 
>  static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>  			    size_t size, loff_t *offp)
> @@ -658,7 +760,6 @@ static TOOL_FOPS_RDWR(tool_mw_fops,
>  		      tool_mw_read,
>  		      tool_mw_write);
> 
> -
>  static ssize_t tool_peer_mw_read(struct file *filep, char __user *ubuf,
>  				   size_t size, loff_t *offp)
>  {
> @@ -713,6 +814,12 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>  	debugfs_create_file("peer_spad", S_IRUSR | S_IWUSR, tc->dbgfs,
>  			    tc, &tool_peer_spad_fops);
> 
> +	debugfs_create_file("link", S_IRUSR | S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_fops);
> +
> +	debugfs_create_file("link_event", S_IRUSR | S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_event_fops);
> +
>  	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
>  	for (i = 0; i < mw_count; i++) {
>  		char buf[30];
> @@ -746,8 +853,10 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  	}
> 
>  	tc->ntb = ntb;
> +	init_waitqueue_head(&tc->link_wq);
>  	INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
>  	INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
> +	tc->link_event = true;
> 
>  	tool_setup_dbgfs(tc);
> 
> --
> 2.1.4

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

[Logan Gunthorpe]

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

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


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

Technically, yes.

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

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

>> +	wake_up(&tc->link_wq);
>>  }
>>
>>  static void tool_link_event(void *ctx)
>> @@ -578,6 +591,95 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
>>  		      tool_peer_spad_read,
>>  		      tool_peer_spad_write);
>>
>> +static ssize_t tool_link_read(struct file *filep, char __user *ubuf,
>> +			      size_t size, loff_t *offp)
>> +{
>> +	struct tool_ctx *tc = filep->private_data;
>> +	char buf[3];
>> +
>> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> 
> I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).

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

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

See above.

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

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


Logan

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

[Allen Hubbe]

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

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

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

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

> 
> >> +	wake_up(&tc->link_wq);
> >>  }
> >>
> >>  static void tool_link_event(void *ctx)
> >> @@ -578,6 +591,95 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
> >>  		      tool_peer_spad_read,
> >>  		      tool_peer_spad_write);
> >>
> >> +static ssize_t tool_link_read(struct file *filep, char __user *ubuf,
> >> +			      size_t size, loff_t *offp)
> >> +{
> >> +	struct tool_ctx *tc = filep->private_data;
> >> +	char buf[3];
> >> +
> >> +	buf[0] = tc->link_is_up ? 'Y' : 'N';
> >
> > I think tc->link_is_up should instead be ntb_link_is_up(tc->ntb).
> 
> I disagree. Bad things will happen if the user waits on the event and
> then immediately uses the memory windows. It will just be buggy and
> racy. I can't see a situation where the user would want to wait for the
> link to come up and not have everything in ntb_tool ready and usable.

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

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

The name link_event is ok.

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

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

[Logan Gunthorpe]

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

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

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

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


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

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

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

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

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


Logan

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

[Allen Hubbe]

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

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

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

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

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

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

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

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

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

> 
> 
> Logan

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

[Logan Gunthorpe]

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

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


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

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

Logan