[PATCH v1 1/3] dmaengine: dmatest: Fix regression when run command on misconfigured channel

[PATCH v1 1/3] dmaengine: dmatest: Fix regression when run command on misconfigured channel

[Andy Shevchenko]

From: Vladimir Murzin <vladimir.murzin@arm.com>

Andy reported that commit 6b41030fdc79 ("dmaengine: dmatest:
Restore default for channel") broke his scripts for the case
where "busy" channel is used for configuration with expectation
that run command would do nothing (and return 0). Instead,
behavior was (unintentionally) changed to treat such case as
under-configuration and progress with defaults, i.e. run command
would start a test with default setting for channel (which would
use all channels).

Restore original behavior with tracking status of channel setter
so we can distinguish between misconfigured and under-configured
cases in run command and act accordingly.

Fixes: 6b41030fdc79 ("dmaengine: dmatest: Restore default for channel")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index b2790641370a..4c9a9d7b48bb 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -129,6 +129,7 @@ struct dmatest_params {
  * @nr_channels:	number of channels under test
  * @lock:		access protection to the fields of this structure
  * @did_init:		module has been initialized completely
+ * @last_error:		test has faced configuration issues
  */
 static struct dmatest_info {
 	/* Test parameters */
@@ -137,6 +138,7 @@ static struct dmatest_info {
 	/* Internal state */
 	struct list_head	channels;
 	unsigned int		nr_channels;
+	int			last_error;
 	struct mutex		lock;
 	bool			did_init;
 } test_info = {
@@ -1202,10 +1204,22 @@ static int dmatest_run_set(const char *val, const struct kernel_param *kp)
 		return ret;
 	} else if (dmatest_run) {
 		if (!is_threaded_test_pending(info)) {
-			pr_info("No channels configured, continue with any\n");
-			if (!is_threaded_test_run(info))
-				stop_threaded_test(info);
-			add_threaded_test(info);
+			/*
+			 * We have nothing to run. This can be due to:
+			 */
+			ret = info->last_error;
+			if (ret) {
+				/* 1) Mis-configuration */
+				pr_warn("Channel misconfigured, can't continue\n");
+				mutex_unlock(&info->lock);
+				return ret;
+			} else {
+				/* 2) We rely on defaults */
+				pr_info("No channels configured, continue with any\n");
+				if (!is_threaded_test_run(info))
+					stop_threaded_test(info);
+				add_threaded_test(info);
+			}
 		}
 		start_threaded_tests(info);
 	} else {
@@ -1222,7 +1236,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
 	struct dmatest_info *info = &test_info;
 	struct dmatest_chan *dtc;
 	char chan_reset_val[20];
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&info->lock);
 	ret = param_set_copystring(val, kp);
@@ -1230,7 +1244,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 */
@@ -1277,12 +1291,14 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
 		goto add_chan_err;
 	}
 
+	info->last_error = ret;
 	mutex_unlock(&info->lock);
 
 	return ret;
 
 add_chan_err:
 	param_set_copystring(chan_reset_val, kp);
+	info->last_error = ret;
 	mutex_unlock(&info->lock);
 
 	return ret;
-- 
2.28.0

[PATCH v1 2/3] dmaengine: dmatest: Check list for emptiness before access its last entry

[Andy Shevchenko]

After writing a garbage to the channel we get an Oops in dmatest_chan_set()
due to access to last entry in the empty list.

[  212.670672] BUG: unable to handle page fault for address: fffffff000000020
[  212.677562] #PF: supervisor read access in kernel mode
[  212.682702] #PF: error_code(0x0000) - not-present page
...
[  212.710074] RIP: 0010:dmatest_chan_set+0x149/0x2d0 [dmatest]
[  212.715739] Code: e8 cc f9 ff ff 48 8b 1d 0d 55 00 00 48 83 7b 10 00 0f 84 63 01 00 00 48 c7 c7 d0 65 4d c0 e8 ee 4a f5 e1 48 89 c6 48 8b 43 10 <48> 8b 40 20 48 8b 78 58 48 85 ff 0f 84 f5 00 00 00 e8 b1 41 f5 e1

Fix this by checking list for emptiness before accessing its last entry.

Fixes: d53513d5dc28 ("dmaengine: dmatest: Add support for multi channel testing")
Cc: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 4c9a9d7b48bb..757eb1727a04 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -1267,15 +1267,14 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
 	add_threaded_test(info);
 
 	/* Check if channel was added successfully */
-	dtc = list_last_entry(&info->channels, struct dmatest_chan, node);
-
-	if (dtc->chan) {
+	if (!list_empty(&info->channels)) {
 		/*
 		 * if new channel was not successfully added, revert the
 		 * "test_channel" string to the name of the last successfully
 		 * added channel. exception for when users issues empty string
 		 * to channel parameter.
 		 */
+		dtc = list_last_entry(&info->channels, struct dmatest_chan, node);
 		if ((strcmp(dma_chan_name(dtc->chan), strim(test_channel)) != 0)
 		    && (strcmp("", strim(test_channel)) != 0)) {
 			ret = -EINVAL;
-- 
2.28.0

[PATCH v1 3/3] dmaengine: dmatest: Return boolean result directly in filter()

[Andy Shevchenko]

There is no need to have a conditional for boolean expression when
function returns bool. Drop unnecessary code and return boolean
result directly.

While at it, drop unneeded casting from void *.

Cc: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dmatest.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 757eb1727a04..cf1379189316 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -1070,13 +1070,7 @@ static int dmatest_add_channel(struct dmatest_info *info,
 
 static bool filter(struct dma_chan *chan, void *param)
 {
-	struct dmatest_params *params = param;
-
-	if (!dmatest_match_channel(params, chan) ||
-	    !dmatest_match_device(params, chan->device))
-		return false;
-	else
-		return true;
+	return dmatest_match_channel(param, chan) && dmatest_match_device(param, chan->device);
 }
 
 static void request_channels(struct dmatest_info *info,
-- 
2.28.0

Re: [PATCH v1 3/3] dmaengine: dmatest: Return boolean result directly in filter()

[Peter Ujfalusi]

Hi Andy,

On 16/09/2020 16.34, Andy Shevchenko wrote:
> There is no need to have a conditional for boolean expression when
> function returns bool. Drop unnecessary code and return boolean
> result directly.
> 
> While at it, drop unneeded casting from void *.

my test scripts are working fine with the three patch applied.

Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Vinod: for all three patches ^^

Andy, Vladimir: thanks for the fixes!

- Péter

> Cc: Vladimir Murzin <vladimir.murzin@arm.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 757eb1727a04..cf1379189316 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -1070,13 +1070,7 @@ static int dmatest_add_channel(struct dmatest_info *info,
>  
>  static bool filter(struct dma_chan *chan, void *param)
>  {
> -	struct dmatest_params *params = param;
> -
> -	if (!dmatest_match_channel(params, chan) ||
> -	    !dmatest_match_device(params, chan->device))
> -		return false;
> -	else
> -		return true;
> +	return dmatest_match_channel(param, chan) && dmatest_match_device(param, chan->device);
>  }
>  
>  static void request_channels(struct dmatest_info *info,
> 

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v1 1/3] dmaengine: dmatest: Fix regression when run command on misconfigured channel

[Vinod Koul]

HI Andy/Vladimir
On 16-09-20, 16:34, Andy Shevchenko wrote:
> From: Vladimir Murzin <vladimir.murzin@arm.com>

Subject of the patch is not apt. It describes the effect of this patch,
which is to fix the regression but does not describe the change. Please
revise it to describe the change done in this patch

> Andy reported that commit 6b41030fdc79 ("dmaengine: dmatest:
> Restore default for channel") broke his scripts for the case
> where "busy" channel is used for configuration with expectation
> that run command would do nothing (and return 0). Instead,
> behavior was (unintentionally) changed to treat such case as
> under-configuration and progress with defaults, i.e. run command
> would start a test with default setting for channel (which would
> use all channels).

but a mis-configured channel returning success and doing nothing does
not look as a good behaviour, I agree it broke Andy's script but the
behaviour was not good to start with ;)

> Restore original behavior with tracking status of channel setter
> so we can distinguish between misconfigured and under-configured
> cases in run command and act accordingly.
> 
> Fixes: 6b41030fdc79 ("dmaengine: dmatest: Restore default for channel")
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dmatest.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index b2790641370a..4c9a9d7b48bb 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -129,6 +129,7 @@ struct dmatest_params {
>   * @nr_channels:	number of channels under test
>   * @lock:		access protection to the fields of this structure
>   * @did_init:		module has been initialized completely
> + * @last_error:		test has faced configuration issues
>   */
>  static struct dmatest_info {
>  	/* Test parameters */
> @@ -137,6 +138,7 @@ static struct dmatest_info {
>  	/* Internal state */
>  	struct list_head	channels;
>  	unsigned int		nr_channels;
> +	int			last_error;
>  	struct mutex		lock;
>  	bool			did_init;
>  } test_info = {
> @@ -1202,10 +1204,22 @@ static int dmatest_run_set(const char *val, const struct kernel_param *kp)
>  		return ret;
>  	} else if (dmatest_run) {
>  		if (!is_threaded_test_pending(info)) {
> -			pr_info("No channels configured, continue with any\n");
> -			if (!is_threaded_test_run(info))
> -				stop_threaded_test(info);
> -			add_threaded_test(info);
> +			/*
> +			 * We have nothing to run. This can be due to:
> +			 */
> +			ret = info->last_error;
> +			if (ret) {
> +				/* 1) Mis-configuration */
> +				pr_warn("Channel misconfigured, can't continue\n");

I would think this should be error

> +				mutex_unlock(&info->lock);
> +				return ret;
> +			} else {
> +				/* 2) We rely on defaults */
> +				pr_info("No channels configured, continue with any\n");
> +				if (!is_threaded_test_run(info))
> +					stop_threaded_test(info);
> +				add_threaded_test(info);
> +			}
>  		}
>  		start_threaded_tests(info);
>  	} else {
> @@ -1222,7 +1236,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
>  	struct dmatest_info *info = &test_info;
>  	struct dmatest_chan *dtc;
>  	char chan_reset_val[20];
> -	int ret = 0;
> +	int ret;

Does this belong to this fix?

>  
>  	mutex_lock(&info->lock);
>  	ret = param_set_copystring(val, kp);
> @@ -1230,7 +1244,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 */

Lets keep style issues away from fixes please

>  	if (!is_threaded_test_run(info) && !is_threaded_test_pending(info))
>  		stop_threaded_test(info);
>  	/* Reject channels that are already registered */
> @@ -1277,12 +1291,14 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
>  		goto add_chan_err;
>  	}
>  
> +	info->last_error = ret;
>  	mutex_unlock(&info->lock);
>  
>  	return ret;
>  
>  add_chan_err:
>  	param_set_copystring(chan_reset_val, kp);
> +	info->last_error = ret;
>  	mutex_unlock(&info->lock);
>  
>  	return ret;
> -- 
> 2.28.0

-- 
~Vinod

Re: [PATCH v1 1/3] dmaengine: dmatest: Fix regression when run command on misconfigured channel

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 04:00:34PM +0530, Vinod Koul wrote:

Thanks for review, my answers below.

> On 16-09-20, 16:34, Andy Shevchenko wrote:
> > From: Vladimir Murzin <vladimir.murzin@arm.com>
> 
> Subject of the patch is not apt. It describes the effect of this patch,
> which is to fix the regression but does not describe the change. Please
> revise it to describe the change done in this patch

OK!

> > Andy reported that commit 6b41030fdc79 ("dmaengine: dmatest:
> > Restore default for channel") broke his scripts for the case
> > where "busy" channel is used for configuration with expectation
> > that run command would do nothing (and return 0). Instead,
> > behavior was (unintentionally) changed to treat such case as
> > under-configuration and progress with defaults, i.e. run command
> > would start a test with default setting for channel (which would
> > use all channels).
> 
> but a mis-configured channel returning success and doing nothing does
> not look as a good behaviour, I agree it broke Andy's script but the
> behaviour was not good to start with ;)

Which used to be a previous behaviour. I don't understand what should I do here
as after this patch (and even after the initial multi-channel support patch)
the behaviour is like you desire.

> > Restore original behavior with tracking status of channel setter
> > so we can distinguish between misconfigured and under-configured
> > cases in run command and act accordingly.
> > 
> > Fixes: 6b41030fdc79 ("dmaengine: dmatest: Restore default for channel")
> > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dmatest.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index b2790641370a..4c9a9d7b48bb 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -129,6 +129,7 @@ struct dmatest_params {
> >   * @nr_channels:	number of channels under test
> >   * @lock:		access protection to the fields of this structure
> >   * @did_init:		module has been initialized completely
> > + * @last_error:		test has faced configuration issues
> >   */
> >  static struct dmatest_info {
> >  	/* Test parameters */
> > @@ -137,6 +138,7 @@ static struct dmatest_info {
> >  	/* Internal state */
> >  	struct list_head	channels;
> >  	unsigned int		nr_channels;
> > +	int			last_error;
> >  	struct mutex		lock;
> >  	bool			did_init;
> >  } test_info = {
> > @@ -1202,10 +1204,22 @@ static int dmatest_run_set(const char *val, const struct kernel_param *kp)
> >  		return ret;
> >  	} else if (dmatest_run) {
> >  		if (!is_threaded_test_pending(info)) {
> > -			pr_info("No channels configured, continue with any\n");
> > -			if (!is_threaded_test_run(info))
> > -				stop_threaded_test(info);
> > -			add_threaded_test(info);
> > +			/*
> > +			 * We have nothing to run. This can be due to:
> > +			 */
> > +			ret = info->last_error;
> > +			if (ret) {
> > +				/* 1) Mis-configuration */
> > +				pr_warn("Channel misconfigured, can't continue\n");
> 
> I would think this should be error

OK!

> > +				mutex_unlock(&info->lock);
> > +				return ret;
> > +			} else {
> > +				/* 2) We rely on defaults */
> > +				pr_info("No channels configured, continue with any\n");
> > +				if (!is_threaded_test_run(info))
> > +					stop_threaded_test(info);
> > +				add_threaded_test(info);
> > +			}
> >  		}
> >  		start_threaded_tests(info);
> >  	} else {
> > @@ -1222,7 +1236,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
> >  	struct dmatest_info *info = &test_info;
> >  	struct dmatest_chan *dtc;
> >  	char chan_reset_val[20];
> > -	int ret = 0;
> > +	int ret;
> 
> Does this belong to this fix?

Relatively. It goes under category that we can clean this up because it has
relation to what we need below. I will leave it in v2 as it is in v1.

> >  	mutex_lock(&info->lock);
> >  	ret = param_set_copystring(val, kp);
> > @@ -1230,7 +1244,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 */
> 
> Lets keep style issues away from fixes please

OK!

> >  	if (!is_threaded_test_run(info) && !is_threaded_test_pending(info))
> >  		stop_threaded_test(info);
> >  	/* Reject channels that are already registered */
> > @@ -1277,12 +1291,14 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
> >  		goto add_chan_err;
> >  	}
> >  
> > +	info->last_error = ret;
> >  	mutex_unlock(&info->lock);
> >  
> >  	return ret;
> >  
> >  add_chan_err:
> >  	param_set_copystring(chan_reset_val, kp);
> > +	info->last_error = ret;
> >  	mutex_unlock(&info->lock);
> >  
> >  	return ret;
> > -- 
> > 2.28.0
> 
> -- 
> ~Vinod

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] dmaengine: dmatest: Return boolean result directly in filter()

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 12:39:27PM +0300, Peter Ujfalusi wrote:
> On 16/09/2020 16.34, Andy Shevchenko wrote:
> > There is no need to have a conditional for boolean expression when
> > function returns bool. Drop unnecessary code and return boolean
> > result directly.
> > 
> > While at it, drop unneeded casting from void *.
> 
> my test scripts are working fine with the three patch applied.
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Vinod: for all three patches ^^
> 
> Andy, Vladimir: thanks for the fixes!

Thanks for testing! I'll apply your tag to v2 (assuming you are okay with
slight changes Vinod requested).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] dmaengine: dmatest: Fix regression when run command on misconfigured channel

[Andy Shevchenko]

On Tue, Sep 22, 2020 at 02:47:22PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 04:00:34PM +0530, Vinod Koul wrote:
> > On 16-09-20, 16:34, Andy Shevchenko wrote:
> > > From: Vladimir Murzin <vladimir.murzin@arm.com>

> > > Andy reported that commit 6b41030fdc79 ("dmaengine: dmatest:
> > > Restore default for channel") broke his scripts for the case
> > > where "busy" channel is used for configuration with expectation
> > > that run command would do nothing (and return 0). Instead,
> > > behavior was (unintentionally) changed to treat such case as
> > > under-configuration and progress with defaults, i.e. run command
> > > would start a test with default setting for channel (which would
> > > use all channels).
> > 
> > but a mis-configured channel returning success and doing nothing does
> > not look as a good behaviour, I agree it broke Andy's script but the
> > behaviour was not good to start with ;)
> 
> Which used to be a previous behaviour. I don't understand what should I do here
> as after this patch (and even after the initial multi-channel support patch)
> the behaviour is like you desire.

Okay, I have dropped the part '(and return 0)' to avoid ambiguity.

> > > Restore original behavior with tracking status of channel setter
> > > so we can distinguish between misconfigured and under-configured
> > > cases in run command and act accordingly.

-- 
With Best Regards,
Andy Shevchenko