[PATCH 00/10] dmatest: update the module to use debugfs

[PATCH 00/10] dmatest: update the module to use debugfs

[Andy Shevchenko]

The first approach to get dmatest module more flexible and easier to play with.
The amount of patches could be reduced, but I would like to get a comments
first on smaller pieces. The entire series creates dmatest.txt file in the
Documentation folder. Similar description is scattered through the commit
messages.

Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
controller embedded.

Andy Shevchenko (10):
  dmatest: cancel thread immediately when asked for
  dmatest: allocate memory for pq_coefs from heap
  dmatest: create dmatest_info to keep test parameters
  dmatest: move dmatest_channels and nr_channels to dmatest_info
  dmatest: split test parameters to separate structure
  dmatest: run test via debugfs
  dmatest: return actual state in 'run' file
  dmatest: define MAX_ERROR_COUNT constant
  dmatest: gather test results in the linked list
  dmatest: append verify result to results

 Documentation/dmatest.txt |  81 +++++
 drivers/dma/dmatest.c     | 887 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 832 insertions(+), 136 deletions(-)
 create mode 100644 Documentation/dmatest.txt

-- 
1.8.2.rc0.22.gb3600c3

[PATCH 01/10] dmatest: cancel thread immediately when asked for

[Andy Shevchenko]

If user have the timeout alike issues and wants to cancel the thread
immediately, the current call of wait_event_freezable_timeout is preventing to
this until timeout is expired. Thus, user will experience the unnecessary
delays.

Adding kthread_should_stop() check inside wait_event_freezable_timeout() solves
that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a2c8904..e6b4cfa 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -454,7 +454,8 @@ static int dmatest_func(void *data)
 		}
 		dma_async_issue_pending(chan);
 
-		wait_event_freezable_timeout(done_wait, done.done,
+		wait_event_freezable_timeout(done_wait,
+					     done.done || kthread_should_stop(),
 					     msecs_to_jiffies(timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 01/10] dmatest: cancel thread immediately when asked for

[Viresh Kumar]

On 4 March 2013 17:09, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> If user have the timeout alike issues and wants to cancel the thread
> immediately, the current call of wait_event_freezable_timeout is preventing to
> this until timeout is expired. Thus, user will experience the unnecessary
> delays.
>
> Adding kthread_should_stop() check inside wait_event_freezable_timeout() solves
> that.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 02/10] dmatest: allocate memory for pq_coefs from heap

[Andy Shevchenko]

This will help in future to hide a global variable usage.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index e6b4cfa..e3955be 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -278,7 +278,7 @@ static int dmatest_func(void *data)
 	dma_cookie_t		cookie;
 	enum dma_status		status;
 	enum dma_ctrl_flags 	flags;
-	u8			pq_coefs[pq_sources + 1];
+	u8			*pq_coefs = NULL;
 	int			ret;
 	int			src_cnt;
 	int			dst_cnt;
@@ -302,10 +302,15 @@ static int dmatest_func(void *data)
 		/* force odd to ensure dst = src */
 		src_cnt = min_odd(pq_sources | 1, dma_maxpq(dev, 0));
 		dst_cnt = 2;
+
+		pq_coefs = kmalloc(pq_sources+1, GFP_KERNEL);
+		if (!pq_coefs)
+			goto err_thread_type;
+
 		for (i = 0; i < src_cnt; i++)
 			pq_coefs[i] = 1;
 	} else
-		goto err_srcs;
+		goto err_thread_type;
 
 	thread->srcs = kcalloc(src_cnt+1, sizeof(u8 *), GFP_KERNEL);
 	if (!thread->srcs)
@@ -533,6 +538,8 @@ err_dsts:
 err_srcbuf:
 	kfree(thread->srcs);
 err_srcs:
+	kfree(pq_coefs);
+err_thread_type:
 	pr_notice("%s: terminating after %u tests, %u failures (status %d)\n",
 			thread_name, total_tests, failed_tests, ret);
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 02/10] dmatest: allocate memory for pq_coefs from heap

[Viresh Kumar]

On 4 March 2013 17:09, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> This will help in future to hide a global variable usage.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 03/10] dmatest: create dmatest_info to keep test parameters

[Andy Shevchenko]

The proposed change will remove usage of the module parameters as global
variables. In future it helps to run different test cases sequentially.

The patch introduces the run_threaded_test() and stop_threaded_test() functions
that could be used later outside of dmatest_init, dmatest_exit scope.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 160 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 113 insertions(+), 47 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index e3955be..7f9e3cc 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -78,8 +78,11 @@ MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
 #define PATTERN_OVERWRITE	0x20
 #define PATTERN_COUNT_MASK	0x1f
 
+struct dmatest_info;
+
 struct dmatest_thread {
 	struct list_head	node;
+	struct dmatest_info	*info;
 	struct task_struct	*task;
 	struct dma_chan		*chan;
 	u8			**srcs;
@@ -93,6 +96,32 @@ struct dmatest_chan {
 	struct list_head	threads;
 };
 
+/**
+ * struct dmatest_info - test information.
+ * @buf_size:		size of the memcpy test buffer
+ * @channel:		bus ID of the channel to test
+ * @device:		bus ID of the DMA Engine to test
+ * @threads_per_chan:	number of threads to start per channel
+ * @max_channels:	maximum number of channels to use
+ * @iterations:		iterations before stopping test
+ * @xor_sources:	number of xor source buffers
+ * @pq_sources:		number of p+q source buffers
+ * @timeout:		transfer timeout in msec, -1 for infinite timeout
+ */
+struct dmatest_info {
+	unsigned int	buf_size;
+	char		channel[20];
+	char		device[20];
+	unsigned int	threads_per_chan;
+	unsigned int	max_channels;
+	unsigned int	iterations;
+	unsigned int	xor_sources;
+	unsigned int	pq_sources;
+	int		timeout;
+};
+
+static struct dmatest_info test_info;
+
 /*
  * These are protected by dma_list_mutex since they're only used by
  * the DMA filter function callback
@@ -100,18 +129,20 @@ struct dmatest_chan {
 static LIST_HEAD(dmatest_channels);
 static unsigned int nr_channels;
 
-static bool dmatest_match_channel(struct dma_chan *chan)
+static bool dmatest_match_channel(struct dmatest_info *info,
+		struct dma_chan *chan)
 {
-	if (test_channel[0] == '\0')
+	if (info->channel[0] == '\0')
 		return true;
-	return strcmp(dma_chan_name(chan), test_channel) == 0;
+	return strcmp(dma_chan_name(chan), info->channel) == 0;
 }
 
-static bool dmatest_match_device(struct dma_device *device)
+static bool dmatest_match_device(struct dmatest_info *info,
+		struct dma_device *device)
 {
-	if (test_device[0] == '\0')
+	if (info->device[0] == '\0')
 		return true;
-	return strcmp(dev_name(device->dev), test_device) == 0;
+	return strcmp(dev_name(device->dev), info->device) == 0;
 }
 
 static unsigned long dmatest_random(void)
@@ -122,7 +153,8 @@ static unsigned long dmatest_random(void)
 	return buf;
 }
 
-static void dmatest_init_srcs(u8 **bufs, unsigned int start, unsigned int len)
+static void dmatest_init_srcs(u8 **bufs, unsigned int start, unsigned int len,
+		unsigned int buf_size)
 {
 	unsigned int i;
 	u8 *buf;
@@ -133,13 +165,14 @@ static void dmatest_init_srcs(u8 **bufs, unsigned int start, unsigned int len)
 		for ( ; i < start + len; i++)
 			buf[i] = PATTERN_SRC | PATTERN_COPY
 				| (~i & PATTERN_COUNT_MASK);
-		for ( ; i < test_buf_size; i++)
+		for ( ; i < buf_size; i++)
 			buf[i] = PATTERN_SRC | (~i & PATTERN_COUNT_MASK);
 		buf++;
 	}
 }
 
-static void dmatest_init_dsts(u8 **bufs, unsigned int start, unsigned int len)
+static void dmatest_init_dsts(u8 **bufs, unsigned int start, unsigned int len,
+		unsigned int buf_size)
 {
 	unsigned int i;
 	u8 *buf;
@@ -150,7 +183,7 @@ static void dmatest_init_dsts(u8 **bufs, unsigned int start, unsigned int len)
 		for ( ; i < start + len; i++)
 			buf[i] = PATTERN_DST | PATTERN_OVERWRITE
 				| (~i & PATTERN_COUNT_MASK);
-		for ( ; i < test_buf_size; i++)
+		for ( ; i < buf_size; i++)
 			buf[i] = PATTERN_DST | (~i & PATTERN_COUNT_MASK);
 	}
 }
@@ -268,6 +301,7 @@ static int dmatest_func(void *data)
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(done_wait);
 	struct dmatest_thread	*thread = data;
 	struct dmatest_done	done = { .wait = &done_wait };
+	struct dmatest_info	*info;
 	struct dma_chan		*chan;
 	struct dma_device	*dev;
 	const char		*thread_name;
@@ -290,20 +324,21 @@ static int dmatest_func(void *data)
 	ret = -ENOMEM;
 
 	smp_rmb();
+	info = thread->info;
 	chan = thread->chan;
 	dev = chan->device;
 	if (thread->type == DMA_MEMCPY)
 		src_cnt = dst_cnt = 1;
 	else if (thread->type == DMA_XOR) {
 		/* force odd to ensure dst = src */
-		src_cnt = min_odd(xor_sources | 1, dev->max_xor);
+		src_cnt = min_odd(info->xor_sources | 1, dev->max_xor);
 		dst_cnt = 1;
 	} else if (thread->type == DMA_PQ) {
 		/* force odd to ensure dst = src */
-		src_cnt = min_odd(pq_sources | 1, dma_maxpq(dev, 0));
+		src_cnt = min_odd(info->pq_sources | 1, dma_maxpq(dev, 0));
 		dst_cnt = 2;
 
-		pq_coefs = kmalloc(pq_sources+1, GFP_KERNEL);
+		pq_coefs = kmalloc(info->pq_sources+1, GFP_KERNEL);
 		if (!pq_coefs)
 			goto err_thread_type;
 
@@ -316,7 +351,7 @@ static int dmatest_func(void *data)
 	if (!thread->srcs)
 		goto err_srcs;
 	for (i = 0; i < src_cnt; i++) {
-		thread->srcs[i] = kmalloc(test_buf_size, GFP_KERNEL);
+		thread->srcs[i] = kmalloc(info->buf_size, GFP_KERNEL);
 		if (!thread->srcs[i])
 			goto err_srcbuf;
 	}
@@ -326,7 +361,7 @@ static int dmatest_func(void *data)
 	if (!thread->dsts)
 		goto err_dsts;
 	for (i = 0; i < dst_cnt; i++) {
-		thread->dsts[i] = kmalloc(test_buf_size, GFP_KERNEL);
+		thread->dsts[i] = kmalloc(info->buf_size, GFP_KERNEL);
 		if (!thread->dsts[i])
 			goto err_dstbuf;
 	}
@@ -342,7 +377,7 @@ static int dmatest_func(void *data)
 	      | DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SRC_UNMAP_SINGLE;
 
 	while (!kthread_should_stop()
-	       && !(iterations && total_tests >= iterations)) {
+	       && !(info->iterations && total_tests >= info->iterations)) {
 		struct dma_async_tx_descriptor *tx = NULL;
 		dma_addr_t dma_srcs[src_cnt];
 		dma_addr_t dma_dsts[dst_cnt];
@@ -358,24 +393,24 @@ static int dmatest_func(void *data)
 		else if (thread->type == DMA_PQ)
 			align = dev->pq_align;
 
-		if (1 << align > test_buf_size) {
+		if (1 << align > info->buf_size) {
 			pr_err("%u-byte buffer too small for %d-byte alignment\n",
-			       test_buf_size, 1 << align);
+			       info->buf_size, 1 << align);
 			break;
 		}
 
-		len = dmatest_random() % test_buf_size + 1;
+		len = dmatest_random() % info->buf_size + 1;
 		len = (len >> align) << align;
 		if (!len)
 			len = 1 << align;
-		src_off = dmatest_random() % (test_buf_size - len + 1);
-		dst_off = dmatest_random() % (test_buf_size - len + 1);
+		src_off = dmatest_random() % (info->buf_size - len + 1);
+		dst_off = dmatest_random() % (info->buf_size - len + 1);
 
 		src_off = (src_off >> align) << align;
 		dst_off = (dst_off >> align) << align;
 
-		dmatest_init_srcs(thread->srcs, src_off, len);
-		dmatest_init_dsts(thread->dsts, dst_off, len);
+		dmatest_init_srcs(thread->srcs, src_off, len, info->buf_size);
+		dmatest_init_dsts(thread->dsts, dst_off, len, info->buf_size);
 
 		for (i = 0; i < src_cnt; i++) {
 			u8 *buf = thread->srcs[i] + src_off;
@@ -396,16 +431,16 @@ static int dmatest_func(void *data)
 		/* map with DMA_BIDIRECTIONAL to force writeback/invalidate */
 		for (i = 0; i < dst_cnt; i++) {
 			dma_dsts[i] = dma_map_single(dev->dev, thread->dsts[i],
-						     test_buf_size,
+						     info->buf_size,
 						     DMA_BIDIRECTIONAL);
 			ret = dma_mapping_error(dev->dev, dma_dsts[i]);
 			if (ret) {
 				unmap_src(dev->dev, dma_srcs, len, src_cnt);
-				unmap_dst(dev->dev, dma_dsts, test_buf_size, i);
+				unmap_dst(dev->dev, dma_dsts, info->buf_size, i);
 				pr_warn("%s: #%u: mapping error %d with "
 					"dst_off=0x%x len=0x%x\n",
 					thread_name, total_tests - 1, ret,
-					dst_off, test_buf_size);
+					dst_off, info->buf_size);
 				failed_tests++;
 				continue;
 			}
@@ -433,7 +468,7 @@ static int dmatest_func(void *data)
 
 		if (!tx) {
 			unmap_src(dev->dev, dma_srcs, len, src_cnt);
-			unmap_dst(dev->dev, dma_dsts, test_buf_size, dst_cnt);
+			unmap_dst(dev->dev, dma_dsts, info->buf_size, dst_cnt);
 			pr_warning("%s: #%u: prep error with src_off=0x%x "
 					"dst_off=0x%x len=0x%x\n",
 					thread_name, total_tests - 1,
@@ -461,7 +496,7 @@ static int dmatest_func(void *data)
 
 		wait_event_freezable_timeout(done_wait,
 					     done.done || kthread_should_stop(),
-					     msecs_to_jiffies(timeout));
+					     msecs_to_jiffies(info->timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
 
@@ -488,7 +523,7 @@ static int dmatest_func(void *data)
 		}
 
 		/* Unmap by myself (see DMA_COMPL_SKIP_DEST_UNMAP above) */
-		unmap_dst(dev->dev, dma_dsts, test_buf_size, dst_cnt);
+		unmap_dst(dev->dev, dma_dsts, info->buf_size, dst_cnt);
 
 		error_count = 0;
 
@@ -499,7 +534,7 @@ static int dmatest_func(void *data)
 				src_off + len, src_off,
 				PATTERN_SRC | PATTERN_COPY, true);
 		error_count += dmatest_verify(thread->srcs, src_off + len,
-				test_buf_size, src_off + len,
+				info->buf_size, src_off + len,
 				PATTERN_SRC, true);
 
 		pr_debug("%s: verifying dest buffer...\n",
@@ -510,7 +545,7 @@ static int dmatest_func(void *data)
 				dst_off + len, src_off,
 				PATTERN_SRC | PATTERN_COPY, false);
 		error_count += dmatest_verify(thread->dsts, dst_off + len,
-				test_buf_size, dst_off + len,
+				info->buf_size, dst_off + len,
 				PATTERN_DST, false);
 
 		if (error_count) {
@@ -547,7 +582,7 @@ err_thread_type:
 	if (ret)
 		dmaengine_terminate_all(chan);
 
-	if (iterations > 0)
+	if (info->iterations > 0)
 		while (!kthread_should_stop()) {
 			DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wait_dmatest_exit);
 			interruptible_sleep_on(&wait_dmatest_exit);
@@ -576,7 +611,8 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
 	kfree(dtc);
 }
 
-static int dmatest_add_threads(struct dmatest_chan *dtc, enum dma_transaction_type type)
+static int dmatest_add_threads(struct dmatest_info *info,
+		struct dmatest_chan *dtc, enum dma_transaction_type type)
 {
 	struct dmatest_thread *thread;
 	struct dma_chan *chan = dtc->chan;
@@ -592,7 +628,7 @@ static int dmatest_add_threads(struct dmatest_chan *dtc, enum dma_transaction_ty
 	else
 		return -EINVAL;
 
-	for (i = 0; i < threads_per_chan; i++) {
+	for (i = 0; i < info->threads_per_chan; i++) {
 		thread = kzalloc(sizeof(struct dmatest_thread), GFP_KERNEL);
 		if (!thread) {
 			pr_warning("dmatest: No memory for %s-%s%u\n",
@@ -600,6 +636,7 @@ static int dmatest_add_threads(struct dmatest_chan *dtc, enum dma_transaction_ty
 
 			break;
 		}
+		thread->info = info;
 		thread->chan = dtc->chan;
 		thread->type = type;
 		smp_wmb();
@@ -620,7 +657,8 @@ static int dmatest_add_threads(struct dmatest_chan *dtc, enum dma_transaction_ty
 	return i;
 }
 
-static int dmatest_add_channel(struct dma_chan *chan)
+static int dmatest_add_channel(struct dmatest_info *info,
+		struct dma_chan *chan)
 {
 	struct dmatest_chan	*dtc;
 	struct dma_device	*dma_dev = chan->device;
@@ -637,15 +675,15 @@ static int dmatest_add_channel(struct dma_chan *chan)
 	INIT_LIST_HEAD(&dtc->threads);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
-		cnt = dmatest_add_threads(dtc, DMA_MEMCPY);
+		cnt = dmatest_add_threads(info, dtc, DMA_MEMCPY);
 		thread_count += cnt > 0 ? cnt : 0;
 	}
 	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)) {
-		cnt = dmatest_add_threads(dtc, DMA_XOR);
+		cnt = dmatest_add_threads(info, dtc, DMA_XOR);
 		thread_count += cnt > 0 ? cnt : 0;
 	}
 	if (dma_has_cap(DMA_PQ, dma_dev->cap_mask)) {
-		cnt = dmatest_add_threads(dtc, DMA_PQ);
+		cnt = dmatest_add_threads(info, dtc, DMA_PQ);
 		thread_count += cnt > 0 ? cnt : 0;
 	}
 
@@ -660,13 +698,16 @@ static int dmatest_add_channel(struct dma_chan *chan)
 
 static bool filter(struct dma_chan *chan, void *param)
 {
-	if (!dmatest_match_channel(chan) || !dmatest_match_device(chan->device))
+	struct dmatest_info *info = param;
+
+	if (!dmatest_match_channel(info, chan) ||
+	    !dmatest_match_device(info, chan->device))
 		return false;
 	else
 		return true;
 }
 
-static int __init dmatest_init(void)
+static int run_threaded_test(struct dmatest_info *info)
 {
 	dma_cap_mask_t mask;
 	struct dma_chan *chan;
@@ -675,25 +716,22 @@ static int __init dmatest_init(void)
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_MEMCPY, mask);
 	for (;;) {
-		chan = dma_request_channel(mask, filter, NULL);
+		chan = dma_request_channel(mask, filter, info);
 		if (chan) {
-			err = dmatest_add_channel(chan);
+			err = dmatest_add_channel(info, chan);
 			if (err) {
 				dma_release_channel(chan);
 				break; /* add_channel failed, punt */
 			}
 		} else
 			break; /* no more channels available */
-		if (max_channels && nr_channels >= max_channels)
+		if (info->max_channels && nr_channels >= info->max_channels)
 			break; /* we have all we need */
 	}
-
 	return err;
 }
-/* when compiled-in wait for drivers to load first */
-late_initcall(dmatest_init);
 
-static void __exit dmatest_exit(void)
+static void stop_threaded_test(struct dmatest_info *info)
 {
 	struct dmatest_chan *dtc, *_dtc;
 	struct dma_chan *chan;
@@ -707,6 +745,34 @@ static void __exit dmatest_exit(void)
 		dma_release_channel(chan);
 	}
 }
+
+static int __init dmatest_init(void)
+{
+	struct dmatest_info *info = &test_info;
+
+	memset(info, 0, sizeof(*info));
+
+	info->buf_size = test_buf_size;
+	strlcpy(info->channel, test_channel, sizeof(info->channel));
+	strlcpy(info->device, test_device, sizeof(info->device));
+	info->threads_per_chan = threads_per_chan;
+	info->max_channels = max_channels;
+	info->iterations = iterations;
+	info->xor_sources = xor_sources;
+	info->pq_sources = pq_sources;
+	info->timeout = timeout;
+
+	return run_threaded_test(info);
+}
+/* when compiled-in wait for drivers to load first */
+late_initcall(dmatest_init);
+
+static void __exit dmatest_exit(void)
+{
+	struct dmatest_info *info = &test_info;
+
+	stop_threaded_test(info);
+}
 module_exit(dmatest_exit);
 
 MODULE_AUTHOR("Haavard Skinnemoen (Atmel)");
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 03/10] dmatest: create dmatest_info to keep test parameters

[Viresh Kumar]

On 4 March 2013 17:09, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The proposed change will remove usage of the module parameters as global
> variables. In future it helps to run different test cases sequentially.
>
> The patch introduces the run_threaded_test() and stop_threaded_test() functions
> that could be used later outside of dmatest_init, dmatest_exit scope.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 160 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 113 insertions(+), 47 deletions(-)
>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 04/10] dmatest: move dmatest_channels and nr_channels to dmatest_info

[Andy Shevchenko]

We don't need to have them global and later we would like to protect access to
them as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 7f9e3cc..475a21a 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -109,6 +109,7 @@ struct dmatest_chan {
  * @timeout:		transfer timeout in msec, -1 for infinite timeout
  */
 struct dmatest_info {
+	/* Test parameters */
 	unsigned int	buf_size;
 	char		channel[20];
 	char		device[20];
@@ -118,17 +119,14 @@ struct dmatest_info {
 	unsigned int	xor_sources;
 	unsigned int	pq_sources;
 	int		timeout;
+
+	/* Internal state */
+	struct list_head	channels;
+	unsigned int		nr_channels;
 };
 
 static struct dmatest_info test_info;
 
-/*
- * These are protected by dma_list_mutex since they're only used by
- * the DMA filter function callback
- */
-static LIST_HEAD(dmatest_channels);
-static unsigned int nr_channels;
-
 static bool dmatest_match_channel(struct dmatest_info *info,
 		struct dma_chan *chan)
 {
@@ -690,8 +688,8 @@ static int dmatest_add_channel(struct dmatest_info *info,
 	pr_info("dmatest: Started %u threads using %s\n",
 		thread_count, dma_chan_name(chan));
 
-	list_add_tail(&dtc->node, &dmatest_channels);
-	nr_channels++;
+	list_add_tail(&dtc->node, &info->channels);
+	info->nr_channels++;
 
 	return 0;
 }
@@ -725,7 +723,8 @@ static int run_threaded_test(struct dmatest_info *info)
 			}
 		} else
 			break; /* no more channels available */
-		if (info->max_channels && nr_channels >= info->max_channels)
+		if (info->max_channels &&
+		    info->nr_channels >= info->max_channels)
 			break; /* we have all we need */
 	}
 	return err;
@@ -736,14 +735,15 @@ static void stop_threaded_test(struct dmatest_info *info)
 	struct dmatest_chan *dtc, *_dtc;
 	struct dma_chan *chan;
 
-	list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node) {
+	list_for_each_entry_safe(dtc, _dtc, &info->channels, node) {
 		list_del(&dtc->node);
 		chan = dtc->chan;
 		dmatest_cleanup_channel(dtc);
-		pr_debug("dmatest: dropped channel %s\n",
-			 dma_chan_name(chan));
+		pr_debug("dmatest: dropped channel %s\n", dma_chan_name(chan));
 		dma_release_channel(chan);
 	}
+
+	info->nr_channels = 0;
 }
 
 static int __init dmatest_init(void)
@@ -752,6 +752,9 @@ static int __init dmatest_init(void)
 
 	memset(info, 0, sizeof(*info));
 
+	INIT_LIST_HEAD(&info->channels);
+
+	/* Set default parameters */
 	info->buf_size = test_buf_size;
 	strlcpy(info->channel, test_channel, sizeof(info->channel));
 	strlcpy(info->device, test_device, sizeof(info->device));
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 04/10] dmatest: move dmatest_channels and nr_channels to dmatest_info

[Vinod Koul]

On Mon, Mar 04, 2013 at 11:09:28AM +0200, Andy Shevchenko wrote:
> We don't need to have them global and later we would like to protect access to
> them as well.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 7f9e3cc..475a21a 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -109,6 +109,7 @@ struct dmatest_chan {
>   * @timeout:		transfer timeout in msec, -1 for infinite timeout
>   */
>  struct dmatest_info {
> +	/* Test parameters */
>  	unsigned int	buf_size;
>  	char		channel[20];
>  	char		device[20];
> @@ -118,17 +119,14 @@ struct dmatest_info {
>  	unsigned int	xor_sources;
>  	unsigned int	pq_sources;
>  	int		timeout;
> +
> +	/* Internal state */
> +	struct list_head	channels;
> +	unsigned int		nr_channels;
>  };
Ideally this should be folded into previous...

>  
>  static struct dmatest_info test_info;
>  
> -/*
> - * These are protected by dma_list_mutex since they're only used by
> - * the DMA filter function callback
> - */
> -static LIST_HEAD(dmatest_channels);
> -static unsigned int nr_channels;
> -
>  static bool dmatest_match_channel(struct dmatest_info *info,
>  		struct dma_chan *chan)
>  {
> @@ -690,8 +688,8 @@ static int dmatest_add_channel(struct dmatest_info *info,
>  	pr_info("dmatest: Started %u threads using %s\n",
>  		thread_count, dma_chan_name(chan));
>  
> -	list_add_tail(&dtc->node, &dmatest_channels);
> -	nr_channels++;
> +	list_add_tail(&dtc->node, &info->channels);
> +	info->nr_channels++;
>  
>  	return 0;
>  }
> @@ -725,7 +723,8 @@ static int run_threaded_test(struct dmatest_info *info)
>  			}
>  		} else
>  			break; /* no more channels available */
> -		if (info->max_channels && nr_channels >= info->max_channels)
> +		if (info->max_channels &&
> +		    info->nr_channels >= info->max_channels)
>  			break; /* we have all we need */
>  	}
>  	return err;
> @@ -736,14 +735,15 @@ static void stop_threaded_test(struct dmatest_info *info)
>  	struct dmatest_chan *dtc, *_dtc;
>  	struct dma_chan *chan;
>  
> -	list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node) {
> +	list_for_each_entry_safe(dtc, _dtc, &info->channels, node) {
>  		list_del(&dtc->node);
>  		chan = dtc->chan;
>  		dmatest_cleanup_channel(dtc);
> -		pr_debug("dmatest: dropped channel %s\n",
> -			 dma_chan_name(chan));
> +		pr_debug("dmatest: dropped channel %s\n", dma_chan_name(chan));
>  		dma_release_channel(chan);
>  	}
> +
> +	info->nr_channels = 0;
>  }
>  
>  static int __init dmatest_init(void)
> @@ -752,6 +752,9 @@ static int __init dmatest_init(void)
>  
>  	memset(info, 0, sizeof(*info));
>  
> +	INIT_LIST_HEAD(&info->channels);
> +
> +	/* Set default parameters */
>  	info->buf_size = test_buf_size;
>  	strlcpy(info->channel, test_channel, sizeof(info->channel));
>  	strlcpy(info->device, test_device, sizeof(info->device));
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

Re: [PATCH 04/10] dmatest: move dmatest_channels and nr_channels to dmatest_info

[Viresh Kumar]

On 4 March 2013 17:09, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We don't need to have them global and later we would like to protect access to
> them as well.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 7f9e3cc..475a21a 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -109,6 +109,7 @@ struct dmatest_chan {
>   * @timeout:           transfer timeout in msec, -1 for infinite timeout
>   */
>  struct dmatest_info {
> +       /* Test parameters */
>         unsigned int    buf_size;
>         char            channel[20];
>         char            device[20];
> @@ -118,17 +119,14 @@ struct dmatest_info {
>         unsigned int    xor_sources;
>         unsigned int    pq_sources;
>         int             timeout;
> +
> +       /* Internal state */
> +       struct list_head        channels;
> +       unsigned int            nr_channels;
>  };

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

What about merging this patch with the patch which added this structure.

Re: [PATCH 04/10] dmatest: move dmatest_channels and nr_channels to dmatest_info

[Andy Shevchenko]

On Tue, 2013-03-05 at 17:12 +0800, Viresh Kumar wrote: 
> On 4 March 2013 17:09, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > We don't need to have them global and later we would like to protect access to
> > them as well.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dmatest.c | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index 7f9e3cc..475a21a 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -109,6 +109,7 @@ struct dmatest_chan {
> >   * @timeout:           transfer timeout in msec, -1 for infinite timeout
> >   */
> >  struct dmatest_info {
> > +       /* Test parameters */
> >         unsigned int    buf_size;
> >         char            channel[20];
> >         char            device[20];
> > @@ -118,17 +119,14 @@ struct dmatest_info {
> >         unsigned int    xor_sources;
> >         unsigned int    pq_sources;
> >         int             timeout;
> > +
> > +       /* Internal state */
> > +       struct list_head        channels;
> > +       unsigned int            nr_channels;
> >  };
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> What about merging this patch with the patch which added this structure.

As I said in cover letter I believe that few patches could be merged,
but I would like to discuss the main point first.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 05/10] dmatest: split test parameters to separate structure

[Andy Shevchenko]

Better to keep test parameters separate from internal variables.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 109 ++++++++++++++++++++++++++++----------------------
 1 file changed, 62 insertions(+), 47 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 475a21a..c6e5d83 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -97,7 +97,7 @@ struct dmatest_chan {
 };
 
 /**
- * struct dmatest_info - test information.
+ * struct dmatest_params - test parameters.
  * @buf_size:		size of the memcpy test buffer
  * @channel:		bus ID of the channel to test
  * @device:		bus ID of the DMA Engine to test
@@ -108,8 +108,7 @@ struct dmatest_chan {
  * @pq_sources:		number of p+q source buffers
  * @timeout:		transfer timeout in msec, -1 for infinite timeout
  */
-struct dmatest_info {
-	/* Test parameters */
+struct dmatest_params {
 	unsigned int	buf_size;
 	char		channel[20];
 	char		device[20];
@@ -119,6 +118,15 @@ struct dmatest_info {
 	unsigned int	xor_sources;
 	unsigned int	pq_sources;
 	int		timeout;
+};
+
+/**
+ * struct dmatest_info - test information.
+ * @params:		test parameters
+ */
+struct dmatest_info {
+	/* Test parameters */
+	struct dmatest_params	params;
 
 	/* Internal state */
 	struct list_head	channels;
@@ -127,20 +135,20 @@ struct dmatest_info {
 
 static struct dmatest_info test_info;
 
-static bool dmatest_match_channel(struct dmatest_info *info,
+static bool dmatest_match_channel(struct dmatest_params *params,
 		struct dma_chan *chan)
 {
-	if (info->channel[0] == '\0')
+	if (params->channel[0] == '\0')
 		return true;
-	return strcmp(dma_chan_name(chan), info->channel) == 0;
+	return strcmp(dma_chan_name(chan), params->channel) == 0;
 }
 
-static bool dmatest_match_device(struct dmatest_info *info,
+static bool dmatest_match_device(struct dmatest_params *params,
 		struct dma_device *device)
 {
-	if (info->device[0] == '\0')
+	if (params->device[0] == '\0')
 		return true;
-	return strcmp(dev_name(device->dev), info->device) == 0;
+	return strcmp(dev_name(device->dev), params->device) == 0;
 }
 
 static unsigned long dmatest_random(void)
@@ -300,6 +308,7 @@ static int dmatest_func(void *data)
 	struct dmatest_thread	*thread = data;
 	struct dmatest_done	done = { .wait = &done_wait };
 	struct dmatest_info	*info;
+	struct dmatest_params	*params;
 	struct dma_chan		*chan;
 	struct dma_device	*dev;
 	const char		*thread_name;
@@ -323,20 +332,21 @@ static int dmatest_func(void *data)
 
 	smp_rmb();
 	info = thread->info;
+	params = &info->params;
 	chan = thread->chan;
 	dev = chan->device;
 	if (thread->type == DMA_MEMCPY)
 		src_cnt = dst_cnt = 1;
 	else if (thread->type == DMA_XOR) {
 		/* force odd to ensure dst = src */
-		src_cnt = min_odd(info->xor_sources | 1, dev->max_xor);
+		src_cnt = min_odd(params->xor_sources | 1, dev->max_xor);
 		dst_cnt = 1;
 	} else if (thread->type == DMA_PQ) {
 		/* force odd to ensure dst = src */
-		src_cnt = min_odd(info->pq_sources | 1, dma_maxpq(dev, 0));
+		src_cnt = min_odd(params->pq_sources | 1, dma_maxpq(dev, 0));
 		dst_cnt = 2;
 
-		pq_coefs = kmalloc(info->pq_sources+1, GFP_KERNEL);
+		pq_coefs = kmalloc(params->pq_sources+1, GFP_KERNEL);
 		if (!pq_coefs)
 			goto err_thread_type;
 
@@ -349,7 +359,7 @@ static int dmatest_func(void *data)
 	if (!thread->srcs)
 		goto err_srcs;
 	for (i = 0; i < src_cnt; i++) {
-		thread->srcs[i] = kmalloc(info->buf_size, GFP_KERNEL);
+		thread->srcs[i] = kmalloc(params->buf_size, GFP_KERNEL);
 		if (!thread->srcs[i])
 			goto err_srcbuf;
 	}
@@ -359,7 +369,7 @@ static int dmatest_func(void *data)
 	if (!thread->dsts)
 		goto err_dsts;
 	for (i = 0; i < dst_cnt; i++) {
-		thread->dsts[i] = kmalloc(info->buf_size, GFP_KERNEL);
+		thread->dsts[i] = kmalloc(params->buf_size, GFP_KERNEL);
 		if (!thread->dsts[i])
 			goto err_dstbuf;
 	}
@@ -375,7 +385,7 @@ static int dmatest_func(void *data)
 	      | DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SRC_UNMAP_SINGLE;
 
 	while (!kthread_should_stop()
-	       && !(info->iterations && total_tests >= info->iterations)) {
+	       && !(params->iterations && total_tests >= params->iterations)) {
 		struct dma_async_tx_descriptor *tx = NULL;
 		dma_addr_t dma_srcs[src_cnt];
 		dma_addr_t dma_dsts[dst_cnt];
@@ -391,24 +401,24 @@ static int dmatest_func(void *data)
 		else if (thread->type == DMA_PQ)
 			align = dev->pq_align;
 
-		if (1 << align > info->buf_size) {
+		if (1 << align > params->buf_size) {
 			pr_err("%u-byte buffer too small for %d-byte alignment\n",
-			       info->buf_size, 1 << align);
+			       params->buf_size, 1 << align);
 			break;
 		}
 
-		len = dmatest_random() % info->buf_size + 1;
+		len = dmatest_random() % params->buf_size + 1;
 		len = (len >> align) << align;
 		if (!len)
 			len = 1 << align;
-		src_off = dmatest_random() % (info->buf_size - len + 1);
-		dst_off = dmatest_random() % (info->buf_size - len + 1);
+		src_off = dmatest_random() % (params->buf_size - len + 1);
+		dst_off = dmatest_random() % (params->buf_size - len + 1);
 
 		src_off = (src_off >> align) << align;
 		dst_off = (dst_off >> align) << align;
 
-		dmatest_init_srcs(thread->srcs, src_off, len, info->buf_size);
-		dmatest_init_dsts(thread->dsts, dst_off, len, info->buf_size);
+		dmatest_init_srcs(thread->srcs, src_off, len, params->buf_size);
+		dmatest_init_dsts(thread->dsts, dst_off, len, params->buf_size);
 
 		for (i = 0; i < src_cnt; i++) {
 			u8 *buf = thread->srcs[i] + src_off;
@@ -429,16 +439,17 @@ static int dmatest_func(void *data)
 		/* map with DMA_BIDIRECTIONAL to force writeback/invalidate */
 		for (i = 0; i < dst_cnt; i++) {
 			dma_dsts[i] = dma_map_single(dev->dev, thread->dsts[i],
-						     info->buf_size,
+						     params->buf_size,
 						     DMA_BIDIRECTIONAL);
 			ret = dma_mapping_error(dev->dev, dma_dsts[i]);
 			if (ret) {
 				unmap_src(dev->dev, dma_srcs, len, src_cnt);
-				unmap_dst(dev->dev, dma_dsts, info->buf_size, i);
+				unmap_dst(dev->dev, dma_dsts, params->buf_size,
+					  i);
 				pr_warn("%s: #%u: mapping error %d with "
 					"dst_off=0x%x len=0x%x\n",
 					thread_name, total_tests - 1, ret,
-					dst_off, info->buf_size);
+					dst_off, params->buf_size);
 				failed_tests++;
 				continue;
 			}
@@ -466,7 +477,8 @@ static int dmatest_func(void *data)
 
 		if (!tx) {
 			unmap_src(dev->dev, dma_srcs, len, src_cnt);
-			unmap_dst(dev->dev, dma_dsts, info->buf_size, dst_cnt);
+			unmap_dst(dev->dev, dma_dsts, params->buf_size,
+				  dst_cnt);
 			pr_warning("%s: #%u: prep error with src_off=0x%x "
 					"dst_off=0x%x len=0x%x\n",
 					thread_name, total_tests - 1,
@@ -494,7 +506,7 @@ static int dmatest_func(void *data)
 
 		wait_event_freezable_timeout(done_wait,
 					     done.done || kthread_should_stop(),
-					     msecs_to_jiffies(info->timeout));
+					     msecs_to_jiffies(params->timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
 
@@ -521,7 +533,7 @@ static int dmatest_func(void *data)
 		}
 
 		/* Unmap by myself (see DMA_COMPL_SKIP_DEST_UNMAP above) */
-		unmap_dst(dev->dev, dma_dsts, info->buf_size, dst_cnt);
+		unmap_dst(dev->dev, dma_dsts, params->buf_size, dst_cnt);
 
 		error_count = 0;
 
@@ -532,7 +544,7 @@ static int dmatest_func(void *data)
 				src_off + len, src_off,
 				PATTERN_SRC | PATTERN_COPY, true);
 		error_count += dmatest_verify(thread->srcs, src_off + len,
-				info->buf_size, src_off + len,
+				params->buf_size, src_off + len,
 				PATTERN_SRC, true);
 
 		pr_debug("%s: verifying dest buffer...\n",
@@ -543,7 +555,7 @@ static int dmatest_func(void *data)
 				dst_off + len, src_off,
 				PATTERN_SRC | PATTERN_COPY, false);
 		error_count += dmatest_verify(thread->dsts, dst_off + len,
-				info->buf_size, dst_off + len,
+				params->buf_size, dst_off + len,
 				PATTERN_DST, false);
 
 		if (error_count) {
@@ -580,7 +592,7 @@ err_thread_type:
 	if (ret)
 		dmaengine_terminate_all(chan);
 
-	if (info->iterations > 0)
+	if (params->iterations > 0)
 		while (!kthread_should_stop()) {
 			DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wait_dmatest_exit);
 			interruptible_sleep_on(&wait_dmatest_exit);
@@ -612,6 +624,7 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
 static int dmatest_add_threads(struct dmatest_info *info,
 		struct dmatest_chan *dtc, enum dma_transaction_type type)
 {
+	struct dmatest_params *params = &info->params;
 	struct dmatest_thread *thread;
 	struct dma_chan *chan = dtc->chan;
 	char *op;
@@ -626,7 +639,7 @@ static int dmatest_add_threads(struct dmatest_info *info,
 	else
 		return -EINVAL;
 
-	for (i = 0; i < info->threads_per_chan; i++) {
+	for (i = 0; i < params->threads_per_chan; i++) {
 		thread = kzalloc(sizeof(struct dmatest_thread), GFP_KERNEL);
 		if (!thread) {
 			pr_warning("dmatest: No memory for %s-%s%u\n",
@@ -696,10 +709,10 @@ static int dmatest_add_channel(struct dmatest_info *info,
 
 static bool filter(struct dma_chan *chan, void *param)
 {
-	struct dmatest_info *info = param;
+	struct dmatest_params *params = param;
 
-	if (!dmatest_match_channel(info, chan) ||
-	    !dmatest_match_device(info, chan->device))
+	if (!dmatest_match_channel(params, chan) ||
+	    !dmatest_match_device(params, chan->device))
 		return false;
 	else
 		return true;
@@ -709,12 +722,13 @@ static int run_threaded_test(struct dmatest_info *info)
 {
 	dma_cap_mask_t mask;
 	struct dma_chan *chan;
+	struct dmatest_params *params = &info->params;
 	int err = 0;
 
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_MEMCPY, mask);
 	for (;;) {
-		chan = dma_request_channel(mask, filter, info);
+		chan = dma_request_channel(mask, filter, params);
 		if (chan) {
 			err = dmatest_add_channel(info, chan);
 			if (err) {
@@ -723,8 +737,8 @@ static int run_threaded_test(struct dmatest_info *info)
 			}
 		} else
 			break; /* no more channels available */
-		if (info->max_channels &&
-		    info->nr_channels >= info->max_channels)
+		if (params->max_channels &&
+		    info->nr_channels >= params->max_channels)
 			break; /* we have all we need */
 	}
 	return err;
@@ -749,21 +763,22 @@ static void stop_threaded_test(struct dmatest_info *info)
 static int __init dmatest_init(void)
 {
 	struct dmatest_info *info = &test_info;
+	struct dmatest_params *params = &info->params;
 
 	memset(info, 0, sizeof(*info));
 
 	INIT_LIST_HEAD(&info->channels);
 
 	/* Set default parameters */
-	info->buf_size = test_buf_size;
-	strlcpy(info->channel, test_channel, sizeof(info->channel));
-	strlcpy(info->device, test_device, sizeof(info->device));
-	info->threads_per_chan = threads_per_chan;
-	info->max_channels = max_channels;
-	info->iterations = iterations;
-	info->xor_sources = xor_sources;
-	info->pq_sources = pq_sources;
-	info->timeout = timeout;
+	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;
 
 	return run_threaded_test(info);
 }
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 05/10] dmatest: split test parameters to separate structure

[Viresh Kumar]

On 4 March 2013 17:09, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Better to keep test parameters separate from internal variables.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 109 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 62 insertions(+), 47 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Though i don't feel a real need of keeping this in separate patches.
We can have one for fixing these :)

Re: [PATCH 05/10] dmatest: split test parameters to separate structure

[Andy Shevchenko]

On Tue, 2013-03-05 at 17:16 +0800, Viresh Kumar wrote: 
> On 4 March 2013 17:09, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Better to keep test parameters separate from internal variables.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dmatest.c | 109 ++++++++++++++++++++++++++++----------------------
> >  1 file changed, 62 insertions(+), 47 deletions(-)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Though i don't feel a real need of keeping this in separate patches.
> We can have one for fixing these :)

Do you mean to merge patche 3,4, and 5 into one?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 06/10] dmatest: run test via debugfs

[Andy Shevchenko]

Instead of doing
	modprobe dmatest ...
	modprobe -r dmatest
we allow user to run tests interactively.

The dmatest could be built as module or inside kernel. Let's consider those
cases.

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

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 1 > /sys/kernel/debug/dmatest/run

After a while you will start to get messages about current status or error like
in the original code.

Note that running a new test will stop any in progress test.

2. When built-in in the kernel...

The module parameters that is supplied to the kernel command line will be used
for the first performed test. After user gets a control, the test could be
interrupted or re-run with same or different parameters. For the details see
the above section "1. 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
	% grep -H . /sys/module/dmatest/parameters/*

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/dmatest.txt |  48 +++++++++
 drivers/dma/dmatest.c     | 257 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 303 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/dmatest.txt

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
new file mode 100644
index 0000000..9a90729
--- /dev/null
+++ b/Documentation/dmatest.txt
@@ -0,0 +1,48 @@
+				DMA Test Guide
+				==============
+
+		Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+This small document introduces how to test DMA drivers using dmatest module.
+
+	Part 1 - How to build the test module
+
+The menuconfig contains an option that could be found by following path:
+	Device Drivers -> DMA Engine support -> DMA Test client
+
+In the configuration file the option called CONFIG_DMATEST. The dmatest could
+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.
+
+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 1 > /sys/kernel/debug/dmatest/run
+
+Hint: available channel list could be extracted by running the following
+command:
+	% ls -1 /sys/class/dma/
+
+After a while you will start to get messages about current status or error like
+in the original code.
+
+Note that running a new test will stop any in progress test.
+
+	Part 3 - When built-in in the kernel...
+
+The module parameters that is supplied to the kernel command line will be used
+for the first performed test. After user gets a control, the test could be
+interrupted or re-run with 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
+	% grep -H . /sys/module/dmatest/parameters/*
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index c6e5d83..fc31542 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -2,6 +2,7 @@
  * DMA Engine test module
  *
  * Copyright (C) 2007 Atmel Corporation
+ * Copyright (C) 2013 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -18,6 +19,10 @@
 #include <linux/random.h>
 #include <linux/slab.h>
 #include <linux/wait.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+#include <linux/seq_file.h>
 
 static unsigned int test_buf_size = 16384;
 module_param(test_buf_size, uint, S_IRUGO);
@@ -123,6 +128,7 @@ struct dmatest_params {
 /**
  * struct dmatest_info - test information.
  * @params:		test parameters
+ * @lock:		access protection to the fields of this structure
  */
 struct dmatest_info {
 	/* Test parameters */
@@ -131,6 +137,11 @@ struct dmatest_info {
 	/* Internal state */
 	struct list_head	channels;
 	unsigned int		nr_channels;
+	struct mutex		lock;
+
+	/* debugfs related stuff */
+	struct dentry		*root;
+	struct dmatest_params	dbgfs_params;
 };
 
 static struct dmatest_info test_info;
@@ -718,7 +729,7 @@ static bool filter(struct dma_chan *chan, void *param)
 		return true;
 }
 
-static int run_threaded_test(struct dmatest_info *info)
+static int __run_threaded_test(struct dmatest_info *info)
 {
 	dma_cap_mask_t mask;
 	struct dma_chan *chan;
@@ -744,7 +755,19 @@ static int run_threaded_test(struct dmatest_info *info)
 	return err;
 }
 
-static void stop_threaded_test(struct dmatest_info *info)
+#ifndef MODULE
+static int run_threaded_test(struct dmatest_info *info)
+{
+	int ret;
+
+	mutex_lock(&info->lock);
+	ret = __run_threaded_test(info);
+	mutex_unlock(&info->lock);
+	return ret;
+}
+#endif
+
+static void __stop_threaded_test(struct dmatest_info *info)
 {
 	struct dmatest_chan *dtc, *_dtc;
 	struct dma_chan *chan;
@@ -760,13 +783,234 @@ static void stop_threaded_test(struct dmatest_info *info)
 	info->nr_channels = 0;
 }
 
+static void stop_threaded_test(struct dmatest_info *info)
+{
+	mutex_lock(&info->lock);
+	__stop_threaded_test(info);
+	mutex_unlock(&info->lock);
+}
+
+static int __restart_threaded_test(struct dmatest_info *info, bool run)
+{
+	struct dmatest_params *params = &info->params;
+	int ret;
+
+	/* Stop any running test first */
+	__stop_threaded_test(info);
+
+	if (run == false)
+		return 0;
+
+	/* Copy test parameters */
+	memcpy(params, &info->dbgfs_params, sizeof(*params));
+
+	/* Run test with new parameters */
+	ret = __run_threaded_test(info);
+	if (ret) {
+		__stop_threaded_test(info);
+		pr_err("dmatest: Can't run test\n");
+	}
+
+	return ret;
+}
+
+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)
+{
+	struct dmatest_info *info = file->private_data;
+	char buf[3];
+
+	mutex_lock(&info->lock);
+	if (info->nr_channels)
+		buf[0] = 'Y';
+	else
+		buf[0] = 'N';
+	mutex_unlock(&info->lock);
+	buf[1] = '\n';
+	buf[2] = 0x00;
+	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
+		size_t count, loff_t *ppos)
+{
+	struct dmatest_info *info = file->private_data;
+	char buf[16];
+	bool bv;
+	int ret = 0;
+
+	if (copy_from_user(buf, user_buf, min(count, (sizeof(buf) - 1))))
+		return -EFAULT;
+
+	if (strtobool(buf, &bv) == 0) {
+		mutex_lock(&info->lock);
+		ret = __restart_threaded_test(info, bv);
+		mutex_unlock(&info->lock);
+	}
+
+	return ret ? ret : count;
+}
+
+static const struct file_operations dtf_run_fops = {
+	.read	= dtf_read_run,
+	.write	= dtf_write_run,
+	.open	= simple_open,
+	.llseek	= default_llseek,
+};
+
+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);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+	if (!d)
+		goto err_root;
+
+	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);
+	if (IS_ERR_OR_NULL(d))
+		goto err_node;
+
+	return 0;
+
+err_node:
+	debugfs_remove_recursive(info->root);
+err_root:
+	pr_err("dmatest: Failed to initialize debugfs\n");
+	return ret;
+}
+
 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));
 
+	mutex_init(&info->lock);
 	INIT_LIST_HEAD(&info->channels);
 
 	/* Set default parameters */
@@ -780,7 +1024,15 @@ static int __init dmatest_init(void)
 	params->pq_sources = pq_sources;
 	params->timeout = timeout;
 
+	ret = dmatest_register_dbgfs(info);
+	if (ret)
+		return ret;
+
+#ifdef MODULE
+	return 0;
+#else
 	return run_threaded_test(info);
+#endif
 }
 /* when compiled-in wait for drivers to load first */
 late_initcall(dmatest_init);
@@ -789,6 +1041,7 @@ static void __exit dmatest_exit(void)
 {
 	struct dmatest_info *info = &test_info;
 
+	debugfs_remove_recursive(info->root);
 	stop_threaded_test(info);
 }
 module_exit(dmatest_exit);
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 06/10] dmatest: run test via debugfs

[Vinod Koul]

On Mon, Mar 04, 2013 at 11:09:30AM +0200, Andy Shevchenko wrote:
> @@ -0,0 +1,48 @@
> +				DMA Test Guide
> +				==============
> +
> +		Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> +
> +This small document introduces how to test DMA drivers using dmatest module.
> +
> +	Part 1 - How to build the test module
> +
> +The menuconfig contains an option that could be found by following path:
> +	Device Drivers -> DMA Engine support -> DMA Test client
> +
> +In the configuration file the option called CONFIG_DMATEST. The dmatest could
> +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.
> +
> +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 1 > /sys/kernel/debug/dmatest/run
How do you terminate a test? Esp if you have started bunch of concurent tests
and want to terminate one of them...

>  
>  static struct dmatest_info test_info;
> @@ -718,7 +729,7 @@ static bool filter(struct dma_chan *chan, void *param)
>  		return true;
>  }
>  
> -static int run_threaded_test(struct dmatest_info *info)
> +static int __run_threaded_test(struct dmatest_info *info)
>  {
>  	dma_cap_mask_t mask;
>  	struct dma_chan *chan;
> @@ -744,7 +755,19 @@ static int run_threaded_test(struct dmatest_info *info)
>  	return err;
>  }
>  
> -static void stop_threaded_test(struct dmatest_info *info)
> +#ifndef MODULE
> +static int run_threaded_test(struct dmatest_info *info)
> +{
> +	int ret;
> +
> +	mutex_lock(&info->lock);
> +	ret = __run_threaded_test(info);
> +	mutex_unlock(&info->lock);
> +	return ret;
> +}
> +#endif
> +
> +static void __stop_threaded_test(struct dmatest_info *info)
>  {
>  	struct dmatest_chan *dtc, *_dtc;
>  	struct dma_chan *chan;
> @@ -760,13 +783,234 @@ static void stop_threaded_test(struct dmatest_info *info)
>  	info->nr_channels = 0;
>  }
>  
> +static void stop_threaded_test(struct dmatest_info *info)
> +{
> +	mutex_lock(&info->lock);
> +	__stop_threaded_test(info);
> +	mutex_unlock(&info->lock);
> +}
> +
> +static int __restart_threaded_test(struct dmatest_info *info, bool run)
> +{
> +	struct dmatest_params *params = &info->params;
> +	int ret;
> +
> +	/* Stop any running test first */
> +	__stop_threaded_test(info);
> +
> +	if (run == false)
> +		return 0;
> +
> +	/* Copy test parameters */
> +	memcpy(params, &info->dbgfs_params, sizeof(*params));
what if the params are not yet filled by user... for module case?


--
~Vinod

Re: [PATCH 06/10] dmatest: run test via debugfs

[Andy Shevchenko]

On Tue, 2013-03-05 at 14:56 +0530, Vinod Koul wrote: 
> On Mon, Mar 04, 2013 at 11:09:30AM +0200, Andy Shevchenko wrote:
> > @@ -0,0 +1,48 @@
> > +				DMA Test Guide
> > +				==============
> > +
> > +		Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > +
> > +This small document introduces how to test DMA drivers using dmatest module.
> > +
> > +	Part 1 - How to build the test module
> > +
> > +The menuconfig contains an option that could be found by following path:
> > +	Device Drivers -> DMA Engine support -> DMA Test client
> > +
> > +In the configuration file the option called CONFIG_DMATEST. The dmatest could
> > +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.
> > +
> > +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 1 > /sys/kernel/debug/dmatest/run
> How do you terminate a test? Esp if you have started bunch of concurent tests
> and want to terminate one of them...

There is only one test in progress is possible.

To stop the test simple echo 0 > run (Probable that should be in
documentation as well)

> >  static struct dmatest_info test_info;
> > @@ -718,7 +729,7 @@ static bool filter(struct dma_chan *chan, void *param)
> >  		return true;
> >  }
> >  
> > -static int run_threaded_test(struct dmatest_info *info)
> > +static int __run_threaded_test(struct dmatest_info *info)
> >  {
> >  	dma_cap_mask_t mask;
> >  	struct dma_chan *chan;
> > @@ -744,7 +755,19 @@ static int run_threaded_test(struct dmatest_info *info)
> >  	return err;
> >  }
> >  
> > -static void stop_threaded_test(struct dmatest_info *info)
> > +#ifndef MODULE
> > +static int run_threaded_test(struct dmatest_info *info)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&info->lock);
> > +	ret = __run_threaded_test(info);
> > +	mutex_unlock(&info->lock);
> > +	return ret;
> > +}
> > +#endif
> > +
> > +static void __stop_threaded_test(struct dmatest_info *info)
> >  {
> >  	struct dmatest_chan *dtc, *_dtc;
> >  	struct dma_chan *chan;
> > @@ -760,13 +783,234 @@ static void stop_threaded_test(struct dmatest_info *info)
> >  	info->nr_channels = 0;
> >  }
> >  
> > +static void stop_threaded_test(struct dmatest_info *info)
> > +{
> > +	mutex_lock(&info->lock);
> > +	__stop_threaded_test(info);
> > +	mutex_unlock(&info->lock);
> > +}
> > +
> > +static int __restart_threaded_test(struct dmatest_info *info, bool run)
> > +{
> > +	struct dmatest_params *params = &info->params;
> > +	int ret;
> > +
> > +	/* Stop any running test first */
> > +	__stop_threaded_test(info);
> > +
> > +	if (run == false)
> > +		return 0;
> > +
> > +	/* Copy test parameters */
> > +	memcpy(params, &info->dbgfs_params, sizeof(*params));
> what if the params are not yet filled by user... for module case?

Defaults are provided by constants in the top of file. They are copied
when module_init is called.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 06/10] dmatest: run test via debugfs

[Vinod Koul]

On Tue, Mar 05, 2013 at 12:36:44PM +0200, Andy Shevchenko wrote:
> On Tue, 2013-03-05 at 14:56 +0530, Vinod Koul wrote: 
> > On Mon, Mar 04, 2013 at 11:09:30AM +0200, Andy Shevchenko wrote:
> > > @@ -0,0 +1,48 @@
> > > +				DMA Test Guide
> > > +				==============
> > > +
> > > +		Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > +
> > > +This small document introduces how to test DMA drivers using dmatest module.
> > > +
> > > +	Part 1 - How to build the test module
> > > +
> > > +The menuconfig contains an option that could be found by following path:
> > > +	Device Drivers -> DMA Engine support -> DMA Test client
> > > +
> > > +In the configuration file the option called CONFIG_DMATEST. The dmatest could
> > > +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.
> > > +
> > > +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 1 > /sys/kernel/debug/dmatest/run
> > How do you terminate a test? Esp if you have started bunch of concurent tests
> > and want to terminate one of them...
> 
> There is only one test in progress is possible.
Wasnt the idea to run concurrent tests, or I read wrongly...

--
~Vinod

Re: [PATCH 06/10] dmatest: run test via debugfs

[Andy Shevchenko]

Oh, missed Cc list in the middle. Return it back and thus leave a full
message below.

On Thu, Mar 7, 2013 at 8:11 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Wed, Mar 06, 2013 at 05:53:20PM +0200, Andy Shevchenko wrote:
>> >> > How do you terminate a test? Esp if you have started bunch of concurent tests
>> >> > and want to terminate one of them...
>> >>
>> >> There is only one test in progress is possible.
>> > Wasnt the idea to run concurrent tests, or I read wrongly...
>>
>> No, the idea was to run different tests sequentially from userspace
>> withou loading/unloading
>> module each time and with storage support for the results that could
>> be parsed automatically.
>>
>> Running concurrent tests is a good feature to be in TODO list.
> I dont think anything in code prevents you from doing so. Only issue woud
> triggering bunch of tests together and killing them
>
> What we could build on this is
> - configuringa  test but running later (so that we configure bunch of tests and
>   then start them later, when ready)
> - killing a specific task

As it's not our main goal, I prefer leave in TODO this for now.
debugfs approach opens
a wide directions to get nice QA tool for DMA drivers.

> Both of these would need some 'cookie' for tests or some otehr scheme to
> identify a test

True. It could be just a idr mapping like integer # <-> test case parameters.
I think most important feature is to add  the timestamps to the results.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 07/10] dmatest: return actual state in 'run' file

[Andy Shevchenko]

The following command should return actual state of the test.
	% cat /sys/kernel/debug/dmatest/run

To wait for test done the user may perform a busy loop that checks the state.
	% while [ $(cat /sys/kernel/debug/dmatest/run) = "Y" ]
	> do
	> 	echo -n "."
	> 	sleep 1
	> done
	> echo

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/dmatest.txt | 12 ++++++++++++
 drivers/dma/dmatest.c     | 23 +++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index 9a90729..3e17b55 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -36,6 +36,18 @@ in the original code.
 
 Note that running a new test will stop any in progress test.
 
+The following command should return actual state of the test.
+	% cat /sys/kernel/debug/dmatest/run
+
+To wait for test done the user may perform a busy loop that checks the state.
+
+	% while [ $(cat /sys/kernel/debug/dmatest/run) = "Y" ]
+	> do
+	> 	echo -n "."
+	> 	sleep 1
+	> done
+	> echo
+
 	Part 3 - When built-in in the kernel...
 
 The module parameters that is supplied to the kernel command line will be used
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index fc31542..d19234b 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -93,6 +93,7 @@ struct dmatest_thread {
 	u8			**srcs;
 	u8			**dsts;
 	enum dma_transaction_type type;
+	bool			done;
 };
 
 struct dmatest_chan {
@@ -603,6 +604,8 @@ err_thread_type:
 	if (ret)
 		dmaengine_terminate_all(chan);
 
+	thread->done = true;
+
 	if (params->iterations > 0)
 		while (!kthread_should_stop()) {
 			DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wait_dmatest_exit);
@@ -884,12 +887,28 @@ static ssize_t dtf_read_run(struct file *file, char __user *user_buf,
 {
 	struct dmatest_info *info = file->private_data;
 	char buf[3];
+	struct dmatest_chan *dtc;
+	bool alive = false;
 
 	mutex_lock(&info->lock);
-	if (info->nr_channels)
+	list_for_each_entry(dtc, &info->channels, node) {
+		struct dmatest_thread *thread;
+
+		list_for_each_entry(thread, &dtc->threads, node) {
+			if (!thread->done) {
+				alive = true;
+				break;
+			}
+		}
+	}
+
+	if (alive) {
 		buf[0] = 'Y';
-	else
+	} else {
+		__stop_threaded_test(info);
 		buf[0] = 'N';
+	}
+
 	mutex_unlock(&info->lock);
 	buf[1] = '\n';
 	buf[2] = 0x00;
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 08/10] dmatest: define MAX_ERROR_COUNT constant

[Andy Shevchenko]

Its meaning is to limit amount of error messages to be printed out when buffer
mismatch is occured.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index d19234b..4225a29 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -66,6 +66,9 @@ module_param(timeout, uint, S_IRUGO);
 MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
 		 "Pass -1 for infinite timeout");
 
+/* Maximum amount of mismatched bytes in buffer to print */
+#define MAX_ERROR_COUNT		32
+
 /*
  * Initialization patterns. All bytes in the source buffer has bit 7
  * set, all bytes in the destination buffer has bit 7 cleared.
@@ -249,7 +252,7 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
 			actual = buf[i];
 			expected = pattern | (~counter & PATTERN_COUNT_MASK);
 			if (actual != expected) {
-				if (error_count < 32)
+				if (error_count < MAX_ERROR_COUNT)
 					dmatest_mismatch(actual, pattern, i,
 							 counter, is_srcbuf);
 				error_count++;
@@ -258,9 +261,9 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
 		}
 	}
 
-	if (error_count > 32)
+	if (error_count > MAX_ERROR_COUNT)
 		pr_warning("%s: %u errors suppressed\n",
-			current->comm, error_count - 32);
+			current->comm, error_count - MAX_ERROR_COUNT);
 
 	return error_count;
 }
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 09/10] dmatest: gather test results in the linked list

[Andy Shevchenko]

The patch provides a storage for the test results in the linked list. The
gathered data could be used after test is done.

The new file 'results' represents gathered data of the in progress test. The
messages collected are printed to the kernel log as well.

Example of output:
	% cat /sys/kernel/debug/dmatest/results
	dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)

The message format is unified across the different types of errors. A number in
the parens represents additional information, e.g. error code, error counter,
or status.

Note that the buffer comparison is done in the old way, i.e. data is not
collected and just printed out.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/dmatest.txt |  19 ++++
 drivers/dma/dmatest.c     | 234 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 223 insertions(+), 30 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index 3e17b55..d05782b 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -58,3 +58,22 @@ 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
 	% grep -H . /sys/module/dmatest/parameters/*
+
+	Part 4 - Gathering the test results
+
+The module provides a storage for the test results in the memory. The gathered
+data could be used after test is done.
+
+The special file 'results' in the debugfs represents gathered data of the in
+progress test. The messages collected are printed to the kernel log as well.
+
+Example of output:
+	% cat /sys/kernel/debug/dmatest/results
+	dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)
+
+The message format is unified across the different types of errors. A number in
+the parens represents additional information, e.g. error code, error counter,
+or status.
+
+Note that the buffer comparison is done in the old way, i.e. data is not
+collected and just printed out.
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 4225a29..3697bd4 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -86,6 +86,39 @@ MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
 #define PATTERN_OVERWRITE	0x20
 #define PATTERN_COUNT_MASK	0x1f
 
+enum dmatest_error_type {
+	DMATEST_ET_OK,
+	DMATEST_ET_MAP_SRC,
+	DMATEST_ET_MAP_DST,
+	DMATEST_ET_PREP,
+	DMATEST_ET_SUBMIT,
+	DMATEST_ET_TIMEOUT,
+	DMATEST_ET_DMA_ERROR,
+	DMATEST_ET_DMA_IN_PROGRESS,
+	DMATEST_ET_VERIFY,
+};
+
+struct dmatest_thread_result {
+	struct list_head	node;
+	unsigned int		n;
+	unsigned int		src_off;
+	unsigned int		dst_off;
+	unsigned int		len;
+	enum dmatest_error_type	type;
+	union {
+		unsigned long		data;
+		dma_cookie_t		cookie;
+		enum dma_status		status;
+		int			error;
+	};
+};
+
+struct dmatest_result {
+	struct list_head	node;
+	char			*name;
+	struct list_head	results;
+};
+
 struct dmatest_info;
 
 struct dmatest_thread {
@@ -146,6 +179,10 @@ struct dmatest_info {
 	/* debugfs related stuff */
 	struct dentry		*root;
 	struct dmatest_params	dbgfs_params;
+
+	/* Test results */
+	struct list_head	results;
+	struct mutex		results_lock;
 };
 
 static struct dmatest_info test_info;
@@ -303,6 +340,98 @@ static unsigned int min_odd(unsigned int x, unsigned int y)
 	return val % 2 ? val : val - 1;
 }
 
+static char *thread_result_get(const char *name,
+		struct dmatest_thread_result *tr)
+{
+	static const char * const messages[] = {
+		[DMATEST_ET_OK]			= "No errors",
+		[DMATEST_ET_MAP_SRC]		= "src mapping error",
+		[DMATEST_ET_MAP_DST]		= "dst mapping error",
+		[DMATEST_ET_PREP]		= "prep error",
+		[DMATEST_ET_SUBMIT]		= "submit error",
+		[DMATEST_ET_TIMEOUT]		= "test timed out",
+		[DMATEST_ET_DMA_ERROR]		=
+			"got completion callback (DMA_ERROR)",
+		[DMATEST_ET_DMA_IN_PROGRESS]	=
+			"got completion callback (DMA_IN_PROGRESS)",
+		[DMATEST_ET_VERIFY]		= "errors",
+	};
+	static char buf[512];
+
+	snprintf(buf, sizeof(buf) - 1,
+		 "%s: #%u: %s with src_off=0x%x ""dst_off=0x%x len=0x%x (%lu)",
+		 name, tr->n, messages[tr->type], tr->src_off, tr->dst_off,
+		 tr->len, tr->data);
+
+	return buf;
+}
+
+static int thread_result_add(struct dmatest_info *info,
+		struct dmatest_result *r, enum dmatest_error_type type,
+		unsigned int n, unsigned int src_off, unsigned int dst_off,
+		unsigned int len, unsigned long data)
+{
+	struct dmatest_thread_result *tr;
+
+	tr = kzalloc(sizeof(*tr), GFP_KERNEL);
+	if (!tr)
+		return -ENOMEM;
+
+	tr->type = type;
+	tr->n = n;
+	tr->src_off = src_off;
+	tr->dst_off = dst_off;
+	tr->len = len;
+	tr->data = data;
+
+	mutex_lock(&info->results_lock);
+	list_add_tail(&tr->node, &r->results);
+	mutex_unlock(&info->results_lock);
+
+	pr_warn("%s\n", thread_result_get(r->name, tr));
+	return 0;
+}
+
+static void result_free(struct dmatest_info *info, const char *name)
+{
+	struct dmatest_result *r, *_r;
+
+	mutex_lock(&info->results_lock);
+	list_for_each_entry_safe(r, _r, &info->results, node) {
+		struct dmatest_thread_result *tr, *_tr;
+
+		if (name && strcmp(r->name, name))
+			continue;
+
+		list_for_each_entry_safe(tr, _tr, &r->results, node) {
+			list_del(&tr->node);
+			kfree(tr);
+		}
+
+		kfree(r->name);
+		list_del(&r->node);
+		kfree(r);
+	}
+
+	mutex_unlock(&info->results_lock);
+}
+
+static struct dmatest_result *result_init(struct dmatest_info *info,
+		const char *name)
+{
+	struct dmatest_result *r;
+
+	r = kzalloc(sizeof(*r), GFP_KERNEL);
+	if (r) {
+		r->name = kstrdup(name, GFP_KERNEL);
+		INIT_LIST_HEAD(&r->results);
+		mutex_lock(&info->results_lock);
+		list_add_tail(&r->node, &info->results);
+		mutex_unlock(&info->results_lock);
+	}
+	return r;
+}
+
 /*
  * This function repeatedly tests DMA transfers of various lengths and
  * offsets for a given operation type until it is told to exit by
@@ -339,6 +468,7 @@ static int dmatest_func(void *data)
 	int			src_cnt;
 	int			dst_cnt;
 	int			i;
+	struct dmatest_result	*result;
 
 	thread_name = current->comm;
 	set_freezable();
@@ -370,6 +500,10 @@ static int dmatest_func(void *data)
 	} else
 		goto err_thread_type;
 
+	result = result_init(info, thread_name);
+	if (!result)
+		goto err_srcs;
+
 	thread->srcs = kcalloc(src_cnt+1, sizeof(u8 *), GFP_KERNEL);
 	if (!thread->srcs)
 		goto err_srcs;
@@ -443,10 +577,10 @@ static int dmatest_func(void *data)
 			ret = dma_mapping_error(dev->dev, dma_srcs[i]);
 			if (ret) {
 				unmap_src(dev->dev, dma_srcs, len, i);
-				pr_warn("%s: #%u: mapping error %d with "
-					"src_off=0x%x len=0x%x\n",
-					thread_name, total_tests - 1, ret,
-					src_off, len);
+				thread_result_add(info, result,
+						  DMATEST_ET_MAP_SRC,
+						  total_tests, src_off, dst_off,
+						  len, ret);
 				failed_tests++;
 				continue;
 			}
@@ -461,10 +595,10 @@ static int dmatest_func(void *data)
 				unmap_src(dev->dev, dma_srcs, len, src_cnt);
 				unmap_dst(dev->dev, dma_dsts, params->buf_size,
 					  i);
-				pr_warn("%s: #%u: mapping error %d with "
-					"dst_off=0x%x len=0x%x\n",
-					thread_name, total_tests - 1, ret,
-					dst_off, params->buf_size);
+				thread_result_add(info, result,
+						  DMATEST_ET_MAP_DST,
+						  total_tests, src_off, dst_off,
+						  len, ret);
 				failed_tests++;
 				continue;
 			}
@@ -494,10 +628,9 @@ static int dmatest_func(void *data)
 			unmap_src(dev->dev, dma_srcs, len, src_cnt);
 			unmap_dst(dev->dev, dma_dsts, params->buf_size,
 				  dst_cnt);
-			pr_warning("%s: #%u: prep error with src_off=0x%x "
-					"dst_off=0x%x len=0x%x\n",
-					thread_name, total_tests - 1,
-					src_off, dst_off, len);
+			thread_result_add(info, result, DMATEST_ET_PREP,
+					  total_tests, src_off, dst_off,
+					  len, 0);
 			msleep(100);
 			failed_tests++;
 			continue;
@@ -509,10 +642,9 @@ static int dmatest_func(void *data)
 		cookie = tx->tx_submit(tx);
 
 		if (dma_submit_error(cookie)) {
-			pr_warning("%s: #%u: submit error %d with src_off=0x%x "
-					"dst_off=0x%x len=0x%x\n",
-					thread_name, total_tests - 1, cookie,
-					src_off, dst_off, len);
+			thread_result_add(info, result, DMATEST_ET_SUBMIT,
+					  total_tests, src_off, dst_off,
+					  len, cookie);
 			msleep(100);
 			failed_tests++;
 			continue;
@@ -534,15 +666,17 @@ static int dmatest_func(void *data)
 			 * free it this time?" dancing.  For now, just
 			 * leave it dangling.
 			 */
-			pr_warning("%s: #%u: test timed out\n",
-				   thread_name, total_tests - 1);
+			thread_result_add(info, result, DMATEST_ET_TIMEOUT,
+					  total_tests, src_off, dst_off,
+					  len, 0);
 			failed_tests++;
 			continue;
 		} else if (status != DMA_SUCCESS) {
-			pr_warning("%s: #%u: got completion callback,"
-				   " but status is \'%s\'\n",
-				   thread_name, total_tests - 1,
-				   status == DMA_ERROR ? "error" : "in progress");
+			enum dmatest_error_type type = (status == DMA_ERROR) ?
+				DMATEST_ET_DMA_ERROR : DMATEST_ET_DMA_IN_PROGRESS;
+			thread_result_add(info, result, type,
+					  total_tests, src_off, dst_off,
+					  len, status);
 			failed_tests++;
 			continue;
 		}
@@ -574,16 +708,14 @@ static int dmatest_func(void *data)
 				PATTERN_DST, false);
 
 		if (error_count) {
-			pr_warning("%s: #%u: %u errors with "
-				"src_off=0x%x dst_off=0x%x len=0x%x\n",
-				thread_name, total_tests - 1, error_count,
-				src_off, dst_off, len);
+			thread_result_add(info, result, DMATEST_ET_VERIFY,
+					  total_tests, src_off, dst_off,
+					  len, error_count);
 			failed_tests++;
 		} else {
-			pr_debug("%s: #%u: No errors with "
-				"src_off=0x%x dst_off=0x%x len=0x%x\n",
-				thread_name, total_tests - 1,
-				src_off, dst_off, len);
+			thread_result_add(info, result, DMATEST_ET_OK,
+					  total_tests, src_off, dst_off,
+					  len, 0);
 		}
 	}
 
@@ -807,6 +939,9 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
 	if (run == false)
 		return 0;
 
+	/* Clear results from previous run */
+	result_free(info, NULL);
+
 	/* Copy test parameters */
 	memcpy(params, &info->dbgfs_params, sizeof(*params));
 
@@ -945,6 +1080,35 @@ static const struct file_operations dtf_run_fops = {
 	.llseek	= default_llseek,
 };
 
+static int dtf_results_show(struct seq_file *sf, void *data)
+{
+	struct dmatest_info *info = sf->private;
+	struct dmatest_result *result;
+	struct dmatest_thread_result *tr;
+
+	mutex_lock(&info->results_lock);
+	list_for_each_entry(result, &info->results, node) {
+		list_for_each_entry(tr, &result->results, node)
+			seq_printf(sf, "%s\n",
+				thread_result_get(result->name, tr));
+	}
+
+	mutex_unlock(&info->results_lock);
+	return 0;
+}
+
+static int dtf_results_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dtf_results_show, inode->i_private);
+}
+
+static const struct file_operations dtf_results_fops = {
+	.open		= dtf_results_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int dmatest_register_dbgfs(struct dmatest_info *info)
 {
 	struct dentry *d;
@@ -1015,6 +1179,12 @@ static int dmatest_register_dbgfs(struct dmatest_info *info)
 	if (IS_ERR_OR_NULL(d))
 		goto err_node;
 
+	/* 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;
+
 	return 0;
 
 err_node:
@@ -1035,6 +1205,9 @@ static int __init dmatest_init(void)
 	mutex_init(&info->lock);
 	INIT_LIST_HEAD(&info->channels);
 
+	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));
@@ -1065,6 +1238,7 @@ static void __exit dmatest_exit(void)
 
 	debugfs_remove_recursive(info->root);
 	stop_threaded_test(info);
+	result_free(info, NULL);
 }
 module_exit(dmatest_exit);
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 09/10] dmatest: gather test results in the linked list

[Dan Williams]

On Mon, Mar 4, 2013 at 1:09 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The patch provides a storage for the test results in the linked list. The
> gathered data could be used after test is done.
>
> The new file 'results' represents gathered data of the in progress test. The
> messages collected are printed to the kernel log as well.
>
> Example of output:
>         % cat /sys/kernel/debug/dmatest/results
>         dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)
>
> The message format is unified across the different types of errors. A number in
> the parens represents additional information, e.g. error code, error counter,
> or status.
>
> Note that the buffer comparison is done in the old way, i.e. data is not
> collected and just printed out.
>

I need to revert this to get my testing done as it just leaks memory
for no real benefit that I can see.  Outside of making the log
messages have a uniform format I'm not seeing the case for this?  I'll
revert and add a pr_fmt to make dmatest messages readily parseable.

--
Dan

Re: [PATCH 09/10] dmatest: gather test results in the linked list

[Andy Shevchenko]

On Tue, 2013-11-05 at 11:58 -0800, Dan Williams wrote:
> On Mon, Mar 4, 2013 at 1:09 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > The patch provides a storage for the test results in the linked list. The
> > gathered data could be used after test is done.
> >
> > The new file 'results' represents gathered data of the in progress test. The
> > messages collected are printed to the kernel log as well.
> >
> > Example of output:
> >         % cat /sys/kernel/debug/dmatest/results
> >         dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)
> >
> > The message format is unified across the different types of errors. A number in
> > the parens represents additional information, e.g. error code, error counter,
> > or status.
> >
> > Note that the buffer comparison is done in the old way, i.e. data is not
> > collected and just printed out.
> >
> 
> I need to revert this to get my testing done as it just leaks memory
> for no real benefit that I can see.  Outside of making the log
> messages have a uniform format I'm not seeing the case for this?  I'll
> revert and add a pr_fmt to make dmatest messages readily parseable.


The benefit of it is to access to the results asynchronously as many
times as tester wants to.

Actually I wonder where you found memory leak. It keeps the result only
for the last test run.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 09/10] dmatest: gather test results in the linked list

[Dan Williams]

[ excuse the resend, always forget that mobile-gmail sends html by default ]

On Wed, Nov 6, 2013 at 6:13 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2013-11-05 at 11:58 -0800, Dan Williams wrote:
>> On Mon, Mar 4, 2013 at 1:09 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > The patch provides a storage for the test results in the linked list. The
>> > gathered data could be used after test is done.
>> >
>> > The new file 'results' represents gathered data of the in progress test. The
>> > messages collected are printed to the kernel log as well.
>> >
>> > Example of output:
>> >         % cat /sys/kernel/debug/dmatest/results
>> >         dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)
>> >
>> > The message format is unified across the different types of errors. A number in
>> > the parens represents additional information, e.g. error code, error counter,
>> > or status.
>> >
>> > Note that the buffer comparison is done in the old way, i.e. data is not
>> > collected and just printed out.
>> >
>>
>> I need to revert this to get my testing done as it just leaks memory
>> for no real benefit that I can see.  Outside of making the log
>> messages have a uniform format I'm not seeing the case for this?  I'll
>> revert and add a pr_fmt to make dmatest messages readily parseable.
>
>
> The benefit of it is to access to the results asynchronously as many
> times as tester wants to.
>

tail -f /var/log/messages is also asynchronous.

> Actually I wonder where you found memory leak. It keeps the result only
> for the last test run.

In my test I hit the oom killer allocating the "OK" results.

Re: [PATCH 09/10] dmatest: gather test results in the linked list

[Andy Shevchenko]

On Wed, 2013-11-06 at 10:11 -0800, Dan Williams wrote:
> [ excuse the resend, always forget that mobile-gmail sends html by default ]
> 
> On Wed, Nov 6, 2013 at 6:13 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, 2013-11-05 at 11:58 -0800, Dan Williams wrote:
> >> On Mon, Mar 4, 2013 at 1:09 AM, Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:
> >> > The patch provides a storage for the test results in the linked list. The
> >> > gathered data could be used after test is done.
> >> >
> >> > The new file 'results' represents gathered data of the in progress test. The
> >> > messages collected are printed to the kernel log as well.
> >> >
> >> > Example of output:
> >> >         % cat /sys/kernel/debug/dmatest/results
> >> >         dma0chan0-copy0: #1: No errors with src_off=0x7bf dst_off=0x8ad len=0x3fea (0)
> >> >
> >> > The message format is unified across the different types of errors. A number in
> >> > the parens represents additional information, e.g. error code, error counter,
> >> > or status.
> >> >
> >> > Note that the buffer comparison is done in the old way, i.e. data is not
> >> > collected and just printed out.
> >> >
> >>
> >> I need to revert this to get my testing done as it just leaks memory
> >> for no real benefit that I can see.  Outside of making the log
> >> messages have a uniform format I'm not seeing the case for this?  I'll
> >> revert and add a pr_fmt to make dmatest messages readily parseable.
> >
> >
> > The benefit of it is to access to the results asynchronously as many
> > times as tester wants to.
> >
> 
> tail -f /var/log/messages is also asynchronous.

It's harder to collect them by the script.

> 
> > Actually I wonder where you found memory leak. It keeps the result only
> > for the last test run.
> 
> In my test I hit the oom killer allocating the "OK" results.

Any description and steps you can share to reproduce your test?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 10/10] dmatest: append verify result to results

[Andy Shevchenko]

Comparison between buffers is stored to the dedicated structure.

Note that the verify result is now accessible only via file 'results' in the
debugfs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/dmatest.txt |   6 +-
 drivers/dma/dmatest.c     | 182 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 132 insertions(+), 56 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index d05782b..279ac0a 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -75,5 +75,7 @@ The message format is unified across the different types of errors. A number in
 the parens represents additional information, e.g. error code, error counter,
 or status.
 
-Note that the buffer comparison is done in the old way, i.e. data is not
-collected and just printed out.
+Comparison between buffers is stored to the dedicated structure.
+
+Note that the verify result is now accessible only via file 'results' in the
+debugfs.
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 3697bd4..d8ce4ec 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -96,6 +96,20 @@ enum dmatest_error_type {
 	DMATEST_ET_DMA_ERROR,
 	DMATEST_ET_DMA_IN_PROGRESS,
 	DMATEST_ET_VERIFY,
+	DMATEST_ET_VERIFY_BUF,
+};
+
+struct dmatest_verify_buffer {
+	unsigned int	index;
+	u8		expected;
+	u8		actual;
+};
+
+struct dmatest_verify_result {
+	unsigned int			error_count;
+	struct dmatest_verify_buffer	data[MAX_ERROR_COUNT];
+	u8				pattern;
+	bool				is_srcbuf;
 };
 
 struct dmatest_thread_result {
@@ -106,10 +120,11 @@ struct dmatest_thread_result {
 	unsigned int		len;
 	enum dmatest_error_type	type;
 	union {
-		unsigned long		data;
-		dma_cookie_t		cookie;
-		enum dma_status		status;
-		int			error;
+		unsigned long			data;
+		dma_cookie_t			cookie;
+		enum dma_status			status;
+		int				error;
+		struct dmatest_verify_result	*vr;
 	};
 };
 
@@ -246,35 +261,9 @@ static void dmatest_init_dsts(u8 **bufs, unsigned int start, unsigned int len,
 	}
 }
 
-static void dmatest_mismatch(u8 actual, u8 pattern, unsigned int index,
-		unsigned int counter, bool is_srcbuf)
-{
-	u8		diff = actual ^ pattern;
-	u8		expected = pattern | (~counter & PATTERN_COUNT_MASK);
-	const char	*thread_name = current->comm;
-
-	if (is_srcbuf)
-		pr_warning("%s: srcbuf[0x%x] overwritten!"
-				" Expected %02x, got %02x\n",
-				thread_name, index, expected, actual);
-	else if ((pattern & PATTERN_COPY)
-			&& (diff & (PATTERN_COPY | PATTERN_OVERWRITE)))
-		pr_warning("%s: dstbuf[0x%x] not copied!"
-				" Expected %02x, got %02x\n",
-				thread_name, index, expected, actual);
-	else if (diff & PATTERN_SRC)
-		pr_warning("%s: dstbuf[0x%x] was copied!"
-				" Expected %02x, got %02x\n",
-				thread_name, index, expected, actual);
-	else
-		pr_warning("%s: dstbuf[0x%x] mismatch!"
-				" Expected %02x, got %02x\n",
-				thread_name, index, expected, actual);
-}
-
-static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
-		unsigned int end, unsigned int counter, u8 pattern,
-		bool is_srcbuf)
+static unsigned int dmatest_verify(struct dmatest_verify_result *vr, u8 **bufs,
+		unsigned int start, unsigned int end, unsigned int counter,
+		u8 pattern, bool is_srcbuf)
 {
 	unsigned int i;
 	unsigned int error_count = 0;
@@ -282,6 +271,7 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
 	u8 expected;
 	u8 *buf;
 	unsigned int counter_orig = counter;
+	struct dmatest_verify_buffer *vb;
 
 	for (; (buf = *bufs); bufs++) {
 		counter = counter_orig;
@@ -289,9 +279,12 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
 			actual = buf[i];
 			expected = pattern | (~counter & PATTERN_COUNT_MASK);
 			if (actual != expected) {
-				if (error_count < MAX_ERROR_COUNT)
-					dmatest_mismatch(actual, pattern, i,
-							 counter, is_srcbuf);
+				if (error_count < MAX_ERROR_COUNT && vr) {
+					vb = &vr->data[error_count];
+					vb->index = i;
+					vb->expected = expected;
+					vb->actual = actual;
+				}
 				error_count++;
 			}
 			counter++;
@@ -340,6 +333,30 @@ static unsigned int min_odd(unsigned int x, unsigned int y)
 	return val % 2 ? val : val - 1;
 }
 
+static char *verify_result_get_one(struct dmatest_verify_result *vr,
+		unsigned int i)
+{
+	struct dmatest_verify_buffer *vb = &vr->data[i];
+	u8 diff = vb->actual ^ vr->pattern;
+	static char buf[512];
+	char *msg;
+
+	if (vr->is_srcbuf)
+		msg = "srcbuf overwritten!";
+	else if ((vr->pattern & PATTERN_COPY)
+			&& (diff & (PATTERN_COPY | PATTERN_OVERWRITE)))
+		msg = "dstbuf not copied!";
+	else if (diff & PATTERN_SRC)
+		msg = "dstbuf was copied!";
+	else
+		msg = "dstbuf mismatch!";
+
+	snprintf(buf, sizeof(buf) - 1, "%s [0x%x] Expected %02x, got %02x", msg,
+		 vb->index, vb->expected, vb->actual);
+
+	return buf;
+}
+
 static char *thread_result_get(const char *name,
 		struct dmatest_thread_result *tr)
 {
@@ -355,6 +372,7 @@ static char *thread_result_get(const char *name,
 		[DMATEST_ET_DMA_IN_PROGRESS]	=
 			"got completion callback (DMA_IN_PROGRESS)",
 		[DMATEST_ET_VERIFY]		= "errors",
+		[DMATEST_ET_VERIFY_BUF]		= "verify errors",
 	};
 	static char buf[512];
 
@@ -392,6 +410,51 @@ static int thread_result_add(struct dmatest_info *info,
 	return 0;
 }
 
+static unsigned int verify_result_add(struct dmatest_info *info,
+		struct dmatest_result *r, unsigned int n,
+		unsigned int src_off, unsigned int dst_off, unsigned int len,
+		u8 **bufs, int whence, unsigned int counter, u8 pattern,
+		bool is_srcbuf)
+{
+	struct dmatest_verify_result *vr;
+	unsigned int error_count;
+	unsigned int buf_off = is_srcbuf ? src_off : dst_off;
+	unsigned int start, end;
+
+	if (whence < 0) {
+		start = 0;
+		end = buf_off;
+	} else if (whence > 0) {
+		start = buf_off + len;
+		end = info->params.buf_size;
+	} else {
+		start = buf_off;
+		end = buf_off + len;
+	}
+
+	vr = kmalloc(sizeof(*vr), GFP_KERNEL);
+	if (!vr) {
+		pr_warn("dmatest: No memory to store verify result\n");
+		return dmatest_verify(NULL, bufs, start, end, counter, pattern,
+				      is_srcbuf);
+	}
+
+	vr->pattern = pattern;
+	vr->is_srcbuf = is_srcbuf;
+
+	error_count = dmatest_verify(vr, bufs, start, end, counter, pattern,
+				     is_srcbuf);
+	if (error_count) {
+		vr->error_count = error_count;
+		thread_result_add(info, r, DMATEST_ET_VERIFY_BUF, n, src_off,
+				  dst_off, len, (unsigned long)vr);
+		return error_count;
+	}
+
+	kfree(vr);
+	return 0;
+}
+
 static void result_free(struct dmatest_info *info, const char *name)
 {
 	struct dmatest_result *r, *_r;
@@ -404,6 +467,8 @@ static void result_free(struct dmatest_info *info, const char *name)
 			continue;
 
 		list_for_each_entry_safe(tr, _tr, &r->results, node) {
+			if (tr->type == DMATEST_ET_VERIFY_BUF)
+				kfree(tr->vr);
 			list_del(&tr->node);
 			kfree(tr);
 		}
@@ -687,25 +752,26 @@ static int dmatest_func(void *data)
 		error_count = 0;
 
 		pr_debug("%s: verifying source buffer...\n", thread_name);
-		error_count += dmatest_verify(thread->srcs, 0, src_off,
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->srcs, -1,
 				0, PATTERN_SRC, true);
-		error_count += dmatest_verify(thread->srcs, src_off,
-				src_off + len, src_off,
-				PATTERN_SRC | PATTERN_COPY, true);
-		error_count += dmatest_verify(thread->srcs, src_off + len,
-				params->buf_size, src_off + len,
-				PATTERN_SRC, true);
-
-		pr_debug("%s: verifying dest buffer...\n",
-				thread->task->comm);
-		error_count += dmatest_verify(thread->dsts, 0, dst_off,
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->srcs, 0,
+				src_off, PATTERN_SRC | PATTERN_COPY, true);
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->srcs, 1,
+				src_off + len, PATTERN_SRC, true);
+
+		pr_debug("%s: verifying dest buffer...\n", thread_name);
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->dsts, -1,
 				0, PATTERN_DST, false);
-		error_count += dmatest_verify(thread->dsts, dst_off,
-				dst_off + len, src_off,
-				PATTERN_SRC | PATTERN_COPY, false);
-		error_count += dmatest_verify(thread->dsts, dst_off + len,
-				params->buf_size, dst_off + len,
-				PATTERN_DST, false);
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->dsts, 0,
+				src_off, PATTERN_SRC | PATTERN_COPY, false);
+		error_count += verify_result_add(info, result, total_tests,
+				src_off, dst_off, len, thread->dsts, 1,
+				dst_off + len, PATTERN_DST, false);
 
 		if (error_count) {
 			thread_result_add(info, result, DMATEST_ET_VERIFY,
@@ -1085,12 +1151,20 @@ static int dtf_results_show(struct seq_file *sf, void *data)
 	struct dmatest_info *info = sf->private;
 	struct dmatest_result *result;
 	struct dmatest_thread_result *tr;
+	unsigned int i;
 
 	mutex_lock(&info->results_lock);
 	list_for_each_entry(result, &info->results, node) {
-		list_for_each_entry(tr, &result->results, node)
+		list_for_each_entry(tr, &result->results, node) {
 			seq_printf(sf, "%s\n",
 				thread_result_get(result->name, tr));
+			if (tr->type == DMATEST_ET_VERIFY_BUF) {
+				for (i = 0; i < tr->vr->error_count; i++) {
+					seq_printf(sf, "\t%s\n",
+						verify_result_get_one(tr->vr, i));
+				}
+			}
+		}
 	}
 
 	mutex_unlock(&info->results_lock);
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 00/10] dmatest: update the module to use debugfs

[Vinod Koul]

On Mon, Mar 04, 2013 at 11:09:24AM +0200, Andy Shevchenko wrote:
> The first approach to get dmatest module more flexible and easier to play with.
> The amount of patches could be reduced, but I would like to get a comments
> first on smaller pieces. The entire series creates dmatest.txt file in the
> Documentation folder. Similar description is scattered through the commit
> messages.
> 
> Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
> controller embedded.
Overall the series looks good. I think it can applied in this form as well.

> 
> Andy Shevchenko (10):
>   dmatest: cancel thread immediately when asked for
>   dmatest: allocate memory for pq_coefs from heap
>   dmatest: create dmatest_info to keep test parameters
>   dmatest: move dmatest_channels and nr_channels to dmatest_info
>   dmatest: split test parameters to separate structure
>   dmatest: run test via debugfs
>   dmatest: return actual state in 'run' file
>   dmatest: define MAX_ERROR_COUNT constant
>   dmatest: gather test results in the linked list
>   dmatest: append verify result to results
> 
>  Documentation/dmatest.txt |  81 +++++
>  drivers/dma/dmatest.c     | 887 +++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 832 insertions(+), 136 deletions(-)
>  create mode 100644 Documentation/dmatest.txt
> 
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

Re: [PATCH 00/10] dmatest: update the module to use debugfs

[Andy Shevchenko]

On Thu, 2013-03-07 at 11:50 +0530, Vinod Koul wrote: 
> On Mon, Mar 04, 2013 at 11:09:24AM +0200, Andy Shevchenko wrote:
> > The first approach to get dmatest module more flexible and easier to play with.
> > The amount of patches could be reduced, but I would like to get a comments
> > first on smaller pieces. The entire series creates dmatest.txt file in the
> > Documentation folder. Similar description is scattered through the commit
> > messages.
> > 
> > Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
> > controller embedded.
> Overall the series looks good. I think it can applied in this form as well.

Viresh, so, you are the only one who speaks about reducing amount of
patches. Do you think is better to either reduce, or leave as is? In
case of go with smaller amount, could you provide simple scheme which
patch should be merged with which?


> > 
> > Andy Shevchenko (10):
> >   dmatest: cancel thread immediately when asked for
> >   dmatest: allocate memory for pq_coefs from heap
> >   dmatest: create dmatest_info to keep test parameters
> >   dmatest: move dmatest_channels and nr_channels to dmatest_info
> >   dmatest: split test parameters to separate structure
> >   dmatest: run test via debugfs
> >   dmatest: return actual state in 'run' file
> >   dmatest: define MAX_ERROR_COUNT constant
> >   dmatest: gather test results in the linked list
> >   dmatest: append verify result to results
> > 
> >  Documentation/dmatest.txt |  81 +++++
> >  drivers/dma/dmatest.c     | 887 +++++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 832 insertions(+), 136 deletions(-)
> >  create mode 100644 Documentation/dmatest.txt

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 00/10] dmatest: update the module to use debugfs

[Viresh Kumar]

On 8 March 2013 21:11, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2013-03-07 at 11:50 +0530, Vinod Koul wrote:
>> On Mon, Mar 04, 2013 at 11:09:24AM +0200, Andy Shevchenko wrote:
>> > The first approach to get dmatest module more flexible and easier to play with.
>> > The amount of patches could be reduced, but I would like to get a comments
>> > first on smaller pieces. The entire series creates dmatest.txt file in the
>> > Documentation folder. Similar description is scattered through the commit
>> > messages.
>> >
>> > Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
>> > controller embedded.
>> Overall the series looks good. I think it can applied in this form as well.
>
> Viresh, so, you are the only one who speaks about reducing amount of
> patches. Do you think is better to either reduce, or leave as is? In
> case of go with smaller amount, could you provide simple scheme which
> patch should be merged with which?

Hi Andy,

I have been damn busy with Linaro Connect last week and couldn't review all
patches too. Probably you can just go ahead with this patchset only as Vinod
is okay with it.

--
viresh

Re: [PATCH 00/10] dmatest: update the module to use debugfs

[Andy Shevchenko]

On Sun, Mar 10, 2013 at 3:44 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 March 2013 21:11, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Thu, 2013-03-07 at 11:50 +0530, Vinod Koul wrote:
>>> On Mon, Mar 04, 2013 at 11:09:24AM +0200, Andy Shevchenko wrote:
>>> > The first approach to get dmatest module more flexible and easier to play with.
>>> > The amount of patches could be reduced, but I would like to get a comments
>>> > first on smaller pieces. The entire series creates dmatest.txt file in the
>>> > Documentation folder. Similar description is scattered through the commit
>>> > messages.
>>> >
>>> > Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
>>> > controller embedded.
>>> Overall the series looks good. I think it can applied in this form as well.
>>
>> Viresh, so, you are the only one who speaks about reducing amount of
>> patches. Do you think is better to either reduce, or leave as is? In
>> case of go with smaller amount, could you provide simple scheme which
>> patch should be merged with which?

> I have been damn busy with Linaro Connect last week and couldn't review all
> patches too. Probably you can just go ahead with this patchset only as Vinod
> is okay with it.

It seems Vinod is on vacation. So, take your time to finish review. I
appreciate it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/10] dmatest: update the module to use debugfs

[Vinod Koul]

On Mon, Mar 04, 2013 at 11:09:24AM +0200, Andy Shevchenko wrote:
> The first approach to get dmatest module more flexible and easier to play with.
> The amount of patches could be reduced, but I would like to get a comments
> first on smaller pieces. The entire series creates dmatest.txt file in the
> Documentation folder. Similar description is scattered through the commit
> messages.
> 
> Module was tested on Intel Medfield and Lynxpoint systems with dw_dmac DMA
> controller embedded.
Applied thanks...

> 
> Andy Shevchenko (10):
>   dmatest: cancel thread immediately when asked for
>   dmatest: allocate memory for pq_coefs from heap
>   dmatest: create dmatest_info to keep test parameters
>   dmatest: move dmatest_channels and nr_channels to dmatest_info
>   dmatest: split test parameters to separate structure
>   dmatest: run test via debugfs
>   dmatest: return actual state in 'run' file
>   dmatest: define MAX_ERROR_COUNT constant
>   dmatest: gather test results in the linked list
>   dmatest: append verify result to results
> 
>  Documentation/dmatest.txt |  81 +++++
>  drivers/dma/dmatest.c     | 887 +++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 832 insertions(+), 136 deletions(-)
>  create mode 100644 Documentation/dmatest.txt
> 
> -- 
> 1.8.2.rc0.22.gb3600c3
>