[PATCH v1 1/6] dmaengine: dmatest: Fix iteration non-stop logic

[PATCH v1 1/6] dmaengine: dmatest: Fix iteration non-stop logic

[Andy Shevchenko]

Under some circumstances, i.e. when test is still running and about to
time out and user runs, for example,

	grep -H . /sys/module/dmatest/parameters/*

the iterations parameter is not respected and test is going on and on until
user gives

	echo 0 > /sys/module/dmatest/parameters/run

This is not what expected.

The history of this bug is interesting. I though that the commit
  2d88ce76eb98 ("dmatest: add a 'wait' parameter")
is a culprit, but looking closer to the code I think it simple revealed the
broken logic from the day one, i.e. in the commit
  0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations")
which adds iterations parameter.

So, to the point, the conditional of checking the thread to be stopped being
first part of conjunction logic prevents to check iterations. Thus, we have to
always check both conditions to be able to stop after given iterations.

Since it wasn't visible before second commit appeared, I add a respective
Fixes tag.

Fixes: 2d88ce76eb98 ("dmatest: add a 'wait' parameter")
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a2cadfa2e6d78..4993e3e5c5b01 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -662,8 +662,8 @@ static int dmatest_func(void *data)
 		flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
 
 	ktime = ktime_get();
-	while (!kthread_should_stop()
-	       && !(params->iterations && total_tests >= params->iterations)) {
+	while (!(kthread_should_stop() ||
+	       (params->iterations && total_tests >= params->iterations))) {
 		struct dma_async_tx_descriptor *tx = NULL;
 		struct dmaengine_unmap_data *um;
 		dma_addr_t *dsts;
-- 
2.26.2

[PATCH v1 2/6] dmaengine: dmatest: Fix process hang when reading 'wait' parameter

[Andy Shevchenko]

If we do

  % echo 1 > /sys/module/dmatest/parameters/run
  [  115.851124] dmatest: Could not start test, no channels configured

  % echo dma8chan7 > /sys/module/dmatest/parameters/channel
  [  127.563872] dmatest: Added 1 threads using dma8chan7

  % cat /sys/module/dmatest/parameters/wait
  ... !!! HANG !!! ...

The culprit is the commit 6138f967bccc

  ("dmaengine: dmatest: Use fixed point div to calculate iops")

which makes threads not to run, but pending and being kicked off by writing
to the 'run' node. However, it forgot to consider 'wait' routine to avoid
above mentioned case.

In order to fix this, check for really running threads, i.e. with pending
and done flags unset.

It's pity the culprit commit hadn't updated documentation and tested all
possible scenarios.

Fixes: 6138f967bccc ("dmaengine: dmatest: Use fixed point div to calculate iops")
Cc: Seraj Alijan <seraj.alijan@sondrel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 4993e3e5c5b01..307622e765996 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -240,7 +240,7 @@ static bool is_threaded_test_run(struct dmatest_info *info)
 		struct dmatest_thread *thread;
 
 		list_for_each_entry(thread, &dtc->threads, node) {
-			if (!thread->done)
+			if (!thread->done && !thread->pending)
 				return true;
 		}
 	}
@@ -1192,7 +1192,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
 		mutex_unlock(&info->lock);
 		return ret;
 	}
-	/*Clear any previously run threads */
+	/* Clear any previously run threads */
 	if (!is_threaded_test_run(info) && !is_threaded_test_pending(info))
 		stop_threaded_test(info);
 	/* Reject channels that are already registered */
-- 
2.26.2

Re: [PATCH v1 2/6] dmaengine: dmatest: Fix process hang when reading 'wait' parameter

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> If we do
> 
>   % echo 1 > /sys/module/dmatest/parameters/run
>   [  115.851124] dmatest: Could not start test, no channels configured
> 
>   % echo dma8chan7 > /sys/module/dmatest/parameters/channel
>   [  127.563872] dmatest: Added 1 threads using dma8chan7
> 
>   % cat /sys/module/dmatest/parameters/wait
>   ... !!! HANG !!! ...
> 
> The culprit is the commit 6138f967bccc
> 
>   ("dmaengine: dmatest: Use fixed point div to calculate iops")
> 
> which makes threads not to run, but pending and being kicked off by writing
> to the 'run' node. However, it forgot to consider 'wait' routine to avoid
> above mentioned case.
> 
> In order to fix this, check for really running threads, i.e. with pending
> and done flags unset.
> 
> It's pity the culprit commit hadn't updated documentation and tested all
> possible scenarios.
> 
> Fixes: 6138f967bccc ("dmaengine: dmatest: Use fixed point div to calculate iops")
> Cc: Seraj Alijan <seraj.alijan@sondrel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 4993e3e5c5b01..307622e765996 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -240,7 +240,7 @@ static bool is_threaded_test_run(struct dmatest_info *info)
>  		struct dmatest_thread *thread;
>  
>  		list_for_each_entry(thread, &dtc->threads, node) {
> -			if (!thread->done)
> +			if (!thread->done && !thread->pending)
>  				return true;
>  		}
>  	}
> @@ -1192,7 +1192,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
>  		mutex_unlock(&info->lock);
>  		return ret;
>  	}
> -	/*Clear any previously run threads */
> +	/* Clear any previously run threads */

This does not belong to this patch, can you please split it out...

-- 
~Vinod

[PATCH v1 3/6] Revert "dmaengine: dmatest: timeout value of -1 should specify infinite wait"

[Andy Shevchenko]

This reverts commit ed04b7c57c3383ed4573f1d1d1dbdc1108ba0bed.

While it gives a good description what happens, the approach seems too
confusing. Let's fix it in the following patch.

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

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 307622e765996..a521067751651 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -62,7 +62,7 @@ MODULE_PARM_DESC(pq_sources,
 static int timeout = 3000;
 module_param(timeout, uint, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
-		 "Pass 0xFFFFFFFF (4294967295) for maximum timeout");
+		 "Pass -1 for infinite timeout");
 
 static bool noverify;
 module_param(noverify, bool, S_IRUGO | S_IWUSR);
@@ -98,7 +98,7 @@ MODULE_PARM_DESC(transfer_size, "Optional custom transfer size in bytes (default
  * @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, 0 - 0xFFFFFFFF (4294967295)
+ * @timeout:		transfer timeout in msec, -1 for infinite timeout
  */
 struct dmatest_params {
 	unsigned int	buf_size;
@@ -109,7 +109,7 @@ struct dmatest_params {
 	unsigned int	iterations;
 	unsigned int	xor_sources;
 	unsigned int	pq_sources;
-	unsigned int	timeout;
+	int		timeout;
 	bool		noverify;
 	bool		norandom;
 	int		alignment;
-- 
2.26.2

Re: [PATCH v1 3/6] Revert "dmaengine: dmatest: timeout value of -1 should specify infinite wait"

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> This reverts commit ed04b7c57c3383ed4573f1d1d1dbdc1108ba0bed.
> 
> While it gives a good description what happens, the approach seems too
> confusing. Let's fix it in the following patch.

Applied, thanks

-- 
~Vinod

[PATCH v1 4/6] dmaengine: dmatest: Allow negative timeout value to specify infinite wait

[Andy Shevchenko]

The dmatest module parameter 'timeout' is documented as accepting a -1 to mean
"infinite timeout". However, an infinite timeout is not advised, nor possible
since the module parameter is an unsigned int, which won't accept a negative
value. Change the parameter type to be signed integer.

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

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a521067751651..123b4bd41a085 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -60,7 +60,7 @@ MODULE_PARM_DESC(pq_sources,
 		"Number of p+q source buffers (default: 3)");
 
 static int timeout = 3000;
-module_param(timeout, uint, S_IRUGO | S_IWUSR);
+module_param(timeout, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
 		 "Pass -1 for infinite timeout");
 
-- 
2.26.2

Re: [PATCH v1 4/6] dmaengine: dmatest: Allow negative timeout value to specify infinite wait

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> The dmatest module parameter 'timeout' is documented as accepting a -1 to mean
> "infinite timeout". However, an infinite timeout is not advised, nor possible
> since the module parameter is an unsigned int, which won't accept a negative
> value. Change the parameter type to be signed integer.

Applied, thanks

-- 
~Vinod

[PATCH v1 5/6] dmaengine: dmatest: Describe members of struct dmatest_params

[Andy Shevchenko]

Kernel documentation validator complains that not all members of
struct dmatest_params are being described. Describe them all.

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

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 123b4bd41a085..946b03859cdec 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -72,10 +72,6 @@ static bool norandom;
 module_param(norandom, bool, 0644);
 MODULE_PARM_DESC(norandom, "Disable random offset setup (default: random)");
 
-static bool polled;
-module_param(polled, bool, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(polled, "Use polling for completion instead of interrupts");
-
 static bool verbose;
 module_param(verbose, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(verbose, "Enable \"success\" result messages (default: off)");
@@ -88,6 +84,10 @@ static unsigned int transfer_size;
 module_param(transfer_size, uint, 0644);
 MODULE_PARM_DESC(transfer_size, "Optional custom transfer size in bytes (default: not used (0))");
 
+static bool polled;
+module_param(polled, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(polled, "Use polling for completion instead of interrupts");
+
 /**
  * struct dmatest_params - test parameters.
  * @buf_size:		size of the memcpy test buffer
@@ -99,6 +99,11 @@ MODULE_PARM_DESC(transfer_size, "Optional custom transfer size in bytes (default
  * @xor_sources:	number of xor source buffers
  * @pq_sources:		number of p+q source buffers
  * @timeout:		transfer timeout in msec, -1 for infinite timeout
+ * @noverify:		disable data verification
+ * @norandom:		disable random offset setup
+ * @alignment:		custom data address alignment taken as 2^alignment
+ * @transfer_size:	custom transfer size in bytes
+ * @polled:		use polling for completion instead of interrupts
  */
 struct dmatest_params {
 	unsigned int	buf_size;
-- 
2.26.2

Re: [PATCH v1 5/6] dmaengine: dmatest: Describe members of struct dmatest_params

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> Kernel documentation validator complains that not all members of
> struct dmatest_params are being described. Describe them all.

Applied, thanks

-- 
~Vinod

[PATCH v1 6/6] dmaengine: dmatest: Describe members of struct dmatest_info

[Andy Shevchenko]

Kernel documentation validator complains that not all members of
struct dmatest_info are being described. Describe them all.

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

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 946b03859cdec..31ba5d7b6ae8c 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -125,7 +125,10 @@ struct dmatest_params {
 /**
  * struct dmatest_info - test information.
  * @params:		test parameters
+ * @channels:		channels under test
+ * @nr_channels:	number of channels under test
  * @lock:		access protection to the fields of this structure
+ * @did_init:		module has been initialized completely
  */
 static struct dmatest_info {
 	/* Test parameters */
-- 
2.26.2

Re: [PATCH v1 6/6] dmaengine: dmatest: Describe members of struct dmatest_info

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> Kernel documentation validator complains that not all members of
> struct dmatest_info are being described. Describe them all.

Applied, thanks

-- 
~Vinod

Re: [PATCH v1 1/6] dmaengine: dmatest: Fix iteration non-stop logic

[Nicolas Ferre]

Andy,

On 24/04/2020 at 18:11, Andy Shevchenko wrote:
> Under some circumstances, i.e. when test is still running and about to
> time out and user runs, for example,
> 
>          grep -H . /sys/module/dmatest/parameters/*
> 
> the iterations parameter is not respected and test is going on and on until
> user gives
> 
>          echo 0 > /sys/module/dmatest/parameters/run
> 
> This is not what expected.
> 
> The history of this bug is interesting. I though that the commit
>    2d88ce76eb98 ("dmatest: add a 'wait' parameter")
> is a culprit, but looking closer to the code I think it simple revealed the
> broken logic from the day one, i.e. in the commit
>    0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations")
> which adds iterations parameter.
> 
> So, to the point, the conditional of checking the thread to be stopped being
> first part of conjunction logic prevents to check iterations. Thus, we have to
> always check both conditions to be able to stop after given iterations.
> 
> Since it wasn't visible before second commit appeared, I add a respective
> Fixes tag.
> 
> Fixes: 2d88ce76eb98 ("dmatest: add a 'wait' parameter")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>

Yes, makes sense indeed:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>


> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/dma/dmatest.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index a2cadfa2e6d78..4993e3e5c5b01 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -662,8 +662,8 @@ static int dmatest_func(void *data)
>                  flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
> 
>          ktime = ktime_get();
> -       while (!kthread_should_stop()
> -              && !(params->iterations && total_tests >= params->iterations)) {
> +       while (!(kthread_should_stop() ||
> +              (params->iterations && total_tests >= params->iterations))) {
>                  struct dma_async_tx_descriptor *tx = NULL;
>                  struct dmaengine_unmap_data *um;
>                  dma_addr_t *dsts;
> --
> 2.26.2
> 


-- 
Nicolas Ferre

Re: [PATCH v1 1/6] dmaengine: dmatest: Fix iteration non-stop logic

[Vinod Koul]

On 24-04-20, 19:11, Andy Shevchenko wrote:
> Under some circumstances, i.e. when test is still running and about to
> time out and user runs, for example,
> 
> 	grep -H . /sys/module/dmatest/parameters/*
> 
> the iterations parameter is not respected and test is going on and on until
> user gives
> 
> 	echo 0 > /sys/module/dmatest/parameters/run
> 
> This is not what expected.
> 
> The history of this bug is interesting. I though that the commit
>   2d88ce76eb98 ("dmatest: add a 'wait' parameter")
> is a culprit, but looking closer to the code I think it simple revealed the
> broken logic from the day one, i.e. in the commit
>   0a2ff57d6fba ("dmaengine: dmatest: add a maximum number of test iterations")
> which adds iterations parameter.
> 
> So, to the point, the conditional of checking the thread to be stopped being
> first part of conjunction logic prevents to check iterations. Thus, we have to
> always check both conditions to be able to stop after given iterations.
> 
> Since it wasn't visible before second commit appeared, I add a respective
> Fixes tag.

Applied, thanks

-- 
~Vinod