[PATCH] dmatest: do not allow to interrupt ongoing tests

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

When user interrupts ongoing transfers the dmatest may end up with console
lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
test.

Documentation is updated accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reported-by: Will Deacon <will.deacon@arm.com>
Tested-by: Will Deacon <will.deacon@arm.com>
---
 Documentation/dmatest.txt |  6 +++---
 drivers/dma/dmatest.c     | 45 +++++++++++++++++++++++----------------------
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index 279ac0a..132a094 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -34,7 +34,7 @@ command:
 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.
+Note that running a new test will not stop any in progress test.
 
 The following command should return actual state of the test.
 	% cat /sys/kernel/debug/dmatest/run
@@ -52,8 +52,8 @@ To wait for test done the user may perform a busy loop that checks the state.
 
 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..."
+re-run with the same or different parameters. For the details see the above
+section "Part 2 - When dmatest is built as a module..."
 
 In both cases the module parameters are used as initial values for the test case.
 You always could check them at run-time by running
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index d8ce4ec..e88ded2 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -716,8 +716,7 @@ static int dmatest_func(void *data)
 		}
 		dma_async_issue_pending(chan);
 
-		wait_event_freezable_timeout(done_wait,
-					     done.done || kthread_should_stop(),
+		wait_event_freezable_timeout(done_wait, done.done,
 					     msecs_to_jiffies(params->timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
@@ -997,7 +996,6 @@ static void stop_threaded_test(struct dmatest_info *info)
 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);
@@ -1012,13 +1010,23 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
 	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 __run_threaded_test(info);
+}
+
+static bool __is_threaded_test_run(struct dmatest_info *info)
+{
+	struct dmatest_chan *dtc;
+
+	list_for_each_entry(dtc, &info->channels, node) {
+		struct dmatest_thread *thread;
+
+		list_for_each_entry(thread, &dtc->threads, node) {
+			if (!thread->done)
+				return true;
+		}
 	}
 
-	return ret;
+	return false;
 }
 
 static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
@@ -1091,22 +1099,10 @@ 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);
-	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) {
+	if (__is_threaded_test_run(info)) {
 		buf[0] = 'Y';
 	} else {
 		__stop_threaded_test(info);
@@ -1132,7 +1128,12 @@ static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
 
 	if (strtobool(buf, &bv) == 0) {
 		mutex_lock(&info->lock);
-		ret = __restart_threaded_test(info, bv);
+
+		if (__is_threaded_test_run(info))
+			ret = -EBUSY;
+		else
+			ret = __restart_threaded_test(info, bv);
+
 		mutex_unlock(&info->lock);
 	}
 
-- 
1.8.2.rc0.22.gb3600c3

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

When user interrupts ongoing transfers the dmatest may end up with console
lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
test.

Documentation is updated accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reported-by: Will Deacon <will.deacon@arm.com>
Tested-by: Will Deacon <will.deacon@arm.com>
---
 Documentation/dmatest.txt |  6 +++---
 drivers/dma/dmatest.c     | 45 +++++++++++++++++++++++----------------------
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
index 279ac0a..132a094 100644
--- a/Documentation/dmatest.txt
+++ b/Documentation/dmatest.txt
@@ -34,7 +34,7 @@ command:
 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.
+Note that running a new test will not stop any in progress test.
 
 The following command should return actual state of the test.
 	% cat /sys/kernel/debug/dmatest/run
@@ -52,8 +52,8 @@ To wait for test done the user may perform a busy loop that checks the state.
 
 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..."
+re-run with the same or different parameters. For the details see the above
+section "Part 2 - When dmatest is built as a module..."
 
 In both cases the module parameters are used as initial values for the test case.
 You always could check them at run-time by running
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index d8ce4ec..e88ded2 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -716,8 +716,7 @@ static int dmatest_func(void *data)
 		}
 		dma_async_issue_pending(chan);
 
-		wait_event_freezable_timeout(done_wait,
-					     done.done || kthread_should_stop(),
+		wait_event_freezable_timeout(done_wait, done.done,
 					     msecs_to_jiffies(params->timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
@@ -997,7 +996,6 @@ static void stop_threaded_test(struct dmatest_info *info)
 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);
@@ -1012,13 +1010,23 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
 	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 __run_threaded_test(info);
+}
+
+static bool __is_threaded_test_run(struct dmatest_info *info)
+{
+	struct dmatest_chan *dtc;
+
+	list_for_each_entry(dtc, &info->channels, node) {
+		struct dmatest_thread *thread;
+
+		list_for_each_entry(thread, &dtc->threads, node) {
+			if (!thread->done)
+				return true;
+		}
 	}
 
-	return ret;
+	return false;
 }
 
 static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
@@ -1091,22 +1099,10 @@ 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);
-	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) {
+	if (__is_threaded_test_run(info)) {
 		buf[0] = 'Y';
 	} else {
 		__stop_threaded_test(info);
@@ -1132,7 +1128,12 @@ static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
 
 	if (strtobool(buf, &bv) == 0) {
 		mutex_lock(&info->lock);
-		ret = __restart_threaded_test(info, bv);
+
+		if (__is_threaded_test_run(info))
+			ret = -EBUSY;
+		else
+			ret = __restart_threaded_test(info, bv);
+
 		mutex_unlock(&info->lock);
 	}
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Guennadi Liakhovetski]

Hi Andy

On Thu, 23 May 2013, Andy Shevchenko wrote:

> When user interrupts ongoing transfers the dmatest may end up with console
> lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> test.

Personally I would be against such a change. What about interrupting the 
test with rmmod? Is it still possible after this your patch or not? If not 
- this doesn't seem like a good idea to me. Why don't we just fix those 
bugs, that you're describing?

Thanks
Guennadi

> 
> Documentation is updated accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Tested-by: Will Deacon <will.deacon@arm.com>
> ---
>  Documentation/dmatest.txt |  6 +++---
>  drivers/dma/dmatest.c     | 45 +++++++++++++++++++++++----------------------
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
> index 279ac0a..132a094 100644
> --- a/Documentation/dmatest.txt
> +++ b/Documentation/dmatest.txt
> @@ -34,7 +34,7 @@ command:
>  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.
> +Note that running a new test will not stop any in progress test.
>  
>  The following command should return actual state of the test.
>  	% cat /sys/kernel/debug/dmatest/run
> @@ -52,8 +52,8 @@ To wait for test done the user may perform a busy loop that checks the state.
>  
>  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..."
> +re-run with the same or different parameters. For the details see the above
> +section "Part 2 - When dmatest is built as a module..."
>  
>  In both cases the module parameters are used as initial values for the test case.
>  You always could check them at run-time by running
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index d8ce4ec..e88ded2 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -716,8 +716,7 @@ static int dmatest_func(void *data)
>  		}
>  		dma_async_issue_pending(chan);
>  
> -		wait_event_freezable_timeout(done_wait,
> -					     done.done || kthread_should_stop(),
> +		wait_event_freezable_timeout(done_wait, done.done,
>  					     msecs_to_jiffies(params->timeout));
>  
>  		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> @@ -997,7 +996,6 @@ static void stop_threaded_test(struct dmatest_info *info)
>  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);
> @@ -1012,13 +1010,23 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
>  	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 __run_threaded_test(info);
> +}
> +
> +static bool __is_threaded_test_run(struct dmatest_info *info)
> +{
> +	struct dmatest_chan *dtc;
> +
> +	list_for_each_entry(dtc, &info->channels, node) {
> +		struct dmatest_thread *thread;
> +
> +		list_for_each_entry(thread, &dtc->threads, node) {
> +			if (!thread->done)
> +				return true;
> +		}
>  	}
>  
> -	return ret;
> +	return false;
>  }
>  
>  static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
> @@ -1091,22 +1099,10 @@ 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);
> -	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) {
> +	if (__is_threaded_test_run(info)) {
>  		buf[0] = 'Y';
>  	} else {
>  		__stop_threaded_test(info);
> @@ -1132,7 +1128,12 @@ static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
>  
>  	if (strtobool(buf, &bv) == 0) {
>  		mutex_lock(&info->lock);
> -		ret = __restart_threaded_test(info, bv);
> +
> +		if (__is_threaded_test_run(info))
> +			ret = -EBUSY;
> +		else
> +			ret = __restart_threaded_test(info, bv);
> +
>  		mutex_unlock(&info->lock);
>  	}
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> On Thu, 23 May 2013, Andy Shevchenko wrote:
> 
> > When user interrupts ongoing transfers the dmatest may end up with console
> > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > test.
> 
> Personally I would be against such a change. What about interrupting the 
> test with rmmod?
>  Is it still possible after this your patch or not? If not 
> - this doesn't seem like a good idea to me. Why don't we just fix those 
> bugs, that you're describing?

The behaviour of the module is returned to the same page by this patch
as it was before (w/o debugfs).

The user can interrupt tests by rmmod, but it will take time up to
timeout.

I appreciate if you can do a deeper analysis of what happened in
case Will reported.

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

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> On Thu, 23 May 2013, Andy Shevchenko wrote:
> 
> > When user interrupts ongoing transfers the dmatest may end up with console
> > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > test.
> 
> Personally I would be against such a change. What about interrupting the 
> test with rmmod?
>  Is it still possible after this your patch or not? If not 
> - this doesn't seem like a good idea to me. Why don't we just fix those 
> bugs, that you're describing?

The behaviour of the module is returned to the same page by this patch
as it was before (w/o debugfs).

The user can interrupt tests by rmmod, but it will take time up to
timeout.

I appreciate if you can do a deeper analysis of what happened in
case Will reported.

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

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Will Deacon]

On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > 
> > > When user interrupts ongoing transfers the dmatest may end up with console
> > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > test.
> > 
> > Personally I would be against such a change. What about interrupting the 
> > test with rmmod?
> >  Is it still possible after this your patch or not? If not 
> > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > bugs, that you're describing?
> 
> The behaviour of the module is returned to the same page by this patch
> as it was before (w/o debugfs).
> 
> The user can interrupt tests by rmmod, but it will take time up to
> timeout.
> 
> I appreciate if you can do a deeper analysis of what happened in
> case Will reported.

Did this query hold up the application of this patch? I'd really like to see
*something* in 3.10, otherwise dmatest will be broken.

Will

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Will Deacon]

On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > 
> > > When user interrupts ongoing transfers the dmatest may end up with console
> > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > test.
> > 
> > Personally I would be against such a change. What about interrupting the 
> > test with rmmod?
> >  Is it still possible after this your patch or not? If not 
> > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > bugs, that you're describing?
> 
> The behaviour of the module is returned to the same page by this patch
> as it was before (w/o debugfs).
> 
> The user can interrupt tests by rmmod, but it will take time up to
> timeout.
> 
> I appreciate if you can do a deeper analysis of what happened in
> case Will reported.

Did this query hold up the application of this patch? I'd really like to see
*something* in 3.10, otherwise dmatest will be broken.

Will

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Guennadi Liakhovetski]

On Wed, 5 Jun 2013, Will Deacon wrote:

> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > 
> > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > test.
> > > 
> > > Personally I would be against such a change. What about interrupting the 
> > > test with rmmod?
> > >  Is it still possible after this your patch or not? If not 
> > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > bugs, that you're describing?
> > 
> > The behaviour of the module is returned to the same page by this patch
> > as it was before (w/o debugfs).
> > 
> > The user can interrupt tests by rmmod, but it will take time up to
> > timeout.
> > 
> > I appreciate if you can do a deeper analysis of what happened in
> > case Will reported.
> 
> Did this query hold up the application of this patch? I'd really like to see
> *something* in 3.10, otherwise dmatest will be broken.

Not from me, no. I just expressed a doubt, the author thinks there is no 
problem, and I have no capacity atm to try to verify it, so, my query 
shouldn't be considered a nak.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Guennadi Liakhovetski]

On Wed, 5 Jun 2013, Will Deacon wrote:

> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > 
> > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > test.
> > > 
> > > Personally I would be against such a change. What about interrupting the 
> > > test with rmmod?
> > >  Is it still possible after this your patch or not? If not 
> > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > bugs, that you're describing?
> > 
> > The behaviour of the module is returned to the same page by this patch
> > as it was before (w/o debugfs).
> > 
> > The user can interrupt tests by rmmod, but it will take time up to
> > timeout.
> > 
> > I appreciate if you can do a deeper analysis of what happened in
> > case Will reported.
> 
> Did this query hold up the application of this patch? I'd really like to see
> *something* in 3.10, otherwise dmatest will be broken.

Not from me, no. I just expressed a doubt, the author thinks there is no 
problem, and I have no capacity atm to try to verify it, so, my query 
shouldn't be considered a nak.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

On Wed, Jun 5, 2013 at 8:25 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 5 Jun 2013, Will Deacon wrote:
>> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
>> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote:
>> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
>> > >
>> > > > When user interrupts ongoing transfers the dmatest may end up with console
>> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
>> > > > test.
>> > >
>> > > Personally I would be against such a change. What about interrupting the
>> > > test with rmmod?
>> > >  Is it still possible after this your patch or not? If not
>> > > - this doesn't seem like a good idea to me. Why don't we just fix those
>> > > bugs, that you're describing?
>> >
>> > The behaviour of the module is returned to the same page by this patch
>> > as it was before (w/o debugfs).
>> >
>> > The user can interrupt tests by rmmod, but it will take time up to
>> > timeout.
>> >
>> > I appreciate if you can do a deeper analysis of what happened in
>> > case Will reported.
>>
>> Did this query hold up the application of this patch? I'd really like to see
>> *something* in 3.10, otherwise dmatest will be broken.

Will, Vinod is the slave DMA subsystem maintainer, I hope he could
shed a light on your concerns.
Actually, does it work as expected if you didn't run modprobe -r ?

> Not from me, no. I just expressed a doubt, the author thinks there is no
> problem, and I have no capacity atm to try to verify it, so, my query
> shouldn't be considered a nak.

Guennadi, anyway, thanks for your opinion.

Vinod, have you chance to try last patch?

--
With Best Regards,
Andy Shevchenko

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Andy Shevchenko]

On Wed, Jun 5, 2013 at 8:25 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 5 Jun 2013, Will Deacon wrote:
>> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
>> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote:
>> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
>> > >
>> > > > When user interrupts ongoing transfers the dmatest may end up with console
>> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
>> > > > test.
>> > >
>> > > Personally I would be against such a change. What about interrupting the
>> > > test with rmmod?
>> > >  Is it still possible after this your patch or not? If not
>> > > - this doesn't seem like a good idea to me. Why don't we just fix those
>> > > bugs, that you're describing?
>> >
>> > The behaviour of the module is returned to the same page by this patch
>> > as it was before (w/o debugfs).
>> >
>> > The user can interrupt tests by rmmod, but it will take time up to
>> > timeout.
>> >
>> > I appreciate if you can do a deeper analysis of what happened in
>> > case Will reported.
>>
>> Did this query hold up the application of this patch? I'd really like to see
>> *something* in 3.10, otherwise dmatest will be broken.

Will, Vinod is the slave DMA subsystem maintainer, I hope he could
shed a light on your concerns.
Actually, does it work as expected if you didn't run modprobe -r ?

> Not from me, no. I just expressed a doubt, the author thinks there is no
> problem, and I have no capacity atm to try to verify it, so, my query
> shouldn't be considered a nak.

Guennadi, anyway, thanks for your opinion.

Vinod, have you chance to try last patch?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Vinod Koul]

On Wed, Jun 05, 2013 at 06:10:01PM +0100, Will Deacon wrote:
> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > 
> > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > test.
> > > 
> > > Personally I would be against such a change. What about interrupting the 
> > > test with rmmod?
> > >  Is it still possible after this your patch or not? If not 
> > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > bugs, that you're describing?
> > 
> > The behaviour of the module is returned to the same page by this patch
> > as it was before (w/o debugfs).
> > 
> > The user can interrupt tests by rmmod, but it will take time up to
> > timeout.
> > 
> > I appreciate if you can do a deeper analysis of what happened in
> > case Will reported.
> 
> Did this query hold up the application of this patch? I'd really like to see
> *something* in 3.10, otherwise dmatest will be broken.
I though we had some data corruption on subsequent tests, or was that different
one?

--
~Vinod

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Vinod Koul]

On Wed, Jun 05, 2013 at 06:10:01PM +0100, Will Deacon wrote:
> On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > 
> > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > test.
> > > 
> > > Personally I would be against such a change. What about interrupting the 
> > > test with rmmod?
> > >  Is it still possible after this your patch or not? If not 
> > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > bugs, that you're describing?
> > 
> > The behaviour of the module is returned to the same page by this patch
> > as it was before (w/o debugfs).
> > 
> > The user can interrupt tests by rmmod, but it will take time up to
> > timeout.
> > 
> > I appreciate if you can do a deeper analysis of what happened in
> > case Will reported.
> 
> Did this query hold up the application of this patch? I'd really like to see
> *something* in 3.10, otherwise dmatest will be broken.
I though we had some data corruption on subsequent tests, or was that different
one?

--
~Vinod

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Will Deacon]

On Fri, Jun 07, 2013 at 01:45:25AM +0100, Vinod Koul wrote:
> On Wed, Jun 05, 2013 at 06:10:01PM +0100, Will Deacon wrote:
> > On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > > 
> > > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > > test.
> > > > 
> > > > Personally I would be against such a change. What about interrupting the 
> > > > test with rmmod?
> > > >  Is it still possible after this your patch or not? If not 
> > > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > > bugs, that you're describing?
> > > 
> > > The behaviour of the module is returned to the same page by this patch
> > > as it was before (w/o debugfs).
> > > 
> > > The user can interrupt tests by rmmod, but it will take time up to
> > > timeout.
> > > 
> > > I appreciate if you can do a deeper analysis of what happened in
> > > case Will reported.
> > 
> > Did this query hold up the application of this patch? I'd really like to see
> > *something* in 3.10, otherwise dmatest will be broken.
> I though we had some data corruption on subsequent tests, or was that different
> one?

We ended up with something that worked in the end:

  https://lkml.org/lkml/2013/5/23/196

Will

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Will Deacon]

On Fri, Jun 07, 2013 at 01:45:25AM +0100, Vinod Koul wrote:
> On Wed, Jun 05, 2013 at 06:10:01PM +0100, Will Deacon wrote:
> > On Mon, May 27, 2013 at 08:11:07AM +0100, Andy Shevchenko wrote:
> > > On Fri, 2013-05-24 at 23:55 +0200, Guennadi Liakhovetski wrote: 
> > > > On Thu, 23 May 2013, Andy Shevchenko wrote:
> > > > 
> > > > > When user interrupts ongoing transfers the dmatest may end up with console
> > > > > lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> > > > > test.
> > > > 
> > > > Personally I would be against such a change. What about interrupting the 
> > > > test with rmmod?
> > > >  Is it still possible after this your patch or not? If not 
> > > > - this doesn't seem like a good idea to me. Why don't we just fix those 
> > > > bugs, that you're describing?
> > > 
> > > The behaviour of the module is returned to the same page by this patch
> > > as it was before (w/o debugfs).
> > > 
> > > The user can interrupt tests by rmmod, but it will take time up to
> > > timeout.
> > > 
> > > I appreciate if you can do a deeper analysis of what happened in
> > > case Will reported.
> > 
> > Did this query hold up the application of this patch? I'd really like to see
> > *something* in 3.10, otherwise dmatest will be broken.
> I though we had some data corruption on subsequent tests, or was that different
> one?

We ended up with something that worked in the end:

  https://lkml.org/lkml/2013/5/23/196

Will

Re: [PATCH] dmatest: do not allow to interrupt ongoing tests

[Vinod Koul]

On Thu, May 23, 2013 at 02:29:53PM +0300, Andy Shevchenko wrote:
> When user interrupts ongoing transfers the dmatest may end up with console
> lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> test.
> 
> Documentation is updated accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Tested-by: Will Deacon <will.deacon@arm.com>
Applied, thanks. I will send this to linus shortly

--
~Vinod
> ---
>  Documentation/dmatest.txt |  6 +++---
>  drivers/dma/dmatest.c     | 45 +++++++++++++++++++++++----------------------
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
> index 279ac0a..132a094 100644
> --- a/Documentation/dmatest.txt
> +++ b/Documentation/dmatest.txt
> @@ -34,7 +34,7 @@ command:
>  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.
> +Note that running a new test will not stop any in progress test.
>  
>  The following command should return actual state of the test.
>  	% cat /sys/kernel/debug/dmatest/run
> @@ -52,8 +52,8 @@ To wait for test done the user may perform a busy loop that checks the state.
>  
>  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..."
> +re-run with the same or different parameters. For the details see the above
> +section "Part 2 - When dmatest is built as a module..."
>  
>  In both cases the module parameters are used as initial values for the test case.
>  You always could check them at run-time by running
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index d8ce4ec..e88ded2 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -716,8 +716,7 @@ static int dmatest_func(void *data)
>  		}
>  		dma_async_issue_pending(chan);
>  
> -		wait_event_freezable_timeout(done_wait,
> -					     done.done || kthread_should_stop(),
> +		wait_event_freezable_timeout(done_wait, done.done,
>  					     msecs_to_jiffies(params->timeout));
>  
>  		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> @@ -997,7 +996,6 @@ static void stop_threaded_test(struct dmatest_info *info)
>  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);
> @@ -1012,13 +1010,23 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
>  	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 __run_threaded_test(info);
> +}
> +
> +static bool __is_threaded_test_run(struct dmatest_info *info)
> +{
> +	struct dmatest_chan *dtc;
> +
> +	list_for_each_entry(dtc, &info->channels, node) {
> +		struct dmatest_thread *thread;
> +
> +		list_for_each_entry(thread, &dtc->threads, node) {
> +			if (!thread->done)
> +				return true;
> +		}
>  	}
>  
> -	return ret;
> +	return false;
>  }
>  
>  static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
> @@ -1091,22 +1099,10 @@ 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);
> -	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) {
> +	if (__is_threaded_test_run(info)) {
>  		buf[0] = 'Y';
>  	} else {
>  		__stop_threaded_test(info);
> @@ -1132,7 +1128,12 @@ static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
>  
>  	if (strtobool(buf, &bv) == 0) {
>  		mutex_lock(&info->lock);
> -		ret = __restart_threaded_test(info, bv);
> +
> +		if (__is_threaded_test_run(info))
> +			ret = -EBUSY;
> +		else
> +			ret = __restart_threaded_test(info, bv);
> +
>  		mutex_unlock(&info->lock);
>  	}
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

-- 

[PATCH] dmatest: do not allow to interrupt ongoing tests

[Vinod Koul]

On Thu, May 23, 2013 at 02:29:53PM +0300, Andy Shevchenko wrote:
> When user interrupts ongoing transfers the dmatest may end up with console
> lockup, oops, or data mismatch. This patch prevents user to abort any ongoing
> test.
> 
> Documentation is updated accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reported-by: Will Deacon <will.deacon@arm.com>
> Tested-by: Will Deacon <will.deacon@arm.com>
Applied, thanks. I will send this to linus shortly

--
~Vinod
> ---
>  Documentation/dmatest.txt |  6 +++---
>  drivers/dma/dmatest.c     | 45 +++++++++++++++++++++++----------------------
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/dmatest.txt b/Documentation/dmatest.txt
> index 279ac0a..132a094 100644
> --- a/Documentation/dmatest.txt
> +++ b/Documentation/dmatest.txt
> @@ -34,7 +34,7 @@ command:
>  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.
> +Note that running a new test will not stop any in progress test.
>  
>  The following command should return actual state of the test.
>  	% cat /sys/kernel/debug/dmatest/run
> @@ -52,8 +52,8 @@ To wait for test done the user may perform a busy loop that checks the state.
>  
>  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..."
> +re-run with the same or different parameters. For the details see the above
> +section "Part 2 - When dmatest is built as a module..."
>  
>  In both cases the module parameters are used as initial values for the test case.
>  You always could check them at run-time by running
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index d8ce4ec..e88ded2 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -716,8 +716,7 @@ static int dmatest_func(void *data)
>  		}
>  		dma_async_issue_pending(chan);
>  
> -		wait_event_freezable_timeout(done_wait,
> -					     done.done || kthread_should_stop(),
> +		wait_event_freezable_timeout(done_wait, done.done,
>  					     msecs_to_jiffies(params->timeout));
>  
>  		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> @@ -997,7 +996,6 @@ static void stop_threaded_test(struct dmatest_info *info)
>  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);
> @@ -1012,13 +1010,23 @@ static int __restart_threaded_test(struct dmatest_info *info, bool run)
>  	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 __run_threaded_test(info);
> +}
> +
> +static bool __is_threaded_test_run(struct dmatest_info *info)
> +{
> +	struct dmatest_chan *dtc;
> +
> +	list_for_each_entry(dtc, &info->channels, node) {
> +		struct dmatest_thread *thread;
> +
> +		list_for_each_entry(thread, &dtc->threads, node) {
> +			if (!thread->done)
> +				return true;
> +		}
>  	}
>  
> -	return ret;
> +	return false;
>  }
>  
>  static ssize_t dtf_write_string(void *to, size_t available, loff_t *ppos,
> @@ -1091,22 +1099,10 @@ 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);
> -	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) {
> +	if (__is_threaded_test_run(info)) {
>  		buf[0] = 'Y';
>  	} else {
>  		__stop_threaded_test(info);
> @@ -1132,7 +1128,12 @@ static ssize_t dtf_write_run(struct file *file, const char __user *user_buf,
>  
>  	if (strtobool(buf, &bv) == 0) {
>  		mutex_lock(&info->lock);
> -		ret = __restart_threaded_test(info, bv);
> +
> +		if (__is_threaded_test_run(info))
> +			ret = -EBUSY;
> +		else
> +			ret = __restart_threaded_test(info, bv);
> +
>  		mutex_unlock(&info->lock);
>  	}
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

--