On Sat, Jun 11, 2016 at 11:28 AM, Logan Gunthorpe <logang@xxxxxxxxxxxx> wrote:
> Hey Allen,
>
> Thanks for the feedback it's a bit more complicated but I don't object to
> that. I'll work something up on Monday.
>
> I was trying to avoid adding link controls, but if we do, would you say the
> module should still enable the link when it's installed? Or would we have
> the user explicitly have to enable the link before using it?
I would vote to keep the current behavior and enable the link when the
module loads.
>
> Thanks,
>
> Logan
>
>
> On 10/06/16 08:27 PM, Allen Hubbe wrote:
>>
>> On Fri, Jun 10, 2016 at 6:54 PM, Logan Gunthorpe <logang@xxxxxxxxxxxx>
>> wrote:
>>>
>>> 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
>>> 0 or 1 depending on the link status. For scripting convenience, writing
>>> will block until the link is up (discarding anything that was written).
>>>
>>> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
>>> ---
>>> drivers/ntb/test/ntb_tool.c | 45
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
>>> index 954e1d5..116352e 100644
>>> --- a/drivers/ntb/test/ntb_tool.c
>>> +++ b/drivers/ntb/test/ntb_tool.c
>>> @@ -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 > $DBG_DIR/link
>>
>>
>> I think a file to get and set the link status is a good idea, but the
>> way it is done as proposed here is not in a similar style to other
>> ntb_tool operations. Other operations simply read a register and
>> format the value, or scan a value and write a register. Similarly, I
>> think the link status could be done in the same way: use the read file
>> operation to get the current status with ntb_link_is_up(), and use the
>> file write operation to enable or disable the link with
>> ntb_link_enable() and ntb_link_disable().
>>
>> Waiting for link status is an interesting concept, too. Really, one
>> might be interested in a change in link status, whether up or down.
>> What about a link event file that supports write to arm the event, and
>> read to block for the event. Consider an implementation based on
>> <linux/completion.h>. It would be used in combination with the link
>> status file, above, as follows.
>>
>> 1: Write 1 to the event file. This arms the event.
>> - The event will be disarmed by the next tool_link_event().
>>
>> 2: The application may read the link status file if it is interested
>> in waiting for a particular event.
>>
>> 3. The application may wait for an event by reading the event file
>> - The application will wait as long as the event is still armed.
>> - If the event was disarmed before waiting, the application will not
>> block.
>>
>> 4. The application should read the link status again.
>>
>> In any case, I think it would be more expected and natural to block
>> while reading a file versus writing it.
>>
>>> + *
>>> * # Set the doorbell mask
>>> * root@self# echo 's 1' > $DBG_DIR/mask
>>> *
>>> @@ -127,6 +133,7 @@ struct tool_ctx {
>>> struct work_struct link_cleanup;
>>> bool link_is_up;
>>> struct delayed_work link_work;
>>> + wait_queue_head_t link_wq;
>>> int mw_count;
>>> struct tool_mw mws[MAX_MWS];
>>> };
>>> @@ -237,6 +244,7 @@ static void tool_link_work(struct work_struct *work)
>>> "Error setting up memory windows: %d\n", rc);
>>>
>>> tc->link_is_up = true;
>>> + wake_up(&tc->link_wq);
>>> }
>>>
>>> static void tool_link_cleanup(struct work_struct *work)
>>> @@ -573,6 +581,39 @@ 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;
>>> + ssize_t pos, rc;
>>> +
>>> + buf = kmalloc(64, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + pos = scnprintf(buf, 64, "%d\n", tc->link_is_up);
>>> + rc = simple_read_from_buffer(ubuf, size, offp, buf, pos);
>>> +
>>> + kfree(buf);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +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;
>>> +
>>> + if (wait_event_interruptible(tc->link_wq, tc->link_is_up))
>>> + return -ERESTART;
>>> +
>>> + return size;
>>> +}
>>> +
>>> +static TOOL_FOPS_RDWR(tool_link_fops,
>>> + tool_link_read,
>>> + tool_link_write);
>>>
>>> static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>>> size_t size, loff_t *offp)
>>> @@ -708,6 +749,9 @@ 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);
>>> +
>>> mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
>>> for (i = 0; i < mw_count; i++) {
>>> char buf[30];
>>> @@ -741,6 +785,7 @@ 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
