2013-07-23 15:37:03

by Andy Shevchenko

Subject: [PATCH 1/3] dmatest: make module parameters writable

The debugfs interface brought a copy of the test case parameters. This makes
different set of values under /sys/module/dmatest/parameters/ and
/sys/kernel/debug/dmatest/. The user might be confused by the divergence of
values.

The proposed solution in this patch is to make module parameters writable and
remove them from the debugfs. Though we're still using debugfs to control test
runner and getting results.

Documentation part is updated accordingly.

Signed-off-by: Andy Shevchenko <[email protected]>
Suggested-by: Dan Williams <[email protected]>
---
Documentation/dmatest.txt | 15 +++--
drivers/dma/dmatest.c | 159 ++++++----------------------------------------
2 files changed, 28 insertions(+), 146 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index 132a094..a2b5663 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -16,15 +16,16 @@ be built as module or inside kernel. Let's consider those cases.
Part 2 - When dmatest is built as a module...

After mounting debugfs and loading the module, the /sys/kernel/debug/dmatest
-folder with nodes will be created. They are the same as module parameters with
-addition of the 'run' node that controls run and stop phases of the test.
+folder with nodes will be created. There are two important files located. First
+is the 'run' node that controls run and stop phases of the test, and the second
+one, 'results', is used to get the test case results.

Note that in this case test will not run on load automatically.

Example of usage:
- % echo dma0chan0 > /sys/kernel/debug/dmatest/channel
- % echo 2000 > /sys/kernel/debug/dmatest/timeout
- % echo 1 > /sys/kernel/debug/dmatest/iterations
+ % echo dma0chan0 > /sys/module/dmatest/parameters/channel
+ % echo 2000 > /sys/module/dmatest/parameters/timeout
+ % echo 1 > /sys/module/dmatest/parameters/iterations
% echo 1 > /sys/kernel/debug/dmatest/run

Hint: available channel list could be extracted by running the following
@@ -55,8 +56,8 @@ for the first performed test. After user gets a control, the test could be
re-run with the same or different parameters. For the details see the above
section "Part 2 - When dmatest is built as a module..."

-In both cases the module parameters are used as initial values for the test case.
-You always could check them at run-time by running
+In both cases the module parameters are used as the actual values for the test
+case. You always could check them at run-time by running
% grep -H . /sys/module/dmatest/parameters/*

Part 4 - Gathering the test results
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index e88ded2..91716f4 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -25,44 +25,46 @@
#include <linux/seq_file.h>

static unsigned int test_buf_size = 16384;
-module_param(test_buf_size, uint, S_IRUGO);
+module_param(test_buf_size, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(test_buf_size, "Size of the memcpy test buffer");

static char test_channel[20];
-module_param_string(channel, test_channel, sizeof(test_channel), S_IRUGO);
+module_param_string(channel, test_channel, sizeof(test_channel),
+ S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(channel, "Bus ID of the channel to test (default: any)");

static char test_device[20];
-module_param_string(device, test_device, sizeof(test_device), S_IRUGO);
+module_param_string(device, test_device, sizeof(test_device),
+ S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(device, "Bus ID of the DMA Engine to test (default: any)");

static unsigned int threads_per_chan = 1;
-module_param(threads_per_chan, uint, S_IRUGO);
+module_param(threads_per_chan, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(threads_per_chan,
"Number of threads to start per channel (default: 1)");

static unsigned int max_channels;
-module_param(max_channels, uint, S_IRUGO);
+module_param(max_channels, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(max_channels,
"Maximum number of channels to use (default: all)");

static unsigned int iterations;
-module_param(iterations, uint, S_IRUGO);
+module_param(iterations, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(iterations,
"Iterations before stopping test (default: infinite)");

static unsigned int xor_sources = 3;
-module_param(xor_sources, uint, S_IRUGO);
+module_param(xor_sources, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(xor_sources,
"Number of xor source buffers (default: 3)");

static unsigned int pq_sources = 3;
-module_param(pq_sources, uint, S_IRUGO);
+module_param(pq_sources, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(pq_sources,
"Number of p+q source buffers (default: 3)");

static int timeout = 3000;
-module_param(timeout, uint, S_IRUGO);
+module_param(timeout, uint, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
"Pass -1 for infinite timeout");

@@ -193,7 +195,6 @@ struct dmatest_info {

/* debugfs related stuff */
struct dentry *root;
- struct dmatest_params dbgfs_params;

/* Test results */
struct list_head results;
@@ -1007,7 +1008,15 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
result_free(info, NULL);

/* Copy test parameters */
- memcpy(params, &info->dbgfs_params, sizeof(*params));
+ params->buf_size = test_buf_size;
+ strlcpy(params->channel, strim(test_channel), sizeof(params->channel));
+ strlcpy(params->device, strim(test_device), sizeof(params->device));
+ params->threads_per_chan = threads_per_chan;
+ params->max_channels = max_channels;
+ params->iterations = iterations;
+ params->xor_sources = xor_sources;
+ params->pq_sources = pq_sources;
+ params->timeout = timeout;

/* Run test with new parameters */
return __run_threaded_test(info);
@@ -1029,71 +1038,6 @@ static bool __is_threaded_test_run(struct dmatest_info *info)
return false;
}

-static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
- const void __user *from, size_t count)
-{
- char tmp[20];
- ssize_t len;
-
- len = simple_write_to_buffer(tmp, sizeof(tmp) - 1, ppos, from, count);
- if (len >= 0) {
- tmp[len] = '\0';
- strlcpy(to, strim(tmp), available);
- }
-
- return len;
-}
-
-static ssize_t dtf_read_channel(struct file *file, char __user *buf,
- size_t count, loff_t *ppos)
-{
- struct dmatest_info *info = file->private_data;
- return simple_read_from_buffer(buf, count, ppos,
- info->dbgfs_params.channel,
- strlen(info->dbgfs_params.channel));
-}
-
-static ssize_t dtf_write_channel(struct file *file, const char __user *buf,
- size_t size, loff_t *ppos)
-{
- struct dmatest_info *info = file->private_data;
- return dtf_write_string(info->dbgfs_params.channel,
- sizeof(info->dbgfs_params.channel),
- ppos, buf, size);
-}
-
-static const struct file_operations dtf_channel_fops = {
- .read = dtf_read_channel,
- .write = dtf_write_channel,
- .open = simple_open,
- .llseek = default_llseek,
-};
-
-static ssize_t dtf_read_device(struct file *file, char __user *buf,
- size_t count, loff_t *ppos)
-{
- struct dmatest_info *info = file->private_data;
- return simple_read_from_buffer(buf, count, ppos,
- info->dbgfs_params.device,
- strlen(info->dbgfs_params.device));
-}
-
-static ssize_t dtf_write_device(struct file *file, const char __user *buf,
- size_t size, loff_t *ppos)
-{
- struct dmatest_info *info = file->private_data;
- return dtf_write_string(info->dbgfs_params.device,
- sizeof(info->dbgfs_params.device),
- ppos, buf, size);
-}
-
-static const struct file_operations dtf_device_fops = {
- .read = dtf_read_device,
- .write = dtf_write_device,
- .open = simple_open,
- .llseek = default_llseek,
-};
-
static ssize_t dtf_read_run(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
@@ -1187,7 +1131,6 @@ static const struct file_operations dtf_results_fops = {
static int dmatest_register_dbgfs(struct dmatest_info *info)
{
struct dentry *d;
- struct dmatest_params *params = &info->dbgfs_params;
int ret = -ENOMEM;

d = debugfs_create_dir("dmatest", NULL);
@@ -1198,56 +1141,6 @@ static int dmatest_register_dbgfs(struct dmatest_info *info)

info->root = d;

- /* Copy initial values */
- memcpy(params, &info->params, sizeof(*params));
-
- /* Test parameters */
-
- d = debugfs_create_u32("test_buf_size", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->buf_size);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_file("channel", S_IRUGO | S_IWUSR, info->root,
- info, &dtf_channel_fops);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_file("device", S_IRUGO | S_IWUSR, info->root,
- info, &dtf_device_fops);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("threads_per_chan", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->threads_per_chan);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("max_channels", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->max_channels);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("iterations", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->iterations);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("xor_sources", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->xor_sources);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("pq_sources", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->pq_sources);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
- d = debugfs_create_u32("timeout", S_IWUSR | S_IRUGO, info->root,
- (u32 *)&params->timeout);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
-
/* Run or stop threaded test */
d = debugfs_create_file("run", S_IWUSR | S_IRUGO, info->root,
info, &dtf_run_fops);
@@ -1272,7 +1165,6 @@ err_root:
static int __init dmatest_init(void)
{
struct dmatest_info *info = &test_info;
- struct dmatest_params *params = &info->params;
int ret;

memset(info, 0, sizeof(*info));
@@ -1283,17 +1175,6 @@ static int __init dmatest_init(void)
mutex_init(&info->results_lock);
INIT_LIST_HEAD(&info->results);

- /* Set default parameters */
- params->buf_size = test_buf_size;
- strlcpy(params->channel, test_channel, sizeof(params->channel));
- strlcpy(params->device, test_device, sizeof(params->device));
- params->threads_per_chan = threads_per_chan;
- params->max_channels = max_channels;
- params->iterations = iterations;
- params->xor_sources = xor_sources;
- params->pq_sources = pq_sources;
- params->timeout = timeout;
-
ret = dmatest_register_dbgfs(info);
if (ret)
return ret;
--
1.8.3.2

2013-07-23 15:37:05

by Andy Shevchenko

Subject: [PATCH 3/3] dmatest: print message on debug level in case of no error

Let's move the behaviour of printing no error message back to the pre v3.10
times. It means we will use debug level in the described case, and a warning
level otherwise.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/dma/dmatest.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index eae5989..92f796c 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -407,7 +407,11 @@ static int thread_result_add(struct dmatest_info *info,
list_add_tail(&tr->node, &r->results);
mutex_unlock(&info->results_lock);

- pr_warn("%s\n", thread_result_get(r->name, tr));
+ if (tr->type == DMATEST_ET_OK)
+ pr_debug("%s\n", thread_result_get(r->name, tr));
+ else
+ pr_warn("%s\n", thread_result_get(r->name, tr));
+
return 0;
}

--
1.8.3.2

2013-07-23 15:37:04

by Andy Shevchenko

Subject: [PATCH 2/3] dmatest: remove IS_ERR_OR_NULL checks of debugfs calls

There is a really little chance when we are able to create a directory and are
not able to create nodes under it. So, this patch just removes those checks.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/dma/dmatest.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 91716f4..eae5989 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -1131,7 +1131,6 @@ static const struct file_operations dtf_results_fops = {
static int dmatest_register_dbgfs(struct dmatest_info *info)
{
struct dentry *d;
- int ret = -ENOMEM;

d = debugfs_create_dir("dmatest", NULL);
if (IS_ERR(d))
@@ -1142,24 +1141,18 @@ static int dmatest_register_dbgfs(struct dmatest_info *info)
info->root = d;

/* Run or stop threaded test */
- d = debugfs_create_file("run", S_IWUSR | S_IRUGO, info->root,
- info, &dtf_run_fops);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
+ debugfs_create_file("run", S_IWUSR | S_IRUGO, info->root, info,
+ &dtf_run_fops);

/* Results of test in progress */
- d = debugfs_create_file("results", S_IRUGO, info->root, info,
- &dtf_results_fops);
- if (IS_ERR_OR_NULL(d))
- goto err_node;
+ debugfs_create_file("results", S_IRUGO, info->root, info,
+ &dtf_results_fops);

return 0;

-err_node:
- debugfs_remove_recursive(info->root);
err_root:
pr_err("dmatest: Failed to initialize debugfs\n");
- return ret;
+ return -ENOMEM;
}

static int __init dmatest_init(void)
--
1.8.3.2

2013-07-31 14:28:28

by Andy Shevchenko

Subject: Re: [PATCH 1/3] dmatest: make module parameters writable

On Tue, 2013-07-23 at 18:36 +0300, Andy Shevchenko wrote:
> The debugfs interface brought a copy of the test case parameters. This makes
> different set of values under /sys/module/dmatest/parameters/ and
> /sys/kernel/debug/dmatest/. The user might be confused by the divergence of
> values.
>
> The proposed solution in this patch is to make module parameters writable and
> remove them from the debugfs. Though we're still using debugfs to control test
> runner and getting results.
>
> Documentation part is updated accordingly.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Suggested-by: Dan Williams <[email protected]>

Dan, do you have any comments against this series?
I'd like to have this incorporated into v3.12.


> ---
> Documentation/dmatest.txt | 15 +++--
> drivers/dma/dmatest.c | 159 ++++++----------------------------------------
> 2 files changed, 28 insertions(+), 146 deletions(-)
>
> diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
> index 132a094..a2b5663 100644
> --- a/Documentation/dmatest.txt
> +++ b/Documentation/dmatest.txt
> @@ -16,15 +16,16 @@ be built as module or inside kernel. Let's consider those cases.
> Part 2 - When dmatest is built as a module...
>
> After mounting debugfs and loading the module, the /sys/kernel/debug/dmatest
> -folder with nodes will be created. They are the same as module parameters with
> -addition of the 'run' node that controls run and stop phases of the test.
> +folder with nodes will be created. There are two important files located. First
> +is the 'run' node that controls run and stop phases of the test, and the second
> +one, 'results', is used to get the test case results.
>
> Note that in this case test will not run on load automatically.
>
> Example of usage:
> - % echo dma0chan0 > /sys/kernel/debug/dmatest/channel
> - % echo 2000 > /sys/kernel/debug/dmatest/timeout
> - % echo 1 > /sys/kernel/debug/dmatest/iterations
> + % echo dma0chan0 > /sys/module/dmatest/parameters/channel
> + % echo 2000 > /sys/module/dmatest/parameters/timeout
> + % echo 1 > /sys/module/dmatest/parameters/iterations
> % echo 1 > /sys/kernel/debug/dmatest/run
>
> Hint: available channel list could be extracted by running the following
> @@ -55,8 +56,8 @@ for the first performed test. After user gets a control, the test could be
> re-run with the same or different parameters. For the details see the above
> section "Part 2 - When dmatest is built as a module..."
>
> -In both cases the module parameters are used as initial values for the test case.
> -You always could check them at run-time by running
> +In both cases the module parameters are used as the actual values for the test
> +case. You always could check them at run-time by running
> % grep -H . /sys/module/dmatest/parameters/*
>
> Part 4 - Gathering the test results
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index e88ded2..91716f4 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -25,44 +25,46 @@
> #include <linux/seq_file.h>
>
> static unsigned int test_buf_size = 16384;
> -module_param(test_buf_size, uint, S_IRUGO);
> +module_param(test_buf_size, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(test_buf_size, "Size of the memcpy test buffer");
>
> static char test_channel[20];
> -module_param_string(channel, test_channel, sizeof(test_channel), S_IRUGO);
> +module_param_string(channel, test_channel, sizeof(test_channel),
> + S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(channel, "Bus ID of the channel to test (default: any)");
>
> static char test_device[20];
> -module_param_string(device, test_device, sizeof(test_device), S_IRUGO);
> +module_param_string(device, test_device, sizeof(test_device),
> + S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(device, "Bus ID of the DMA Engine to test (default: any)");
>
> static unsigned int threads_per_chan = 1;
> -module_param(threads_per_chan, uint, S_IRUGO);
> +module_param(threads_per_chan, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(threads_per_chan,
> "Number of threads to start per channel (default: 1)");
>
> static unsigned int max_channels;
> -module_param(max_channels, uint, S_IRUGO);
> +module_param(max_channels, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(max_channels,
> "Maximum number of channels to use (default: all)");
>
> static unsigned int iterations;
> -module_param(iterations, uint, S_IRUGO);
> +module_param(iterations, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(iterations,
> "Iterations before stopping test (default: infinite)");
>
> static unsigned int xor_sources = 3;
> -module_param(xor_sources, uint, S_IRUGO);
> +module_param(xor_sources, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(xor_sources,
> "Number of xor source buffers (default: 3)");
>
> static unsigned int pq_sources = 3;
> -module_param(pq_sources, uint, S_IRUGO);
> +module_param(pq_sources, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(pq_sources,
> "Number of p+q source buffers (default: 3)");
>
> static int timeout = 3000;
> -module_param(timeout, uint, S_IRUGO);
> +module_param(timeout, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
> "Pass -1 for infinite timeout");
>
> @@ -193,7 +195,6 @@ struct dmatest_info {
>
> /* debugfs related stuff */
> struct dentry *root;
> - struct dmatest_params dbgfs_params;
>
> /* Test results */
> struct list_head results;
> @@ -1007,7 +1008,15 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
> result_free(info, NULL);
>
> /* Copy test parameters */
> - memcpy(params, &info->dbgfs_params, sizeof(*params));
> + params->buf_size = test_buf_size;
> + strlcpy(params->channel, strim(test_channel), sizeof(params->channel));
> + strlcpy(params->device, strim(test_device), sizeof(params->device));
> + params->threads_per_chan = threads_per_chan;
> + params->max_channels = max_channels;
> + params->iterations = iterations;
> + params->xor_sources = xor_sources;
> + params->pq_sources = pq_sources;
> + params->timeout = timeout;
>
> /* Run test with new parameters */
> return __run_threaded_test(info);
> @@ -1029,71 +1038,6 @@ static bool __is_threaded_test_run(struct dmatest_info *info)
> return false;
> }
>
> -static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
> - const void __user *from, size_t count)
> -{
> - char tmp[20];
> - ssize_t len;
> -
> - len = simple_write_to_buffer(tmp, sizeof(tmp) - 1, ppos, from, count);
> - if (len >= 0) {
> - tmp[len] = '\0';
> - strlcpy(to, strim(tmp), available);
> - }
> -
> - return len;
> -}
> -
> -static ssize_t dtf_read_channel(struct file *file, char __user *buf,
> - size_t count, loff_t *ppos)
> -{
> - struct dmatest_info *info = file->private_data;
> - return simple_read_from_buffer(buf, count, ppos,
> - info->dbgfs_params.channel,
> - strlen(info->dbgfs_params.channel));
> -}
> -
> -static ssize_t dtf_write_channel(struct file *file, const char __user *buf,
> - size_t size, loff_t *ppos)
> -{
> - struct dmatest_info *info = file->private_data;
> - return dtf_write_string(info->dbgfs_params.channel,
> - sizeof(info->dbgfs_params.channel),
> - ppos, buf, size);
> -}
> -
> -static const struct file_operations dtf_channel_fops = {
> - .read = dtf_read_channel,
> - .write = dtf_write_channel,
> - .open = simple_open,
> - .llseek = default_llseek,
> -};
> -
> -static ssize_t dtf_read_device(struct file *file, char __user *buf,
> - size_t count, loff_t *ppos)
> -{
> - struct dmatest_info *info = file->private_data;
> - return simple_read_from_buffer(buf, count, ppos,
> - info->dbgfs_params.device,
> - strlen(info->dbgfs_params.device));
> -}
> -
> -static ssize_t dtf_write_device(struct file *file, const char __user *buf,
> - size_t size, loff_t *ppos)
> -{
> - struct dmatest_info *info = file->private_data;
> - return dtf_write_string(info->dbgfs_params.device,
> - sizeof(info->dbgfs_params.device),
> - ppos, buf, size);
> -}
> -
> -static const struct file_operations dtf_device_fops = {
> - .read = dtf_read_device,
> - .write = dtf_write_device,
> - .open = simple_open,
> - .llseek = default_llseek,
> -};
> -
> static ssize_t dtf_read_run(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> @@ -1187,7 +1131,6 @@ static const struct file_operations dtf_results_fops = {
> static int dmatest_register_dbgfs(struct dmatest_info *info)
> {
> struct dentry *d;
> - struct dmatest_params *params = &info->dbgfs_params;
> int ret = -ENOMEM;
>
> d = debugfs_create_dir("dmatest", NULL);
> @@ -1198,56 +1141,6 @@ static int dmatest_register_dbgfs(struct dmatest_info *info)
>
> info->root = d;
>
> - /* Copy initial values */
> - memcpy(params, &info->params, sizeof(*params));
> -
> - /* Test parameters */
> -
> - d = debugfs_create_u32("test_buf_size", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->buf_size);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_file("channel", S_IRUGO | S_IWUSR, info->root,
> - info, &dtf_channel_fops);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_file("device", S_IRUGO | S_IWUSR, info->root,
> - info, &dtf_device_fops);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("threads_per_chan", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->threads_per_chan);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("max_channels", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->max_channels);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("iterations", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->iterations);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("xor_sources", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->xor_sources);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("pq_sources", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->pq_sources);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> - d = debugfs_create_u32("timeout", S_IWUSR | S_IRUGO, info->root,
> - (u32 *)&params->timeout);
> - if (IS_ERR_OR_NULL(d))
> - goto err_node;
> -
> /* Run or stop threaded test */
> d = debugfs_create_file("run", S_IWUSR | S_IRUGO, info->root,
> info, &dtf_run_fops);
> @@ -1272,7 +1165,6 @@ err_root:
> static int __init dmatest_init(void)
> {
> struct dmatest_info *info = &test_info;
> - struct dmatest_params *params = &info->params;
> int ret;
>
> memset(info, 0, sizeof(*info));
> @@ -1283,17 +1175,6 @@ static int __init dmatest_init(void)
> mutex_init(&info->results_lock);
> INIT_LIST_HEAD(&info->results);
>
> - /* Set default parameters */
> - params->buf_size = test_buf_size;
> - strlcpy(params->channel, test_channel, sizeof(params->channel));
> - strlcpy(params->device, test_device, sizeof(params->device));
> - params->threads_per_chan = threads_per_chan;
> - params->max_channels = max_channels;
> - params->iterations = iterations;
> - params->xor_sources = xor_sources;
> - params->pq_sources = pq_sources;
> - params->timeout = timeout;
> -
> ret = dmatest_register_dbgfs(info);
> if (ret)
> return ret;

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2013-08-02 17:49:55

by Dan Williams

Subject: Re: [PATCH 1/3] dmatest: make module parameters writable

On 7/31/13 7:28 AM, "Andy Shevchenko" <[email protected]>
wrote:

>On Tue, 2013-07-23 at 18:36 +0300, Andy Shevchenko wrote:
>> The debugfs interface brought a copy of the test case parameters. This
>>makes
>> different set of values under /sys/module/dmatest/parameters/ and
>> /sys/kernel/debug/dmatest/. The user might be confused by the
>>divergence of
>> values.
>>
>> The proposed solution in this patch is to make module parameters
>>writable and
>> remove them from the debugfs. Though we're still using debugfs to
>>control test
>> runner and getting results.
>>
>> Documentation part is updated accordingly.
>>
>> Signed-off-by: Andy Shevchenko <[email protected]>
>> Suggested-by: Dan Williams <[email protected]>
>
>Dan, do you have any comments against this series?
>I'd like to have this incorporated into v3.12.

I like this direction, and these can go in as is. But I think with a bit
more work we can eventually trim the ?results' file and the
thread_{add|get}_result infrastructure. Ideally we could move all result
reporting to ftrace and let a module parameter control which errors are
reported.

However, unless I missed it, I don?t see this support[1] upstream yet.


--
Dan

[1]: https://lwn.net/Articles/432186/

2013-08-02 17:56:59

by Steven Rostedt

Subject: Re: [PATCH 1/3] dmatest: make module parameters writable

On Fri, 2013-08-02 at 17:49 +0000, Dan Williams wrote:

> However, unless I missed it, I don¹t see this support[1] upstream yet.
>
>
> --
> Dan
>
> [1]: https://lwn.net/Articles/432186/

IIRC, there was some push back from doing this. I don't really remember
who? Rusty maybe?

Or maybe not, it seems he had some comments on it in that thread:

http://thread.gmane.org/gmane.linux.kernel/1110481

Perhaps I got busy and dropped it :-/

Anyway, if there's a demand for it, we can always revisit the idea.

-- Steve

2013-08-02 18:03:43

by Dan Williams

Subject: Re: [PATCH 1/3] dmatest: make module parameters writable

On 8/2/13 10:56 AM, "Steven Rostedt" <[email protected]> wrote:

>On Fri, 2013-08-02 at 17:49 +0000, Dan Williams wrote:
>
>> However, unless I missed it, I don??t see this support[1] upstream yet.
>>
>>
>> --
>> Dan
>>
>> [1]: https://lwn.net/Articles/432186/
>
>IIRC, there was some push back from doing this. I don't really remember
>who? Rusty maybe?
>
>Or maybe not, it seems he had some comments on it in that thread:
>
>http://thread.gmane.org/gmane.linux.kernel/1110481
>
>Perhaps I got busy and dropped it :-/
>
>Anyway, if there's a demand for it, we can always revisit the idea.

Dmatest has infrastructure to collect a list of test result messages and
output them on-demand when reading a debugfs file. I figure we might as
well trace them but would want to enable the tracepoints when the module
is loaded.

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-08-08 08:42:31

by Andy Shevchenko

Subject: Re: [PATCH 1/3] dmatest: make module parameters writable

On Fri, 2013-08-02 at 17:49 +0000, Dan Williams wrote:
>
> On 7/31/13 7:28 AM, "Andy Shevchenko" <[email protected]>
> wrote:
>
> >On Tue, 2013-07-23 at 18:36 +0300, Andy Shevchenko wrote:
> >> The debugfs interface brought a copy of the test case parameters. This
> >>makes
> >> different set of values under /sys/module/dmatest/parameters/ and
> >> /sys/kernel/debug/dmatest/. The user might be confused by the
> >>divergence of
> >> values.
> >>
> >> The proposed solution in this patch is to make module parameters
> >>writable and
> >> remove them from the debugfs. Though we're still using debugfs to
> >>control test
> >> runner and getting results.
> >>
> >> Documentation part is updated accordingly.
> >>
> >> Signed-off-by: Andy Shevchenko <[email protected]>
> >> Suggested-by: Dan Williams <[email protected]>
> >
> >Dan, do you have any comments against this series?
> >I'd like to have this incorporated into v3.12.
>
> I like this direction, and these can go in as is.

So, could it be considered as your ACK?
If so, I think it would be better to push it early to be in time for
v3.12

> But I think with a bit
> more work we can eventually trim the Œresults' file and the
> thread_{add|get}_result infrastructure. Ideally we could move all result
> reporting to ftrace and let a module parameter control which errors are
> reported.
>
> However, unless I missed it, I don¹t see this support[1] upstream yet.
>
>
> --
> Dan
>
> [1]: https://lwn.net/Articles/432186/
>

--
Andy Shevchenko <[email protected]>
Intel Finland Oy