Re: 6b41030fdc790 broke dmatest badly

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 9/15/20 1:35 PM, Andy Shevchenko wrote:
> 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).


That's good, but what about behaviour prior d53513d5dc28? Did you (or somebody
else) have a chance to confirm that it won't run with plain defaults?

> 
> 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;

Do not you change behavour from "not run, return 0" to "not run, return error"? Wouldn't somebody
come after releases and complain that we broke her scripts?

Cheers
Vladimir


> +			} 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;
>