On 2014-04-15 18:37, H Hartley Sweeten wrote:
The only time the chanlist can be NULL and chanlist_len < 1 is when a
COMEDI_CMDTEST ioctl is done, usually for the comedilib function
comedi_get_cmd_src_mask(). This comedilib function sets all the command
sources to TRIG_ANY and Step 1 of the (*do_cmdtest) will fail returning
a the mask of valid sources.
For the rest of the (*do_cmdtest) and the (*do_cmd) functions a valid
chanlist and chanlist_len >= 1 is required.
Modify the check in __comedi_get_user_chanlist() to allow a NULL
chanlist or chanlist_len < 1 only when all the command sources are
TRIG_ANY.
Remove the checks in the comedi_driver (*do_cmdtest) functions that
don't result in to much diff noise. The remaining ones will be removed
in following patches.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
NAK. This will break existing libcomedi library function
comedi_get_generic_timed(), which will call the COMEDI_CMDTEST ioctl
with a NULL chanlist (but with chanlist_len > 1) and with start_src etc.
set to values other than TRIG_ANY.
For COMEDI_CMDTEST, chanlist is allowed to be NULL and chanlist_len is
allowed to be 0 (although some drivers' do_cmdtest handlers currently
check it is at least 1). (However, the command structure might get
modified in strange ways if the command test gets as far as "step 3"
with a zero chanlist_len, so it's best for the user-level code to set a
proper chanlist_len if it's expecting the test to get as far as "step 3".)
However, for the COMEDI_CMD ioctl, chanlist_len needs to be at least 1
and chanlist needs to be non-NULL, but its NULL-ness shouldn't be
checked until after s->do_cmdtest() has been called, as that can modify
the erroneous command before passing it back to the user.
---
drivers/staging/comedi/comedi_fops.c | 31 +++++++++++++++-------
.../comedi/drivers/addi-data/hwdrv_apci3120.c | 2 --
drivers/staging/comedi/drivers/adl_pci9118.c | 5 +---
drivers/staging/comedi/drivers/amplc_pci224.c | 5 ----
drivers/staging/comedi/drivers/cb_pcidas64.c | 2 --
drivers/staging/comedi/drivers/comedi_test.c | 1 -
drivers/staging/comedi/drivers/das1800.c | 1 -
drivers/staging/comedi/drivers/das800.c | 1 -
drivers/staging/comedi/drivers/me4000.c | 16 -----------
drivers/staging/comedi/drivers/ni_at_a2150.c | 1 -
drivers/staging/comedi/drivers/ni_labpc.c | 9 -------
drivers/staging/comedi/drivers/pcl812.c | 1 -
drivers/staging/comedi/drivers/pcl816.c | 10 +++----
drivers/staging/comedi/drivers/pcl818.c | 9 +++----
drivers/staging/comedi/drivers/usbduxfast.c | 3 ---
15 files changed, 28 insertions(+), 69 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index e283e6c..5a0e702 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1465,9 +1465,27 @@ static int __comedi_get_user_chanlist(struct comedi_device *dev,
unsigned int *chanlist;
int ret;
- /* user_chanlist could be NULL for do_cmdtest ioctls */
- if (!user_chanlist)
- return 0;
+ /*
+ * The only time the user_chanlist is allowed to be NULL
+ * or chanlist_len < 1 is when a COMEDI_CMDTEST ioctl is
+ * done to get the mask of valid command sources. This is
+ * done by setting all the command sources to TRIG_ANY
+ * and Step 1 of the (*do_cmdtest) will fail returning a
+ * mask of valid sources.
+ */
+ if (!user_chanlist || cmd->chanlist_len < 1) {
+ if (cmd->start_src == TRIG_ANY &&
+ cmd->scan_begin_src == TRIG_ANY &&
+ cmd->convert_src == TRIG_ANY &&
+ cmd->scan_end_src == TRIG_ANY &&
+ cmd->stop_src == TRIG_ANY) {
+ return 0;
+ } else {
+ dev_err(dev->class_dev,
+ "async command must have a chanlist\n");
+ return -EINVAL;
+ }
+ }
chanlist = memdup_user(user_chanlist,
cmd->chanlist_len * sizeof(unsigned int));
I think the safest option is to leave __comedi_get_user_chanlist()
unchanged and do additional tests in do_cmd_ioctl().
Another possible change that may lead to some simplifications is to
force chanlist to be NULL if cmd->chanlist_len is 0 like this:
if (cmd->chanlist_len)
chanlist = memdup_user(user_chanlist,
cmd->chanlist_len * sizeof(unsigned int));
else
chanlist = NULL;
(Normally, the linux kernel memory allocation functions return a
non-NULL pointer value even for a zero-length allocation.)
@@ -1518,13 +1536,6 @@ static int do_cmd_ioctl(struct comedi_device *dev,
return -EBUSY;
}
- /* make sure channel/gain list isn't too short */
- if (cmd.chanlist_len < 1) {
- dev_dbg(dev->class_dev, "channel/gain list too short %u < 1\n",
- cmd.chanlist_len);
- return -EINVAL;
- }
-
Can leave that test in do_cmd_ioctl().
async->cmd = cmd;
async->cmd.data = NULL;
Further down do_cmd_ioctl() we have:
ret = s->do_cmdtest(dev, s, &async->cmd);
if (async->cmd.flags & TRIG_BOGUS || ret) {
dev_dbg(dev->class_dev, "test returned %d\n", ret);
cmd = async->cmd;
/* restore chanlist pointer before copying back */
cmd.chanlist = (unsigned int __force *)user_chanlist;
cmd.data = NULL;
if (copy_to_user(arg, &cmd, sizeof(cmd))) {
dev_dbg(dev->class_dev, "fault writing cmd\n");
ret = -EFAULT;
goto cleanup;
}
ret = -EAGAIN;
goto cleanup;
}
This is a good place to make sure that async->cmd->chanlist is set,
perhaps by changing the code to something like this:
ret = s->do_cmdtest(dev, s, &async->cmd);
if (!ret && !async->cmd->chanlist) {
dev_dbg(dev->class_dev, "channel/gain list is NULL\n");
ret = -EINVAL;
}
if (async->cmd.flags & TRIG_BOGUS || ret) {
dev_dbg(dev->class_dev, "test returned %d\n", ret);
cmd = async->cmd;
/* restore chanlist pointer before copying back */
cmd.chanlist = (unsigned int __force *)user_chanlist;
cmd.data = NULL;
if (copy_to_user(arg, &cmd, sizeof(cmd))) {
dev_dbg(dev->class_dev, "fault writing cmd\n");
ret = -EFAULT;
}
if (ret >= 0)
ret = -EAGAIN;
goto cleanup;
}
(You could test user_chanlist instead of async->cmd->chanlist if you
like. You can also omit the dev_dbg() if you think it's superfluous.)
I was toying with the idea of setting ret = 5 instead of ret = -EINVAL
but think -EINVAL is better since the command is not fixable by trying
again. However, the code then needed changing to return something other
than -EAGAIN in that case, hence the `if (ret >= 0) ret = -EAGAIN` bit.
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
index 66c24e4..c7323b5 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c
@@ -777,8 +777,6 @@ static int apci3120_ai_cmdtest(struct comedi_device *dev,
}
}
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
-
Yes, you can still get rid of these minimum value checks in the
do_cmdtest handlers.
if (cmd->stop_src == TRIG_COUNT)
err |= cfc_check_trigger_arg_min(&cmd->stop_arg, 1);
else /* TRIG_NONE */
diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c b/drivers/staging/comedi/drivers/adl_pci9118.c
index 53bbc59..465b304 100644
--- a/drivers/staging/comedi/drivers/adl_pci9118.c
+++ b/drivers/staging/comedi/drivers/adl_pci9118.c
@@ -1258,10 +1258,7 @@ static int pci9118_ai_cmdtest(struct comedi_device *dev,
else /* TRIG_NONE */
err |= cfc_check_trigger_arg_is(&cmd->stop_arg, 0);
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
-
- err |= cfc_check_trigger_arg_min(&cmd->scan_end_arg,
- cmd->chanlist_len);
+ err |= cfc_check_trigger_arg_min(&cmd->scan_end_arg, cmd->chanlist_len);
if ((cmd->scan_end_arg % cmd->chanlist_len)) {
cmd->scan_end_arg =
Since we allow cmd->chanlist_len to be 0, the above test needs changing to:
if (cmd->chanlist_len && (cmd->scan_end_arg % cmd->chanlist_len)) {
to avoid dividing by 0.
Also for consistency, near the end of pci9118_ai_cmdtest(), the test
if (cmd->chanlist)
should be changed to:
if (cmd->chanlist && cmd->chanlist_len)
(But if cmd->chanlist is forced to NULL by __comedi_get_user_chanlist()
when cmd->chanlist_len is 0, as mentioned in my remarks on
__comedi_get_user_chanlist() above, the simpler test is fine.)
diff --git a/drivers/staging/comedi/drivers/amplc_pci224.c b/drivers/staging/comedi/drivers/amplc_pci224.c
index 29e01e2..9f0c20f 100644
--- a/drivers/staging/comedi/drivers/amplc_pci224.c
+++ b/drivers/staging/comedi/drivers/amplc_pci224.c
@@ -933,11 +933,6 @@ static int pci224_ao_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
unsigned int rank;
unsigned long flags;
- /* Cannot handle null/empty chanlist. */
- if (cmd->chanlist == NULL || cmd->chanlist_len == 0)
- return -EINVAL;
-
-
/* Determine which channels are enabled and their load order. */
devpriv->ao_enab = 0;
diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index f9afcbe..032bafe 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -2064,7 +2064,6 @@ static int ai_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
}
}
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
switch (cmd->stop_src) {
Near the bottom of ai_cmdtest() ("step 5") there's a test:
if (cmd->chanlist) {
that would need changing to:
if (cmd->chanlist && cmd->chanlist_len) {
unless the "force chanlist to NULL for zero chanlist_len" change I
mentioned earlier for __comedi_get_user_chanlist() is done. Otherwise,
the access to cmd->chanlist[0] within this `if` block would fail.
@@ -3296,7 +3295,6 @@ static int ao_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
}
}
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (err)
And the same for ao_cmdtest() step 5 as for ai_cmdtest() step 5.
diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
index cd95625..0f7c844 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -262,7 +262,6 @@ static int waveform_ai_cmdtest(struct comedi_device *dev,
cmd->convert_arg * cmd->chanlist_len);
}
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->stop_src == TRIG_COUNT)
diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
index 8e975d6..c67f67a 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -806,7 +806,6 @@ static int das1800_ai_do_cmdtest(struct comedi_device *dev,
err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
thisboard->ai_speed);
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
switch (cmd->stop_src) {
As for the cb_pcidas64 driver mentioned above, the test near the end of
das1800_ai_do_cmdtest() (step 5) needs changing to test both chanlist
and chanlist_len, unless chanlist gets forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0. Otherwise the
access to cmd->chanlist[0] in the `if` block would fail.
diff --git a/drivers/staging/comedi/drivers/das800.c b/drivers/staging/comedi/drivers/das800.c
index 3e40837..ef5f65f 100644
--- a/drivers/staging/comedi/drivers/das800.c
+++ b/drivers/staging/comedi/drivers/das800.c
@@ -338,7 +338,6 @@ static int das800_ai_do_cmdtest(struct comedi_device *dev,
err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
thisboard->ai_speed);
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->stop_src == TRIG_COUNT)
As for previously mentioned drivers, the test near the end of
das800_ai_do_cmdtest() (step 5) needs changing to test both chanlist and
chanlist_len, unless chanlist gets forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0. Otherwise the
access to cmd->chanlist[0] in the `if` block would fail.
diff --git a/drivers/staging/comedi/drivers/me4000.c b/drivers/staging/comedi/drivers/me4000.c
index bd03a15..2657e5c 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -605,18 +605,6 @@ static int ai_check_chanlist(struct comedi_device *dev,
int aref;
int i;
- /* Check whether a channel list is available */
- if (!cmd->chanlist_len) {
- dev_err(dev->class_dev, "No channel list available\n");
- return -EINVAL;
- }
-
- /* Check the pointer */
- if (!cmd->chanlist) {
- dev_err(dev->class_dev, "NULL pointer to channel list\n");
- return -EFAULT;
- }
-
/* Check whether aref is equal for all entries */
aref = CR_AREF(cmd->chanlist[0]);
for (i = 0; i < cmd->chanlist_len; i++) {
@@ -944,10 +932,6 @@ static int me4000_ai_do_cmd_test(struct comedi_device *dev,
/* Step 3: check if arguments are trivially valid */
- if (cmd->chanlist_len < 1) {
- cmd->chanlist_len = 1;
- err |= -EINVAL;
- }
if (init_ticks < 66) {
cmd->start_arg = 2000;
err |= -EINVAL;
The end of me4000_ai_do_cmd_test() (step 5) also needs changing to only
call ai_check_chanlist() if cmd->chanlist is non-NULL and
cmd->chanlist_len is non-0, something like:
if (cmd->chanlist && cmd->chanlist_len &&
ai_check_chanlist(dev, s, cmd))
return 5;
The test can be simplified if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, as mentioned earlier.
diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
index 4e39b1f..dc1c47c 100644
--- a/drivers/staging/comedi/drivers/ni_at_a2150.c
+++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
@@ -325,7 +325,6 @@ static int a2150_ai_cmdtest(struct comedi_device *dev,
err |= cfc_check_trigger_arg_min(&cmd->convert_arg,
thisboard->ai_speed);
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->stop_src == TRIG_COUNT)
The test at the end of a2150_ai_cmdtest() (step 5) needs changing to
test both cmd->chanlist and cmd->chanlist_len, unless chanlist gets
forced to NULL by __comedi_get_user_chanlist() when chanlist_len is 0.
diff --git a/drivers/staging/comedi/drivers/ni_labpc.c b/drivers/staging/comedi/drivers/ni_labpc.c
index c33b9fc..b0754a2 100644
--- a/drivers/staging/comedi/drivers/ni_labpc.c
+++ b/drivers/staging/comedi/drivers/ni_labpc.c
@@ -529,10 +529,6 @@ static enum scan_mode labpc_ai_scan_mode(const struct comedi_cmd *cmd)
if (cmd->chanlist_len == 1)
return MODE_SINGLE_CHAN;
- /* chanlist may be NULL during cmdtest. */
- if (cmd->chanlist == NULL)
- return MODE_MULT_CHAN_UP;
-
We'd need to keep that test in labpc_ai_scan_mode().
if (CR_CHAN(cmd->chanlist[0]) == CR_CHAN(cmd->chanlist[1]))
return MODE_SINGLE_CHAN_INTERVAL;
@@ -552,9 +548,6 @@ static int labpc_ai_chanlist_invalid(const struct comedi_device *dev,
{
int channel, range, aref, i;
- if (cmd->chanlist == NULL)
- return 0;
-
if (mode == MODE_SINGLE_CHAN)
return 0;
@@ -656,8 +649,6 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
if (cmd->start_arg == TRIG_NOW)
err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
- if (!cmd->chanlist_len)
- err |= -EINVAL;
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->convert_src == TRIG_TIMER)
The end of labpc_ai_cmd_test() (step 5) also needs changing to only call
labpc_ai_chanlist_invalid() if cmd->chanlist is non-NULL and
cmd->chanlist_len is non-0, something like:
if (cmd->chanlist && cmd->chanlist_len &&
labpc_ai_chanlist_invalid(dev, cmd, mode))
return 5;
The test can be simplified if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, as mentioned earlier.
diff --git a/drivers/staging/comedi/drivers/pcl812.c b/drivers/staging/comedi/drivers/pcl812.c
index 0cf115e..1d4ec6e 100644
--- a/drivers/staging/comedi/drivers/pcl812.c
+++ b/drivers/staging/comedi/drivers/pcl812.c
@@ -760,7 +760,6 @@ static int pcl812_ai_cmdtest(struct comedi_device *dev,
else /* TRIG_EXT */
err |= cfc_check_trigger_arg_is(&cmd->convert_arg, 0);
- err |= cfc_check_trigger_arg_min(&cmd->chanlist_len, 1);
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->stop_src == TRIG_COUNT)
diff --git a/drivers/staging/comedi/drivers/pcl816.c b/drivers/staging/comedi/drivers/pcl816.c
index 6f276f2..d4ead5e 100644
--- a/drivers/staging/comedi/drivers/pcl816.c
+++ b/drivers/staging/comedi/drivers/pcl816.c
@@ -431,14 +431,10 @@ static int pcl816_ai_cmdtest(struct comedi_device *dev,
if (err)
return 4;
+ /* Step 5: check channel list */
- /* step 5: complain about special chanlist considerations */
-
- if (cmd->chanlist) {
- if (!check_channel_list(dev, s, cmd->chanlist,
- cmd->chanlist_len))
- return 5; /* incorrect channels list */
- }
+ if (!check_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len))
+ return 5;
We'd only want to call check_channel_list() if cmd->chanlist is non-NULL
and cmd->chanlist_len is non-0 as for the other drivers. But if
cmd->chanlist is forced to NULL by __comedi_get_user_chanlist() when
chanlist_len is 0, it is sufficient to test only cmd->chanlist.
return 0;
}
diff --git a/drivers/staging/comedi/drivers/pcl818.c b/drivers/staging/comedi/drivers/pcl818.c
index 6463476..36b8d22 100644
--- a/drivers/staging/comedi/drivers/pcl818.c
+++ b/drivers/staging/comedi/drivers/pcl818.c
@@ -802,13 +802,10 @@ static int ai_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
if (err)
return 4;
- /* step 5: complain about special chanlist considerations */
+ /* Step 5: check channel list */
- if (cmd->chanlist) {
- if (!check_channel_list(dev, s, cmd->chanlist,
- cmd->chanlist_len))
- return 5; /* incorrect channels list */
- }
+ if (!check_channel_list(dev, s, cmd->chanlist, cmd->chanlist_len))
+ return 5;
return 0;
}
Ditto as for pcl816.c, need to check both cmd->chanlist and
cmd->chanlist_len. But if cmd->chanlist is forced to NULL by
__comedi_get_user_chanlist() when chanlist_len is 0, it is sufficient to
test only cmd->chanlist.
diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
index d6fae11..9756110 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -401,9 +401,6 @@ static int usbduxfast_ai_cmdtest(struct comedi_device *dev,
if (cmd->start_src == TRIG_NOW)
err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
- if (!cmd->chanlist_len)
- err |= -EINVAL;
-
err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
if (cmd->chanlist_len == 1)
--
-=( 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
