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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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