Re: 6b41030fdc790 broke dmatest badly

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

On Fri, Sep 11, 2020 at 09:34:04AM +0100, Vladimir Murzin wrote:
> On 9/7/20 5:52 PM, Vladimir Murzin wrote:
> > On 9/7/20 3:05 PM, Andy Shevchenko wrote:
> >> On Mon, Sep 07, 2020 at 02:06:23PM +0100, Vladimir Murzin wrote:
> >>> On 9/7/20 1:06 PM, Andy Shevchenko wrote:
> >>>> On Mon, Sep 07, 2020 at 12:03:26PM +0100, Vladimir Murzin wrote:
> >>>>> On 9/4/20 6:34 PM, Andy Shevchenko wrote:
> >>>>>> It becomes a bit annoying to fix dmatest after almost each release.
> >>>>>> The commit 6b41030fdc79 ("dmaengine: dmatest: Restore default for channel")
> >>>>>> broke my use case when I tried to start busy channel.
> >>>>>>
> >>>>>> So, before this patch
> >>>>>> 	...
> >>>>>> 	echo "busy_chan" > channel
> >>>>>> 	echo 1 > run
> >>>>>> 	sh: write error: Device or resource busy
> >>>>>> 	[ 1013.868313] dmatest: Could not start test, no channels configured
> >>>>>>
> >>>>>> After I have got it run on *ALL* available channels.
> >>>>>
> >>>>> Is not that controlled with max_channels? 
> >>>>
> >>>> How? I would like to run the test against specific channel. That channel is
> >>>> occupied and thus I should get an error. This is how it suppose to work and
> >>>> actually did before your patch.
> >>>
> >>> Since you highlighted "ALL" I though that was an issue, yet looks like you
> >>> expect run command would do nothing, correct?
> >>
> >> Yes!
> >>
> >>> IIUC attempt to add already occupied channel is producing error regardless of
> >>> my patch and I do not see how error could come from run command.
> >>
> >> We need to save the status somewhere that the channel setter has been called
> >> unsuccessfully. And propagate an error to the run routine.
> > 
> > I'm not familiar with the code to propose nice and elegant solution, but for the
> > start (build only)
> > 
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index 45d4d92..40dba6b 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
> > + * @misconfig:		test has faced configuration issues
> >   */
> >  static struct dmatest_info {
> >  	/* Test parameters */
> > @@ -139,6 +140,7 @@ static struct dmatest_info {
> >  	unsigned int		nr_channels;
> >  	struct mutex		lock;
> >  	bool			did_init;
> > +	bool			misconfig;
> >  } test_info = {
> >  	.channels = LIST_HEAD_INIT(test_info.channels),
> >  	.lock = __MUTEX_INITIALIZER(test_info.lock),
> > @@ -1184,16 +1186,26 @@ 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:
> > +			 */
> > +			if (info->misconfig) {
> > +				/* 1) Mis-configuration */
> > +				pr_warn("Channels mis-configured, could not continue\n");
> > +				goto out;
> > +			} 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 {
> >  		stop_threaded_test(info);
> >  	}
> > -
> > +out:
> >  	mutex_unlock(&info->lock);
> >  
> >  	return ret;
> > @@ -1226,6 +1238,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
> >  				strlcpy(chan_reset_val,
> >  					dma_chan_name(dtc->chan),
> >  					sizeof(chan_reset_val));
> > +				info->misconfig = true;
> >  				ret = -EBUSY;
> >  				goto add_chan_err;
> >  			}
> > @@ -1246,6 +1259,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
> >  		 */
> >  		if ((strcmp(dma_chan_name(dtc->chan), strim(test_channel)) != 0)
> >  		    && (strcmp("", strim(test_channel)) != 0)) {
> > +			info->misconfig = true;
> >  			ret = -EINVAL;
> >  			strlcpy(chan_reset_val, dma_chan_name(dtc->chan),
> >  				sizeof(chan_reset_val));
> > @@ -1255,6 +1269,7 @@ static int dmatest_chan_set(const char *val, const struct kernel_param *kp)
> >  	} else {
> >  		/* Clear test_channel if no channels were added successfully */
> >  		strlcpy(chan_reset_val, "", sizeof(chan_reset_val));
> > +		info->misconfig = true;
> >  		ret = -EBUSY;
> >  		goto add_chan_err;
> >  	}
> > 
> >>
> >>> As for my patch it restores behaviour of how it supposed to work prior d53513d5dc28
> >>> where run command would execute with default settings if under-configured.
> >>
> >> Yeah, yet another breaking patch series (I have fixed one bug in that) which
> >> has been dumped and someone disappeared...
> >>
> >> Yes, and here is a corner case. I have batch script which fills sysfs
> >> parameters with something meaningful. However, when error happens in channel
> >> setter the run kick off, luckily, b/c of regression you have noticed, doesn't
> >> happen.
> >>
> >> And this behaviour as far as I remember was previously before the d53513d5dc28.
> >> At least I remember that I wrote my scripts few years ago and they worked.
> > 
> > Can we actually confirm behaviour before d53513d5dc28? That would add confidence
> > that we are doing right thing.
> >
> 
> An update on this?

Sorry for delay. I have tested your patch and it works for my case. Though I
would amend it a bit (commit message is still a due).

From 2c4acb5fd65e53a97173d910c7155df8c0dfb3c8 Mon Sep 17 00:00:00 2001
From: Vladimir Murzin <vladimir.murzin@arm.com>
Date: Mon, 7 Sep 2020 17:52:15 +0100
Subject: [PATCH 1/1] dmaengine: dmatest: 6b41030fdc790 broke dmatest badly

On 9/7/20 3:05 PM, Andy Shevchenko wrote:
> On Mon, Sep 07, 2020 at 02:06:23PM +0100, Vladimir Murzin wrote:
>> On 9/7/20 1:06 PM, Andy Shevchenko wrote:

>> IIUC attempt to add already occupied channel is producing error regardless of
>> my patch and I do not see how error could come from run command.
>
> We need to save the status somewhere that the channel setter has been called
> unsuccessfully. And propagate an error to the run routine.

I'm not familiar with the code to propose nice and elegant solution, but for the
start (build only)

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


> > As for scripts it looks like folk have them covering different cases yet seems
> > not something run regularly, so should not we cooperate with kernel CI team or its
> > equivalent and try to get (some of?) them into their environment?

> >>>>>> dmatest compiled as a module.
> >>>>>>
> >>>>>> Fix this ASAP, otherwise I will send revert of this and followed up patch next
> >>>>>> week.
> >>>>>
> >>>>> I don't quite get it, you are sending revert and then a fix rather then helping
> >>>>> with a fix?
> >>>>
> >>>> Correct.
> >>>>
> >>>>> What is reason for such extreme (and non-cooperative) flow?
> >>>>
> >>>> There are few reasons:
> >>>>  - the patch made a clear regression
> >>>>  - I do not understand what that patch is doing and how
> >>>>  - I do not have time to look at it
> >>>>  - we are now at v5.9-rc4 and it seems like one or two weeks time to get it
> >>>>    into v5.9 release
> >>>>  - and I'm annoyed by breaking this module not the first time for the last
> >>>>    couple of years
> >>>>
> >>>> And on top of that it's not how OSS community works. Since you replied, I give
> >>>> you time to figure out what's going on and provide necessary testing if needed.
> >>>>
> >>>>> P.S.
> >>>>> Unfortunately, I do not have access to hardware to run reproducer.
> >>>>
> >>>> So, please, propose a fix without it. I will test myself.

-- 
With Best Regards,
Andy Shevchenko