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@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> > Tested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@xxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> > 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
