[PATCH 00/15] staging: comedi: comedi_bond: some bug fixes and tidy up

[PATCH 00/15] staging: comedi: comedi_bond: some bug fixes and tidy up

[Ian Abbott]

The "comedi_bond" driver can be used to map the DIO subdevices of some
other comedi devices into one, giant, uber DIO subdevice.  However, it
has a few bugs such as memory leaks (but only if a memory allocation
fails), buffer overflow when more than 256 channels are added, not
handling `COMEDI_INSN_BITS` properly when there are more than 32
channels, and various other problems.  Also it needs tidying up a bit
due to the use of CamelCase, etc.

Some of the changes involve changes to the "kcomedilib" module as well,
but that is only used by the "comedi_bond" module (for in-tree modules,
anyway, which is all we care about at the moment).

01) staging: comedi: comedi_bond: reformat some comments
02) staging: comedi: comedi_bond: shorten module description
03) staging: comedi: comedi_bond: rename CamelCase identifiers
04) staging: comedi: comedi_bond: no need to initialize file[]
05) staging: comedi: comedi_bond: return error code in do_dev_config()
06) staging: comedi: comedi_bond: change return value of
    bonding_attach()
07) staging: comedi: comedi_bond: use bitmap to record opened/closed
    minors
08) staging: comedi: comedi_bond: don't map channels individually
09) staging: comedi: comedi_bond: remove unused subdev_type
10) staging: comedi: comedi_bond: no need to free dev->private on detach
11) staging: comedi: comedi_bond: just check devprivs->devs once on
    detach
12) staging: comedi: comedi_bond: get INSN_CONFIG_DIO_QUERY info from
    horse's mouth
13) staging: comedi: comedi_bond: handle base channel for insn_bits
14) staging: comedi: comedi_bond: use krealloc() and fix memory leak
15) staging: comedi: comedi_bond: use correct minor device numbers in
    name

 drivers/staging/comedi/comedilib.h                 |   7 +-
 drivers/staging/comedi/drivers/comedi_bond.c       | 361 ++++++++++-----------
 .../staging/comedi/kcomedilib/kcomedilib_main.c    |  58 +++-
 3 files changed, 230 insertions(+), 196 deletions(-)

[PATCH 01/15] staging: comedi: comedi_bond: reformat some comments

[Ian Abbott]

Reformat some comments according to CodingStyle and remove some comments
inherited from the comedi 'skel' example driver.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 141 ++++++++++++++-------------
 1 file changed, 73 insertions(+), 68 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 7e20bf0..32a26f2 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -1,49 +1,50 @@
 /*
-    comedi/drivers/comedi_bond.c
-    A Comedi driver to 'bond' or merge multiple drivers and devices as one.
-
-    COMEDI - Linux Control and Measurement Device Interface
-    Copyright (C) 2000 David A. Schleef <ds@schleef.org>
-    Copyright (C) 2005 Calin A. Culianu <calin@ajvar.org>
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-*/
+ * comedi_bond.c
+ * A Comedi driver to 'bond' or merge multiple drivers and devices as one.
+ *
+ * COMEDI - Linux Control and Measurement Device Interface
+ * Copyright (C) 2000 David A. Schleef <ds@schleef.org>
+ * Copyright (C) 2005 Calin A. Culianu <calin@ajvar.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
 /*
-Driver: comedi_bond
-Description: A driver to 'bond' (merge) multiple subdevices from multiple
-	     devices together as one.
-Devices:
-Author: ds
-Updated: Mon, 10 Oct 00:18:25 -0500
-Status: works
-
-This driver allows you to 'bond' (merge) multiple comedi subdevices
-(coming from possibly difference boards and/or drivers) together.  For
-example, if you had a board with 2 different DIO subdevices, and
-another with 1 DIO subdevice, you could 'bond' them with this driver
-so that they look like one big fat DIO subdevice.  This makes writing
-applications slightly easier as you don't have to worry about managing
-different subdevices in the application -- you just worry about
-indexing one linear array of channel id's.
-
-Right now only DIO subdevices are supported as that's the personal itch
-I am scratching with this driver.  If you want to add support for AI and AO
-subdevs, go right on ahead and do so!
-
-Commands aren't supported -- although it would be cool if they were.
-
-Configuration Options:
-  List of comedi-minors to bond.  All subdevices of the same type
-  within each minor will be concatenated together in the order given here.
-*/
+ * Driver: comedi_bond
+ * Description: A driver to 'bond' (merge) multiple subdevices from multiple
+ * devices together as one.
+ * Devices:
+ * Author: ds
+ * Updated: Mon, 10 Oct 00:18:25 -0500
+ * Status: works
+ *
+ * This driver allows you to 'bond' (merge) multiple comedi subdevices
+ * (coming from possibly difference boards and/or drivers) together.  For
+ * example, if you had a board with 2 different DIO subdevices, and
+ * another with 1 DIO subdevice, you could 'bond' them with this driver
+ * so that they look like one big fat DIO subdevice.  This makes writing
+ * applications slightly easier as you don't have to worry about managing
+ * different subdevices in the application -- you just worry about
+ * indexing one linear array of channel id's.
+ *
+ * Right now only DIO subdevices are supported as that's the personal itch
+ * I am scratching with this driver.  If you want to add support for AI and AO
+ * subdevs, go right on ahead and do so!
+ *
+ * Commands aren't supported -- although it would be cool if they were.
+ *
+ * Configuration Options:
+ *   List of comedi-minors to bond.  All subdevices of the same type
+ *   within each minor will be concatenated together in the order given here.
+ */
 
 #include <linux/module.h>
 #include <linux/string.h>
@@ -62,13 +63,10 @@ struct BondedDevice {
 	unsigned subdev_type;
 	unsigned nchans;
 	unsigned chanid_offset;	/* The offset into our unified linear
-				   channel-id's of chanid 0 on this
-				   subdevice. */
+				 * channel-id's of chanid 0 on this
+				 * subdevice. */
 };
 
-/* this structure is for data unique to this hardware driver.  If
-   several hardware drivers keep similar information in this structure,
-   feel free to suggest moving the variable to the struct comedi_device struct.  */
 struct comedi_bond_private {
 # define MAX_BOARD_NAME 256
 	char name[MAX_BOARD_NAME];
@@ -78,11 +76,6 @@ struct comedi_bond_private {
 	unsigned nchans;
 };
 
-/* DIO devices are slightly special.  Although it is possible to
- * implement the insn_read/insn_write interface, it is much more
- * useful to applications if you implement the insn_bits interface.
- * This allows packed reading/writing of the DIO channels.  The
- * comedi core can convert between insn_bits and insn_read/write */
 static int bonding_dio_insn_bits(struct comedi_device *dev,
 				 struct comedi_subdevice *s,
 				 struct comedi_insn *insn, unsigned int *data)
@@ -94,13 +87,17 @@ static int bonding_dio_insn_bits(struct comedi_device *dev,
 	if (devpriv->nchans < nchans)
 		nchans = devpriv->nchans;
 
-	/* The insn data is a mask in data[0] and the new data
-	 * in data[1], each channel cooresponding to a bit. */
+	/*
+	 * The insn data is a mask in data[0] and the new data
+	 * in data[1], each channel cooresponding to a bit.
+	 */
 	for (i = 0; num_done < nchans && i < devpriv->ndevs; ++i) {
 		struct BondedDevice *bdev = devpriv->devs[i];
-		/* Grab the channel mask and data of only the bits corresponding
-		   to this subdevice.. need to shift them to zero position of
-		   course. */
+		/*
+		 * Grab the channel mask and data of only the bits corresponding
+		 * to this subdevice.. need to shift them to zero position of
+		 * course.
+		 */
 		/* Bits corresponding to this subdev. */
 		unsigned int subdevMask = ((1 << bdev->nchans) - 1);
 		unsigned int writeMask, dataBits;
@@ -143,10 +140,12 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 		return -EINVAL;
 	bdev = devpriv->chanIdDevMap[chan];
 
-	/* The input or output configuration of each digital line is
+	/*
+	 * The input or output configuration of each digital line is
 	 * configured by a special insn_config instruction.  chanspec
 	 * contains the channel to be changed, and data[0] contains the
-	 * value COMEDI_INPUT or COMEDI_OUTPUT. */
+	 * value COMEDI_INPUT or COMEDI_OUTPUT.
+	 */
 	switch (data[0]) {
 	case INSN_CONFIG_DIO_OUTPUT:
 		io = COMEDI_OUTPUT;	/* is this really necessary? */
@@ -170,8 +169,10 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 	ret = comedi_dio_config(bdev->dev, bdev->subdev, chan, io);
 	if (ret != 1)
 		return -EINVAL;
-	/* Finally, save the new io_bits values since we didn't get
-	   an error above. */
+	/*
+	 * Finally, save the new io_bits values since we didn't get an error
+	 * above.
+	 */
 	s->io_bits = io_bits;
 	return insn->n;
 }
@@ -194,8 +195,10 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	memset(devs_opened, 0, sizeof(devs_opened));
 	devpriv->name[0] = 0;
-	/* Loop through all comedi devices specified on the command-line,
-	   building our device list */
+	/*
+	 * Loop through all comedi devices specified on the command-line,
+	 * building our device list.
+	 */
 	for (i = 0; i < COMEDI_NDEVCONFOPTS && (!i || it->options[i]); ++i) {
 		char file[] = "/dev/comediXXXXXX";
 		int minor = it->options[i];
@@ -255,8 +258,10 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 			while (nchans--)
 				devpriv->chanIdDevMap[devpriv->nchans++] = bdev;
 
-			/* Now put bdev pointer at end of devpriv->devs array
-			 * list.. */
+			/*
+			 * Now put bdev pointer at end of devpriv->devs array
+			 * list..
+			 */
 
 			/* ergh.. ugly.. we need to realloc :(  */
 			tmp = devpriv->ndevs * sizeof(bdev);
@@ -271,7 +276,7 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 
 			devpriv->devs[devpriv->ndevs - 1] = bdev;
 			{
-	/** Append dev:subdev to devpriv->name */
+				/* Append dev:subdev to devpriv->name */
 				char buf[20];
 				int left =
 				    MAX_BOARD_NAME - strlen(devpriv->name) - 1;
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/15] staging: comedi: comedi_bond: shorten module description

[Ian Abbott]

Omit the fanciful prose from the MODULE_DESCRIPTION() line and combine
concantenated string literals.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 32a26f2..a995b0b 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -371,7 +371,5 @@ static struct comedi_driver bonding_driver = {
 module_comedi_driver(bonding_driver);
 
 MODULE_AUTHOR("Calin A. Culianu");
-MODULE_DESCRIPTION("comedi_bond: A driver for COMEDI to bond multiple COMEDI "
-		   "devices together as one.  In the words of John Lennon: "
-		   "'And the world will live as one...'");
+MODULE_DESCRIPTION("comedi_bond: A driver for COMEDI to bond multiple COMEDI devices together as one.");
 MODULE_LICENSE("GPL");
-- 
1.8.3.2

[PATCH 03/15] staging: comedi: comedi_bond: rename CamelCase identifiers

[Ian Abbott]

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 47 ++++++++++++++--------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index a995b0b..253a67d 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -56,7 +56,7 @@
 /* The maxiumum number of channels per subdevice. */
 #define MAX_CHANS 256
 
-struct BondedDevice {
+struct bonded_device {
 	struct comedi_device *dev;
 	unsigned minor;
 	unsigned subdev;
@@ -70,9 +70,9 @@ struct BondedDevice {
 struct comedi_bond_private {
 # define MAX_BOARD_NAME 256
 	char name[MAX_BOARD_NAME];
-	struct BondedDevice **devs;
+	struct bonded_device **devs;
 	unsigned ndevs;
-	struct BondedDevice *chanIdDevMap[MAX_CHANS];
+	struct bonded_device *chan_id_dev_map[MAX_CHANS];
 	unsigned nchans;
 };
 
@@ -92,32 +92,32 @@ static int bonding_dio_insn_bits(struct comedi_device *dev,
 	 * in data[1], each channel cooresponding to a bit.
 	 */
 	for (i = 0; num_done < nchans && i < devpriv->ndevs; ++i) {
-		struct BondedDevice *bdev = devpriv->devs[i];
+		struct bonded_device *bdev = devpriv->devs[i];
 		/*
 		 * Grab the channel mask and data of only the bits corresponding
 		 * to this subdevice.. need to shift them to zero position of
 		 * course.
 		 */
 		/* Bits corresponding to this subdev. */
-		unsigned int subdevMask = ((1 << bdev->nchans) - 1);
-		unsigned int writeMask, dataBits;
+		unsigned int subdev_mask = ((1 << bdev->nchans) - 1);
+		unsigned int write_mask, data_bits;
 
 		/* Argh, we have >= LSAMPL_BITS chans.. take all bits */
 		if (bdev->nchans >= LSAMPL_BITS)
-			subdevMask = (unsigned int)(-1);
+			subdev_mask = (unsigned int)(-1);
 
-		writeMask = (data[0] >> num_done) & subdevMask;
-		dataBits = (data[1] >> num_done) & subdevMask;
+		write_mask = (data[0] >> num_done) & subdev_mask;
+		data_bits = (data[1] >> num_done) & subdev_mask;
 
 		/* Read/Write the new digital lines */
-		if (comedi_dio_bitfield(bdev->dev, bdev->subdev, writeMask,
-					&dataBits) != 2)
+		if (comedi_dio_bitfield(bdev->dev, bdev->subdev, write_mask,
+					&data_bits) != 2)
 			return -EINVAL;
 
 		/* Make room for the new bits in data[1], the return value */
-		data[1] &= ~(subdevMask << num_done);
+		data[1] &= ~(subdev_mask << num_done);
 		/* Put the bits in the return value */
-		data[1] |= (dataBits & subdevMask) << num_done;
+		data[1] |= (data_bits & subdev_mask) << num_done;
 		/* Save the new bits to the saved state.. */
 		s->state = data[1];
 
@@ -134,11 +134,11 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 	struct comedi_bond_private *devpriv = dev->private;
 	int chan = CR_CHAN(insn->chanspec), ret, io_bits = s->io_bits;
 	unsigned int io;
-	struct BondedDevice *bdev;
+	struct bonded_device *bdev;
 
 	if (chan < 0 || chan >= devpriv->nchans)
 		return -EINVAL;
-	bdev = devpriv->chanIdDevMap[chan];
+	bdev = devpriv->chan_id_dev_map[chan];
 
 	/*
 	 * The input or output configuration of each digital line is
@@ -177,7 +177,7 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 	return insn->n;
 }
 
-static void *Realloc(const void *oldmem, size_t newlen, size_t oldlen)
+static void *realloc(const void *oldmem, size_t newlen, size_t oldlen)
 {
 	void *newmem = kmalloc(newlen, GFP_KERNEL);
 
@@ -187,7 +187,7 @@ static void *Realloc(const void *oldmem, size_t newlen, size_t oldlen)
 	return newmem;
 }
 
-static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
+static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 {
 	struct comedi_bond_private *devpriv = dev->private;
 	int i;
@@ -204,7 +204,7 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 		int minor = it->options[i];
 		struct comedi_device *d;
 		int sdev = -1, nchans, tmp;
-		struct BondedDevice *bdev = NULL;
+		struct bonded_device *bdev = NULL;
 
 		if (minor < 0 || minor >= COMEDI_NUM_BOARD_MINORS) {
 			dev_err(dev->class_dev,
@@ -254,9 +254,10 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 			bdev->nchans = nchans;
 			bdev->chanid_offset = devpriv->nchans;
 
-			/* map channel id's to BondedDevice * pointer.. */
+			/* map channel id's to bonded_device * pointer.. */
 			while (nchans--)
-				devpriv->chanIdDevMap[devpriv->nchans++] = bdev;
+				devpriv->chan_id_dev_map[devpriv->nchans++] =
+				    bdev;
 
 			/*
 			 * Now put bdev pointer at end of devpriv->devs array
@@ -266,7 +267,7 @@ static int doDevConfig(struct comedi_device *dev, struct comedi_devconfig *it)
 			/* ergh.. ugly.. we need to realloc :(  */
 			tmp = devpriv->ndevs * sizeof(bdev);
 			devpriv->devs =
-			    Realloc(devpriv->devs,
+			    realloc(devpriv->devs,
 				    ++devpriv->ndevs * sizeof(bdev), tmp);
 			if (!devpriv->devs) {
 				dev_err(dev->class_dev,
@@ -311,7 +312,7 @@ static int bonding_attach(struct comedi_device *dev,
 	/*
 	 * Setup our bonding from config params.. sets up our private struct..
 	 */
-	if (!doDevConfig(dev, it))
+	if (!do_dev_config(dev, it))
 		return -EINVAL;
 
 	dev->board_name = devpriv->name;
@@ -344,7 +345,7 @@ static void bonding_detach(struct comedi_device *dev)
 
 	if (devpriv) {
 		while (devpriv->ndevs-- && devpriv->devs) {
-			struct BondedDevice *bdev;
+			struct bonded_device *bdev;
 
 			bdev = devpriv->devs[devpriv->ndevs];
 			if (!bdev)
-- 
1.8.3.2

[PATCH 04/15] staging: comedi: comedi_bond: no need to initialize file[]

[Ian Abbott]

The `char file[]` variable in `do_dev_config()` doesn't need to be
initialized as it gets overwritten with a `snprintf()`.  It just needs
to be long enough.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 253a67d..00c065a 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -200,7 +200,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 	 * building our device list.
 	 */
 	for (i = 0; i < COMEDI_NDEVCONFOPTS && (!i || it->options[i]); ++i) {
-		char file[] = "/dev/comediXXXXXX";
+		char file[sizeof("/dev/comediXXXXXX")];
 		int minor = it->options[i];
 		struct comedi_device *d;
 		int sdev = -1, nchans, tmp;
-- 
1.8.3.2

[PATCH 05/15] staging: comedi: comedi_bond: return error code in do_dev_config()

[Ian Abbott]

Change `do_dev_config()` to return an error code on failure and 0 on
success, instead of 0 on failure and 1 on success.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 00c065a..4a1135b 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -209,17 +209,17 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 		if (minor < 0 || minor >= COMEDI_NUM_BOARD_MINORS) {
 			dev_err(dev->class_dev,
 				"Minor %d is invalid!\n", minor);
-			return 0;
+			return -EINVAL;
 		}
 		if (minor == dev->minor) {
 			dev_err(dev->class_dev,
 				"Cannot bond this driver to itself!\n");
-			return 0;
+			return -EINVAL;
 		}
 		if (devs_opened[minor]) {
 			dev_err(dev->class_dev,
 				"Minor %d specified more than once!\n", minor);
-			return 0;
+			return -EINVAL;
 		}
 
 		snprintf(file, sizeof(file), "/dev/comedi%u", minor);
@@ -230,7 +230,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 		if (!d) {
 			dev_err(dev->class_dev,
 				"Minor %u could not be opened\n", minor);
-			return 0;
+			return -ENODEV;
 		}
 
 		/* Do DIO, as that's all we support now.. */
@@ -241,11 +241,11 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 				dev_err(dev->class_dev,
 					"comedi_get_n_channels() returned %d on minor %u subdev %d!\n",
 					nchans, minor, sdev);
-				return 0;
+				return -EINVAL;
 			}
 			bdev = kmalloc(sizeof(*bdev), GFP_KERNEL);
 			if (!bdev)
-				return 0;
+				return -ENOMEM;
 
 			bdev->dev = d;
 			bdev->minor = minor;
@@ -272,7 +272,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 			if (!devpriv->devs) {
 				dev_err(dev->class_dev,
 					"Could not allocate memory. Out of memory?\n");
-				return 0;
+				return -ENOMEM;
 			}
 
 			devpriv->devs[devpriv->ndevs - 1] = bdev;
@@ -292,10 +292,10 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	if (!devpriv->nchans) {
 		dev_err(dev->class_dev, "No channels found!\n");
-		return 0;
+		return -EINVAL;
 	}
 
-	return 1;
+	return 0;
 }
 
 static int bonding_attach(struct comedi_device *dev,
@@ -312,8 +312,9 @@ static int bonding_attach(struct comedi_device *dev,
 	/*
 	 * Setup our bonding from config params.. sets up our private struct..
 	 */
-	if (!do_dev_config(dev, it))
-		return -EINVAL;
+	ret = do_dev_config(dev, it);
+	if (ret)
+		return ret;
 
 	dev->board_name = devpriv->name;
 
-- 
1.8.3.2

[PATCH 06/15] staging: comedi: comedi_bond: change return value of bonding_attach()

[Ian Abbott]

`bonding_attach()` is the comedi "attach" handler for the driver.  Any
non-negative return value is treated as successful, but 0 is the
preferred return value on success.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 4a1135b..dd93e4f 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -336,7 +336,7 @@ static int bonding_attach(struct comedi_device *dev,
 		dev->driver->driver_name, dev->board_name,
 		devpriv->nchans, devpriv->ndevs);
 
-	return 1;
+	return 0;
 }
 
 static void bonding_detach(struct comedi_device *dev)
-- 
1.8.3.2

[PATCH 07/15] staging: comedi: comedi_bond: use bitmap to record opened/closed minors

[Ian Abbott]

`do_dev_config()` currently records the comedi minor devices it has
opened by setting `devs_opened[minor]` to the pointer returned by
`comedi_open()`.  This is checked to avoid opening the same minor device
twice.  The pointer values in `devs_opened[]` aren't needed; we only
need to record which minor device numbers are being used.  Change
`devs_opened` to a bitmap (declared with `DECLARE_BITMAP()`) of length
`COMEDI_NUM_BOARD_MINORS` as the minor device numbers are range-checked
to fit in a bitmap of this length.  Use `test_and_set_bit()` to record
the minor device numbers we attempt to open with `comedi_open()`.

`bonding_detach()` calls `comedi_close()` to close the comedi minor
devices.  Since the minor device numbers may be repeated in its list of
bonded subdevices, it currently uses a simple `unsigned long
devs_closed` variable as a bitmap to keep track of which minor device
numbers it has already closed to avoid closing them twice.  As a single
`unsigned long` consists of less than `COMEDI_NUM_BOARD_MINORS` bits on
a 32-bit machine, change `devs_closed to a bitmap of this length using
`DECLARE_BITMAP()` and use `test_and_set_bit()` to avoid calling
`comedi_close()` more than once for each minor device number in use.

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

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index dd93e4f..c0a427c 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -190,10 +190,10 @@ static void *realloc(const void *oldmem, size_t newlen, size_t oldlen)
 static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 {
 	struct comedi_bond_private *devpriv = dev->private;
+	DECLARE_BITMAP(devs_opened, COMEDI_NUM_BOARD_MINORS);
 	int i;
-	struct comedi_device *devs_opened[COMEDI_NUM_BOARD_MINORS];
 
-	memset(devs_opened, 0, sizeof(devs_opened));
+	memset(&devs_opened, 0, sizeof(devs_opened));
 	devpriv->name[0] = 0;
 	/*
 	 * Loop through all comedi devices specified on the command-line,
@@ -216,7 +216,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 				"Cannot bond this driver to itself!\n");
 			return -EINVAL;
 		}
-		if (devs_opened[minor]) {
+		if (test_and_set_bit(minor, devs_opened)) {
 			dev_err(dev->class_dev,
 				"Minor %d specified more than once!\n", minor);
 			return -EINVAL;
@@ -225,7 +225,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 		snprintf(file, sizeof(file), "/dev/comedi%u", minor);
 		file[sizeof(file) - 1] = 0;
 
-		d = devs_opened[minor] = comedi_open(file);
+		d = comedi_open(file);
 
 		if (!d) {
 			dev_err(dev->class_dev,
@@ -342,19 +342,19 @@ static int bonding_attach(struct comedi_device *dev,
 static void bonding_detach(struct comedi_device *dev)
 {
 	struct comedi_bond_private *devpriv = dev->private;
-	unsigned long devs_closed = 0;
 
 	if (devpriv) {
+		DECLARE_BITMAP(devs_closed, COMEDI_NUM_BOARD_MINORS);
+
+		memset(&devs_closed, 0, sizeof(devs_closed));
 		while (devpriv->ndevs-- && devpriv->devs) {
 			struct bonded_device *bdev;
 
 			bdev = devpriv->devs[devpriv->ndevs];
 			if (!bdev)
 				continue;
-			if (!(devs_closed & (0x1 << bdev->minor))) {
+			if (!test_and_set_bit(bdev->minor, devs_closed))
 				comedi_close(bdev->dev);
-				devs_closed |= (0x1 << bdev->minor);
-			}
 			kfree(bdev);
 		}
 		kfree(devpriv->devs);
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 08/15] staging: comedi: comedi_bond: don't map channels individually

[Ian Abbott]

The private data structure (`struct comedi_bond_private`) for the
overall "comedi_bond" device maps each channel individually to a pointer
to the `struct bonded_device` it belongs to via array member
`chan_id_dev_map[MAX_CHANS]`. This speeds up look-ups from channel
number to bonded device a bit, but the length of the array used to look
this up is currently fixed at `MAX_CHANS` (256) and there are no
overflow checks when filling the array.

In practice, there will only be a few bonded devices (actually bonded
subdevices) and it is practical to just skip through the list until we
reach the one containing the desired channel.

The only place where the bonded device is looked up from the channel
number is in `bonding_dio_insn_config()`.  Change it to do the look-up
by skipping through the list of bonded devices and remove the
`chan_id_dev_map[]` member.  The `chanid_offset` member of `struct
bonded_device` is also no longer needed as the value can be derived
while skipping through the list of bonded devices, so remove that member
as well.

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

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index c0a427c..493fdb7 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -53,18 +53,12 @@
 #include "../comedilib.h"
 #include "../comedidev.h"
 
-/* The maxiumum number of channels per subdevice. */
-#define MAX_CHANS 256
-
 struct bonded_device {
 	struct comedi_device *dev;
 	unsigned minor;
 	unsigned subdev;
 	unsigned subdev_type;
 	unsigned nchans;
-	unsigned chanid_offset;	/* The offset into our unified linear
-				 * channel-id's of chanid 0 on this
-				 * subdevice. */
 };
 
 struct comedi_bond_private {
@@ -72,7 +66,6 @@ struct comedi_bond_private {
 	char name[MAX_BOARD_NAME];
 	struct bonded_device **devs;
 	unsigned ndevs;
-	struct bonded_device *chan_id_dev_map[MAX_CHANS];
 	unsigned nchans;
 };
 
@@ -133,12 +126,22 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 {
 	struct comedi_bond_private *devpriv = dev->private;
 	int chan = CR_CHAN(insn->chanspec), ret, io_bits = s->io_bits;
+	unsigned int chanid_offset;
 	unsigned int io;
 	struct bonded_device *bdev;
+	struct bonded_device **devs;
 
 	if (chan < 0 || chan >= devpriv->nchans)
 		return -EINVAL;
-	bdev = devpriv->chan_id_dev_map[chan];
+
+	/*
+	 * Locate bonded subdevice.
+	 */
+	chanid_offset = 0;
+	devs = devpriv->devs;
+	for (bdev = *devs++; chan >= chanid_offset + bdev->nchans;
+	     bdev = *devs++)
+		chanid_offset += bdev->nchans;
 
 	/*
 	 * The input or output configuration of each digital line is
@@ -165,7 +168,7 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 		break;
 	}
 	/* 'real' channel id for this subdev.. */
-	chan -= bdev->chanid_offset;
+	chan -= chanid_offset;
 	ret = comedi_dio_config(bdev->dev, bdev->subdev, chan, io);
 	if (ret != 1)
 		return -EINVAL;
@@ -252,12 +255,7 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 			bdev->subdev = sdev;
 			bdev->subdev_type = COMEDI_SUBD_DIO;
 			bdev->nchans = nchans;
-			bdev->chanid_offset = devpriv->nchans;
-
-			/* map channel id's to bonded_device * pointer.. */
-			while (nchans--)
-				devpriv->chan_id_dev_map[devpriv->nchans++] =
-				    bdev;
+			devpriv->nchans += nchans;
 
 			/*
 			 * Now put bdev pointer at end of devpriv->devs array
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 09/15] staging: comedi: comedi_bond: remove unused subdev_type

[Ian Abbott]

The `subdev_type` member of `struct bonded_device` is set but not used.
Remove it.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 493fdb7..ab5451e 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -57,7 +57,6 @@ struct bonded_device {
 	struct comedi_device *dev;
 	unsigned minor;
 	unsigned subdev;
-	unsigned subdev_type;
 	unsigned nchans;
 };
 
@@ -253,7 +252,6 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 			bdev->dev = d;
 			bdev->minor = minor;
 			bdev->subdev = sdev;
-			bdev->subdev_type = COMEDI_SUBD_DIO;
 			bdev->nchans = nchans;
 			devpriv->nchans += nchans;
 
-- 
1.8.3.2

[PATCH 10/15] staging: comedi: comedi_bond: no need to free dev->private on detach

[Ian Abbott]

The comedi core will free `dev->private` if it is non-NULL after calling
the "detach" handler (`bonding_detach()`), so don't bother freeing it in
`bonding_detach()`.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index ab5451e..a2cc285 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -355,8 +355,6 @@ static void bonding_detach(struct comedi_device *dev)
 		}
 		kfree(devpriv->devs);
 		devpriv->devs = NULL;
-		kfree(devpriv);
-		dev->private = NULL;
 	}
 }
 
-- 
1.8.3.2

[PATCH 11/15] staging: comedi: comedi_bond: just check devprivs->devs once on detach

[Ian Abbott]

The `while` loop in `bonding_detach()` doesn't need to check
`devpriv->devs` each time round the loop.  Move the test outside the
loop.  The enclosing `if (devpriv)` can be changed to `if (devpriv &&
devpriv->devs)` as everything in this `if` statement is associated with
`devpriv->devs` anyway.

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

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index a2cc285..2ea605f 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -339,11 +339,11 @@ static void bonding_detach(struct comedi_device *dev)
 {
 	struct comedi_bond_private *devpriv = dev->private;
 
-	if (devpriv) {
+	if (devpriv && devpriv->devs) {
 		DECLARE_BITMAP(devs_closed, COMEDI_NUM_BOARD_MINORS);
 
 		memset(&devs_closed, 0, sizeof(devs_closed));
-		while (devpriv->ndevs-- && devpriv->devs) {
+		while (devpriv->ndevs--) {
 			struct bonded_device *bdev;
 
 			bdev = devpriv->devs[devpriv->ndevs];
-- 
1.8.3.2

[PATCH 12/15] staging: comedi: comedi_bond: get INSN_CONFIG_DIO_QUERY info from horse's mouth

[Ian Abbott]

The DIO subdevice of the "comedi_bond" device attempts to remember the
directions of DIO channels itself in the `io_bits` member of the
subdevice, but that is only large enough for the first 32 channels and
it might not be accurate anyway as changing the direction of one channel
may have affected a whole group of channels and we have no idea of the
initial directions before the "bonded" device was linked to the the
"comedi_bond" device.  It would be better to ask the bonded device for
this information when handling a `INSN_CONFIG_DIO_QUERY` configuration
instruction.  Add new function `comedi_dio_get_config()` to the
"kcomedilib" module to allow us to get the DIO direction of a channel
and use it.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedilib.h                 |  2 +
 drivers/staging/comedi/drivers/comedi_bond.c       | 49 ++++++++--------------
 .../staging/comedi/kcomedilib/kcomedilib_main.c    | 21 ++++++++++
 3 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/comedi/comedilib.h b/drivers/staging/comedi/comedilib.h
index 1a78b15..47b1640 100644
--- a/drivers/staging/comedi/comedilib.h
+++ b/drivers/staging/comedi/comedilib.h
@@ -21,6 +21,8 @@
 
 struct comedi_device *comedi_open(const char *path);
 int comedi_close(struct comedi_device *dev);
+int comedi_dio_get_config(struct comedi_device *dev, unsigned int subdev,
+			  unsigned int chan, unsigned int *io);
 int comedi_dio_config(struct comedi_device *dev, unsigned int subdev,
 		      unsigned int chan, unsigned int io);
 int comedi_dio_bitfield(struct comedi_device *dev, unsigned int subdev,
diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 2ea605f..a1c51a2 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -124,59 +124,44 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 				   struct comedi_insn *insn, unsigned int *data)
 {
 	struct comedi_bond_private *devpriv = dev->private;
-	int chan = CR_CHAN(insn->chanspec), ret, io_bits = s->io_bits;
-	unsigned int chanid_offset;
-	unsigned int io;
+	unsigned int chan = CR_CHAN(insn->chanspec);
+	int ret;
 	struct bonded_device *bdev;
 	struct bonded_device **devs;
 
-	if (chan < 0 || chan >= devpriv->nchans)
-		return -EINVAL;
-
 	/*
-	 * Locate bonded subdevice.
+	 * Locate bonded subdevice and adjust channel.
 	 */
-	chanid_offset = 0;
 	devs = devpriv->devs;
-	for (bdev = *devs++; chan >= chanid_offset + bdev->nchans;
-	     bdev = *devs++)
-		chanid_offset += bdev->nchans;
+	for (bdev = *devs++; chan >= bdev->nchans; bdev = *devs++)
+		chan -= bdev->nchans;
 
 	/*
 	 * The input or output configuration of each digital line is
 	 * configured by a special insn_config instruction.  chanspec
 	 * contains the channel to be changed, and data[0] contains the
-	 * value COMEDI_INPUT or COMEDI_OUTPUT.
+	 * configuration instruction INSN_CONFIG_DIO_OUTPUT,
+	 * INSN_CONFIG_DIO_INPUT or INSN_CONFIG_DIO_QUERY.
+	 *
+	 * Note that INSN_CONFIG_DIO_OUTPUT == COMEDI_OUTPUT,
+	 * and INSN_CONFIG_DIO_INPUT == COMEDI_INPUT.  This is deliberate ;)
 	 */
 	switch (data[0]) {
 	case INSN_CONFIG_DIO_OUTPUT:
-		io = COMEDI_OUTPUT;	/* is this really necessary? */
-		io_bits |= 1 << chan;
-		break;
 	case INSN_CONFIG_DIO_INPUT:
-		io = COMEDI_INPUT;	/* is this really necessary? */
-		io_bits &= ~(1 << chan);
+		ret = comedi_dio_config(bdev->dev, bdev->subdev, chan, data[0]);
 		break;
 	case INSN_CONFIG_DIO_QUERY:
-		data[1] =
-		    (io_bits & (1 << chan)) ? COMEDI_OUTPUT : COMEDI_INPUT;
-		return insn->n;
+		ret = comedi_dio_get_config(bdev->dev, bdev->subdev, chan,
+					    &data[1]);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 		break;
 	}
-	/* 'real' channel id for this subdev.. */
-	chan -= chanid_offset;
-	ret = comedi_dio_config(bdev->dev, bdev->subdev, chan, io);
-	if (ret != 1)
-		return -EINVAL;
-	/*
-	 * Finally, save the new io_bits values since we didn't get an error
-	 * above.
-	 */
-	s->io_bits = io_bits;
-	return insn->n;
+	if (ret >= 0)
+		ret = insn->n;
+	return ret;
 }
 
 static void *realloc(const void *oldmem, size_t newlen, size_t oldlen)
diff --git a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
index 066083c..c7e809b 100644
--- a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
+++ b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
@@ -123,6 +123,27 @@ error:
 	return ret;
 }
 
+int comedi_dio_get_config(struct comedi_device *dev, unsigned int subdev,
+			  unsigned int chan, unsigned int *io)
+{
+	struct comedi_insn insn;
+	unsigned int data[2];
+	int ret;
+
+	memset(&insn, 0, sizeof(insn));
+	insn.insn = INSN_CONFIG;
+	insn.n = 2;
+	insn.subdev = subdev;
+	insn.chanspec = CR_PACK(chan, 0, 0);
+	data[0] = INSN_CONFIG_DIO_QUERY;
+	data[1] = 0;
+	ret = comedi_do_insn(dev, &insn, data);
+	if (ret >= 0)
+		*io = data[1];
+	return ret;
+}
+EXPORT_SYMBOL_GPL(comedi_dio_get_config);
+
 int comedi_dio_config(struct comedi_device *dev, unsigned int subdev,
 		      unsigned int chan, unsigned int io)
 {
-- 
1.8.3.2

[PATCH 13/15] staging: comedi: comedi_bond: handle base channel for insn_bits

[Ian Abbott]

If a DIO subdevice has more than 32 channels, its 'insn_bits' handler is
supposed to take account of the base channel from
`CR_CHAN(insn->chanspec)`.  (The comedi core will adjust the base
channel to 0 and shift the mask and data to compensate if the subdevice
has less than or equal to 32 channels.)  The "comedi_bond" driver
currently ignores the base channel and assumes it is 0.

Replace `comedi_dio_bitfield()` in the "kcomedilib" module with
`comedi_dio_bitfield2()` that takes account of the base channel, and
rewrite the "comedi_bond" driver's 'insn_bits' handler
(`bonding_dio_insn_bits()`) to take account of the base channel and use
the new function.

No other modules use `comedi_dio_bitfield()` so it is safe to replace it
with `comedi_dio_bitfield2()`.  The name follows that of the equivalent
function in the user-space comedilib.  If the base channel is non-zero
and the subdevice has less than or equal to 32 channels it needs to
adjust things in the same way as the comedi core (same as `parse_insn()`
in "comedi_fops.c") due to most drivers ignoring the base channel.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedilib.h                 |  5 +-
 drivers/staging/comedi/drivers/comedi_bond.c       | 87 ++++++++++++----------
 .../staging/comedi/kcomedilib/kcomedilib_main.c    | 37 +++++++--
 3 files changed, 83 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/comedi/comedilib.h b/drivers/staging/comedi/comedilib.h
index 47b1640..56baf85 100644
--- a/drivers/staging/comedi/comedilib.h
+++ b/drivers/staging/comedi/comedilib.h
@@ -25,8 +25,9 @@ int comedi_dio_get_config(struct comedi_device *dev, unsigned int subdev,
 			  unsigned int chan, unsigned int *io);
 int comedi_dio_config(struct comedi_device *dev, unsigned int subdev,
 		      unsigned int chan, unsigned int io);
-int comedi_dio_bitfield(struct comedi_device *dev, unsigned int subdev,
-			unsigned int mask, unsigned int *bits);
+int comedi_dio_bitfield2(struct comedi_device *dev, unsigned int subdev,
+			 unsigned int mask, unsigned int *bits,
+			 unsigned int base_channel);
 int comedi_find_subdevice_by_type(struct comedi_device *dev, int type,
 				  unsigned int subd);
 int comedi_get_n_channels(struct comedi_device *dev, unsigned int subdevice);
diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index a1c51a2..8e2696c 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -73,48 +73,59 @@ static int bonding_dio_insn_bits(struct comedi_device *dev,
 				 struct comedi_insn *insn, unsigned int *data)
 {
 	struct comedi_bond_private *devpriv = dev->private;
-#define LSAMPL_BITS (sizeof(unsigned int)*8)
-	unsigned nchans = LSAMPL_BITS, num_done = 0, i;
+	unsigned int n_left, n_done, base_chan;
+	unsigned int write_mask, data_bits;
+	struct bonded_device **devs;
 
-	if (devpriv->nchans < nchans)
-		nchans = devpriv->nchans;
+	write_mask = data[0];
+	data_bits = data[1];
+	base_chan = CR_CHAN(insn->chanspec);
+	/* do a maximum of 32 channels, starting from base_chan. */
+	n_left = devpriv->nchans - base_chan;
+	if (n_left > 32)
+		n_left = 32;
 
-	/*
-	 * The insn data is a mask in data[0] and the new data
-	 * in data[1], each channel cooresponding to a bit.
-	 */
-	for (i = 0; num_done < nchans && i < devpriv->ndevs; ++i) {
-		struct bonded_device *bdev = devpriv->devs[i];
-		/*
-		 * Grab the channel mask and data of only the bits corresponding
-		 * to this subdevice.. need to shift them to zero position of
-		 * course.
-		 */
-		/* Bits corresponding to this subdev. */
-		unsigned int subdev_mask = ((1 << bdev->nchans) - 1);
-		unsigned int write_mask, data_bits;
-
-		/* Argh, we have >= LSAMPL_BITS chans.. take all bits */
-		if (bdev->nchans >= LSAMPL_BITS)
-			subdev_mask = (unsigned int)(-1);
-
-		write_mask = (data[0] >> num_done) & subdev_mask;
-		data_bits = (data[1] >> num_done) & subdev_mask;
-
-		/* Read/Write the new digital lines */
-		if (comedi_dio_bitfield(bdev->dev, bdev->subdev, write_mask,
-					&data_bits) != 2)
-			return -EINVAL;
+	n_done = 0;
+	devs = devpriv->devs;
+	do {
+		struct bonded_device *bdev = *devs++;
 
-		/* Make room for the new bits in data[1], the return value */
-		data[1] &= ~(subdev_mask << num_done);
-		/* Put the bits in the return value */
-		data[1] |= (data_bits & subdev_mask) << num_done;
-		/* Save the new bits to the saved state.. */
-		s->state = data[1];
+		if (base_chan < bdev->nchans) {
+			/* base channel falls within bonded device */
+			unsigned int b_chans, b_mask, b_write_mask, b_data_bits;
+			int ret;
 
-		num_done += bdev->nchans;
-	}
+			/*
+			 * Get num channels to do for bonded device and set
+			 * up mask and data bits for bonded device.
+			 */
+			b_chans = bdev->nchans - base_chan;
+			if (b_chans > n_left)
+				b_chans = n_left;
+			b_mask = (1U << b_chans) - 1;
+			b_write_mask = (write_mask >> n_done) & b_mask;
+			b_data_bits = (data_bits >> n_done) & b_mask;
+			/* Read/Write the new digital lines. */
+			ret = comedi_dio_bitfield2(bdev->dev, bdev->subdev,
+						   b_write_mask, &b_data_bits,
+						   base_chan);
+			if (ret < 0)
+				return ret;
+			/* Place read bits into data[1]. */
+			data[1] &= ~(b_mask << n_done);
+			data[1] |= (b_data_bits & b_mask) << n_done;
+			/*
+			 * Set up for following bonded device (if still have
+			 * channels to read/write).
+			 */
+			base_chan = 0;
+			n_done += b_chans;
+			n_left -= b_chans;
+		} else {
+			/* Skip bonded devices before base channel. */
+			base_chan -= bdev->nchans;
+		}
+	} while (n_left);
 
 	return insn->n;
 }
diff --git a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
index c7e809b..cd60677 100644
--- a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
+++ b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
@@ -159,28 +159,53 @@ int comedi_dio_config(struct comedi_device *dev, unsigned int subdev,
 }
 EXPORT_SYMBOL_GPL(comedi_dio_config);
 
-int comedi_dio_bitfield(struct comedi_device *dev, unsigned int subdev,
-			unsigned int mask, unsigned int *bits)
+int comedi_dio_bitfield2(struct comedi_device *dev, unsigned int subdev,
+			 unsigned int mask, unsigned int *bits,
+			 unsigned int base_channel)
 {
 	struct comedi_insn insn;
 	unsigned int data[2];
+	unsigned int n_chan;
+	unsigned int shift;
 	int ret;
 
+	if (subdev >= dev->n_subdevices)
+		return -EINVAL;
+
+	base_channel = CR_CHAN(base_channel);
+	n_chan = comedi_get_n_channels(dev, subdev);
+	if (base_channel >= n_chan)
+		return -EINVAL;
+
 	memset(&insn, 0, sizeof(insn));
 	insn.insn = INSN_BITS;
+	insn.chanspec = base_channel;
 	insn.n = 2;
 	insn.subdev = subdev;
 
 	data[0] = mask;
 	data[1] = *bits;
 
-	ret = comedi_do_insn(dev, &insn, data);
-
-	*bits = data[1];
+	/*
+	 * Most drivers ignore the base channel in insn->chanspec.
+	 * Fix this here if the subdevice has <= 32 channels.
+	 */
+	if (n_chan <= 32) {
+		shift = base_channel;
+		if (shift) {
+			insn.chanspec = 0;
+			data[0] <<= shift;
+			data[1] <<= shift;
+		}
+	} else {
+		shift = 0;
+	}
 
+	ret = comedi_do_insn(dev, &insn, data);
+	*bits = data[1] >> shift;
 	return ret;
 }
-EXPORT_SYMBOL_GPL(comedi_dio_bitfield);
+EXPORT_SYMBOL_GPL(comedi_dio_bitfield2);
 
 int comedi_find_subdevice_by_type(struct comedi_device *dev, int type,
 				  unsigned int subd)
-- 
1.8.3.2

[PATCH 14/15] staging: comedi: comedi_bond: use krealloc() and fix memory leak

[Ian Abbott]

`do_dev_config()` is called from the comedi 'attach' handler,
`bonding_attach()`.  The device private data structure contains a
dynamically allocated array of pointers to "bonded" device structures
which grows during the `do_dev_config()` call.  The length of this array
is in `devpriv->ndevs`.  It currently uses a local function `realloc()`
to allocate a new array, copy the old contents over and free the old
array.  It should be more efficient to use `krealloc()` as it may be
able to use slack space at the end of the previous array and avoid a
copy.

The old `realloc()` function always freed the old buffer which meant
that if it failed to allocate the new buffer it would lose the contents
of the old buffer.  Unfortunately, that contained pointers to more
dynamically allocated memory, leading to a memory leak.  If `krealloc()`
fails, keep the old buffer and avoid the memory leak.  The
aforementioned pointers to more dynamically allocated memory will be
cleaned up by the 'detach' handler, `bonding_detach()` which will be
called by the comedi core as a consequence of `krealloc()` failing in
`do_dev_config()`.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_bond.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 8e2696c..ccac7b9 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -175,16 +175,6 @@ static int bonding_dio_insn_config(struct comedi_device *dev,
 	return ret;
 }
 
-static void *realloc(const void *oldmem, size_t newlen, size_t oldlen)
-{
-	void *newmem = kmalloc(newlen, GFP_KERNEL);
-
-	if (newmem && oldmem)
-		memcpy(newmem, oldmem, min(oldlen, newlen));
-	kfree(oldmem);
-	return newmem;
-}
-
 static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 {
 	struct comedi_bond_private *devpriv = dev->private;
@@ -201,8 +191,9 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 		char file[sizeof("/dev/comediXXXXXX")];
 		int minor = it->options[i];
 		struct comedi_device *d;
-		int sdev = -1, nchans, tmp;
-		struct bonded_device *bdev = NULL;
+		int sdev = -1, nchans;
+		struct bonded_device *bdev;
+		struct bonded_device **devs;
 
 		if (minor < 0 || minor >= COMEDI_NUM_BOARD_MINORS) {
 			dev_err(dev->class_dev,
@@ -257,17 +248,16 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 			 */
 
 			/* ergh.. ugly.. we need to realloc :(  */
-			tmp = devpriv->ndevs * sizeof(bdev);
-			devpriv->devs =
-			    realloc(devpriv->devs,
-				    ++devpriv->ndevs * sizeof(bdev), tmp);
-			if (!devpriv->devs) {
+			devs = krealloc(devpriv->devs,
+					(devpriv->ndevs + 1) * sizeof(*devs),
+					GFP_KERNEL);
+			if (!devs) {
 				dev_err(dev->class_dev,
 					"Could not allocate memory. Out of memory?\n");
 				return -ENOMEM;
 			}
-
-			devpriv->devs[devpriv->ndevs - 1] = bdev;
+			devpriv->devs = devs;
+			devpriv->devs[devpriv->ndevs++] = bdev;
 			{
 				/* Append dev:subdev to devpriv->name */
 				char buf[20];
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 15/15] staging: comedi: comedi_bond: use correct minor device numbers in name

[Ian Abbott]

The board name for "comedi_bond" is constructed from a space-separated
list of items of the form "minor:subdevice" where "minor" is a minor
device number and "subdevice" is a subdevice number.  Currently, all the
"minor" device numbers are for the "comedi_bond" device itself and the
"subdevice" numbers are for the bonded devices.  It makes makes more
sense for the "minor" device numbers to come from the bonded devices as
well so that the string is a list of bonded "minor:subdevice" pairs.
Fix it.

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

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index ccac7b9..51a59e5 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -263,8 +263,8 @@ static int do_dev_config(struct comedi_device *dev, struct comedi_devconfig *it)
 				char buf[20];
 				int left =
 				    MAX_BOARD_NAME - strlen(devpriv->name) - 1;
-				snprintf(buf, sizeof(buf), "%d:%d ", dev->minor,
-					 bdev->subdev);
+				snprintf(buf, sizeof(buf), "%d:%d ",
+					 bdev->minor, bdev->subdev);
 				buf[sizeof(buf) - 1] = 0;
 				strncat(devpriv->name, buf, left);
 			}
-- 
1.8.3.2

RE: [PATCH 13/15] staging: comedi: comedi_bond: handle base channel for insn_bits

[HartleyS]

On Friday, August 23, 2013 6:45 AM, Ian Abbott wrote:
> If a DIO subdevice has more than 32 channels, its 'insn_bits' handler is
> supposed to take account of the base channel from
> `CR_CHAN(insn->chanspec)`.  (The comedi core will adjust the base
> channel to 0 and shift the mask and data to compensate if the subdevice
> has less than or equal to 32 channels.)  The "comedi_bond" driver
> currently ignores the base channel and assumes it is 0.
>
> Replace `comedi_dio_bitfield()` in the "kcomedilib" module with
> `comedi_dio_bitfield2()` that takes account of the base channel, and
> rewrite the "comedi_bond" driver's 'insn_bits' handler
> (`bonding_dio_insn_bits()`) to take account of the base channel and use
> the new function.
>
> No other modules use `comedi_dio_bitfield()` so it is safe to replace it
> with `comedi_dio_bitfield2()`.  The name follows that of the equivalent
> function in the user-space comedilib.  If the base channel is non-zero
> and the subdevice has less than or equal to 32 channels it needs to
> adjust things in the same way as the comedi core (same as `parse_insn()`
> in "comedi_fops.c") due to most drivers ignoring the base channel.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>

Ian,

I have not had a chance to look this series over yet but I had an off topic
comment about kcomedilib.

Is it too late to rename the exports from that driver? It would be a lot
clearer if the "core" exports were prefixed with "comedi_" and the
exports provided to the kernel by kcomedilib where prefixed differently,
maybe as "kcomedi_" or even "komedi_".

Right now I think the only driver using kcomedilib is the comedi_bond driver.

Thanks,
Hartley
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/15] staging: comedi: comedi_bond: handle base channel for insn_bits

[Ian Abbott]

On 2013-08-23 16:01, HartleyS@visionengravers.com wrote:
> Ian,
>
> I have not had a chance to look this series over yet but I had an off topic
> comment about kcomedilib.
>
> Is it too late to rename the exports from that driver? It would be a lot
> clearer if the "core" exports were prefixed with "comedi_" and the
> exports provided to the kernel by kcomedilib where prefixed differently,
> maybe as "kcomedi_" or even "komedi_".
>
> Right now I think the only driver using kcomedilib is the comedi_bond driver.

I like to think of kcomedilib as an extended offshoot of the comedi 
core.  At one time it was used heavily by RTAI (www.rtai.org) 
applications running at kernel level, but now it has been stripped down 
to the bare minimum and is only currently used to support the 
comedi_bond driver.  Because it was originally a kernel-level equivalent 
of the user-space comedilib library, the function names were chosen to 
match.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel