[PATCH 0/3] staging: comedi: ni_mio_common: ni_ai_insn_read() fixes

[PATCH 0/3] staging: comedi: ni_mio_common: ni_ai_insn_read() fixes

[Ian Abbott]

A couple of bug fixes for incorrect sample data read by
ni_ai_insn_read(), plus removal of an unnecessary variable.

1) staging: comedi: ni_mio_common: fix M Series ni_ai_insn_read() data
   mask
2) staging: comedi: ni_mio_common: fix E series ni_ai_insn_read() data
3) staging: comedi: ni_mio_common: remove variable 'dl' in
   ni_ai_insn_read()

The bug fix patches 1) and 2) apply cleanly to kernel version 4.7
onwards, but will need backporting to longterm kernels 3.18 onwards.

 drivers/staging/comedi/drivers/ni_mio_common.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

[PATCH 1/3] staging: comedi: ni_mio_common: fix M Series ni_ai_insn_read() data mask

[Ian Abbott]

For NI M Series cards, the Comedi `insn_read` handler for the AI
subdevice is broken due to ANDing the value read from the AI FIFO data
register with an incorrect mask.  The incorrect mask clears all but the
most significant bit of the sample data.  It should preserve all the
sample data bits.  Correct it.

Fixes: 817144ae7fda ("staging: comedi: ni_mio_common: remove unnecessary use of 'board->adbits'")
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: <stable@vger.kernel.org> # 4.7+
---
Needs backporting to stable kernels >= 3.17
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 9812508..9543539 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -1832,7 +1832,7 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 			   unsigned int *data)
 {
 	struct ni_private *devpriv = dev->private;
-	unsigned int mask = (s->maxdata + 1) >> 1;
+	unsigned int mask = s->maxdata;
 	int i, n;
 	unsigned int signbits;
 	unsigned int d;
-- 
2.10.2

[PATCH 2/3] staging: comedi: ni_mio_common: fix E series ni_ai_insn_read() data

[Ian Abbott]

Commit 0557344e2149 ("staging: comedi: ni_mio_common: fix local var for
32-bit read") changed the type of local variable `d` from `unsigned
short` to `unsigned int` to fix a bug introduced in
commit 9c340ac934db ("staging: comedi: ni_stc.h: add read/write
callbacks to struct ni_private") when reading AI data for NI PCI-6110
and PCI-6111 cards.  Unfortunately, other parts of the function rely on
the variable being `unsigned short` when an offset value in local
variable `signbits` is added to `d` before writing the value to the
`data` array:

			d += signbits;
		  	data[n] = d;

The `signbits` variable will be non-zero in bipolar mode, and is used to
convert the hardware's 2's complement, 16-bit numbers to Comedi's
straight binary sample format (with 0 representing the most negative
voltage).  This breaks because `d` is now 32 bits wide instead of 16
bits wide, so after the addition of `signbits`, `data[n]` ends up being
set to values above 65536 for negative voltages.  This affects all
supported "E series" cards except PCI-6143 (and PXI-6143). Fix it by
ANDing the value written to the `data[n]` with the mask 0xffff.

Fixes: 0557344e2149 ("staging: comedi: ni_mio_common: fix local var for 32-bit read")
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: <stable@vger.kernel.org> # 4.7+
----
Needs backporting to stable kernels 3.18 onwards.
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 9543539..a974ab7 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -1875,7 +1875,7 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 				return -ETIME;
 			}
 			d += signbits;
-			data[n] = d;
+			data[n] = d & 0xffff;
 		}
 	} else if (devpriv->is_6143) {
 		for (n = 0; n < insn->n; n++) {
@@ -1924,9 +1924,8 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 				data[n] = dl;
 			} else {
 				d = ni_readw(dev, NI_E_AI_FIFO_DATA_REG);
-				/* subtle: needs to be short addition */
 				d += signbits;
-				data[n] = d;
+				data[n] = d & 0xffff;
 			}
 		}
 	}
-- 
2.10.2

[PATCH 3/3] staging: comedi: ni_mio_common: remove variable 'dl' in ni_ai_insn_read()

[Ian Abbott]

In `ni_ai_insn_read()`, local variable `dl` is declared as `unsigned
long`, but `unsigned int` will do.  Get rid of it and use local variable
`d` instead.  (That used to be `unsigned short`, but has been `unsigned
int` since kernel version 3.18.)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index a974ab7..b2e3828 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -1836,7 +1836,6 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 	int i, n;
 	unsigned int signbits;
 	unsigned int d;
-	unsigned long dl;
 
 	ni_load_channelgain_list(dev, s, 1, &insn->chanspec);
 
@@ -1887,15 +1886,15 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 			 * bit to move a single 16bit stranded sample into
 			 * the FIFO.
 			 */
-			dl = 0;
+			d = 0;
 			for (i = 0; i < NI_TIMEOUT; i++) {
 				if (ni_readl(dev, NI6143_AI_FIFO_STATUS_REG) &
 				    0x01) {
 					/* Get stranded sample into FIFO */
 					ni_writel(dev, 0x01,
 						  NI6143_AI_FIFO_CTRL_REG);
-					dl = ni_readl(dev,
-						      NI6143_AI_FIFO_DATA_REG);
+					d = ni_readl(dev,
+						     NI6143_AI_FIFO_DATA_REG);
 					break;
 				}
 			}
@@ -1903,7 +1902,7 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 				dev_err(dev->class_dev, "timeout\n");
 				return -ETIME;
 			}
-			data[n] = (((dl >> 16) & 0xFFFF) + signbits) & 0xFFFF;
+			data[n] = (((d >> 16) & 0xFFFF) + signbits) & 0xFFFF;
 		}
 	} else {
 		for (n = 0; n < insn->n; n++) {
@@ -1919,9 +1918,9 @@ static int ni_ai_insn_read(struct comedi_device *dev,
 				return -ETIME;
 			}
 			if (devpriv->is_m_series) {
-				dl = ni_readl(dev, NI_M_AI_FIFO_DATA_REG);
-				dl &= mask;
-				data[n] = dl;
+				d = ni_readl(dev, NI_M_AI_FIFO_DATA_REG);
+				d &= mask;
+				data[n] = d;
 			} else {
 				d = ni_readw(dev, NI_E_AI_FIFO_DATA_REG);
 				d += signbits;
-- 
2.10.2