Re: [PATCH] staging/comedi: Fix undefined array subscript

From: Ian Abbott
Date: Wed Feb 13 2013 - 06:56:22 EST


On 2013-02-13 07:32, Dan Carpenter wrote:
On Wed, Feb 13, 2013 at 04:30:54AM +0100, Peter Huewe wrote:
In vmk80xx_do_insn_bits the local variable reg, which is used as an
index to the tx_buf array, can be used uninitialized if
- data[0] == 0
and
- devpriv->model != VMK8061_MODEL
-> we get into the else branch without having reg initialized.

It's weird that GCC doesn't warn about this...

This patch works, or at least it doesn't break anything that wasn't
already broken, but it doesn't feel like the right thing. Probably
we could move the reg setting outside the if statement.

if (devpriv->model == VMK8055_MODEL) {
reg = VMK8055_DO_REG;
cmd = VMK8055_CMD_WRT_AD;
} else { /* VMK8061_MODEL */
reg = VMK8061_DO_REG;
cmd = VMK8061_CMD_DO;
}

if (data[0]) {
tx_buf[reg] &= ~data[0];

Either method would be fine.

Or maybe data[0] == 0 needs to be handled differently.

Ian would know for sure.

The insn_bits instruction always reads back the channels after any modification of the channels specified by bitmask data[0] with the new channel values bitmask data[1]. data[0] == 0 implies you are not changing any of the channels so only read back the current values.

For a digital output subdevice, you could either read back the current values directly from the hardware or just use the value previously written. The Velleman K8055 doesn't have a command to read back the digital outputs from the hardware, so the last written value has to be used. But what if the digital outputs have never been written (or the analog outputs have never been written, since the same command updates all analog and digital channels)? A "reset" command is sent to the hardware on initialization by vmk80xx_reset_device() (only called for the K8055), but I don't know what effect this has on the actual digital (and analog) outputs (though I could find out easily enough as we appear to have one of these kits (assembled) lying around in the office). If necessary, we may have to also send a "write" command on initialization to make the hardware outputs match the initial software state.

For the Velleman K8061, reading the digital outputs from the hardware every time is a bit inefficient as it should only be necessary in the case where the digital outputs have never been written (similarly for the analog outputs). We could read back the initial state of the digital and analog outputs during hardware initialization and keep the state up-to-date in software without having to query the hardware again.

--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/