On 2014-09-10 19:39, Hartley Sweeten wrote:
On Wednesday, September 10, 2014 3:56 AM, Ian Abbott wrote:
On 2014-09-10 00:15, H Hartley Sweeten wrote:
The validation of the cmd->stop_arg when the cmd->stop_src == TRIG_EXT
is a bit over thought. The comments state that the stop_arg is validated
to force an external trigger of 0 and allow the CR_EDGE flag, which is
ignored. In reality the stop_arg is not even used by the driver when
the stop_src is TRIG_EXT.
Simplify the validation so that the stop_arg must simply be '0' when
the stop_src is TRIG_EXT.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
I'd say that CR_EDGE is a perfectly legal flag to use here (as the
external trigger *is* edge-triggered), even if the driver allows the
user not to set it. If the user code went to the trouble of setting the
flag only for the driver to clear it and flag it as an errors, its like
the driver saying "Sorry, I don't support edge-triggered triggers here."
Fair enough. I asked Greg to drop this patch (along with patch 8).
I still feel the TRIG_EXT arg validation is a bit messy. How about
adding a new helper to comedi_fc? Something like:
/**
* cfc_check_ext_trigger_arg() - trivially validate a TRIG_EXT argument
* @arg: pointer to the trigger arg to validate
* @min_chan: the minimum channel to use as the external trigger
* @max_chan: the maximum channel to use as the external trigger
* @allowed_flags: mask of allowed flags
* @reg_flags: mask of required flags
*/
static int cfc_check_ext_trigger_arg(unsigned int *arg,
unsigned int min_chan,
unsigned int max_chan,
unsigned int allowed_flags,
unsigned int req_flags)
{
unsigned int chan = CR_CHAN(arg);
unsigned int flags = arg & CR_FLAGS_MASK;
int ret = 0;
/* validate that the channel is in range */
if (chan < min_chan || chan > max_chan) {
/*preserve flags and default to the minimum channel */
*arg &= CR_FLAGS_MASK;
*arg |= CR_PACK(min_chan, 0, 0);
ret |= -EINVAL;
}
/* validate that only the allowed flags are set */
if (flags & ~allowed_flags) {
/* preserve channel and default to the required flags */
*arg &= ~ CR_FLAGS_MASK;
*arg |= req_flags;
ret |= -EINVAL;
}
/* validate that the required flags are set */
if ((flags & req_flags) == 0) {
*arg |= req_flags;
ret |= -EINVAL;
}
return ret;
}
The (*do_cmdtest) functions then just do something like:
/* trigger is channel 0, CR_EDGE is allowed but ignored */
err |= cfc_check_ext_trigger_arg(&cmd->stop_arg, 0, 0,
CR_EDGE, 0);
There are a few drivers that could use something like that right now,
namely amplc_pci224, amplc_pci230, cb_pcidas, ni_mio_common, ni_pcidio.
Perhaps a few others could use it in the future. A few drivers don't
seem to check the argument at all for TRIG_EXT, but it's up to the
drivers how they want to interpret the argument (e.g. ni_tiocmd does
it's own thing with the argument). So the TRIG_EXT arg checking by this
function wouldn't apply to all drivers, only the ones that interpret the
argument in this way.
There are some parts of a 32-bit value not covered by the union of
CR_FLAGS_MASK and the (non-existent) CR_CHAN_MASK (0xffff). For a
chanlist value, those parts would describe the range and 'aref', but are
probably meaningless for external triggers following the pattern of a
trigger number plus a few flags.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
