[PATCH] staging: comedi: ni_mio_common: fix AO inttrig backwards compatibility

[PATCH] staging: comedi: ni_mio_common: fix AO inttrig backwards compatibility

[Ian Abbott]

Commit ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
cmd->start_arg validation and use") introduced a backwards compatibility
issue in the use of asynchronous commands on the AO subdevice when
`start_src` is `TRIG_EXT`.  Valid values for `start_src` are `TRIG_INT`
(for internal, software trigger), and `TRIG_EXT` (for external trigger).
When set to `TRIG_EXT`.  In both cases, the driver relies on an
internal, software trigger to set things up (allowing the user
application to write sufficient samples to the data buffer before the
trigger), so it acts as a software "pre-trigger" in the `TRIG_EXT` case.
The software trigger is handled by `ni_ao_inttrig()`.

Prior to the above change, when `start_src` was `TRIG_INT`, `start_arg`
was required to be 0, and `ni_ao_inttrig()` checked that the software
trigger number was also 0.  After the above change, when `start_src` was
`TRIG_INT`, any value was allowed for `start_arg`, and `ni_ao_inttrig()`
checked that the software trigger number matched this `start_arg` value.
The backwards compatibility issue is that the internal trigger number
now has to match `start_arg` when `start_src` is `TRIG_EXT` when it
previously had to be 0.

Fix the backwards compatibility issue in `ni_ao_inttrig()` by always
allowing software trigger number 0 when `start_src` is something other
than `TRIG_INT`.

Thanks to Spencer Olson for reporting the issue.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reported-by: Spencer Olson <olsonse@umich.edu>
Fixes: ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
  cmd->start_arg validation and use")
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 8dabb19..9f4036f 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2772,7 +2772,15 @@ static int ni_ao_inttrig(struct comedi_device *dev,
 	int i;
 	static const int timeout = 1000;
 
-	if (trig_num != cmd->start_arg)
+	/*
+	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
+	 * For backwards compatibility, also allow trig_num == 0 when
+	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
+	 * in that case, the internal trigger is being used as a pre-trigger
+	 * before the external trigger.
+	 */
+	if (!(trig_num == cmd->start_arg ||
+	      (trig_num == 0 && cmd->start_src != TRIG_INT)))
 		return -EINVAL;
 
 	/*
-- 
2.8.1

RE: [PATCH] staging: comedi: ni_mio_common: fix AO inttrig backwards compatibility

[Hartley Sweeten]

On Tuesday, July 19, 2016 4:18 AM, Ian Abbott wrote:
> Commit ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
> cmd->start_arg validation and use") introduced a backwards compatibility
> issue in the use of asynchronous commands on the AO subdevice when
> `start_src` is `TRIG_EXT`.  Valid values for `start_src` are `TRIG_INT`
> (for internal, software trigger), and `TRIG_EXT` (for external trigger).
> When set to `TRIG_EXT`.  In both cases, the driver relies on an
> internal, software trigger to set things up (allowing the user
> application to write sufficient samples to the data buffer before the
> trigger), so it acts as a software "pre-trigger" in the `TRIG_EXT` case.
> The software trigger is handled by `ni_ao_inttrig()`.
>
> Prior to the above change, when `start_src` was `TRIG_INT`, `start_arg`
> was required to be 0, and `ni_ao_inttrig()` checked that the software
> trigger number was also 0.  After the above change, when `start_src` was
> `TRIG_INT`, any value was allowed for `start_arg`, and `ni_ao_inttrig()`
> checked that the software trigger number matched this `start_arg` value.
> The backwards compatibility issue is that the internal trigger number
> now has to match `start_arg` when `start_src` is `TRIG_EXT` when it
> previously had to be 0.
>
> Fix the backwards compatibility issue in `ni_ao_inttrig()` by always
> allowing software trigger number 0 when `start_src` is something other
> than `TRIG_INT`.
>
> Thanks to Spencer Olson for reporting the issue.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> Reported-by: Spencer Olson <olsonse@umich.edu>
> Fixes: ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
>   cmd->start_arg validation and use")
> ---
>  drivers/staging/comedi/drivers/ni_mio_common.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 8dabb19..9f4036f 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -2772,7 +2772,15 @@ static int ni_ao_inttrig(struct comedi_device *dev,
>  	int i;
>  	static const int timeout = 1000;
>  
> -	if (trig_num != cmd->start_arg)
> +	/*
> +	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
> +	 * For backwards compatibility, also allow trig_num == 0 when
> +	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
> +	 * in that case, the internal trigger is being used as a pre-trigger
> +	 * before the external trigger.
> +	 */
> +	if (!(trig_num == cmd->start_arg ||
> +	      (trig_num == 0 && cmd->start_src != TRIG_INT)))
>  		return -EINVAL;

Ian,

I think this test is a bit clearer:

+	/*
+	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
+	 * For backwards compatibility, any trig_num is valid when
+	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
+	 * in that case, the internal trigger is being used as a pre-trigger
+	 * before the external trigger.
+	 */
+	if (cmd->start_src == TRIG_INT && trig_num != cmd->start_arg)
		return -EINVAL;

But, either way:

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks!

Re: [PATCH] staging: comedi: ni_mio_common: fix AO inttrig backwards compatibility

[Ian Abbott]

On 20/07/16 16:55, Hartley Sweeten wrote:
> On Tuesday, July 19, 2016 4:18 AM, Ian Abbott wrote:
>> Commit ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
>> cmd->start_arg validation and use") introduced a backwards compatibility
>> issue in the use of asynchronous commands on the AO subdevice when
>> `start_src` is `TRIG_EXT`.  Valid values for `start_src` are `TRIG_INT`
>> (for internal, software trigger), and `TRIG_EXT` (for external trigger).
>> When set to `TRIG_EXT`.  In both cases, the driver relies on an
>> internal, software trigger to set things up (allowing the user
>> application to write sufficient samples to the data buffer before the
>> trigger), so it acts as a software "pre-trigger" in the `TRIG_EXT` case.
>> The software trigger is handled by `ni_ao_inttrig()`.
>>
>> Prior to the above change, when `start_src` was `TRIG_INT`, `start_arg`
>> was required to be 0, and `ni_ao_inttrig()` checked that the software
>> trigger number was also 0.  After the above change, when `start_src` was
>> `TRIG_INT`, any value was allowed for `start_arg`, and `ni_ao_inttrig()`
>> checked that the software trigger number matched this `start_arg` value.
>> The backwards compatibility issue is that the internal trigger number
>> now has to match `start_arg` when `start_src` is `TRIG_EXT` when it
>> previously had to be 0.
>>
>> Fix the backwards compatibility issue in `ni_ao_inttrig()` by always
>> allowing software trigger number 0 when `start_src` is something other
>> than `TRIG_INT`.
>>
>> Thanks to Spencer Olson for reporting the issue.
>>
>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>> Reported-by: Spencer Olson <olsonse@umich.edu>
>> Fixes: ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
>>   cmd->start_arg validation and use")
>> ---
>>  drivers/staging/comedi/drivers/ni_mio_common.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
>> index 8dabb19..9f4036f 100644
>> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
>> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>> @@ -2772,7 +2772,15 @@ static int ni_ao_inttrig(struct comedi_device *dev,
>>  	int i;
>>  	static const int timeout = 1000;
>>
>> -	if (trig_num != cmd->start_arg)
>> +	/*
>> +	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
>> +	 * For backwards compatibility, also allow trig_num == 0 when
>> +	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
>> +	 * in that case, the internal trigger is being used as a pre-trigger
>> +	 * before the external trigger.
>> +	 */
>> +	if (!(trig_num == cmd->start_arg ||
>> +	      (trig_num == 0 && cmd->start_src != TRIG_INT)))
>>  		return -EINVAL;
>
> Ian,
>
> I think this test is a bit clearer:
>
> +	/*
> +	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
> +	 * For backwards compatibility, any trig_num is valid when
> +	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
> +	 * in that case, the internal trigger is being used as a pre-trigger
> +	 * before the external trigger.
> +	 */
> +	if (cmd->start_src == TRIG_INT && trig_num != cmd->start_arg)
> 		return -EINVAL;

True, but I restricted it to only accept trig_num values that have been 
valid in the past.

>
> But, either way:
>
> Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>
> Thanks!
>

Thanks for the review.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH] staging: comedi: ni_mio_common: fix AO inttrig backwards compatibility

[Spencer E Olson]

Sorry for the very belated reply on this.  I'm assuming that this was
already accepted, but I've been working with this patch for a bit.  This
fixes the problems I raised in any case.

Reviewed-by: Spencer E Olson <olsonse@umich.edu>

On Wed, 2016-07-20 at 17:07 +0100, Ian Abbott wrote:
> On 20/07/16 16:55, Hartley Sweeten wrote:
> > On Tuesday, July 19, 2016 4:18 AM, Ian Abbott wrote:
> >> Commit ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
> >> cmd->start_arg validation and use") introduced a backwards compatibility
> >> issue in the use of asynchronous commands on the AO subdevice when
> >> `start_src` is `TRIG_EXT`.  Valid values for `start_src` are `TRIG_INT`
> >> (for internal, software trigger), and `TRIG_EXT` (for external trigger).
> >> When set to `TRIG_EXT`.  In both cases, the driver relies on an
> >> internal, software trigger to set things up (allowing the user
> >> application to write sufficient samples to the data buffer before the
> >> trigger), so it acts as a software "pre-trigger" in the `TRIG_EXT` case.
> >> The software trigger is handled by `ni_ao_inttrig()`.
> >>
> >> Prior to the above change, when `start_src` was `TRIG_INT`, `start_arg`
> >> was required to be 0, and `ni_ao_inttrig()` checked that the software
> >> trigger number was also 0.  After the above change, when `start_src` was
> >> `TRIG_INT`, any value was allowed for `start_arg`, and `ni_ao_inttrig()`
> >> checked that the software trigger number matched this `start_arg` value.
> >> The backwards compatibility issue is that the internal trigger number
> >> now has to match `start_arg` when `start_src` is `TRIG_EXT` when it
> >> previously had to be 0.
> >>
> >> Fix the backwards compatibility issue in `ni_ao_inttrig()` by always
> >> allowing software trigger number 0 when `start_src` is something other
> >> than `TRIG_INT`.
> >>
> >> Thanks to Spencer Olson for reporting the issue.
> >>
> >> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> >> Reported-by: Spencer Olson <olsonse@umich.edu>
> >> Fixes: ebb657babfa9 ("staging: comedi: ni_mio_common: clarify the
> >>   cmd->start_arg validation and use")
> >> ---
> >>  drivers/staging/comedi/drivers/ni_mio_common.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> >> index 8dabb19..9f4036f 100644
> >> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> >> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> >> @@ -2772,7 +2772,15 @@ static int ni_ao_inttrig(struct comedi_device *dev,
> >>  	int i;
> >>  	static const int timeout = 1000;
> >>
> >> -	if (trig_num != cmd->start_arg)
> >> +	/*
> >> +	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
> >> +	 * For backwards compatibility, also allow trig_num == 0 when
> >> +	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
> >> +	 * in that case, the internal trigger is being used as a pre-trigger
> >> +	 * before the external trigger.
> >> +	 */
> >> +	if (!(trig_num == cmd->start_arg ||
> >> +	      (trig_num == 0 && cmd->start_src != TRIG_INT)))
> >>  		return -EINVAL;
> >
> > Ian,
> >
> > I think this test is a bit clearer:
> >
> > +	/*
> > +	 * Require trig_num == cmd->start_arg when cmd->start_src == TRIG_INT.
> > +	 * For backwards compatibility, any trig_num is valid when
> > +	 * cmd->start_src != TRIG_INT (i.e. when cmd->start_src == TRIG_EXT);
> > +	 * in that case, the internal trigger is being used as a pre-trigger
> > +	 * before the external trigger.
> > +	 */
> > +	if (cmd->start_src == TRIG_INT && trig_num != cmd->start_arg)
> > 		return -EINVAL;
> 
> True, but I restricted it to only accept trig_num values that have been 
> valid in the past.
> 
> >
> > But, either way:
> >
> > Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> >
> > Thanks!
> >
> 
> Thanks for the review.
>