public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] [RESEND] VME framework fixes
@ 2011-08-10  9:33 Manohar Vanga
  2011-08-10  9:33 ` [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers Manohar Vanga
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

Hey Martyn,

This is the resend of the patches with the recommended fixes. Some notes
on the various patches:

  staging: vme: allow explicit assignment of bus numbers

I have left this as it is with a simple array of numbers. I think we should
apply this for now and work on the UUID method separately as this solves a
major issue in a relatively decent way.

  staging: vme: make [alloc|free]_consistent bridge specific

Added the bridge names to the prink's.

  staging: vme: add functions for bridge module refcounting

I have added calls in probe and remove to automatically refcount the devices
that are probed. I have left symbol exports for the refcounting functions if
any drivers want to use them in some unforseen way.

  staging: vme: add struct vme_dev for VME devices
  staging: vme: make match() driver specific to improve non-VME64x
    support

Fixes based on the changes introduced before. Note that I have removed the
patch with the rename of the vme_slot_get function. Also removed the
kzalloc patch as it can be applied from the previous send.

Another note: I have not added the documentation yet. Based on the changes
that are approved, I will write it up today and send it in :)

Looking forward to your feedback. Thanks!

--
/manohar

Emilio G. Cota (1):
  staging: vme: allow explicit assignment of bus numbers

Manohar Vanga (5):
  staging: vme: make [alloc|free]_consistent bridge specific
  staging: vme: keep track of registered buses
  staging: vme: add functions for bridge module refcounting
  staging: vme: add struct vme_dev for VME devices
  staging: vme: make match() driver specific to improve non-VME64x
    support

 drivers/staging/vme/bridges/vme_ca91cx42.c |   46 +++
 drivers/staging/vme/bridges/vme_tsi148.c   |   47 +++
 drivers/staging/vme/devices/vme_user.c     |   39 ++-
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  418 ++++++++++++++++------------
 drivers/staging/vme/vme.h                  |   56 +++-
 drivers/staging/vme/vme_bridge.h           |   17 +-
 7 files changed, 419 insertions(+), 206 deletions(-)

-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:02   ` Martyn Welch
  2011-08-10  9:33 ` [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific Manohar Vanga
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

From: Emilio G. Cota <cota@braap.org>

In a configuration with several bridges, each bridge is
assigned a certain bus number depending on the order in which
bridge modules are loaded. This can complicate multi-bridge
installations because the bus numbers will depend on the load
order of the bridges.

This patch allows bridges to register with a bus number of
their choice, while keeping the previous 'first come, first
serve' behaviour as the default.

Module parameters have been added to the bridge drivers to
allow overriding of the auto-assignment during load time.

This patch also explicitly defines VME_MAX_BRIDGES as the,
size of vme_bus_numbers (unsigned int); something that was
implicit earlier in the code.

Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/bridges/vme_ca91cx42.c |   20 ++++++++++++
 drivers/staging/vme/bridges/vme_tsi148.c   |   21 +++++++++++++
 drivers/staging/vme/vme.c                  |   46 ++++++++++++++++++++++-----
 drivers/staging/vme/vme.h                  |    2 +
 4 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 5122c13..c378819 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -41,6 +41,13 @@ static void __exit ca91cx42_exit(void);
 
 /* Module parameters */
 static int geoid;
+static int bus_ids[VME_MAX_BRIDGES];
+static int id_count;
+
+/* The number of registered buses */
+static int bus_count;
+/* Mutex for protecting ID access */
+static DEFINE_MUTEX(bus_id_mutex);
 
 static const char driver_name[] = "vme_ca91cx42";
 
@@ -1779,6 +1786,12 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ca91cx42_crcsr_init(ca91cx42_bridge, pdev))
 		dev_err(&pdev->dev, "CR/CSR configuration failed.\n");
 
+	mutex_lock(&bus_id_mutex);
+	if (bus_count >= id_count)
+		ca91cx42_bridge->num = -1;
+	else
+		ca91cx42_bridge->num = bus_ids[bus_count];
+
 	/* Need to save ca91cx42_bridge pointer locally in link list for use in
 	 * ca91cx42_remove()
 	 */
@@ -1788,11 +1801,15 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_reg;
 	}
 
+	bus_count++;
+	mutex_unlock(&bus_id_mutex);
+
 	pci_set_drvdata(pdev, ca91cx42_bridge);
 
 	return 0;
 
 err_reg:
+	mutex_unlock(&bus_id_mutex);
 	ca91cx42_crcsr_exit(ca91cx42_bridge, pdev);
 err_lm:
 	/* resources are stored in link list */
@@ -1930,5 +1947,8 @@ module_param(geoid, int, 0);
 MODULE_DESCRIPTION("VME driver for the Tundra Universe II VME bridge");
 MODULE_LICENSE("GPL");
 
+MODULE_PARM_DESC(bus_num, "Explicitly set bus number (override auto-assign)");
+module_param_array(bus_ids, int, &id_count, 0);
+
 module_init(ca91cx42_init);
 module_exit(ca91cx42_exit);
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index 9c53951..e3f021e 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -44,6 +44,14 @@ static void __exit tsi148_exit(void);
 static int err_chk;
 static int geoid;
 
+static int bus_ids[VME_MAX_BRIDGES];
+static int id_count;
+
+/* The number of registered buses */
+static int bus_count;
+/* Mutex for protecting ID access */
+static DEFINE_MUTEX(bus_id_mutex);
+
 static const char driver_name[] = "vme_tsi148";
 
 static DEFINE_PCI_DEVICE_TABLE(tsi148_ids) = {
@@ -2462,12 +2470,21 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_crcsr;
 	}
 
+	mutex_lock(&bus_id_mutex);
+	if (bus_count >= id_count)
+		tsi148_bridge->num = -1;
+	else
+		tsi148_bridge->num = bus_ids[bus_count];
+
 	retval = vme_register_bridge(tsi148_bridge);
 	if (retval != 0) {
 		dev_err(&pdev->dev, "Chip Registration failed.\n");
 		goto err_reg;
 	}
 
+	bus_count++;
+	mutex_unlock(&bus_id_mutex);
+
 	pci_set_drvdata(pdev, tsi148_bridge);
 
 	/* Clear VME bus "board fail", and "power-up reset" lines */
@@ -2479,6 +2496,7 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 
 err_reg:
+	mutex_unlock(&bus_id_mutex);
 	tsi148_crcsr_exit(tsi148_bridge, pdev);
 err_crcsr:
 err_lm:
@@ -2633,6 +2651,9 @@ module_param(err_chk, bool, 0);
 MODULE_PARM_DESC(geoid, "Override geographical addressing");
 module_param(geoid, int, 0);
 
+MODULE_PARM_DESC(bus_ids, "Explicitly set bus number (override auto-assign)");
+module_param_array(bus_ids, int, &id_count, 0);
+
 MODULE_DESCRIPTION("VME driver for the Tundra Tempe VME bridge");
 MODULE_LICENSE("GPL");
 
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index c078ce3..330a4ff 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -1303,20 +1303,44 @@ EXPORT_SYMBOL(vme_slot_get);
 
 /* - Bridge Registration --------------------------------------------------- */
 
-static int vme_alloc_bus_num(void)
+/* call with vme_bus_num_mtx held */
+static int __vme_alloc_bus_num(int bus_num)
 {
+	int num;
 	int i;
 
-	mutex_lock(&vme_bus_num_mtx);
-	for (i = 0; i < sizeof(vme_bus_numbers) * 8; i++) {
-		if (((vme_bus_numbers >> i) & 0x1) == 0) {
-			vme_bus_numbers |= (0x1 << i);
-			break;
+	if (bus_num == -1) {
+		/* try to find a free bus number */
+		for (i = 0; i < VME_MAX_BRIDGES; i++) {
+			if ((vme_bus_numbers & (1 << i)) == 0) {
+				num = i;
+				break;
+			}
+		}
+		if (i == VME_MAX_BRIDGES) {
+			pr_warn("vme: No bus numbers left\n");
+			return -ENODEV;
 		}
+	} else {
+		/* check if the given bus number is already in use */
+		if (vme_bus_numbers & (1 << bus_num)) {
+			pr_warn("vme: bus number %d already in use\n", bus_num);
+			return -EBUSY;
+		}
+		num = bus_num;
 	}
-	mutex_unlock(&vme_bus_num_mtx);
+	vme_bus_numbers |= 1 << num;
+	return num;
+}
 
-	return i;
+static int vme_alloc_bus_num(int bus_num)
+{
+	int num;
+
+	mutex_lock(&vme_bus_num_mtx);
+	num = __vme_alloc_bus_num(bus_num);
+	mutex_unlock(&vme_bus_num_mtx);
+	return num;
 }
 
 static void vme_free_bus_num(int bus)
@@ -1332,7 +1356,11 @@ int vme_register_bridge(struct vme_bridge *bridge)
 	int retval;
 	int i;
 
-	bridge->num = vme_alloc_bus_num();
+	retval = vme_alloc_bus_num(bridge->num);
+	if (retval < 0)
+		return retval;
+
+	bridge->num = retval;
 
 	/* This creates 32 vme "slot" devices. This equates to a slot for each
 	 * ID available in a system conforming to the ANSI/VITA 1-1994
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 4155d8c..3ccb497 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -88,6 +88,8 @@ struct vme_resource {
 
 extern struct bus_type vme_bus_type;
 
+/* VME_MAX_BRIDGES comes from the type of vme_bus_numbers */
+#define VME_MAX_BRIDGES		(sizeof(unsigned int)*8)
 #define VME_SLOT_CURRENT	-1
 #define VME_SLOT_ALL		-2
 
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
  2011-08-10  9:33 ` [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:04   ` Martyn Welch
  2011-08-10 13:12   ` Joe Perches
  2011-08-10  9:33 ` [PATCH 3/6] staging: vme: keep track of registered buses Manohar Vanga
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

Make PCI dependent functions ([alloc|free]_consistent() in
'vme.c') bridge specific. By removing the dependency of the
VME bridge framework on PCI, this patch allows for addition of
non-PCI based VME bridges.

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/bridges/vme_ca91cx42.c |   24 ++++++++++++++++++
 drivers/staging/vme/bridges/vme_tsi148.c   |   24 ++++++++++++++++++
 drivers/staging/vme/vme.c                  |   36 ++++++++++++++++-----------
 drivers/staging/vme/vme_bridge.h           |   10 +++++--
 4 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index c378819..15a0b19 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1507,6 +1507,28 @@ static int ca91cx42_slot_get(struct vme_bridge *ca91cx42_bridge)
 
 }
 
+void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
+	dma_addr_t *dma)
+{
+	struct pci_dev *pdev;
+
+	/* Find pci_dev container of dev */
+	pdev = container_of(parent, struct pci_dev, dev);
+
+	return pci_alloc_consistent(pdev, size, dma);
+}
+
+void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
+	dma_addr_t dma)
+{
+	struct pci_dev *pdev;
+
+	/* Find pci_dev container of dev */
+	pdev = container_of(parent, struct pci_dev, dev);
+
+	pci_free_consistent(pdev, size, vaddr, dma);
+}
+
 static int __init ca91cx42_init(void)
 {
 	return pci_register_driver(&ca91cx42_driver);
@@ -1776,6 +1798,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ca91cx42_bridge->lm_attach = ca91cx42_lm_attach;
 	ca91cx42_bridge->lm_detach = ca91cx42_lm_detach;
 	ca91cx42_bridge->slot_get = ca91cx42_slot_get;
+	ca91cx42_bridge->alloc_consistent = ca91cx42_alloc_consistent;
+	ca91cx42_bridge->free_consistent = ca91cx42_free_consistent;
 
 	data = ioread32(ca91cx42_device->base + MISC_CTL);
 	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index e3f021e..5c147d6 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
 	return (int)slot;
 }
 
+void *tsi148_alloc_consistent(struct device *parent, size_t size,
+	dma_addr_t *dma)
+{
+	struct pci_dev *pdev;
+
+	/* Find pci_dev container of dev */
+	pdev = container_of(parent, struct pci_dev, dev);
+
+	return pci_alloc_consistent(pdev, size, dma);
+}
+
+void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
+	dma_addr_t dma)
+{
+	struct pci_dev *pdev;
+
+	/* Find pci_dev container of dev */
+	pdev = container_of(parent, struct pci_dev, dev);
+
+	pci_free_consistent(pdev, size, vaddr, dma);
+}
+
 static int __init tsi148_init(void)
 {
 	return pci_register_driver(&tsi148_driver);
@@ -2451,6 +2473,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	tsi148_bridge->lm_attach = tsi148_lm_attach;
 	tsi148_bridge->lm_detach = tsi148_lm_detach;
 	tsi148_bridge->slot_get = tsi148_slot_get;
+	tsi148_bridge->alloc_consistent = tsi148_alloc_consistent;
+	tsi148_bridge->free_consistent = tsi148_free_consistent;
 
 	data = ioread32be(tsi148_device->base + TSI148_LCSR_VSTAT);
 	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 330a4ff..15a6161 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -83,15 +83,11 @@ static struct vme_bridge *find_bridge(struct vme_resource *resource)
 /*
  * Allocate a contiguous block of memory for use by the driver. This is used to
  * create the buffers for the slave windows.
- *
- * XXX VME bridges could be available on buses other than PCI. At the momment
- *     this framework only supports PCI devices.
  */
 void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
 	dma_addr_t *dma)
 {
 	struct vme_bridge *bridge;
-	struct pci_dev *pdev;
 
 	if (resource == NULL) {
 		printk(KERN_ERR "No resource\n");
@@ -104,28 +100,29 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
 		return NULL;
 	}
 
-	/* Find pci_dev container of dev */
 	if (bridge->parent == NULL) {
-		printk(KERN_ERR "Dev entry NULL\n");
+		printk(KERN_ERR "Dev entry NULL for"
+			" bridge %s\n", bridge->name);
+		return NULL;
+	}
+
+	if (bridge->alloc_consistent == NULL) {
+		printk(KERN_ERR "alloc_consistent not supported by"
+			" bridge %s\n", bridge->name);
 		return NULL;
 	}
-	pdev = container_of(bridge->parent, struct pci_dev, dev);
 
-	return pci_alloc_consistent(pdev, size, dma);
+	return bridge->alloc_consistent(bridge->parent, size, dma);
 }
 EXPORT_SYMBOL(vme_alloc_consistent);
 
 /*
  * Free previously allocated contiguous block of memory.
- *
- * XXX VME bridges could be available on buses other than PCI. At the momment
- *     this framework only supports PCI devices.
  */
 void vme_free_consistent(struct vme_resource *resource, size_t size,
 	void *vaddr, dma_addr_t dma)
 {
 	struct vme_bridge *bridge;
-	struct pci_dev *pdev;
 
 	if (resource == NULL) {
 		printk(KERN_ERR "No resource\n");
@@ -138,10 +135,19 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
 		return;
 	}
 
-	/* Find pci_dev container of dev */
-	pdev = container_of(bridge->parent, struct pci_dev, dev);
+	if (bridge->parent == NULL) {
+		printk(KERN_ERR "Dev entry NULL for"
+			" bridge %s\n", bridge->name);
+		return;
+	}
+
+	if (bridge->free_consistent == NULL) {
+		printk(KERN_ERR "free_consistent not supported by"
+			" bridge %s\n", bridge->name);
+		return;
+	}
 
-	pci_free_consistent(pdev, size, vaddr, dma);
+	bridge->free_consistent(bridge->parent, size, vaddr, dma);
 }
 EXPORT_SYMBOL(vme_free_consistent);
 
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index 4c6ec31..a9084f0 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -98,8 +98,6 @@ struct vme_irq {
 /* This structure stores all the information about one bridge
  * The structure should be dynamically allocated by the driver and one instance
  * of the structure should be present for each VME chip present in the system.
- *
- * Currently we assume that all chips are PCI-based
  */
 struct vme_bridge {
 	char name[VMENAMSIZ];
@@ -112,7 +110,7 @@ struct vme_bridge {
 	struct list_head vme_errors;	/* List for errors generated on VME */
 
 	/* Bridge Info - XXX Move to private structure? */
-	struct device *parent;	/* Generic device struct (pdev->dev for PCI) */
+	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
 	void *driver_priv;	/* Private pointer for the bridge driver */
 
 	struct device dev[VME_SLOTS_MAX];	/* Device registered with
@@ -165,6 +163,12 @@ struct vme_bridge {
 
 	/* CR/CSR space functions */
 	int (*slot_get) (struct vme_bridge *);
+
+	/* Bridge parent interface */
+	void *(*alloc_consistent)(struct device *dev, size_t size,
+		dma_addr_t *dma);
+	void (*free_consistent)(struct device *dev, size_t size,
+		void *vaddr, dma_addr_t dma);
 };
 
 void vme_irq_handler(struct vme_bridge *, int, int);
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 3/6] staging: vme: keep track of registered buses
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
  2011-08-10  9:33 ` [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers Manohar Vanga
  2011-08-10  9:33 ` [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:06   ` Martyn Welch
  2011-08-10  9:33 ` [PATCH 4/6] staging: vme: add functions for bridge module refcounting Manohar Vanga
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

This patch adds a list which keeps track of all registered VME
buses. This is required for adding refcounting later to bridge
modules, something that is not currently implemented.

This is based on the changes introduced by Emilio G. Cota in the
patch:

    https://lkml.org/lkml/2010/10/25/486

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/vme.c        |   46 ++++++++++++++++++++-----------------
 drivers/staging/vme/vme_bridge.h |    1 +
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 15a6161..f811d07 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -34,9 +34,10 @@
 #include "vme.h"
 #include "vme_bridge.h"
 
-/* Bitmask and mutex to keep track of bridge numbers */
+/* Bitmask and list of registered buses both protected by common mutex */
 static unsigned int vme_bus_numbers;
-static DEFINE_MUTEX(vme_bus_num_mtx);
+static LIST_HEAD(vme_bus_list);
+static DEFINE_MUTEX(vme_buses_lock);
 
 static void __exit vme_exit(void);
 static int __init vme_init(void);
@@ -1309,11 +1310,12 @@ EXPORT_SYMBOL(vme_slot_get);
 
 /* - Bridge Registration --------------------------------------------------- */
 
-/* call with vme_bus_num_mtx held */
-static int __vme_alloc_bus_num(int bus_num)
+/* call with vme_buses_lock held */
+static int __vme_add_bus(struct vme_bridge *bridge)
 {
 	int num;
 	int i;
+	int bus_num = bridge->num;
 
 	if (bus_num == -1) {
 		/* try to find a free bus number */
@@ -1335,25 +1337,29 @@ static int __vme_alloc_bus_num(int bus_num)
 		}
 		num = bus_num;
 	}
+	bridge->num = num;
+	list_add_tail(&bridge->bus_list, &vme_bus_list);
 	vme_bus_numbers |= 1 << num;
-	return num;
+	return 0;
 }
 
-static int vme_alloc_bus_num(int bus_num)
+static int vme_add_bus(struct vme_bridge *bridge)
 {
-	int num;
+	int ret;
 
-	mutex_lock(&vme_bus_num_mtx);
-	num = __vme_alloc_bus_num(bus_num);
-	mutex_unlock(&vme_bus_num_mtx);
-	return num;
+	mutex_lock(&vme_buses_lock);
+	ret = __vme_add_bus(bridge);
+	mutex_unlock(&vme_buses_lock);
+
+	return ret;
 }
 
-static void vme_free_bus_num(int bus)
+static void vme_remove_bus(struct vme_bridge *bridge)
 {
-	mutex_lock(&vme_bus_num_mtx);
-	vme_bus_numbers &= ~(0x1 << bus);
-	mutex_unlock(&vme_bus_num_mtx);
+	mutex_lock(&vme_buses_lock);
+	vme_bus_numbers &= ~(1 << bridge->num);
+	list_del(&bridge->bus_list);
+	mutex_unlock(&vme_buses_lock);
 }
 
 int vme_register_bridge(struct vme_bridge *bridge)
@@ -1362,12 +1368,10 @@ int vme_register_bridge(struct vme_bridge *bridge)
 	int retval;
 	int i;
 
-	retval = vme_alloc_bus_num(bridge->num);
-	if (retval < 0)
+	retval = vme_add_bus(bridge);
+	if (retval)
 		return retval;
 
-	bridge->num = retval;
-
 	/* This creates 32 vme "slot" devices. This equates to a slot for each
 	 * ID available in a system conforming to the ANSI/VITA 1-1994
 	 * specification.
@@ -1398,7 +1402,7 @@ err_reg:
 		dev = &bridge->dev[i];
 		device_unregister(dev);
 	}
-	vme_free_bus_num(bridge->num);
+	vme_remove_bus(bridge);
 	return retval;
 }
 EXPORT_SYMBOL(vme_register_bridge);
@@ -1413,7 +1417,7 @@ void vme_unregister_bridge(struct vme_bridge *bridge)
 		dev = &bridge->dev[i];
 		device_unregister(dev);
 	}
-	vme_free_bus_num(bridge->num);
+	vme_remove_bus(bridge);
 }
 EXPORT_SYMBOL(vme_unregister_bridge);
 
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index a9084f0..8959670 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -112,6 +112,7 @@ struct vme_bridge {
 	/* Bridge Info - XXX Move to private structure? */
 	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
 	void *driver_priv;	/* Private pointer for the bridge driver */
+	struct list_head bus_list; /* list of VME buses */
 
 	struct device dev[VME_SLOTS_MAX];	/* Device registered with
 						 * device model on VME bus
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 4/6] staging: vme: add functions for bridge module refcounting
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
                   ` (2 preceding siblings ...)
  2011-08-10  9:33 ` [PATCH 3/6] staging: vme: keep track of registered buses Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:09   ` Martyn Welch
  2011-08-10  9:33 ` [PATCH 5/6] staging: vme: add struct vme_dev for VME devices Manohar Vanga
  2011-08-10  9:33 ` [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support Manohar Vanga
  5 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

This patch adds functions that allow for reference counting
bridge modules. The patch introduces the functions
'vme_bridge_get()' and 'vme_bridge_put()'.

The functions are automatically called during .probe and .remove
for drivers.

This patch is based on the changes introduced by Emilio G. Cota
in the patch:

    https://lkml.org/lkml/2010/10/25/492

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/bridges/vme_ca91cx42.c |    2 +
 drivers/staging/vme/bridges/vme_tsi148.c   |    2 +
 drivers/staging/vme/vme.c                  |   54 ++++++++++++++++++++++++++++
 drivers/staging/vme/vme.h                  |    2 +
 drivers/staging/vme/vme_bridge.h           |    1 +
 5 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 15a0b19..2f9c22b 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1680,6 +1680,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ca91cx42_bridge->parent = &pdev->dev;
 	strcpy(ca91cx42_bridge->name, driver_name);
 
+	ca91cx42_bridge->owner = THIS_MODULE;
+
 	/* Setup IRQ */
 	retval = ca91cx42_irq_init(ca91cx42_bridge);
 	if (retval != 0) {
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index 5c147d6..9db89dc 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2320,6 +2320,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	tsi148_bridge->parent = &pdev->dev;
 	strcpy(tsi148_bridge->name, driver_name);
 
+	tsi148_bridge->owner = THIS_MODULE;
+
 	/* Setup IRQ */
 	retval = tsi148_irq_init(tsi148_bridge);
 	if (retval != 0) {
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index f811d07..0504007 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -52,6 +52,54 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
 }
 
 /*
+ * Increments the reference count of a VME bridge.
+ *
+ * If found, a pointer to the bridge is returned and the reference count
+ * of the module that owns it is incremented.
+ *
+ * On success, it can be assumed that the bridge won't be removed until
+ * the corresponding call to vme_put_bridge().
+ *
+ * Normally drivers should call vme_get_bridge() on a successfull .probe,
+ * and vme_put_bridge() when releasing the device, e.g. in .remove.
+ */
+struct vme_bridge *vme_bridge_get(unsigned int bus_id)
+{
+	struct vme_bridge *bridge;
+	struct vme_bridge *retp = NULL;
+
+	mutex_lock(&vme_buses_lock);
+	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
+		if (bridge->num == bus_id) {
+			if (!bridge->owner)
+				dev_warn(bridge->parent,
+					"bridge->owner not set\n");
+			else if (try_module_get(bridge->owner))
+				retp = bridge;
+			break;
+		}
+	}
+	mutex_unlock(&vme_buses_lock);
+	return retp;
+}
+EXPORT_SYMBOL(vme_bridge_get);
+
+/*
+ * Decrements the reference count of a bridge
+ *
+ * This function decrements the reference count of the module that controls
+ * the bridge. It must come after a call to vme_get_bridge().
+ */
+void vme_bridge_put(struct vme_bridge *bridge)
+{
+	if (!bridge->owner)
+		dev_warn(bridge->parent, "bridge->owner not set\n");
+	else
+		module_put(bridge->owner);
+}
+EXPORT_SYMBOL(vme_bridge_put);
+
+/*
  * Find the bridge that the resource is associated with.
  */
 static struct vme_bridge *find_bridge(struct vme_resource *resource)
@@ -1525,9 +1573,13 @@ static int vme_bus_probe(struct device *dev)
 	driver = dev_to_vme_driver(dev);
 	bridge = dev_to_bridge(dev);
 
+	vme_bridge_get(bridge->num);
 	if (driver->probe != NULL)
 		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
 
+	if (retval)
+		vme_bridge_put(bridge);
+
 	return retval;
 }
 
@@ -1543,6 +1595,8 @@ static int vme_bus_remove(struct device *dev)
 	if (driver->remove != NULL)
 		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
 
+	vme_bridge_put(bridge);
+
 	return retval;
 }
 
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 3ccb497..8fb35e2 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -167,6 +167,8 @@ int vme_slot_get(struct device *);
 int vme_register_driver(struct vme_driver *);
 void vme_unregister_driver(struct vme_driver *);
 
+struct vme_bridge *vme_bridge_get(unsigned int bus_id);
+void vme_bridge_put(struct vme_bridge *bridge);
 
 #endif /* _VME_H_ */
 
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index 8959670..ef751a4 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -113,6 +113,7 @@ struct vme_bridge {
 	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
 	void *driver_priv;	/* Private pointer for the bridge driver */
 	struct list_head bus_list; /* list of VME buses */
+	struct module *owner;	/* module that owns the bridge */
 
 	struct device dev[VME_SLOTS_MAX];	/* Device registered with
 						 * device model on VME bus
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 5/6] staging: vme: add struct vme_dev for VME devices
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
                   ` (3 preceding siblings ...)
  2011-08-10  9:33 ` [PATCH 4/6] staging: vme: add functions for bridge module refcounting Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:14   ` Martyn Welch
  2011-08-10  9:33 ` [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support Manohar Vanga
  5 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

Instead of using a vanilla 'struct device' for VME devices, add new
'struct vme_dev'. Modifications have been made to the VME framework
API as well as all in-tree VME drivers.

The new vme_dev structure has the following advantages from the
current model used by the driver:

    * Driver functions (probe, remove) now receive a VME device
      instead of a pointer to the bridge device (cleaner design)
    * It's easier to differenciate API calls as bridge-based or
      device-based (ie. cleaner interface).

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/devices/vme_user.c |   14 ++--
 drivers/staging/vme/vme.c              |  120 +++++++++++++------------------
 drivers/staging/vme/vme.h              |   37 +++++++---
 drivers/staging/vme/vme_bridge.h       |    5 +-
 4 files changed, 85 insertions(+), 91 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 3cbeb2a..bb33dc2 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -116,7 +116,7 @@ static struct driver_stats statistics;
 
 static struct cdev *vme_user_cdev;		/* Character device */
 static struct class *vme_user_sysfs_class;	/* Sysfs class */
-static struct device *vme_user_bridge;		/* Pointer to bridge device */
+static struct vme_dev *vme_user_bridge;		/* Pointer to user device */
 
 
 static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
@@ -135,8 +135,8 @@ static ssize_t vme_user_write(struct file *, const char __user *, size_t,
 static loff_t vme_user_llseek(struct file *, loff_t, int);
 static long vme_user_unlocked_ioctl(struct file *, unsigned int, unsigned long);
 
-static int __devinit vme_user_probe(struct device *, int, int);
-static int __devexit vme_user_remove(struct device *, int, int);
+static int __devinit vme_user_probe(struct vme_dev *);
+static int __devexit vme_user_remove(struct vme_dev *);
 
 static const struct file_operations vme_user_fops = {
 	.open = vme_user_open,
@@ -689,8 +689,7 @@ err_nocard:
  * as practical. We will therefore reserve the buffers and request the images
  * here so that we don't have to do it later.
  */
-static int __devinit vme_user_probe(struct device *dev, int cur_bus,
-	int cur_slot)
+static int __devinit vme_user_probe(struct vme_dev *vdev)
 {
 	int i, err;
 	char name[12];
@@ -702,7 +701,7 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
 		err = -EINVAL;
 		goto err_dev;
 	}
-	vme_user_bridge = dev;
+	vme_user_bridge = vdev;
 
 	/* Initialise descriptors */
 	for (i = 0; i < VME_DEVS; i++) {
@@ -865,8 +864,7 @@ err_dev:
 	return err;
 }
 
-static int __devexit vme_user_remove(struct device *dev, int cur_bus,
-	int cur_slot)
+static int __devexit vme_user_remove(struct vme_dev *dev)
 {
 	int i;
 
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 0504007..360acc9 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -42,13 +42,9 @@ static DEFINE_MUTEX(vme_buses_lock);
 static void __exit vme_exit(void);
 static int __init vme_init(void);
 
-
-/*
- * Find the bridge resource associated with a specific device resource
- */
-static struct vme_bridge *dev_to_bridge(struct device *dev)
+static struct vme_dev *dev_to_vme_dev(struct device *dev)
 {
-	return dev->platform_data;
+	return container_of(dev, struct vme_dev, dev);
 }
 
 /*
@@ -284,7 +280,7 @@ static int vme_check_window(vme_address_t aspace, unsigned long long vme_base,
  * Request a slave image with specific attributes, return some unique
  * identifier.
  */
-struct vme_resource *vme_slave_request(struct device *dev,
+struct vme_resource *vme_slave_request(struct vme_dev *vdev,
 	vme_address_t address, vme_cycle_t cycle)
 {
 	struct vme_bridge *bridge;
@@ -293,7 +289,7 @@ struct vme_resource *vme_slave_request(struct device *dev,
 	struct vme_slave_resource *slave_image = NULL;
 	struct vme_resource *resource = NULL;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		goto err_bus;
@@ -440,7 +436,7 @@ EXPORT_SYMBOL(vme_slave_free);
  * Request a master image with specific attributes, return some unique
  * identifier.
  */
-struct vme_resource *vme_master_request(struct device *dev,
+struct vme_resource *vme_master_request(struct vme_dev *vdev,
 	vme_address_t address, vme_cycle_t cycle, vme_width_t dwidth)
 {
 	struct vme_bridge *bridge;
@@ -449,7 +445,7 @@ struct vme_resource *vme_master_request(struct device *dev,
 	struct vme_master_resource *master_image = NULL;
 	struct vme_resource *resource = NULL;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		goto err_bus;
@@ -698,7 +694,8 @@ EXPORT_SYMBOL(vme_master_free);
  * Request a DMA controller with specific attributes, return some unique
  * identifier.
  */
-struct vme_resource *vme_dma_request(struct device *dev, vme_dma_route_t route)
+struct vme_resource *vme_dma_request(struct vme_dev *vdev,
+	vme_dma_route_t route)
 {
 	struct vme_bridge *bridge;
 	struct list_head *dma_pos = NULL;
@@ -709,7 +706,7 @@ struct vme_resource *vme_dma_request(struct device *dev, vme_dma_route_t route)
 	/* XXX Not checking resource attributes */
 	printk(KERN_ERR "No VME resource Attribute tests done\n");
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		goto err_bus;
@@ -1042,13 +1039,13 @@ void vme_irq_handler(struct vme_bridge *bridge, int level, int statid)
 }
 EXPORT_SYMBOL(vme_irq_handler);
 
-int vme_irq_request(struct device *dev, int level, int statid,
+int vme_irq_request(struct vme_dev *vdev, int level, int statid,
 	void (*callback)(int, int, void *),
 	void *priv_data)
 {
 	struct vme_bridge *bridge;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		return -EINVAL;
@@ -1085,11 +1082,11 @@ int vme_irq_request(struct device *dev, int level, int statid,
 }
 EXPORT_SYMBOL(vme_irq_request);
 
-void vme_irq_free(struct device *dev, int level, int statid)
+void vme_irq_free(struct vme_dev *vdev, int level, int statid)
 {
 	struct vme_bridge *bridge;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		return;
@@ -1120,11 +1117,11 @@ void vme_irq_free(struct device *dev, int level, int statid)
 }
 EXPORT_SYMBOL(vme_irq_free);
 
-int vme_irq_generate(struct device *dev, int level, int statid)
+int vme_irq_generate(struct vme_dev *vdev, int level, int statid)
 {
 	struct vme_bridge *bridge;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		return -EINVAL;
@@ -1147,7 +1144,7 @@ EXPORT_SYMBOL(vme_irq_generate);
 /*
  * Request the location monitor, return resource or NULL
  */
-struct vme_resource *vme_lm_request(struct device *dev)
+struct vme_resource *vme_lm_request(struct vme_dev *vdev)
 {
 	struct vme_bridge *bridge;
 	struct list_head *lm_pos = NULL;
@@ -1155,7 +1152,7 @@ struct vme_resource *vme_lm_request(struct device *dev)
 	struct vme_lm_resource *lm = NULL;
 	struct vme_resource *resource = NULL;
 
-	bridge = dev_to_bridge(dev);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		goto err_bus;
@@ -1336,11 +1333,11 @@ void vme_lm_free(struct vme_resource *resource)
 }
 EXPORT_SYMBOL(vme_lm_free);
 
-int vme_slot_get(struct device *bus)
+int vme_slot_get(struct vme_dev *vdev)
 {
 	struct vme_bridge *bridge;
 
-	bridge = dev_to_bridge(bus);
+	bridge = vdev->bridge;
 	if (bridge == NULL) {
 		printk(KERN_ERR "Can't find VME bus\n");
 		return -EINVAL;
@@ -1412,7 +1409,7 @@ static void vme_remove_bus(struct vme_bridge *bridge)
 
 int vme_register_bridge(struct vme_bridge *bridge)
 {
-	struct device *dev;
+	struct vme_dev *vdev;
 	int retval;
 	int i;
 
@@ -1425,20 +1422,23 @@ int vme_register_bridge(struct vme_bridge *bridge)
 	 * specification.
 	 */
 	for (i = 0; i < VME_SLOTS_MAX; i++) {
-		dev = &bridge->dev[i];
-		memset(dev, 0, sizeof(struct device));
-
-		dev->parent = bridge->parent;
-		dev->bus = &vme_bus_type;
+		vdev = &bridge->dev[i];
+		memset(vdev, 0, sizeof(struct vme_dev));
+
+		vdev->id.bus = bridge->num;
+		vdev->id.slot = i + 1;
+		vdev->bridge = bridge;
+		vdev->dev.parent = bridge->parent;
+		vdev->dev.bus = &vme_bus_type;
 		/*
 		 * We save a pointer to the bridge in platform_data so that we
 		 * can get to it later. We keep driver_data for use by the
 		 * driver that binds against the slot
 		 */
-		dev->platform_data = bridge;
-		dev_set_name(dev, "vme-%x.%x", bridge->num, i + 1);
+		vdev->dev.platform_data = bridge;
+		dev_set_name(&vdev->dev, "vme-%x.%x", bridge->num, i + 1);
 
-		retval = device_register(dev);
+		retval = device_register(&vdev->dev);
 		if (retval)
 			goto err_reg;
 	}
@@ -1447,8 +1447,8 @@ int vme_register_bridge(struct vme_bridge *bridge)
 
 err_reg:
 	while (--i >= 0) {
-		dev = &bridge->dev[i];
-		device_unregister(dev);
+		vdev = &bridge->dev[i];
+		device_unregister(&vdev->dev);
 	}
 	vme_remove_bus(bridge);
 	return retval;
@@ -1458,12 +1458,12 @@ EXPORT_SYMBOL(vme_register_bridge);
 void vme_unregister_bridge(struct vme_bridge *bridge)
 {
 	int i;
-	struct device *dev;
+	struct vme_dev *vdev;
 
 
 	for (i = 0; i < VME_SLOTS_MAX; i++) {
-		dev = &bridge->dev[i];
-		device_unregister(dev);
+		vdev = &bridge->dev[i];
+		device_unregister(&vdev->dev);
 	}
 	vme_remove_bus(bridge);
 }
@@ -1489,32 +1489,6 @@ EXPORT_SYMBOL(vme_unregister_driver);
 
 /* - Bus Registration ------------------------------------------------------ */
 
-static int vme_calc_slot(struct device *dev)
-{
-	struct vme_bridge *bridge;
-	int num;
-
-	bridge = dev_to_bridge(dev);
-
-	/* Determine slot number */
-	num = 0;
-	while (num < VME_SLOTS_MAX) {
-		if (&bridge->dev[num] == dev)
-			break;
-
-		num++;
-	}
-	if (num == VME_SLOTS_MAX) {
-		dev_err(dev, "Failed to identify slot\n");
-		num = 0;
-		goto err_dev;
-	}
-	num++;
-
-err_dev:
-	return num;
-}
-
 static struct vme_driver *dev_to_vme_driver(struct device *dev)
 {
 	if (dev->driver == NULL)
@@ -1525,15 +1499,17 @@ static struct vme_driver *dev_to_vme_driver(struct device *dev)
 
 static int vme_bus_match(struct device *dev, struct device_driver *drv)
 {
+	struct vme_dev *vdev;
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
 	int i, num;
 
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 	driver = container_of(drv, struct vme_driver, driver);
 
-	num = vme_calc_slot(dev);
-	if (!num)
+	num = vdev->id.slot;
+	if (num <= 0 || num >= VME_SLOTS_MAX)
 		goto err_dev;
 
 	if (driver->bind_table == NULL) {
@@ -1553,7 +1529,7 @@ static int vme_bus_match(struct device *dev, struct device_driver *drv)
 				return 1;
 
 			if ((driver->bind_table[i].slot == VME_SLOT_CURRENT) &&
-				(num == vme_slot_get(dev)))
+				(num == vme_slot_get(vdev)))
 				return 1;
 		}
 		i++;
@@ -1568,14 +1544,16 @@ static int vme_bus_probe(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = -ENODEV;
 
 	driver = dev_to_vme_driver(dev);
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 
 	vme_bridge_get(bridge->num);
 	if (driver->probe != NULL)
-		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->probe(vdev);
 
 	if (retval)
 		vme_bridge_put(bridge);
@@ -1587,13 +1565,15 @@ static int vme_bus_remove(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = -ENODEV;
 
 	driver = dev_to_vme_driver(dev);
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 
 	if (driver->remove != NULL)
-		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->remove(vdev);
 
 	vme_bridge_put(bridge);
 
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 8fb35e2..356b06e 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -93,17 +93,34 @@ extern struct bus_type vme_bus_type;
 #define VME_SLOT_CURRENT	-1
 #define VME_SLOT_ALL		-2
 
+/**
+ * VME device identifier structure
+ * @bus: The bus ID of the bus the device is on
+ * @slot: The slot this device is plugged into
+ */
 struct vme_device_id {
 	int bus;
 	int slot;
 };
 
+/**
+ * Structure representing a VME device
+ * @id: The ID of the device (currently the bus and slot number)
+ * @bridge: Pointer to the bridge device this device is on
+ * @dev: Internal device structure
+ */
+struct vme_dev {
+	struct vme_device_id id;
+	struct vme_bridge *bridge;
+	struct device dev;
+};
+
 struct vme_driver {
 	struct list_head node;
 	const char *name;
 	const struct vme_device_id *bind_table;
-	int (*probe)  (struct device *, int, int);
-	int (*remove) (struct device *, int, int);
+	int (*probe)  (struct vme_dev *);
+	int (*remove) (struct vme_dev *);
 	void (*shutdown) (void);
 	struct device_driver    driver;
 };
@@ -114,7 +131,7 @@ void vme_free_consistent(struct vme_resource *, size_t,  void *,
 
 size_t vme_get_size(struct vme_resource *);
 
-struct vme_resource *vme_slave_request(struct device *, vme_address_t,
+struct vme_resource *vme_slave_request(struct vme_dev *, vme_address_t,
 	vme_cycle_t);
 int vme_slave_set(struct vme_resource *, int, unsigned long long,
 	unsigned long long, dma_addr_t, vme_address_t, vme_cycle_t);
@@ -122,7 +139,7 @@ int vme_slave_get(struct vme_resource *, int *, unsigned long long *,
 	unsigned long long *, dma_addr_t *, vme_address_t *, vme_cycle_t *);
 void vme_slave_free(struct vme_resource *);
 
-struct vme_resource *vme_master_request(struct device *, vme_address_t,
+struct vme_resource *vme_master_request(struct vme_dev *, vme_address_t,
 	vme_cycle_t, vme_width_t);
 int vme_master_set(struct vme_resource *, int, unsigned long long,
 	unsigned long long, vme_address_t, vme_cycle_t, vme_width_t);
@@ -134,7 +151,7 @@ unsigned int vme_master_rmw(struct vme_resource *, unsigned int, unsigned int,
 	unsigned int, loff_t);
 void vme_master_free(struct vme_resource *);
 
-struct vme_resource *vme_dma_request(struct device *, vme_dma_route_t);
+struct vme_resource *vme_dma_request(struct vme_dev *, vme_dma_route_t);
 struct vme_dma_list *vme_new_dma_list(struct vme_resource *);
 struct vme_dma_attr *vme_dma_pattern_attribute(u32, vme_pattern_t);
 struct vme_dma_attr *vme_dma_pci_attribute(dma_addr_t);
@@ -147,12 +164,12 @@ int vme_dma_list_exec(struct vme_dma_list *);
 int vme_dma_list_free(struct vme_dma_list *);
 int vme_dma_free(struct vme_resource *);
 
-int vme_irq_request(struct device *, int, int,
+int vme_irq_request(struct vme_dev *, int, int,
 	void (*callback)(int, int, void *), void *);
-void vme_irq_free(struct device *, int, int);
-int vme_irq_generate(struct device *, int, int);
+void vme_irq_free(struct vme_dev *, int, int);
+int vme_irq_generate(struct vme_dev *, int, int);
 
-struct vme_resource * vme_lm_request(struct device *);
+struct vme_resource * vme_lm_request(struct vme_dev *);
 int vme_lm_count(struct vme_resource *);
 int vme_lm_set(struct vme_resource *, unsigned long long, vme_address_t,
 	vme_cycle_t);
@@ -162,7 +179,7 @@ int vme_lm_attach(struct vme_resource *, int, void (*callback)(int));
 int vme_lm_detach(struct vme_resource *, int);
 void vme_lm_free(struct vme_resource *);
 
-int vme_slot_get(struct device *);
+int vme_slot_get(struct vme_dev *);
 
 int vme_register_driver(struct vme_driver *);
 void vme_unregister_driver(struct vme_driver *);
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index ef751a4..74d0103 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -115,9 +115,8 @@ struct vme_bridge {
 	struct list_head bus_list; /* list of VME buses */
 	struct module *owner;	/* module that owns the bridge */
 
-	struct device dev[VME_SLOTS_MAX];	/* Device registered with
-						 * device model on VME bus
-						 */
+	struct vme_dev dev[VME_SLOTS_MAX];	/* Device registered
+						 * on VME bus */
 
 	/* Interrupt callbacks */
 	struct vme_irq irq[7];
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support
  2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
                   ` (4 preceding siblings ...)
  2011-08-10  9:33 ` [PATCH 5/6] staging: vme: add struct vme_dev for VME devices Manohar Vanga
@ 2011-08-10  9:33 ` Manohar Vanga
  2011-08-10 10:18   ` Martyn Welch
  5 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10  9:33 UTC (permalink / raw)
  To: martyn.welch; +Cc: gregkh, cota, devel, linux-kernel, Manohar Vanga

For jumper based boards (non VME64x), there is no mechanism
for detecting the card that is plugged into a specific slot. This
leads to issues in non-autodiscovery crates/cards when a card is
plugged into a slot that is "claimed" by a different driver. In
reality, there is no problem, but the driver rejects such a
configuration due to its dependence on the concept of slots.

This patch makes the concept of slots less critical and pushes the
driver match() to individual drivers (similar to what happens in the
ISA bus in driver/base/isa.c). This allows drivers to register the
number of devices that they expect without any restrictions. Devices
in this new model are now formatted as $driver_name-$bus_id.$device_id
(as compared to the earlier vme-$bus_id.$slot_number).

This model also makes the device model more logical as devices
are only registered when they actually exist whereas earlier,
a set of devices were being created automatically regardless of
them actually being there.

Another change introduced in this patch is that devices are now created
within the VME driver structure rather than in the VME bridge structure.
This way, things don't go haywire if the bridge driver is removed while
a driver is using it (this is also additionally prevented by having
reference counting of used bridge modules).

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/devices/vme_user.c |   25 +++-
 drivers/staging/vme/devices/vme_user.h |    2 +-
 drivers/staging/vme/vme.c              |  220 ++++++++++++++++----------------
 drivers/staging/vme/vme.h              |   19 ++-
 drivers/staging/vme/vme_bridge.h       |    4 -
 5 files changed, 147 insertions(+), 123 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index bb33dc2..41a82a1 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -43,7 +43,7 @@
 static DEFINE_MUTEX(vme_user_mutex);
 static const char driver_name[] = "vme_user";
 
-static int bus[USER_BUS_MAX];
+static int bus[VME_USER_BUS_MAX];
 static unsigned int bus_num;
 
 /* Currently Documentation/devices.txt defines the following for VME:
@@ -135,6 +135,7 @@ static ssize_t vme_user_write(struct file *, const char __user *, size_t,
 static loff_t vme_user_llseek(struct file *, loff_t, int);
 static long vme_user_unlocked_ioctl(struct file *, unsigned int, unsigned long);
 
+static int vme_user_match(struct vme_dev *);
 static int __devinit vme_user_probe(struct vme_dev *);
 static int __devexit vme_user_remove(struct vme_dev *);
 
@@ -620,6 +621,7 @@ static void buf_unalloc(int num)
 
 static struct vme_driver vme_user_driver = {
 	.name = driver_name,
+	.match = vme_user_match,
 	.probe = vme_user_probe,
 	.remove = __devexit_p(vme_user_remove),
 };
@@ -643,10 +645,10 @@ static int __init vme_user_init(void)
 	/* Let's start by supporting one bus, we can support more than one
 	 * in future revisions if that ever becomes necessary.
 	 */
-	if (bus_num > USER_BUS_MAX) {
+	if (bus_num > VME_USER_BUS_MAX) {
 		printk(KERN_ERR "%s: Driver only able to handle %d buses\n",
-			driver_name, USER_BUS_MAX);
-		bus_num = USER_BUS_MAX;
+			driver_name, VME_USER_BUS_MAX);
+		bus_num = VME_USER_BUS_MAX;
 	}
 
 
@@ -671,7 +673,13 @@ static int __init vme_user_init(void)
 
 	vme_user_driver.bind_table = ids;
 
-	retval = vme_register_driver(&vme_user_driver);
+	/*
+	 * Here we just register the maximum number of devices we can and
+	 * leave vme_user_match() to allow only 1 to go through to probe().
+	 * This way, if we later want to allow multiple user access devices,
+	 * we just change the code in vme_user_match().
+	 */
+	retval = vme_register_driver(&vme_user_driver, VME_MAX_SLOTS);
 	if (retval != 0)
 		goto err_reg;
 
@@ -684,6 +692,13 @@ err_nocard:
 	return retval;
 }
 
+static int vme_user_match(struct vme_dev *vdev)
+{
+	if (vdev->id.num >= VME_USER_BUS_MAX)
+		return 0;
+	return 1;
+}
+
 /*
  * In this simple access driver, the old behaviour is being preserved as much
  * as practical. We will therefore reserve the buffers and request the images
diff --git a/drivers/staging/vme/devices/vme_user.h b/drivers/staging/vme/devices/vme_user.h
index 24bf4e5..d85a1e9 100644
--- a/drivers/staging/vme/devices/vme_user.h
+++ b/drivers/staging/vme/devices/vme_user.h
@@ -1,7 +1,7 @@
 #ifndef _VME_USER_H_
 #define _VME_USER_H_
 
-#define USER_BUS_MAX                  1
+#define VME_USER_BUS_MAX	1
 
 /*
  * VMEbus Master Window Configuration Structure
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 360acc9..99d1505 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -42,10 +42,7 @@ static DEFINE_MUTEX(vme_buses_lock);
 static void __exit vme_exit(void);
 static int __init vme_init(void);
 
-static struct vme_dev *dev_to_vme_dev(struct device *dev)
-{
-	return container_of(dev, struct vme_dev, dev);
-}
+#define dev_to_vme_dev(dev) container_of(dev, struct vme_dev, dev)
 
 /*
  * Increments the reference count of a VME bridge.
@@ -1409,150 +1406,160 @@ static void vme_remove_bus(struct vme_bridge *bridge)
 
 int vme_register_bridge(struct vme_bridge *bridge)
 {
-	struct vme_dev *vdev;
-	int retval;
-	int i;
+	return vme_add_bus(bridge);
+}
+EXPORT_SYMBOL(vme_register_bridge);
 
-	retval = vme_add_bus(bridge);
-	if (retval)
-		return retval;
+void vme_unregister_bridge(struct vme_bridge *bridge)
+{
+	vme_remove_bus(bridge);
+}
+EXPORT_SYMBOL(vme_unregister_bridge);
 
-	/* This creates 32 vme "slot" devices. This equates to a slot for each
-	 * ID available in a system conforming to the ANSI/VITA 1-1994
-	 * specification.
-	 */
-	for (i = 0; i < VME_SLOTS_MAX; i++) {
-		vdev = &bridge->dev[i];
-		memset(vdev, 0, sizeof(struct vme_dev));
 
+/* - Driver Registration --------------------------------------------------- */
+
+static void vme_dev_release(struct device *dev)
+{
+	kfree(dev_to_vme_dev(dev));
+}
+
+static int __vme_register_driver_bus(struct vme_driver *drv,
+	struct vme_bridge *bridge, unsigned int ndevs)
+{
+	int err;
+	unsigned int i;
+	struct vme_dev *vdev;
+	struct vme_dev *tmp;
+
+	for (i = 0; i < ndevs; i++) {
+		vdev = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
+		if (!vdev) {
+			err = -ENOMEM;
+			goto err_alloc;
+		}
+		vdev->id.num = i;
 		vdev->id.bus = bridge->num;
 		vdev->id.slot = i + 1;
 		vdev->bridge = bridge;
+		vdev->dev.platform_data = drv;
+		vdev->dev.release = vme_dev_release;
 		vdev->dev.parent = bridge->parent;
 		vdev->dev.bus = &vme_bus_type;
-		/*
-		 * We save a pointer to the bridge in platform_data so that we
-		 * can get to it later. We keep driver_data for use by the
-		 * driver that binds against the slot
-		 */
-		vdev->dev.platform_data = bridge;
-		dev_set_name(&vdev->dev, "vme-%x.%x", bridge->num, i + 1);
-
-		retval = device_register(&vdev->dev);
-		if (retval)
-			goto err_reg;
+		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
+			vdev->id.num);
+
+		err = device_register(&vdev->dev);
+		if (err)
+			goto err_dev_reg;
+
+		if (vdev->dev.platform_data) {
+			list_add_tail(&vdev->list, &drv->devices);
+			drv->ndev++;
+		} else {
+			device_unregister(&vdev->dev);
+			kfree(vdev);
+		}
 	}
+	return 0;
 
-	return retval;
-
-err_reg:
-	while (--i >= 0) {
-		vdev = &bridge->dev[i];
+err_dev_reg:
+	kfree(vdev);
+err_alloc:
+	list_for_each_entry_safe(vdev, tmp, &drv->devices, list) {
+		list_del(&vdev->list);
 		device_unregister(&vdev->dev);
 	}
-	vme_remove_bus(bridge);
-	return retval;
+	return err;
 }
-EXPORT_SYMBOL(vme_register_bridge);
 
-void vme_unregister_bridge(struct vme_bridge *bridge)
+static int __vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
 {
-	int i;
-	struct vme_dev *vdev;
-
+	struct vme_bridge *bridge;
+	int err = 0;
 
-	for (i = 0; i < VME_SLOTS_MAX; i++) {
-		vdev = &bridge->dev[i];
-		device_unregister(&vdev->dev);
+	mutex_lock(&vme_buses_lock);
+	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
+		/*
+		 * We increase the refcount of the bridge module here to
+		 * prevent it from being removed during driver registration
+		 */
+		if (!vme_bridge_get(bridge->num))
+			continue;
+		mutex_unlock(&vme_buses_lock);
+		err = __vme_register_driver_bus(drv, bridge, ndevs);
+		mutex_lock(&vme_buses_lock);
+		vme_bridge_put(bridge);
+		if (err)
+			break;
 	}
-	vme_remove_bus(bridge);
+	mutex_unlock(&vme_buses_lock);
+	return err;
 }
-EXPORT_SYMBOL(vme_unregister_bridge);
 
-
-/* - Driver Registration --------------------------------------------------- */
-
-int vme_register_driver(struct vme_driver *drv)
+int vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
 {
+	int err;
+
 	drv->driver.name = drv->name;
 	drv->driver.bus = &vme_bus_type;
-
-	return driver_register(&drv->driver);
+	INIT_LIST_HEAD(&drv->devices);
+
+	err = driver_register(&drv->driver);
+	if (err)
+		return err;
+
+	err = __vme_register_driver(drv, ndevs);
+	if (!err & (drv->ndev == 0))
+		err = -ENODEV;
+	if (err)
+		vme_unregister_driver(drv);
+	return err;
 }
 EXPORT_SYMBOL(vme_register_driver);
 
 void vme_unregister_driver(struct vme_driver *drv)
 {
+	struct vme_dev *dev, *dev_tmp;
+
+	list_for_each_entry_safe(dev, dev_tmp, &drv->devices, list) {
+		device_unregister(&dev->dev);
+		list_del(&dev->list);
+	}
 	driver_unregister(&drv->driver);
 }
 EXPORT_SYMBOL(vme_unregister_driver);
 
 /* - Bus Registration ------------------------------------------------------ */
 
-static struct vme_driver *dev_to_vme_driver(struct device *dev)
-{
-	if (dev->driver == NULL)
-		printk(KERN_ERR "Bugger dev->driver is NULL\n");
-
-	return container_of(dev->driver, struct vme_driver, driver);
-}
-
 static int vme_bus_match(struct device *dev, struct device_driver *drv)
 {
-	struct vme_dev *vdev;
-	struct vme_bridge *bridge;
-	struct vme_driver *driver;
-	int i, num;
-
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
-	driver = container_of(drv, struct vme_driver, driver);
-
-	num = vdev->id.slot;
-	if (num <= 0 || num >= VME_SLOTS_MAX)
-		goto err_dev;
+	struct vme_driver *vme_drv;
 
-	if (driver->bind_table == NULL) {
-		dev_err(dev, "Bind table NULL\n");
-		goto err_table;
-	}
-
-	i = 0;
-	while ((driver->bind_table[i].bus != 0) ||
-		(driver->bind_table[i].slot != 0)) {
+	vme_drv = container_of(drv, struct vme_driver, driver);
 
-		if (bridge->num == driver->bind_table[i].bus) {
-			if (num == driver->bind_table[i].slot)
-				return 1;
+	if (dev->platform_data == vme_drv) {
+		struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-			if (driver->bind_table[i].slot == VME_SLOT_ALL)
-				return 1;
+		if (vme_drv->match && vme_drv->match(vdev))
+			return 1;
 
-			if ((driver->bind_table[i].slot == VME_SLOT_CURRENT) &&
-				(num == vme_slot_get(vdev)))
-				return 1;
-		}
-		i++;
+		dev->platform_data = NULL;
 	}
-
-err_dev:
-err_table:
 	return 0;
 }
 
 static int vme_bus_probe(struct device *dev)
 {
-	struct vme_bridge *bridge;
+	int retval = 0;
 	struct vme_driver *driver;
-	struct vme_dev *vdev;
-	int retval = -ENODEV;
-
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
+	struct vme_bridge *bridge = vdev->bridge;
 
 	vme_bridge_get(bridge->num);
-	if (driver->probe != NULL)
+
+	driver = dev->platform_data;
+	if (driver->probe)
 		retval = driver->probe(vdev);
 
 	if (retval)
@@ -1563,16 +1570,13 @@ static int vme_bus_probe(struct device *dev)
 
 static int vme_bus_remove(struct device *dev)
 {
-	struct vme_bridge *bridge;
+	int retval = 0;
 	struct vme_driver *driver;
-	struct vme_dev *vdev;
-	int retval = -ENODEV;
-
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
+	struct vme_bridge *bridge = vdev->bridge;
 
-	if (driver->remove != NULL)
+	driver = dev->platform_data;
+	if (driver->remove)
 		retval = driver->remove(vdev);
 
 	vme_bridge_put(bridge);
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 356b06e..80ade64 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -90,15 +90,19 @@ extern struct bus_type vme_bus_type;
 
 /* VME_MAX_BRIDGES comes from the type of vme_bus_numbers */
 #define VME_MAX_BRIDGES		(sizeof(unsigned int)*8)
+#define VME_MAX_SLOTS		32
+
 #define VME_SLOT_CURRENT	-1
 #define VME_SLOT_ALL		-2
 
 /**
  * VME device identifier structure
+ * @num: The device ID (ranges from 0 to N-1 for N devices)
  * @bus: The bus ID of the bus the device is on
  * @slot: The slot this device is plugged into
  */
 struct vme_device_id {
+	int num;
 	int bus;
 	int slot;
 };
@@ -108,21 +112,26 @@ struct vme_device_id {
  * @id: The ID of the device (currently the bus and slot number)
  * @bridge: Pointer to the bridge device this device is on
  * @dev: Internal device structure
+ * @list: List of devices (per driver)
  */
 struct vme_dev {
 	struct vme_device_id id;
 	struct vme_bridge *bridge;
 	struct device dev;
+	struct list_head list;
 };
 
 struct vme_driver {
 	struct list_head node;
 	const char *name;
 	const struct vme_device_id *bind_table;
-	int (*probe)  (struct vme_dev *);
-	int (*remove) (struct vme_dev *);
-	void (*shutdown) (void);
-	struct device_driver    driver;
+	int (*match)(struct vme_dev *);
+	int (*probe)(struct vme_dev *);
+	int (*remove)(struct vme_dev *);
+	void (*shutdown)(void);
+	struct device_driver driver;
+	struct list_head devices;
+	unsigned int ndev;
 };
 
 void *vme_alloc_consistent(struct vme_resource *, size_t, dma_addr_t *);
@@ -181,7 +190,7 @@ void vme_lm_free(struct vme_resource *);
 
 int vme_slot_get(struct vme_dev *);
 
-int vme_register_driver(struct vme_driver *);
+int vme_register_driver(struct vme_driver *, unsigned int);
 void vme_unregister_driver(struct vme_driver *);
 
 struct vme_bridge *vme_bridge_get(unsigned int bus_id);
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index 74d0103..24cb4b8 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -2,7 +2,6 @@
 #define _VME_BRIDGE_H_
 
 #define VME_CRCSR_BUF_SIZE (508*1024)
-#define VME_SLOTS_MAX 32
 /*
  * Resource structures
  */
@@ -115,9 +114,6 @@ struct vme_bridge {
 	struct list_head bus_list; /* list of VME buses */
 	struct module *owner;	/* module that owns the bridge */
 
-	struct vme_dev dev[VME_SLOTS_MAX];	/* Device registered
-						 * on VME bus */
-
 	/* Interrupt callbacks */
 	struct vme_irq irq[7];
 	/* Locking for VME irq callback configuration */
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers
  2011-08-10  9:33 ` [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers Manohar Vanga
@ 2011-08-10 10:02   ` Martyn Welch
  2011-08-10 10:41     ` Manohar Vanga
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:02 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel

On 10/08/11 10:33, Manohar Vanga wrote:
> From: Emilio G. Cota <cota@braap.org>
> 
> In a configuration with several bridges, each bridge is
> assigned a certain bus number depending on the order in which
> bridge modules are loaded. This can complicate multi-bridge
> installations because the bus numbers will depend on the load
> order of the bridges.
> 
> This patch allows bridges to register with a bus number of
> their choice, while keeping the previous 'first come, first
> serve' behaviour as the default.
> 
> Module parameters have been added to the bridge drivers to
> allow overriding of the auto-assignment during load time.
> 
> This patch also explicitly defines VME_MAX_BRIDGES as the,
> size of vme_bus_numbers (unsigned int); something that was
> implicit earlier in the code.
> 

I'm sorry, I'm still simply not convinced by this patch:

1) For a single bus driver (i.e. in the situation where we have 2 bridges of
the same type), the numbering of the buses is still dependent on the order
that they are found in the scan.

2) If the bridge drivers are loaded as modules, I have a feeling they will be
loaded sequentially and therefore the order of the bridges would only change
if the order of the loading of the drivers changed.

3) If the modules are built into the kernel, I believe the drivers will
already be available at PCI scan time, thus we end up with the same problem as
(1).

Martyn

> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/bridges/vme_ca91cx42.c |   20 ++++++++++++
>  drivers/staging/vme/bridges/vme_tsi148.c   |   21 +++++++++++++
>  drivers/staging/vme/vme.c                  |   46 ++++++++++++++++++++++-----
>  drivers/staging/vme/vme.h                  |    2 +
>  4 files changed, 80 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index 5122c13..c378819 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -41,6 +41,13 @@ static void __exit ca91cx42_exit(void);
>  
>  /* Module parameters */
>  static int geoid;
> +static int bus_ids[VME_MAX_BRIDGES];
> +static int id_count;
> +
> +/* The number of registered buses */
> +static int bus_count;
> +/* Mutex for protecting ID access */
> +static DEFINE_MUTEX(bus_id_mutex);
>  
>  static const char driver_name[] = "vme_ca91cx42";
>  
> @@ -1779,6 +1786,12 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (ca91cx42_crcsr_init(ca91cx42_bridge, pdev))
>  		dev_err(&pdev->dev, "CR/CSR configuration failed.\n");
>  
> +	mutex_lock(&bus_id_mutex);
> +	if (bus_count >= id_count)
> +		ca91cx42_bridge->num = -1;
> +	else
> +		ca91cx42_bridge->num = bus_ids[bus_count];
> +
>  	/* Need to save ca91cx42_bridge pointer locally in link list for use in
>  	 * ca91cx42_remove()
>  	 */
> @@ -1788,11 +1801,15 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_reg;
>  	}
>  
> +	bus_count++;
> +	mutex_unlock(&bus_id_mutex);
> +
>  	pci_set_drvdata(pdev, ca91cx42_bridge);
>  
>  	return 0;
>  
>  err_reg:
> +	mutex_unlock(&bus_id_mutex);
>  	ca91cx42_crcsr_exit(ca91cx42_bridge, pdev);
>  err_lm:
>  	/* resources are stored in link list */
> @@ -1930,5 +1947,8 @@ module_param(geoid, int, 0);
>  MODULE_DESCRIPTION("VME driver for the Tundra Universe II VME bridge");
>  MODULE_LICENSE("GPL");
>  
> +MODULE_PARM_DESC(bus_num, "Explicitly set bus number (override auto-assign)");
> +module_param_array(bus_ids, int, &id_count, 0);
> +
>  module_init(ca91cx42_init);
>  module_exit(ca91cx42_exit);
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index 9c53951..e3f021e 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -44,6 +44,14 @@ static void __exit tsi148_exit(void);
>  static int err_chk;
>  static int geoid;
>  
> +static int bus_ids[VME_MAX_BRIDGES];
> +static int id_count;
> +
> +/* The number of registered buses */
> +static int bus_count;
> +/* Mutex for protecting ID access */
> +static DEFINE_MUTEX(bus_id_mutex);
> +
>  static const char driver_name[] = "vme_tsi148";
>  
>  static DEFINE_PCI_DEVICE_TABLE(tsi148_ids) = {
> @@ -2462,12 +2470,21 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_crcsr;
>  	}
>  
> +	mutex_lock(&bus_id_mutex);
> +	if (bus_count >= id_count)
> +		tsi148_bridge->num = -1;
> +	else
> +		tsi148_bridge->num = bus_ids[bus_count];
> +
>  	retval = vme_register_bridge(tsi148_bridge);
>  	if (retval != 0) {
>  		dev_err(&pdev->dev, "Chip Registration failed.\n");
>  		goto err_reg;
>  	}
>  
> +	bus_count++;
> +	mutex_unlock(&bus_id_mutex);
> +
>  	pci_set_drvdata(pdev, tsi148_bridge);
>  
>  	/* Clear VME bus "board fail", and "power-up reset" lines */
> @@ -2479,6 +2496,7 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return 0;
>  
>  err_reg:
> +	mutex_unlock(&bus_id_mutex);
>  	tsi148_crcsr_exit(tsi148_bridge, pdev);
>  err_crcsr:
>  err_lm:
> @@ -2633,6 +2651,9 @@ module_param(err_chk, bool, 0);
>  MODULE_PARM_DESC(geoid, "Override geographical addressing");
>  module_param(geoid, int, 0);
>  
> +MODULE_PARM_DESC(bus_ids, "Explicitly set bus number (override auto-assign)");
> +module_param_array(bus_ids, int, &id_count, 0);
> +
>  MODULE_DESCRIPTION("VME driver for the Tundra Tempe VME bridge");
>  MODULE_LICENSE("GPL");
>  
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index c078ce3..330a4ff 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1303,20 +1303,44 @@ EXPORT_SYMBOL(vme_slot_get);
>  
>  /* - Bridge Registration --------------------------------------------------- */
>  
> -static int vme_alloc_bus_num(void)
> +/* call with vme_bus_num_mtx held */
> +static int __vme_alloc_bus_num(int bus_num)
>  {
> +	int num;
>  	int i;
>  
> -	mutex_lock(&vme_bus_num_mtx);
> -	for (i = 0; i < sizeof(vme_bus_numbers) * 8; i++) {
> -		if (((vme_bus_numbers >> i) & 0x1) == 0) {
> -			vme_bus_numbers |= (0x1 << i);
> -			break;
> +	if (bus_num == -1) {
> +		/* try to find a free bus number */
> +		for (i = 0; i < VME_MAX_BRIDGES; i++) {
> +			if ((vme_bus_numbers & (1 << i)) == 0) {
> +				num = i;
> +				break;
> +			}
> +		}
> +		if (i == VME_MAX_BRIDGES) {
> +			pr_warn("vme: No bus numbers left\n");
> +			return -ENODEV;
>  		}
> +	} else {
> +		/* check if the given bus number is already in use */
> +		if (vme_bus_numbers & (1 << bus_num)) {
> +			pr_warn("vme: bus number %d already in use\n", bus_num);
> +			return -EBUSY;
> +		}
> +		num = bus_num;
>  	}
> -	mutex_unlock(&vme_bus_num_mtx);
> +	vme_bus_numbers |= 1 << num;
> +	return num;
> +}
>  
> -	return i;
> +static int vme_alloc_bus_num(int bus_num)
> +{
> +	int num;
> +
> +	mutex_lock(&vme_bus_num_mtx);
> +	num = __vme_alloc_bus_num(bus_num);
> +	mutex_unlock(&vme_bus_num_mtx);
> +	return num;
>  }
>  
>  static void vme_free_bus_num(int bus)
> @@ -1332,7 +1356,11 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  	int retval;
>  	int i;
>  
> -	bridge->num = vme_alloc_bus_num();
> +	retval = vme_alloc_bus_num(bridge->num);
> +	if (retval < 0)
> +		return retval;
> +
> +	bridge->num = retval;
>  
>  	/* This creates 32 vme "slot" devices. This equates to a slot for each
>  	 * ID available in a system conforming to the ANSI/VITA 1-1994
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 4155d8c..3ccb497 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -88,6 +88,8 @@ struct vme_resource {
>  
>  extern struct bus_type vme_bus_type;
>  
> +/* VME_MAX_BRIDGES comes from the type of vme_bus_numbers */
> +#define VME_MAX_BRIDGES		(sizeof(unsigned int)*8)
>  #define VME_SLOT_CURRENT	-1
>  #define VME_SLOT_ALL		-2
>  


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10  9:33 ` [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific Manohar Vanga
@ 2011-08-10 10:04   ` Martyn Welch
  2011-08-10 13:24     ` Dan Carpenter
  2011-08-10 13:12   ` Joe Perches
  1 sibling, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:04 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel, Dan Carpenter

On 10/08/11 10:33, Manohar Vanga wrote:
> Make PCI dependent functions ([alloc|free]_consistent() in
> 'vme.c') bridge specific. By removing the dependency of the
> VME bridge framework on PCI, this patch allows for addition of
> non-PCI based VME bridges.
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

This appears to be resolving the issue raised by Dan. Assuming he's happy with
this:

Acked-by: Martyn Welch <martyn.welch@ge.com>

> ---
>  drivers/staging/vme/bridges/vme_ca91cx42.c |   24 ++++++++++++++++++
>  drivers/staging/vme/bridges/vme_tsi148.c   |   24 ++++++++++++++++++
>  drivers/staging/vme/vme.c                  |   36 ++++++++++++++++-----------
>  drivers/staging/vme/vme_bridge.h           |   10 +++++--
>  4 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index c378819..15a0b19 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1507,6 +1507,28 @@ static int ca91cx42_slot_get(struct vme_bridge *ca91cx42_bridge)
>  
>  }
>  
> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
> +	dma_addr_t *dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	return pci_alloc_consistent(pdev, size, dma);
> +}
> +
> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
> +	dma_addr_t dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	pci_free_consistent(pdev, size, vaddr, dma);
> +}
> +
>  static int __init ca91cx42_init(void)
>  {
>  	return pci_register_driver(&ca91cx42_driver);
> @@ -1776,6 +1798,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ca91cx42_bridge->lm_attach = ca91cx42_lm_attach;
>  	ca91cx42_bridge->lm_detach = ca91cx42_lm_detach;
>  	ca91cx42_bridge->slot_get = ca91cx42_slot_get;
> +	ca91cx42_bridge->alloc_consistent = ca91cx42_alloc_consistent;
> +	ca91cx42_bridge->free_consistent = ca91cx42_free_consistent;
>  
>  	data = ioread32(ca91cx42_device->base + MISC_CTL);
>  	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index e3f021e..5c147d6 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
>  	return (int)slot;
>  }
>  
> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
> +	dma_addr_t *dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	return pci_alloc_consistent(pdev, size, dma);
> +}
> +
> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
> +	dma_addr_t dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	pci_free_consistent(pdev, size, vaddr, dma);
> +}
> +
>  static int __init tsi148_init(void)
>  {
>  	return pci_register_driver(&tsi148_driver);
> @@ -2451,6 +2473,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	tsi148_bridge->lm_attach = tsi148_lm_attach;
>  	tsi148_bridge->lm_detach = tsi148_lm_detach;
>  	tsi148_bridge->slot_get = tsi148_slot_get;
> +	tsi148_bridge->alloc_consistent = tsi148_alloc_consistent;
> +	tsi148_bridge->free_consistent = tsi148_free_consistent;
>  
>  	data = ioread32be(tsi148_device->base + TSI148_LCSR_VSTAT);
>  	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 330a4ff..15a6161 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -83,15 +83,11 @@ static struct vme_bridge *find_bridge(struct vme_resource *resource)
>  /*
>   * Allocate a contiguous block of memory for use by the driver. This is used to
>   * create the buffers for the slave windows.
> - *
> - * XXX VME bridges could be available on buses other than PCI. At the momment
> - *     this framework only supports PCI devices.
>   */
>  void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>  	dma_addr_t *dma)
>  {
>  	struct vme_bridge *bridge;
> -	struct pci_dev *pdev;
>  
>  	if (resource == NULL) {
>  		printk(KERN_ERR "No resource\n");
> @@ -104,28 +100,29 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>  		return NULL;
>  	}
>  
> -	/* Find pci_dev container of dev */
>  	if (bridge->parent == NULL) {
> -		printk(KERN_ERR "Dev entry NULL\n");
> +		printk(KERN_ERR "Dev entry NULL for"
> +			" bridge %s\n", bridge->name);
> +		return NULL;
> +	}
> +
> +	if (bridge->alloc_consistent == NULL) {
> +		printk(KERN_ERR "alloc_consistent not supported by"
> +			" bridge %s\n", bridge->name);
>  		return NULL;
>  	}
> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
>  
> -	return pci_alloc_consistent(pdev, size, dma);
> +	return bridge->alloc_consistent(bridge->parent, size, dma);
>  }
>  EXPORT_SYMBOL(vme_alloc_consistent);
>  
>  /*
>   * Free previously allocated contiguous block of memory.
> - *
> - * XXX VME bridges could be available on buses other than PCI. At the momment
> - *     this framework only supports PCI devices.
>   */
>  void vme_free_consistent(struct vme_resource *resource, size_t size,
>  	void *vaddr, dma_addr_t dma)
>  {
>  	struct vme_bridge *bridge;
> -	struct pci_dev *pdev;
>  
>  	if (resource == NULL) {
>  		printk(KERN_ERR "No resource\n");
> @@ -138,10 +135,19 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
>  		return;
>  	}
>  
> -	/* Find pci_dev container of dev */
> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
> +	if (bridge->parent == NULL) {
> +		printk(KERN_ERR "Dev entry NULL for"
> +			" bridge %s\n", bridge->name);
> +		return;
> +	}
> +
> +	if (bridge->free_consistent == NULL) {
> +		printk(KERN_ERR "free_consistent not supported by"
> +			" bridge %s\n", bridge->name);
> +		return;
> +	}
>  
> -	pci_free_consistent(pdev, size, vaddr, dma);
> +	bridge->free_consistent(bridge->parent, size, vaddr, dma);
>  }
>  EXPORT_SYMBOL(vme_free_consistent);
>  
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index 4c6ec31..a9084f0 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -98,8 +98,6 @@ struct vme_irq {
>  /* This structure stores all the information about one bridge
>   * The structure should be dynamically allocated by the driver and one instance
>   * of the structure should be present for each VME chip present in the system.
> - *
> - * Currently we assume that all chips are PCI-based
>   */
>  struct vme_bridge {
>  	char name[VMENAMSIZ];
> @@ -112,7 +110,7 @@ struct vme_bridge {
>  	struct list_head vme_errors;	/* List for errors generated on VME */
>  
>  	/* Bridge Info - XXX Move to private structure? */
> -	struct device *parent;	/* Generic device struct (pdev->dev for PCI) */
> +	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
>  	void *driver_priv;	/* Private pointer for the bridge driver */
>  
>  	struct device dev[VME_SLOTS_MAX];	/* Device registered with
> @@ -165,6 +163,12 @@ struct vme_bridge {
>  
>  	/* CR/CSR space functions */
>  	int (*slot_get) (struct vme_bridge *);
> +
> +	/* Bridge parent interface */
> +	void *(*alloc_consistent)(struct device *dev, size_t size,
> +		dma_addr_t *dma);
> +	void (*free_consistent)(struct device *dev, size_t size,
> +		void *vaddr, dma_addr_t dma);
>  };
>  
>  void vme_irq_handler(struct vme_bridge *, int, int);


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/6] staging: vme: keep track of registered buses
  2011-08-10  9:33 ` [PATCH 3/6] staging: vme: keep track of registered buses Manohar Vanga
@ 2011-08-10 10:06   ` Martyn Welch
  0 siblings, 0 replies; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:06 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel

On 10/08/11 10:33, Manohar Vanga wrote:
> This patch adds a list which keeps track of all registered VME
> buses. This is required for adding refcounting later to bridge
> modules, something that is not currently implemented.
> 
> This is based on the changes introduced by Emilio G. Cota in the
> patch:
> 
>     https://lkml.org/lkml/2010/10/25/486
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

Looks OK:

Acked-by: Martyn Welch <martyn.welch@ge.com>

> ---
>  drivers/staging/vme/vme.c        |   46 ++++++++++++++++++++-----------------
>  drivers/staging/vme/vme_bridge.h |    1 +
>  2 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 15a6161..f811d07 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -34,9 +34,10 @@
>  #include "vme.h"
>  #include "vme_bridge.h"
>  
> -/* Bitmask and mutex to keep track of bridge numbers */
> +/* Bitmask and list of registered buses both protected by common mutex */
>  static unsigned int vme_bus_numbers;
> -static DEFINE_MUTEX(vme_bus_num_mtx);
> +static LIST_HEAD(vme_bus_list);
> +static DEFINE_MUTEX(vme_buses_lock);
>  
>  static void __exit vme_exit(void);
>  static int __init vme_init(void);
> @@ -1309,11 +1310,12 @@ EXPORT_SYMBOL(vme_slot_get);
>  
>  /* - Bridge Registration --------------------------------------------------- */
>  
> -/* call with vme_bus_num_mtx held */
> -static int __vme_alloc_bus_num(int bus_num)
> +/* call with vme_buses_lock held */
> +static int __vme_add_bus(struct vme_bridge *bridge)
>  {
>  	int num;
>  	int i;
> +	int bus_num = bridge->num;
>  
>  	if (bus_num == -1) {
>  		/* try to find a free bus number */
> @@ -1335,25 +1337,29 @@ static int __vme_alloc_bus_num(int bus_num)
>  		}
>  		num = bus_num;
>  	}
> +	bridge->num = num;
> +	list_add_tail(&bridge->bus_list, &vme_bus_list);
>  	vme_bus_numbers |= 1 << num;
> -	return num;
> +	return 0;
>  }
>  
> -static int vme_alloc_bus_num(int bus_num)
> +static int vme_add_bus(struct vme_bridge *bridge)
>  {
> -	int num;
> +	int ret;
>  
> -	mutex_lock(&vme_bus_num_mtx);
> -	num = __vme_alloc_bus_num(bus_num);
> -	mutex_unlock(&vme_bus_num_mtx);
> -	return num;
> +	mutex_lock(&vme_buses_lock);
> +	ret = __vme_add_bus(bridge);
> +	mutex_unlock(&vme_buses_lock);
> +
> +	return ret;
>  }
>  
> -static void vme_free_bus_num(int bus)
> +static void vme_remove_bus(struct vme_bridge *bridge)
>  {
> -	mutex_lock(&vme_bus_num_mtx);
> -	vme_bus_numbers &= ~(0x1 << bus);
> -	mutex_unlock(&vme_bus_num_mtx);
> +	mutex_lock(&vme_buses_lock);
> +	vme_bus_numbers &= ~(1 << bridge->num);
> +	list_del(&bridge->bus_list);
> +	mutex_unlock(&vme_buses_lock);
>  }
>  
>  int vme_register_bridge(struct vme_bridge *bridge)
> @@ -1362,12 +1368,10 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  	int retval;
>  	int i;
>  
> -	retval = vme_alloc_bus_num(bridge->num);
> -	if (retval < 0)
> +	retval = vme_add_bus(bridge);
> +	if (retval)
>  		return retval;
>  
> -	bridge->num = retval;
> -
>  	/* This creates 32 vme "slot" devices. This equates to a slot for each
>  	 * ID available in a system conforming to the ANSI/VITA 1-1994
>  	 * specification.
> @@ -1398,7 +1402,7 @@ err_reg:
>  		dev = &bridge->dev[i];
>  		device_unregister(dev);
>  	}
> -	vme_free_bus_num(bridge->num);
> +	vme_remove_bus(bridge);
>  	return retval;
>  }
>  EXPORT_SYMBOL(vme_register_bridge);
> @@ -1413,7 +1417,7 @@ void vme_unregister_bridge(struct vme_bridge *bridge)
>  		dev = &bridge->dev[i];
>  		device_unregister(dev);
>  	}
> -	vme_free_bus_num(bridge->num);
> +	vme_remove_bus(bridge);
>  }
>  EXPORT_SYMBOL(vme_unregister_bridge);
>  
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index a9084f0..8959670 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -112,6 +112,7 @@ struct vme_bridge {
>  	/* Bridge Info - XXX Move to private structure? */
>  	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
>  	void *driver_priv;	/* Private pointer for the bridge driver */
> +	struct list_head bus_list; /* list of VME buses */
>  
>  	struct device dev[VME_SLOTS_MAX];	/* Device registered with
>  						 * device model on VME bus


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/6] staging: vme: add functions for bridge module refcounting
  2011-08-10  9:33 ` [PATCH 4/6] staging: vme: add functions for bridge module refcounting Manohar Vanga
@ 2011-08-10 10:09   ` Martyn Welch
  2011-08-10 19:14     ` Emilio G. Cota
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:09 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel

On 10/08/11 10:33, Manohar Vanga wrote:
> This patch adds functions that allow for reference counting
> bridge modules. The patch introduces the functions
> 'vme_bridge_get()' and 'vme_bridge_put()'.
> 
> The functions are automatically called during .probe and .remove
> for drivers.
> 
> This patch is based on the changes introduced by Emilio G. Cota
> in the patch:
> 
>     https://lkml.org/lkml/2010/10/25/492
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/bridges/vme_ca91cx42.c |    2 +
>  drivers/staging/vme/bridges/vme_tsi148.c   |    2 +
>  drivers/staging/vme/vme.c                  |   54 ++++++++++++++++++++++++++++
>  drivers/staging/vme/vme.h                  |    2 +
>  drivers/staging/vme/vme_bridge.h           |    1 +
>  5 files changed, 61 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index 15a0b19..2f9c22b 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1680,6 +1680,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ca91cx42_bridge->parent = &pdev->dev;
>  	strcpy(ca91cx42_bridge->name, driver_name);
>  
> +	ca91cx42_bridge->owner = THIS_MODULE;
> +
>  	/* Setup IRQ */
>  	retval = ca91cx42_irq_init(ca91cx42_bridge);
>  	if (retval != 0) {
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index 5c147d6..9db89dc 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2320,6 +2320,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	tsi148_bridge->parent = &pdev->dev;
>  	strcpy(tsi148_bridge->name, driver_name);
>  
> +	tsi148_bridge->owner = THIS_MODULE;
> +
>  	/* Setup IRQ */
>  	retval = tsi148_irq_init(tsi148_bridge);
>  	if (retval != 0) {
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index f811d07..0504007 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -52,6 +52,54 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
>  }
>  
>  /*
> + * Increments the reference count of a VME bridge.
> + *
> + * If found, a pointer to the bridge is returned and the reference count
> + * of the module that owns it is incremented.
> + *
> + * On success, it can be assumed that the bridge won't be removed until
> + * the corresponding call to vme_put_bridge().
> + *
> + * Normally drivers should call vme_get_bridge() on a successfull .probe,
> + * and vme_put_bridge() when releasing the device, e.g. in .remove.
> + */
> +struct vme_bridge *vme_bridge_get(unsigned int bus_id)
> +{
> +	struct vme_bridge *bridge;
> +	struct vme_bridge *retp = NULL;
> +
> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		if (bridge->num == bus_id) {
> +			if (!bridge->owner)
> +				dev_warn(bridge->parent,
> +					"bridge->owner not set\n");
> +			else if (try_module_get(bridge->owner))
> +				retp = bridge;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&vme_buses_lock);
> +	return retp;
> +}
> +EXPORT_SYMBOL(vme_bridge_get);
> +
> +/*
> + * Decrements the reference count of a bridge
> + *
> + * This function decrements the reference count of the module that controls
> + * the bridge. It must come after a call to vme_get_bridge().
> + */
> +void vme_bridge_put(struct vme_bridge *bridge)
> +{
> +	if (!bridge->owner)
> +		dev_warn(bridge->parent, "bridge->owner not set\n");
> +	else
> +		module_put(bridge->owner);
> +}
> +EXPORT_SYMBOL(vme_bridge_put);
> +
> +/*
>   * Find the bridge that the resource is associated with.
>   */
>  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> @@ -1525,9 +1573,13 @@ static int vme_bus_probe(struct device *dev)
>  	driver = dev_to_vme_driver(dev);
>  	bridge = dev_to_bridge(dev);
>  
> +	vme_bridge_get(bridge->num);
>  	if (driver->probe != NULL)
>  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
>  
> +	if (retval)
> +		vme_bridge_put(bridge);
> +
>  	return retval;
>  }
>  
> @@ -1543,6 +1595,8 @@ static int vme_bus_remove(struct device *dev)
>  	if (driver->remove != NULL)
>  		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
>  
> +	vme_bridge_put(bridge);
> +
>  	return retval;
>  }
>  
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 3ccb497..8fb35e2 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -167,6 +167,8 @@ int vme_slot_get(struct device *);
>  int vme_register_driver(struct vme_driver *);
>  void vme_unregister_driver(struct vme_driver *);
>  
> +struct vme_bridge *vme_bridge_get(unsigned int bus_id);
> +void vme_bridge_put(struct vme_bridge *bridge);
>  

Given that we are recounting automatically in probe and remove, under what
circumstances would we need to call them separately?

Martyn

>  #endif /* _VME_H_ */
>  
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index 8959670..ef751a4 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -113,6 +113,7 @@ struct vme_bridge {
>  	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
>  	void *driver_priv;	/* Private pointer for the bridge driver */
>  	struct list_head bus_list; /* list of VME buses */
> +	struct module *owner;	/* module that owns the bridge */
>  
>  	struct device dev[VME_SLOTS_MAX];	/* Device registered with
>  						 * device model on VME bus


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/6] staging: vme: add struct vme_dev for VME devices
  2011-08-10  9:33 ` [PATCH 5/6] staging: vme: add struct vme_dev for VME devices Manohar Vanga
@ 2011-08-10 10:14   ` Martyn Welch
  2011-08-10 10:33     ` Manohar Vanga
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:14 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel

On 10/08/11 10:33, Manohar Vanga wrote:
> Instead of using a vanilla 'struct device' for VME devices, add new
> 'struct vme_dev'. Modifications have been made to the VME framework
> API as well as all in-tree VME drivers.
> 
> The new vme_dev structure has the following advantages from the
> current model used by the driver:
> 
>     * Driver functions (probe, remove) now receive a VME device
>       instead of a pointer to the bridge device (cleaner design)
>     * It's easier to differenciate API calls as bridge-based or
>       device-based (ie. cleaner interface).
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

This is going to impact vme_api.txt. Please can you update that as well please.

Martyn

> ---
>  drivers/staging/vme/devices/vme_user.c |   14 ++--
>  drivers/staging/vme/vme.c              |  120 +++++++++++++------------------
>  drivers/staging/vme/vme.h              |   37 +++++++---
>  drivers/staging/vme/vme_bridge.h       |    5 +-
>  4 files changed, 85 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index 3cbeb2a..bb33dc2 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -116,7 +116,7 @@ static struct driver_stats statistics;
>  
>  static struct cdev *vme_user_cdev;		/* Character device */
>  static struct class *vme_user_sysfs_class;	/* Sysfs class */
> -static struct device *vme_user_bridge;		/* Pointer to bridge device */
> +static struct vme_dev *vme_user_bridge;		/* Pointer to user device */
>  
>  
>  static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
> @@ -135,8 +135,8 @@ static ssize_t vme_user_write(struct file *, const char __user *, size_t,
>  static loff_t vme_user_llseek(struct file *, loff_t, int);
>  static long vme_user_unlocked_ioctl(struct file *, unsigned int, unsigned long);
>  
> -static int __devinit vme_user_probe(struct device *, int, int);
> -static int __devexit vme_user_remove(struct device *, int, int);
> +static int __devinit vme_user_probe(struct vme_dev *);
> +static int __devexit vme_user_remove(struct vme_dev *);
>  
>  static const struct file_operations vme_user_fops = {
>  	.open = vme_user_open,
> @@ -689,8 +689,7 @@ err_nocard:
>   * as practical. We will therefore reserve the buffers and request the images
>   * here so that we don't have to do it later.
>   */
> -static int __devinit vme_user_probe(struct device *dev, int cur_bus,
> -	int cur_slot)
> +static int __devinit vme_user_probe(struct vme_dev *vdev)
>  {
>  	int i, err;
>  	char name[12];
> @@ -702,7 +701,7 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	vme_user_bridge = dev;
> +	vme_user_bridge = vdev;
>  
>  	/* Initialise descriptors */
>  	for (i = 0; i < VME_DEVS; i++) {
> @@ -865,8 +864,7 @@ err_dev:
>  	return err;
>  }
>  
> -static int __devexit vme_user_remove(struct device *dev, int cur_bus,
> -	int cur_slot)
> +static int __devexit vme_user_remove(struct vme_dev *dev)
>  {
>  	int i;
>  
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 0504007..360acc9 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -42,13 +42,9 @@ static DEFINE_MUTEX(vme_buses_lock);
>  static void __exit vme_exit(void);
>  static int __init vme_init(void);
>  
> -
> -/*
> - * Find the bridge resource associated with a specific device resource
> - */
> -static struct vme_bridge *dev_to_bridge(struct device *dev)
> +static struct vme_dev *dev_to_vme_dev(struct device *dev)
>  {
> -	return dev->platform_data;
> +	return container_of(dev, struct vme_dev, dev);
>  }
>  
>  /*
> @@ -284,7 +280,7 @@ static int vme_check_window(vme_address_t aspace, unsigned long long vme_base,
>   * Request a slave image with specific attributes, return some unique
>   * identifier.
>   */
> -struct vme_resource *vme_slave_request(struct device *dev,
> +struct vme_resource *vme_slave_request(struct vme_dev *vdev,
>  	vme_address_t address, vme_cycle_t cycle)
>  {
>  	struct vme_bridge *bridge;
> @@ -293,7 +289,7 @@ struct vme_resource *vme_slave_request(struct device *dev,
>  	struct vme_slave_resource *slave_image = NULL;
>  	struct vme_resource *resource = NULL;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		goto err_bus;
> @@ -440,7 +436,7 @@ EXPORT_SYMBOL(vme_slave_free);
>   * Request a master image with specific attributes, return some unique
>   * identifier.
>   */
> -struct vme_resource *vme_master_request(struct device *dev,
> +struct vme_resource *vme_master_request(struct vme_dev *vdev,
>  	vme_address_t address, vme_cycle_t cycle, vme_width_t dwidth)
>  {
>  	struct vme_bridge *bridge;
> @@ -449,7 +445,7 @@ struct vme_resource *vme_master_request(struct device *dev,
>  	struct vme_master_resource *master_image = NULL;
>  	struct vme_resource *resource = NULL;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		goto err_bus;
> @@ -698,7 +694,8 @@ EXPORT_SYMBOL(vme_master_free);
>   * Request a DMA controller with specific attributes, return some unique
>   * identifier.
>   */
> -struct vme_resource *vme_dma_request(struct device *dev, vme_dma_route_t route)
> +struct vme_resource *vme_dma_request(struct vme_dev *vdev,
> +	vme_dma_route_t route)
>  {
>  	struct vme_bridge *bridge;
>  	struct list_head *dma_pos = NULL;
> @@ -709,7 +706,7 @@ struct vme_resource *vme_dma_request(struct device *dev, vme_dma_route_t route)
>  	/* XXX Not checking resource attributes */
>  	printk(KERN_ERR "No VME resource Attribute tests done\n");
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		goto err_bus;
> @@ -1042,13 +1039,13 @@ void vme_irq_handler(struct vme_bridge *bridge, int level, int statid)
>  }
>  EXPORT_SYMBOL(vme_irq_handler);
>  
> -int vme_irq_request(struct device *dev, int level, int statid,
> +int vme_irq_request(struct vme_dev *vdev, int level, int statid,
>  	void (*callback)(int, int, void *),
>  	void *priv_data)
>  {
>  	struct vme_bridge *bridge;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		return -EINVAL;
> @@ -1085,11 +1082,11 @@ int vme_irq_request(struct device *dev, int level, int statid,
>  }
>  EXPORT_SYMBOL(vme_irq_request);
>  
> -void vme_irq_free(struct device *dev, int level, int statid)
> +void vme_irq_free(struct vme_dev *vdev, int level, int statid)
>  {
>  	struct vme_bridge *bridge;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		return;
> @@ -1120,11 +1117,11 @@ void vme_irq_free(struct device *dev, int level, int statid)
>  }
>  EXPORT_SYMBOL(vme_irq_free);
>  
> -int vme_irq_generate(struct device *dev, int level, int statid)
> +int vme_irq_generate(struct vme_dev *vdev, int level, int statid)
>  {
>  	struct vme_bridge *bridge;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		return -EINVAL;
> @@ -1147,7 +1144,7 @@ EXPORT_SYMBOL(vme_irq_generate);
>  /*
>   * Request the location monitor, return resource or NULL
>   */
> -struct vme_resource *vme_lm_request(struct device *dev)
> +struct vme_resource *vme_lm_request(struct vme_dev *vdev)
>  {
>  	struct vme_bridge *bridge;
>  	struct list_head *lm_pos = NULL;
> @@ -1155,7 +1152,7 @@ struct vme_resource *vme_lm_request(struct device *dev)
>  	struct vme_lm_resource *lm = NULL;
>  	struct vme_resource *resource = NULL;
>  
> -	bridge = dev_to_bridge(dev);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		goto err_bus;
> @@ -1336,11 +1333,11 @@ void vme_lm_free(struct vme_resource *resource)
>  }
>  EXPORT_SYMBOL(vme_lm_free);
>  
> -int vme_slot_get(struct device *bus)
> +int vme_slot_get(struct vme_dev *vdev)
>  {
>  	struct vme_bridge *bridge;
>  
> -	bridge = dev_to_bridge(bus);
> +	bridge = vdev->bridge;
>  	if (bridge == NULL) {
>  		printk(KERN_ERR "Can't find VME bus\n");
>  		return -EINVAL;
> @@ -1412,7 +1409,7 @@ static void vme_remove_bus(struct vme_bridge *bridge)
>  
>  int vme_register_bridge(struct vme_bridge *bridge)
>  {
> -	struct device *dev;
> +	struct vme_dev *vdev;
>  	int retval;
>  	int i;
>  
> @@ -1425,20 +1422,23 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  	 * specification.
>  	 */
>  	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		dev = &bridge->dev[i];
> -		memset(dev, 0, sizeof(struct device));
> -
> -		dev->parent = bridge->parent;
> -		dev->bus = &vme_bus_type;
> +		vdev = &bridge->dev[i];
> +		memset(vdev, 0, sizeof(struct vme_dev));
> +
> +		vdev->id.bus = bridge->num;
> +		vdev->id.slot = i + 1;
> +		vdev->bridge = bridge;
> +		vdev->dev.parent = bridge->parent;
> +		vdev->dev.bus = &vme_bus_type;
>  		/*
>  		 * We save a pointer to the bridge in platform_data so that we
>  		 * can get to it later. We keep driver_data for use by the
>  		 * driver that binds against the slot
>  		 */
> -		dev->platform_data = bridge;
> -		dev_set_name(dev, "vme-%x.%x", bridge->num, i + 1);
> +		vdev->dev.platform_data = bridge;
> +		dev_set_name(&vdev->dev, "vme-%x.%x", bridge->num, i + 1);
>  
> -		retval = device_register(dev);
> +		retval = device_register(&vdev->dev);
>  		if (retval)
>  			goto err_reg;
>  	}
> @@ -1447,8 +1447,8 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  
>  err_reg:
>  	while (--i >= 0) {
> -		dev = &bridge->dev[i];
> -		device_unregister(dev);
> +		vdev = &bridge->dev[i];
> +		device_unregister(&vdev->dev);
>  	}
>  	vme_remove_bus(bridge);
>  	return retval;
> @@ -1458,12 +1458,12 @@ EXPORT_SYMBOL(vme_register_bridge);
>  void vme_unregister_bridge(struct vme_bridge *bridge)
>  {
>  	int i;
> -	struct device *dev;
> +	struct vme_dev *vdev;
>  
>  
>  	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		dev = &bridge->dev[i];
> -		device_unregister(dev);
> +		vdev = &bridge->dev[i];
> +		device_unregister(&vdev->dev);
>  	}
>  	vme_remove_bus(bridge);
>  }
> @@ -1489,32 +1489,6 @@ EXPORT_SYMBOL(vme_unregister_driver);
>  
>  /* - Bus Registration ------------------------------------------------------ */
>  
> -static int vme_calc_slot(struct device *dev)
> -{
> -	struct vme_bridge *bridge;
> -	int num;
> -
> -	bridge = dev_to_bridge(dev);
> -
> -	/* Determine slot number */
> -	num = 0;
> -	while (num < VME_SLOTS_MAX) {
> -		if (&bridge->dev[num] == dev)
> -			break;
> -
> -		num++;
> -	}
> -	if (num == VME_SLOTS_MAX) {
> -		dev_err(dev, "Failed to identify slot\n");
> -		num = 0;
> -		goto err_dev;
> -	}
> -	num++;
> -
> -err_dev:
> -	return num;
> -}
> -
>  static struct vme_driver *dev_to_vme_driver(struct device *dev)
>  {
>  	if (dev->driver == NULL)
> @@ -1525,15 +1499,17 @@ static struct vme_driver *dev_to_vme_driver(struct device *dev)
>  
>  static int vme_bus_match(struct device *dev, struct device_driver *drv)
>  {
> +	struct vme_dev *vdev;
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
>  	int i, num;
>  
> -	bridge = dev_to_bridge(dev);
> +	vdev = dev_to_vme_dev(dev);
> +	bridge = vdev->bridge;
>  	driver = container_of(drv, struct vme_driver, driver);
>  
> -	num = vme_calc_slot(dev);
> -	if (!num)
> +	num = vdev->id.slot;
> +	if (num <= 0 || num >= VME_SLOTS_MAX)
>  		goto err_dev;
>  
>  	if (driver->bind_table == NULL) {
> @@ -1553,7 +1529,7 @@ static int vme_bus_match(struct device *dev, struct device_driver *drv)
>  				return 1;
>  
>  			if ((driver->bind_table[i].slot == VME_SLOT_CURRENT) &&
> -				(num == vme_slot_get(dev)))
> +				(num == vme_slot_get(vdev)))
>  				return 1;
>  		}
>  		i++;
> @@ -1568,14 +1544,16 @@ static int vme_bus_probe(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> +	struct vme_dev *vdev;
>  	int retval = -ENODEV;
>  
>  	driver = dev_to_vme_driver(dev);
> -	bridge = dev_to_bridge(dev);
> +	vdev = dev_to_vme_dev(dev);
> +	bridge = vdev->bridge;
>  
>  	vme_bridge_get(bridge->num);
>  	if (driver->probe != NULL)
> -		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> +		retval = driver->probe(vdev);
>  
>  	if (retval)
>  		vme_bridge_put(bridge);
> @@ -1587,13 +1565,15 @@ static int vme_bus_remove(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> +	struct vme_dev *vdev;
>  	int retval = -ENODEV;
>  
>  	driver = dev_to_vme_driver(dev);
> -	bridge = dev_to_bridge(dev);
> +	vdev = dev_to_vme_dev(dev);
> +	bridge = vdev->bridge;
>  
>  	if (driver->remove != NULL)
> -		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
> +		retval = driver->remove(vdev);
>  
>  	vme_bridge_put(bridge);
>  
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 8fb35e2..356b06e 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -93,17 +93,34 @@ extern struct bus_type vme_bus_type;
>  #define VME_SLOT_CURRENT	-1
>  #define VME_SLOT_ALL		-2
>  
> +/**
> + * VME device identifier structure
> + * @bus: The bus ID of the bus the device is on
> + * @slot: The slot this device is plugged into
> + */
>  struct vme_device_id {
>  	int bus;
>  	int slot;
>  };
>  
> +/**
> + * Structure representing a VME device
> + * @id: The ID of the device (currently the bus and slot number)
> + * @bridge: Pointer to the bridge device this device is on
> + * @dev: Internal device structure
> + */
> +struct vme_dev {
> +	struct vme_device_id id;
> +	struct vme_bridge *bridge;
> +	struct device dev;
> +};
> +
>  struct vme_driver {
>  	struct list_head node;
>  	const char *name;
>  	const struct vme_device_id *bind_table;
> -	int (*probe)  (struct device *, int, int);
> -	int (*remove) (struct device *, int, int);
> +	int (*probe)  (struct vme_dev *);
> +	int (*remove) (struct vme_dev *);
>  	void (*shutdown) (void);
>  	struct device_driver    driver;
>  };
> @@ -114,7 +131,7 @@ void vme_free_consistent(struct vme_resource *, size_t,  void *,
>  
>  size_t vme_get_size(struct vme_resource *);
>  
> -struct vme_resource *vme_slave_request(struct device *, vme_address_t,
> +struct vme_resource *vme_slave_request(struct vme_dev *, vme_address_t,
>  	vme_cycle_t);
>  int vme_slave_set(struct vme_resource *, int, unsigned long long,
>  	unsigned long long, dma_addr_t, vme_address_t, vme_cycle_t);
> @@ -122,7 +139,7 @@ int vme_slave_get(struct vme_resource *, int *, unsigned long long *,
>  	unsigned long long *, dma_addr_t *, vme_address_t *, vme_cycle_t *);
>  void vme_slave_free(struct vme_resource *);
>  
> -struct vme_resource *vme_master_request(struct device *, vme_address_t,
> +struct vme_resource *vme_master_request(struct vme_dev *, vme_address_t,
>  	vme_cycle_t, vme_width_t);
>  int vme_master_set(struct vme_resource *, int, unsigned long long,
>  	unsigned long long, vme_address_t, vme_cycle_t, vme_width_t);
> @@ -134,7 +151,7 @@ unsigned int vme_master_rmw(struct vme_resource *, unsigned int, unsigned int,
>  	unsigned int, loff_t);
>  void vme_master_free(struct vme_resource *);
>  
> -struct vme_resource *vme_dma_request(struct device *, vme_dma_route_t);
> +struct vme_resource *vme_dma_request(struct vme_dev *, vme_dma_route_t);
>  struct vme_dma_list *vme_new_dma_list(struct vme_resource *);
>  struct vme_dma_attr *vme_dma_pattern_attribute(u32, vme_pattern_t);
>  struct vme_dma_attr *vme_dma_pci_attribute(dma_addr_t);
> @@ -147,12 +164,12 @@ int vme_dma_list_exec(struct vme_dma_list *);
>  int vme_dma_list_free(struct vme_dma_list *);
>  int vme_dma_free(struct vme_resource *);
>  
> -int vme_irq_request(struct device *, int, int,
> +int vme_irq_request(struct vme_dev *, int, int,
>  	void (*callback)(int, int, void *), void *);
> -void vme_irq_free(struct device *, int, int);
> -int vme_irq_generate(struct device *, int, int);
> +void vme_irq_free(struct vme_dev *, int, int);
> +int vme_irq_generate(struct vme_dev *, int, int);
>  
> -struct vme_resource * vme_lm_request(struct device *);
> +struct vme_resource * vme_lm_request(struct vme_dev *);
>  int vme_lm_count(struct vme_resource *);
>  int vme_lm_set(struct vme_resource *, unsigned long long, vme_address_t,
>  	vme_cycle_t);
> @@ -162,7 +179,7 @@ int vme_lm_attach(struct vme_resource *, int, void (*callback)(int));
>  int vme_lm_detach(struct vme_resource *, int);
>  void vme_lm_free(struct vme_resource *);
>  
> -int vme_slot_get(struct device *);
> +int vme_slot_get(struct vme_dev *);
>  
>  int vme_register_driver(struct vme_driver *);
>  void vme_unregister_driver(struct vme_driver *);
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index ef751a4..74d0103 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -115,9 +115,8 @@ struct vme_bridge {
>  	struct list_head bus_list; /* list of VME buses */
>  	struct module *owner;	/* module that owns the bridge */
>  
> -	struct device dev[VME_SLOTS_MAX];	/* Device registered with
> -						 * device model on VME bus
> -						 */
> +	struct vme_dev dev[VME_SLOTS_MAX];	/* Device registered
> +						 * on VME bus */
>  
>  	/* Interrupt callbacks */
>  	struct vme_irq irq[7];


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support
  2011-08-10  9:33 ` [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support Manohar Vanga
@ 2011-08-10 10:18   ` Martyn Welch
  0 siblings, 0 replies; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 10:18 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: gregkh, cota, devel, linux-kernel

On 10/08/11 10:33, Manohar Vanga wrote:
> For jumper based boards (non VME64x), there is no mechanism
> for detecting the card that is plugged into a specific slot. This
> leads to issues in non-autodiscovery crates/cards when a card is
> plugged into a slot that is "claimed" by a different driver. In
> reality, there is no problem, but the driver rejects such a
> configuration due to its dependence on the concept of slots.
> 
> This patch makes the concept of slots less critical and pushes the
> driver match() to individual drivers (similar to what happens in the
> ISA bus in driver/base/isa.c). This allows drivers to register the
> number of devices that they expect without any restrictions. Devices
> in this new model are now formatted as $driver_name-$bus_id.$device_id
> (as compared to the earlier vme-$bus_id.$slot_number).
> 
> This model also makes the device model more logical as devices
> are only registered when they actually exist whereas earlier,
> a set of devices were being created automatically regardless of
> them actually being there.
> 
> Another change introduced in this patch is that devices are now created
> within the VME driver structure rather than in the VME bridge structure.
> This way, things don't go haywire if the bridge driver is removed while
> a driver is using it (this is also additionally prevented by having
> reference counting of used bridge modules).
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

This is going to impact vme_api.txt. Please can you update that as well please.

Martyn

> ---
>  drivers/staging/vme/devices/vme_user.c |   25 +++-
>  drivers/staging/vme/devices/vme_user.h |    2 +-
>  drivers/staging/vme/vme.c              |  220 ++++++++++++++++----------------
>  drivers/staging/vme/vme.h              |   19 ++-
>  drivers/staging/vme/vme_bridge.h       |    4 -
>  5 files changed, 147 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index bb33dc2..41a82a1 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -43,7 +43,7 @@
>  static DEFINE_MUTEX(vme_user_mutex);
>  static const char driver_name[] = "vme_user";
>  
> -static int bus[USER_BUS_MAX];
> +static int bus[VME_USER_BUS_MAX];
>  static unsigned int bus_num;
>  
>  /* Currently Documentation/devices.txt defines the following for VME:
> @@ -135,6 +135,7 @@ static ssize_t vme_user_write(struct file *, const char __user *, size_t,
>  static loff_t vme_user_llseek(struct file *, loff_t, int);
>  static long vme_user_unlocked_ioctl(struct file *, unsigned int, unsigned long);
>  
> +static int vme_user_match(struct vme_dev *);
>  static int __devinit vme_user_probe(struct vme_dev *);
>  static int __devexit vme_user_remove(struct vme_dev *);
>  
> @@ -620,6 +621,7 @@ static void buf_unalloc(int num)
>  
>  static struct vme_driver vme_user_driver = {
>  	.name = driver_name,
> +	.match = vme_user_match,
>  	.probe = vme_user_probe,
>  	.remove = __devexit_p(vme_user_remove),
>  };
> @@ -643,10 +645,10 @@ static int __init vme_user_init(void)
>  	/* Let's start by supporting one bus, we can support more than one
>  	 * in future revisions if that ever becomes necessary.
>  	 */
> -	if (bus_num > USER_BUS_MAX) {
> +	if (bus_num > VME_USER_BUS_MAX) {
>  		printk(KERN_ERR "%s: Driver only able to handle %d buses\n",
> -			driver_name, USER_BUS_MAX);
> -		bus_num = USER_BUS_MAX;
> +			driver_name, VME_USER_BUS_MAX);
> +		bus_num = VME_USER_BUS_MAX;
>  	}
>  
>  
> @@ -671,7 +673,13 @@ static int __init vme_user_init(void)
>  
>  	vme_user_driver.bind_table = ids;
>  
> -	retval = vme_register_driver(&vme_user_driver);
> +	/*
> +	 * Here we just register the maximum number of devices we can and
> +	 * leave vme_user_match() to allow only 1 to go through to probe().
> +	 * This way, if we later want to allow multiple user access devices,
> +	 * we just change the code in vme_user_match().
> +	 */
> +	retval = vme_register_driver(&vme_user_driver, VME_MAX_SLOTS);
>  	if (retval != 0)
>  		goto err_reg;
>  
> @@ -684,6 +692,13 @@ err_nocard:
>  	return retval;
>  }
>  
> +static int vme_user_match(struct vme_dev *vdev)
> +{
> +	if (vdev->id.num >= VME_USER_BUS_MAX)
> +		return 0;
> +	return 1;
> +}
> +
>  /*
>   * In this simple access driver, the old behaviour is being preserved as much
>   * as practical. We will therefore reserve the buffers and request the images
> diff --git a/drivers/staging/vme/devices/vme_user.h b/drivers/staging/vme/devices/vme_user.h
> index 24bf4e5..d85a1e9 100644
> --- a/drivers/staging/vme/devices/vme_user.h
> +++ b/drivers/staging/vme/devices/vme_user.h
> @@ -1,7 +1,7 @@
>  #ifndef _VME_USER_H_
>  #define _VME_USER_H_
>  
> -#define USER_BUS_MAX                  1
> +#define VME_USER_BUS_MAX	1
>  
>  /*
>   * VMEbus Master Window Configuration Structure
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 360acc9..99d1505 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -42,10 +42,7 @@ static DEFINE_MUTEX(vme_buses_lock);
>  static void __exit vme_exit(void);
>  static int __init vme_init(void);
>  
> -static struct vme_dev *dev_to_vme_dev(struct device *dev)
> -{
> -	return container_of(dev, struct vme_dev, dev);
> -}
> +#define dev_to_vme_dev(dev) container_of(dev, struct vme_dev, dev)
>  
>  /*
>   * Increments the reference count of a VME bridge.
> @@ -1409,150 +1406,160 @@ static void vme_remove_bus(struct vme_bridge *bridge)
>  
>  int vme_register_bridge(struct vme_bridge *bridge)
>  {
> -	struct vme_dev *vdev;
> -	int retval;
> -	int i;
> +	return vme_add_bus(bridge);
> +}
> +EXPORT_SYMBOL(vme_register_bridge);
>  
> -	retval = vme_add_bus(bridge);
> -	if (retval)
> -		return retval;
> +void vme_unregister_bridge(struct vme_bridge *bridge)
> +{
> +	vme_remove_bus(bridge);
> +}
> +EXPORT_SYMBOL(vme_unregister_bridge);
>  
> -	/* This creates 32 vme "slot" devices. This equates to a slot for each
> -	 * ID available in a system conforming to the ANSI/VITA 1-1994
> -	 * specification.
> -	 */
> -	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		vdev = &bridge->dev[i];
> -		memset(vdev, 0, sizeof(struct vme_dev));
>  
> +/* - Driver Registration --------------------------------------------------- */
> +
> +static void vme_dev_release(struct device *dev)
> +{
> +	kfree(dev_to_vme_dev(dev));
> +}
> +
> +static int __vme_register_driver_bus(struct vme_driver *drv,
> +	struct vme_bridge *bridge, unsigned int ndevs)
> +{
> +	int err;
> +	unsigned int i;
> +	struct vme_dev *vdev;
> +	struct vme_dev *tmp;
> +
> +	for (i = 0; i < ndevs; i++) {
> +		vdev = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
> +		if (!vdev) {
> +			err = -ENOMEM;
> +			goto err_alloc;
> +		}
> +		vdev->id.num = i;
>  		vdev->id.bus = bridge->num;
>  		vdev->id.slot = i + 1;
>  		vdev->bridge = bridge;
> +		vdev->dev.platform_data = drv;
> +		vdev->dev.release = vme_dev_release;
>  		vdev->dev.parent = bridge->parent;
>  		vdev->dev.bus = &vme_bus_type;
> -		/*
> -		 * We save a pointer to the bridge in platform_data so that we
> -		 * can get to it later. We keep driver_data for use by the
> -		 * driver that binds against the slot
> -		 */
> -		vdev->dev.platform_data = bridge;
> -		dev_set_name(&vdev->dev, "vme-%x.%x", bridge->num, i + 1);
> -
> -		retval = device_register(&vdev->dev);
> -		if (retval)
> -			goto err_reg;
> +		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
> +			vdev->id.num);
> +
> +		err = device_register(&vdev->dev);
> +		if (err)
> +			goto err_dev_reg;
> +
> +		if (vdev->dev.platform_data) {
> +			list_add_tail(&vdev->list, &drv->devices);
> +			drv->ndev++;
> +		} else {
> +			device_unregister(&vdev->dev);
> +			kfree(vdev);
> +		}
>  	}
> +	return 0;
>  
> -	return retval;
> -
> -err_reg:
> -	while (--i >= 0) {
> -		vdev = &bridge->dev[i];
> +err_dev_reg:
> +	kfree(vdev);
> +err_alloc:
> +	list_for_each_entry_safe(vdev, tmp, &drv->devices, list) {
> +		list_del(&vdev->list);
>  		device_unregister(&vdev->dev);
>  	}
> -	vme_remove_bus(bridge);
> -	return retval;
> +	return err;
>  }
> -EXPORT_SYMBOL(vme_register_bridge);
>  
> -void vme_unregister_bridge(struct vme_bridge *bridge)
> +static int __vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
>  {
> -	int i;
> -	struct vme_dev *vdev;
> -
> +	struct vme_bridge *bridge;
> +	int err = 0;
>  
> -	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		vdev = &bridge->dev[i];
> -		device_unregister(&vdev->dev);
> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		/*
> +		 * We increase the refcount of the bridge module here to
> +		 * prevent it from being removed during driver registration
> +		 */
> +		if (!vme_bridge_get(bridge->num))
> +			continue;
> +		mutex_unlock(&vme_buses_lock);
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		mutex_lock(&vme_buses_lock);
> +		vme_bridge_put(bridge);
> +		if (err)
> +			break;
>  	}
> -	vme_remove_bus(bridge);
> +	mutex_unlock(&vme_buses_lock);
> +	return err;
>  }
> -EXPORT_SYMBOL(vme_unregister_bridge);
>  
> -
> -/* - Driver Registration --------------------------------------------------- */
> -
> -int vme_register_driver(struct vme_driver *drv)
> +int vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
>  {
> +	int err;
> +
>  	drv->driver.name = drv->name;
>  	drv->driver.bus = &vme_bus_type;
> -
> -	return driver_register(&drv->driver);
> +	INIT_LIST_HEAD(&drv->devices);
> +
> +	err = driver_register(&drv->driver);
> +	if (err)
> +		return err;
> +
> +	err = __vme_register_driver(drv, ndevs);
> +	if (!err & (drv->ndev == 0))
> +		err = -ENODEV;
> +	if (err)
> +		vme_unregister_driver(drv);
> +	return err;
>  }
>  EXPORT_SYMBOL(vme_register_driver);
>  
>  void vme_unregister_driver(struct vme_driver *drv)
>  {
> +	struct vme_dev *dev, *dev_tmp;
> +
> +	list_for_each_entry_safe(dev, dev_tmp, &drv->devices, list) {
> +		device_unregister(&dev->dev);
> +		list_del(&dev->list);
> +	}
>  	driver_unregister(&drv->driver);
>  }
>  EXPORT_SYMBOL(vme_unregister_driver);
>  
>  /* - Bus Registration ------------------------------------------------------ */
>  
> -static struct vme_driver *dev_to_vme_driver(struct device *dev)
> -{
> -	if (dev->driver == NULL)
> -		printk(KERN_ERR "Bugger dev->driver is NULL\n");
> -
> -	return container_of(dev->driver, struct vme_driver, driver);
> -}
> -
>  static int vme_bus_match(struct device *dev, struct device_driver *drv)
>  {
> -	struct vme_dev *vdev;
> -	struct vme_bridge *bridge;
> -	struct vme_driver *driver;
> -	int i, num;
> -
> -	vdev = dev_to_vme_dev(dev);
> -	bridge = vdev->bridge;
> -	driver = container_of(drv, struct vme_driver, driver);
> -
> -	num = vdev->id.slot;
> -	if (num <= 0 || num >= VME_SLOTS_MAX)
> -		goto err_dev;
> +	struct vme_driver *vme_drv;
>  
> -	if (driver->bind_table == NULL) {
> -		dev_err(dev, "Bind table NULL\n");
> -		goto err_table;
> -	}
> -
> -	i = 0;
> -	while ((driver->bind_table[i].bus != 0) ||
> -		(driver->bind_table[i].slot != 0)) {
> +	vme_drv = container_of(drv, struct vme_driver, driver);
>  
> -		if (bridge->num == driver->bind_table[i].bus) {
> -			if (num == driver->bind_table[i].slot)
> -				return 1;
> +	if (dev->platform_data == vme_drv) {
> +		struct vme_dev *vdev = dev_to_vme_dev(dev);
>  
> -			if (driver->bind_table[i].slot == VME_SLOT_ALL)
> -				return 1;
> +		if (vme_drv->match && vme_drv->match(vdev))
> +			return 1;
>  
> -			if ((driver->bind_table[i].slot == VME_SLOT_CURRENT) &&
> -				(num == vme_slot_get(vdev)))
> -				return 1;
> -		}
> -		i++;
> +		dev->platform_data = NULL;
>  	}
> -
> -err_dev:
> -err_table:
>  	return 0;
>  }
>  
>  static int vme_bus_probe(struct device *dev)
>  {
> -	struct vme_bridge *bridge;
> +	int retval = 0;
>  	struct vme_driver *driver;
> -	struct vme_dev *vdev;
> -	int retval = -ENODEV;
> -
> -	driver = dev_to_vme_driver(dev);
> -	vdev = dev_to_vme_dev(dev);
> -	bridge = vdev->bridge;
> +	struct vme_dev *vdev = dev_to_vme_dev(dev);
> +	struct vme_bridge *bridge = vdev->bridge;
>  
>  	vme_bridge_get(bridge->num);
> -	if (driver->probe != NULL)
> +
> +	driver = dev->platform_data;
> +	if (driver->probe)
>  		retval = driver->probe(vdev);
>  
>  	if (retval)
> @@ -1563,16 +1570,13 @@ static int vme_bus_probe(struct device *dev)
>  
>  static int vme_bus_remove(struct device *dev)
>  {
> -	struct vme_bridge *bridge;
> +	int retval = 0;
>  	struct vme_driver *driver;
> -	struct vme_dev *vdev;
> -	int retval = -ENODEV;
> -
> -	driver = dev_to_vme_driver(dev);
> -	vdev = dev_to_vme_dev(dev);
> -	bridge = vdev->bridge;
> +	struct vme_dev *vdev = dev_to_vme_dev(dev);
> +	struct vme_bridge *bridge = vdev->bridge;
>  
> -	if (driver->remove != NULL)
> +	driver = dev->platform_data;
> +	if (driver->remove)
>  		retval = driver->remove(vdev);
>  
>  	vme_bridge_put(bridge);
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 356b06e..80ade64 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -90,15 +90,19 @@ extern struct bus_type vme_bus_type;
>  
>  /* VME_MAX_BRIDGES comes from the type of vme_bus_numbers */
>  #define VME_MAX_BRIDGES		(sizeof(unsigned int)*8)
> +#define VME_MAX_SLOTS		32
> +
>  #define VME_SLOT_CURRENT	-1
>  #define VME_SLOT_ALL		-2
>  
>  /**
>   * VME device identifier structure
> + * @num: The device ID (ranges from 0 to N-1 for N devices)
>   * @bus: The bus ID of the bus the device is on
>   * @slot: The slot this device is plugged into
>   */
>  struct vme_device_id {
> +	int num;
>  	int bus;
>  	int slot;
>  };
> @@ -108,21 +112,26 @@ struct vme_device_id {
>   * @id: The ID of the device (currently the bus and slot number)
>   * @bridge: Pointer to the bridge device this device is on
>   * @dev: Internal device structure
> + * @list: List of devices (per driver)
>   */
>  struct vme_dev {
>  	struct vme_device_id id;
>  	struct vme_bridge *bridge;
>  	struct device dev;
> +	struct list_head list;
>  };
>  
>  struct vme_driver {
>  	struct list_head node;
>  	const char *name;
>  	const struct vme_device_id *bind_table;
> -	int (*probe)  (struct vme_dev *);
> -	int (*remove) (struct vme_dev *);
> -	void (*shutdown) (void);
> -	struct device_driver    driver;
> +	int (*match)(struct vme_dev *);
> +	int (*probe)(struct vme_dev *);
> +	int (*remove)(struct vme_dev *);
> +	void (*shutdown)(void);
> +	struct device_driver driver;
> +	struct list_head devices;
> +	unsigned int ndev;
>  };
>  
>  void *vme_alloc_consistent(struct vme_resource *, size_t, dma_addr_t *);
> @@ -181,7 +190,7 @@ void vme_lm_free(struct vme_resource *);
>  
>  int vme_slot_get(struct vme_dev *);
>  
> -int vme_register_driver(struct vme_driver *);
> +int vme_register_driver(struct vme_driver *, unsigned int);
>  void vme_unregister_driver(struct vme_driver *);
>  
>  struct vme_bridge *vme_bridge_get(unsigned int bus_id);
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index 74d0103..24cb4b8 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -2,7 +2,6 @@
>  #define _VME_BRIDGE_H_
>  
>  #define VME_CRCSR_BUF_SIZE (508*1024)
> -#define VME_SLOTS_MAX 32
>  /*
>   * Resource structures
>   */
> @@ -115,9 +114,6 @@ struct vme_bridge {
>  	struct list_head bus_list; /* list of VME buses */
>  	struct module *owner;	/* module that owns the bridge */
>  
> -	struct vme_dev dev[VME_SLOTS_MAX];	/* Device registered
> -						 * on VME bus */
> -
>  	/* Interrupt callbacks */
>  	struct vme_irq irq[7];
>  	/* Locking for VME irq callback configuration */


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/6] staging: vme: add struct vme_dev for VME devices
  2011-08-10 10:14   ` Martyn Welch
@ 2011-08-10 10:33     ` Manohar Vanga
  0 siblings, 0 replies; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10 10:33 UTC (permalink / raw)
  To: Martyn Welch; +Cc: gregkh, cota, devel, linux-kernel

Hey Martyn,

> This is going to impact vme_api.txt. Please can you update that as well please.

I was planning to throw in one patch to add all the documentation at once. I'll
instead modify within the patches itself now and resend.

Thanks!

--
/manohar

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers
  2011-08-10 10:02   ` Martyn Welch
@ 2011-08-10 10:41     ` Manohar Vanga
  2011-08-10 12:50       ` Martyn Welch
  0 siblings, 1 reply; 25+ messages in thread
From: Manohar Vanga @ 2011-08-10 10:41 UTC (permalink / raw)
  To: Martyn Welch; +Cc: gregkh, cota, devel, linux-kernel

Hey Martyn,

> I'm sorry, I'm still simply not convinced by this patch:
> 
> 1) For a single bus driver (i.e. in the situation where we have 2 bridges of
> the same type), the numbering of the buses is still dependent on the order
> that they are found in the scan.

Yes this is still a bug. But this patch doesn't address this case.

> 2) If the bridge drivers are loaded as modules, I have a feeling they will be
> loaded sequentially and therefore the order of the bridges would only change
> if the order of the loading of the drivers changed.

And this is a major problem when it comes to multiple bridges of differing
types. What I'm saying is that this patch simply fixes this one problematic
case. We can move this out as soon as we have a more robust implementation.

As of now however, I think applying this is useful as we have a decent
workaround to the problem. If you want I can make the fact of it being
applicable only to cases with differing bridges explicit in the commit
message.

--
/manohar

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers
  2011-08-10 10:41     ` Manohar Vanga
@ 2011-08-10 12:50       ` Martyn Welch
  2011-08-23 22:06         ` Greg KH
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 12:50 UTC (permalink / raw)
  To: gregkh, cota, devel, linux-kernel

On 10/08/11 11:41, Manohar Vanga wrote:
> Hey Martyn,
> 
>> I'm sorry, I'm still simply not convinced by this patch:
>>
>> 1) For a single bus driver (i.e. in the situation where we have 2 bridges of
>> the same type), the numbering of the buses is still dependent on the order
>> that they are found in the scan.
> 
> Yes this is still a bug. But this patch doesn't address this case.
> 
>> 2) If the bridge drivers are loaded as modules, I have a feeling they will be
>> loaded sequentially and therefore the order of the bridges would only change
>> if the order of the loading of the drivers changed.
> 
> And this is a major problem when it comes to multiple bridges of differing
> types. What I'm saying is that this patch simply fixes this one problematic
> case. We can move this out as soon as we have a more robust implementation.
> 
> As of now however, I think applying this is useful as we have a decent
> workaround to the problem. If you want I can make the fact of it being
> applicable only to cases with differing bridges explicit in the commit
> message.
> 

The problem is, I'm not convinced that this is the correct approach to take. I
think this should be parsed from sysfs dynamically (which may require some
work). I shall use the ethernet devices on my machine as an example:

I have 2 ethernet devices (and lo) on my machine:

$ ls /sys/class/net/
eth0  eth1  lo
$

These are symlinks and I can quite quickly find out which each of these
devices in (based on topology):
$ ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2011-08-10 11:56 eth0 ->
../../devices/pci0000:00/0000:00:19.0/net/eth0
lrwxrwxrwx 1 root root 0 2011-08-10 11:56 eth1 ->
../../devices/pci0000:00/0000:00:1c.1/0000:03:00.0/net/eth1
lrwxrwxrwx 1 root root 0 2011-08-10 11:56 lo -> ../../devices/virtual/net/lo
$

I'd think that this contains the information that you have in the config file
(based on the previous discussion we had) and would allow you to map the bus
numbering after booting.

To do this, I think we need to register a class called "vme", I guess in
vme_init() and add a call to class_device_register in vme_register_bridge and
a call to class_device_unregister in vme_unregister_bridge.

Greg: Is that right?

I'm really not convinced that the solution presented in this patch is suitable
for inclusion upstream.

Martyn

-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10  9:33 ` [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific Manohar Vanga
  2011-08-10 10:04   ` Martyn Welch
@ 2011-08-10 13:12   ` Joe Perches
  2011-08-10 13:34     ` Martyn Welch
  1 sibling, 1 reply; 25+ messages in thread
From: Joe Perches @ 2011-08-10 13:12 UTC (permalink / raw)
  To: Manohar Vanga; +Cc: martyn.welch, gregkh, cota, devel, linux-kernel

On Wed, 2011-08-10 at 11:33 +0200, Manohar Vanga wrote:
> Make PCI dependent functions ([alloc|free]_consistent() in
> 'vme.c') bridge specific. By removing the dependency of the
> VME bridge framework on PCI, this patch allows for addition of
> non-PCI based VME bridges.
[]
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
[]
> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
> +	dma_addr_t *dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	return pci_alloc_consistent(pdev, size, dma);
> +}
> +
> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
> +	dma_addr_t dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	pci_free_consistent(pdev, size, vaddr, dma);
> +}
[]
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
[]
> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
>  	return (int)slot;
>  }
>  
> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
> +	dma_addr_t *dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	return pci_alloc_consistent(pdev, size, dma);
> +}
> +
> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
> +	dma_addr_t dma)
> +{
> +	struct pci_dev *pdev;
> +
> +	/* Find pci_dev container of dev */
> +	pdev = container_of(parent, struct pci_dev, dev);
> +
> +	pci_free_consistent(pdev, size, vaddr, dma);
> +}
> +
>  static int __init tsi148_init(void)
>  {
>  	return pci_register_driver(&tsi148_driver);

Except for the name, those 2 blocks are identical.
Maybe create a non-pci generic version instead?

> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
[]
> @@ -104,28 +100,29 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>  		return NULL;
>  	}
>  
> -	/* Find pci_dev container of dev */
>  	if (bridge->parent == NULL) {
> -		printk(KERN_ERR "Dev entry NULL\n");
> +		printk(KERN_ERR "Dev entry NULL for"
> +			" bridge %s\n", bridge->name);

Please don't split formats like this.
Use unsplit format strings even if they exceed 80 cols.
This one doesn't.

		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
or
		pr_err("Dev entry NULL for bridge %s\n", bridge->name);

> +		return NULL;
> +	}
> +
> +	if (bridge->alloc_consistent == NULL) {
> +		printk(KERN_ERR "alloc_consistent not supported by"
> +			" bridge %s\n", bridge->name);

		printk(KERN_ERR "alloc_consistent not supported by bridge %s\n",
		       bridge->name);

> @@ -138,10 +135,19 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
>  		return;
>  	}
>  
> -	/* Find pci_dev container of dev */
> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
> +	if (bridge->parent == NULL) {
> +		printk(KERN_ERR "Dev entry NULL for"
> +			" bridge %s\n", bridge->name);

etc.



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 10:04   ` Martyn Welch
@ 2011-08-10 13:24     ` Dan Carpenter
  0 siblings, 0 replies; 25+ messages in thread
From: Dan Carpenter @ 2011-08-10 13:24 UTC (permalink / raw)
  To: Martyn Welch; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On Wed, Aug 10, 2011 at 11:04:38AM +0100, Martyn Welch wrote:
> On 10/08/11 10:33, Manohar Vanga wrote:
> > Make PCI dependent functions ([alloc|free]_consistent() in
> > 'vme.c') bridge specific. By removing the dependency of the
> > VME bridge framework on PCI, this patch allows for addition of
> > non-PCI based VME bridges.
> > 
> > Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> 
> This appears to be resolving the issue raised by Dan. Assuming he's happy with
> this:
> 
> Acked-by: Martyn Welch <martyn.welch@ge.com>
> 

Yah yah.  Looks good.

Acked-by: Dan Carpenter <error27@gmail.com>

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 13:12   ` Joe Perches
@ 2011-08-10 13:34     ` Martyn Welch
  2011-08-10 13:51       ` Joe Perches
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 13:34 UTC (permalink / raw)
  To: Joe Perches; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On 10/08/11 14:12, Joe Perches wrote:
> On Wed, 2011-08-10 at 11:33 +0200, Manohar Vanga wrote:
>> Make PCI dependent functions ([alloc|free]_consistent() in
>> 'vme.c') bridge specific. By removing the dependency of the
>> VME bridge framework on PCI, this patch allows for addition of
>> non-PCI based VME bridges.
> []
>> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> []
>> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
>> +	dma_addr_t *dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	return pci_alloc_consistent(pdev, size, dma);
>> +}
>> +
>> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
>> +	dma_addr_t dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	pci_free_consistent(pdev, size, vaddr, dma);
>> +}
> []
>> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> []
>> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
>>  	return (int)slot;
>>  }
>>  
>> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
>> +	dma_addr_t *dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	return pci_alloc_consistent(pdev, size, dma);
>> +}
>> +
>> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
>> +	dma_addr_t dma)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	/* Find pci_dev container of dev */
>> +	pdev = container_of(parent, struct pci_dev, dev);
>> +
>> +	pci_free_consistent(pdev, size, vaddr, dma);
>> +}
>> +
>>  static int __init tsi148_init(void)
>>  {
>>  	return pci_register_driver(&tsi148_driver);
> 
> Except for the name, those 2 blocks are identical.
> Maybe create a non-pci generic version instead?
> 

I'm not sure you can (I spent quite a bit of time attempting to do just that
when I wrote the original). It just so happens that both of the bridges we
have at the moment are PCI devices, so this does end up looking like copying
common code into the bridges, but assuming a bridge driver comes along that
doesn't use PCI, having this code per driver makes much more sense.


>> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> []
>> @@ -104,28 +100,29 @@ void *vme_alloc_consistent(struct vme_resource *resource, size_t size,
>>  		return NULL;
>>  	}
>>  
>> -	/* Find pci_dev container of dev */
>>  	if (bridge->parent == NULL) {
>> -		printk(KERN_ERR "Dev entry NULL\n");
>> +		printk(KERN_ERR "Dev entry NULL for"
>> +			" bridge %s\n", bridge->name);
> 
> Please don't split formats like this.
> Use unsplit format strings even if they exceed 80 cols.
> This one doesn't.
> 
> 		printk(KERN_ERR "Dev entry NULL for bridge %s\n", bridge->name);
> or
> 		pr_err("Dev entry NULL for bridge %s\n", bridge->name);
> 

True, didn't think about that.

Martyn

>> +		return NULL;
>> +	}
>> +
>> +	if (bridge->alloc_consistent == NULL) {
>> +		printk(KERN_ERR "alloc_consistent not supported by"
>> +			" bridge %s\n", bridge->name);
> 
> 		printk(KERN_ERR "alloc_consistent not supported by bridge %s\n",
> 		       bridge->name);
> 
>> @@ -138,10 +135,19 @@ void vme_free_consistent(struct vme_resource *resource, size_t size,
>>  		return;
>>  	}
>>  
>> -	/* Find pci_dev container of dev */
>> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
>> +	if (bridge->parent == NULL) {
>> +		printk(KERN_ERR "Dev entry NULL for"
>> +			" bridge %s\n", bridge->name);
> 
> etc.
> 
> 


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 13:34     ` Martyn Welch
@ 2011-08-10 13:51       ` Joe Perches
  2011-08-10 13:55         ` Martyn Welch
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Perches @ 2011-08-10 13:51 UTC (permalink / raw)
  To: Martyn Welch; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On Wed, 2011-08-10 at 14:34 +0100, Martyn Welch wrote:
> On 10/08/11 14:12, Joe Perches wrote:
> > On Wed, 2011-08-10 at 11:33 +0200, Manohar Vanga wrote:
> >> Make PCI dependent functions ([alloc|free]_consistent() in
> >> 'vme.c') bridge specific. By removing the dependency of the
> >> VME bridge framework on PCI, this patch allows for addition of
> >> non-PCI based VME bridges.
> > []
> >> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> > []
> >> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
> >> +	dma_addr_t *dma)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +
> >> +	/* Find pci_dev container of dev */
> >> +	pdev = container_of(parent, struct pci_dev, dev);
> >> +
> >> +	return pci_alloc_consistent(pdev, size, dma);
> >> +}
> >> +
> >> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
> >> +	dma_addr_t dma)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +
> >> +	/* Find pci_dev container of dev */
> >> +	pdev = container_of(parent, struct pci_dev, dev);
> >> +
> >> +	pci_free_consistent(pdev, size, vaddr, dma);
> >> +}
> > []
> >> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> > []
> >> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
> >>  	return (int)slot;
> >>  }
> >>  
> >> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
> >> +	dma_addr_t *dma)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +
> >> +	/* Find pci_dev container of dev */
> >> +	pdev = container_of(parent, struct pci_dev, dev);
> >> +
> >> +	return pci_alloc_consistent(pdev, size, dma);
> >> +}
> >> +
> >> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
> >> +	dma_addr_t dma)
> >> +{
> >> +	struct pci_dev *pdev;
> >> +
> >> +	/* Find pci_dev container of dev */
> >> +	pdev = container_of(parent, struct pci_dev, dev);
> >> +
> >> +	pci_free_consistent(pdev, size, vaddr, dma);
> >> +}
> >> +
> >>  static int __init tsi148_init(void)
> >>  {
> >>  	return pci_register_driver(&tsi148_driver);
> > 
> > Except for the name, those 2 blocks are identical.
> > Maybe create a non-pci generic version instead?
> > 
> 
> I'm not sure you can (I spent quite a bit of time attempting to do just that
> when I wrote the original). It just so happens that both of the bridges we
> have at the moment are PCI devices,

Doesn't something like this work?

void *generic_vme_pci_alloc_consistent(struct device *parent, size_t size,
				       dma_addr_t *dma)
[implementation...]
EXPORT_SYMBOL(generic_vme_pci_alloc_consistent);
(if necessary)

void *generic_vme_pci_free_consistent(struct device *parent, size_t size,
				      void *vaddr, dma_addr_t dma)
[implementation...]
EXPORT_SYMBOL(generic_vme_pci_free_consistent);

and uses like:

+       ca91cx42_bridge->alloc_consistent = generic_vme_pci_alloc_consistent;
+       ca91cx42_bridge->free_consistent = generic_vme_pci_free_consistent;

?


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 13:51       ` Joe Perches
@ 2011-08-10 13:55         ` Martyn Welch
  2011-08-10 14:30           ` Joe Perches
  0 siblings, 1 reply; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 13:55 UTC (permalink / raw)
  To: Joe Perches; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On 10/08/11 14:51, Joe Perches wrote:
> On Wed, 2011-08-10 at 14:34 +0100, Martyn Welch wrote:
>> On 10/08/11 14:12, Joe Perches wrote:
>>> On Wed, 2011-08-10 at 11:33 +0200, Manohar Vanga wrote:
>>>> Make PCI dependent functions ([alloc|free]_consistent() in
>>>> 'vme.c') bridge specific. By removing the dependency of the
>>>> VME bridge framework on PCI, this patch allows for addition of
>>>> non-PCI based VME bridges.
>>> []
>>>> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
>>> []
>>>> +void *ca91cx42_alloc_consistent(struct device *parent, size_t size,
>>>> +	dma_addr_t *dma)
>>>> +{
>>>> +	struct pci_dev *pdev;
>>>> +
>>>> +	/* Find pci_dev container of dev */
>>>> +	pdev = container_of(parent, struct pci_dev, dev);
>>>> +
>>>> +	return pci_alloc_consistent(pdev, size, dma);
>>>> +}
>>>> +
>>>> +void ca91cx42_free_consistent(struct device *parent, size_t size, void *vaddr,
>>>> +	dma_addr_t dma)
>>>> +{
>>>> +	struct pci_dev *pdev;
>>>> +
>>>> +	/* Find pci_dev container of dev */
>>>> +	pdev = container_of(parent, struct pci_dev, dev);
>>>> +
>>>> +	pci_free_consistent(pdev, size, vaddr, dma);
>>>> +}
>>> []
>>>> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
>>> []
>>>> @@ -2122,6 +2122,28 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
>>>>  	return (int)slot;
>>>>  }
>>>>  
>>>> +void *tsi148_alloc_consistent(struct device *parent, size_t size,
>>>> +	dma_addr_t *dma)
>>>> +{
>>>> +	struct pci_dev *pdev;
>>>> +
>>>> +	/* Find pci_dev container of dev */
>>>> +	pdev = container_of(parent, struct pci_dev, dev);
>>>> +
>>>> +	return pci_alloc_consistent(pdev, size, dma);
>>>> +}
>>>> +
>>>> +void tsi148_free_consistent(struct device *parent, size_t size, void *vaddr,
>>>> +	dma_addr_t dma)
>>>> +{
>>>> +	struct pci_dev *pdev;
>>>> +
>>>> +	/* Find pci_dev container of dev */
>>>> +	pdev = container_of(parent, struct pci_dev, dev);
>>>> +
>>>> +	pci_free_consistent(pdev, size, vaddr, dma);
>>>> +}
>>>> +
>>>>  static int __init tsi148_init(void)
>>>>  {
>>>>  	return pci_register_driver(&tsi148_driver);
>>>
>>> Except for the name, those 2 blocks are identical.
>>> Maybe create a non-pci generic version instead?
>>>
>>
>> I'm not sure you can (I spent quite a bit of time attempting to do just that
>> when I wrote the original). It just so happens that both of the bridges we
>> have at the moment are PCI devices,
> 
> Doesn't something like this work?
> 
> void *generic_vme_pci_alloc_consistent(struct device *parent, size_t size,
> 				       dma_addr_t *dma)
> [implementation...]
> EXPORT_SYMBOL(generic_vme_pci_alloc_consistent);
> (if necessary)
> 
> void *generic_vme_pci_free_consistent(struct device *parent, size_t size,
> 				      void *vaddr, dma_addr_t dma)
> [implementation...]
> EXPORT_SYMBOL(generic_vme_pci_free_consistent);
> 
> and uses like:
> 
> +       ca91cx42_bridge->alloc_consistent = generic_vme_pci_alloc_consistent;
> +       ca91cx42_bridge->free_consistent = generic_vme_pci_free_consistent;
> 
> ?
> 

Could. Though we'd have to put this in a new common file for use by
vme_tsi148.c and vme_ca91cx42.c as the point of this was to get it out of the
VME core code. I'm just not sure that it's worth it.

Martyn

-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 13:55         ` Martyn Welch
@ 2011-08-10 14:30           ` Joe Perches
  2011-08-10 14:33             ` Martyn Welch
  0 siblings, 1 reply; 25+ messages in thread
From: Joe Perches @ 2011-08-10 14:30 UTC (permalink / raw)
  To: Martyn Welch; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On Wed, 2011-08-10 at 14:55 +0100, Martyn Welch wrote:
> On 10/08/11 14:51, Joe Perches wrote:
> > On Wed, 2011-08-10 at 14:34 +0100, Martyn Welch wrote:
> >> On 10/08/11 14:12, Joe Perches wrote:
> >>> Except for the name, those 2 blocks are identical.
> >>> Maybe create a non-pci generic version instead?
> >> I'm not sure you can (I spent quite a bit of time attempting to do just that
> >> when I wrote the original).
> > Doesn't something like this work?
[]
> Could. Though we'd have to put this in a new common file for use by
> vme_tsi148.c and vme_ca91cx42.c as the point of this was to get it out of the
> VME core code. I'm just not sure that it's worth it.

That's a bit different than what you wrote earlier.
No worries then.

cheers, Joe



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific
  2011-08-10 14:30           ` Joe Perches
@ 2011-08-10 14:33             ` Martyn Welch
  0 siblings, 0 replies; 25+ messages in thread
From: Martyn Welch @ 2011-08-10 14:33 UTC (permalink / raw)
  To: Joe Perches; +Cc: Manohar Vanga, gregkh, cota, devel, linux-kernel

On 10/08/11 15:30, Joe Perches wrote:
> On Wed, 2011-08-10 at 14:55 +0100, Martyn Welch wrote:
>> On 10/08/11 14:51, Joe Perches wrote:
>>> On Wed, 2011-08-10 at 14:34 +0100, Martyn Welch wrote:
>>>> On 10/08/11 14:12, Joe Perches wrote:
>>>>> Except for the name, those 2 blocks are identical.
>>>>> Maybe create a non-pci generic version instead?
>>>> I'm not sure you can (I spent quite a bit of time attempting to do just that
>>>> when I wrote the original).
>>> Doesn't something like this work?
> []
>> Could. Though we'd have to put this in a new common file for use by
>> vme_tsi148.c and vme_ca91cx42.c as the point of this was to get it out of the
>> VME core code. I'm just not sure that it's worth it.
> 
> That's a bit different than what you wrote earlier.
> No worries then.
> 

Sorry, I originally read your question to mean "create a function that didn't
use PCI specific functions".

Martyn

-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/6] staging: vme: add functions for bridge module refcounting
  2011-08-10 10:09   ` Martyn Welch
@ 2011-08-10 19:14     ` Emilio G. Cota
  0 siblings, 0 replies; 25+ messages in thread
From: Emilio G. Cota @ 2011-08-10 19:14 UTC (permalink / raw)
  To: Martyn Welch; +Cc: Manohar Vanga, gregkh, devel, linux-kernel

On Wed, Aug 10, 2011 at 11:09:56 +0100, Martyn Welch wrote:
> On 10/08/11 10:33, Manohar Vanga wrote:
> >  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> > @@ -1525,9 +1573,13 @@ static int vme_bus_probe(struct device *dev)
> >  	driver = dev_to_vme_driver(dev);
> >  	bridge = dev_to_bridge(dev);
> >  
> > +	vme_bridge_get(bridge->num);

Here the return pointer is not being checked, although things
would need to be very wrong for this to fail here.

> >  	if (driver->probe != NULL)
> >  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> >  
> > +	if (retval)
> > +		vme_bridge_put(bridge);
> > +
> 
> Given that we are recounting automatically in probe and remove, under what
> circumstances would we need to call them separately?

I agree, it's hard to imagine a scenario where "doubly locking"
the module would make any sense at all.

It would also be nice to:

- Increment the refcount of the bridge device with get_device(),
  this way we protect against the device removal as well.
  Bonus points if we increment the bridge module's refcount
  once per device driver using it and the bridge device's
  refcount once per device sitting on it.

- re-think what the return value of vme_bridge_get should be;
  guess my original idea was to return a reference to a bridge,
  which would be safely used by device drivers from there
  on. Since apparently we're not going this way, then perhaps
  an int (returning 0 on success and -ENXIO on failure) would
  be clearer.

		Emilio

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers
  2011-08-10 12:50       ` Martyn Welch
@ 2011-08-23 22:06         ` Greg KH
  0 siblings, 0 replies; 25+ messages in thread
From: Greg KH @ 2011-08-23 22:06 UTC (permalink / raw)
  To: Martyn Welch; +Cc: gregkh, cota, devel, linux-kernel

On Wed, Aug 10, 2011 at 01:50:17PM +0100, Martyn Welch wrote:
> On 10/08/11 11:41, Manohar Vanga wrote:
> > Hey Martyn,
> > 
> >> I'm sorry, I'm still simply not convinced by this patch:
> >>
> >> 1) For a single bus driver (i.e. in the situation where we have 2 bridges of
> >> the same type), the numbering of the buses is still dependent on the order
> >> that they are found in the scan.
> > 
> > Yes this is still a bug. But this patch doesn't address this case.
> > 
> >> 2) If the bridge drivers are loaded as modules, I have a feeling they will be
> >> loaded sequentially and therefore the order of the bridges would only change
> >> if the order of the loading of the drivers changed.
> > 
> > And this is a major problem when it comes to multiple bridges of differing
> > types. What I'm saying is that this patch simply fixes this one problematic
> > case. We can move this out as soon as we have a more robust implementation.
> > 
> > As of now however, I think applying this is useful as we have a decent
> > workaround to the problem. If you want I can make the fact of it being
> > applicable only to cases with differing bridges explicit in the commit
> > message.
> > 
> 
> The problem is, I'm not convinced that this is the correct approach to take. I
> think this should be parsed from sysfs dynamically (which may require some
> work). I shall use the ethernet devices on my machine as an example:
> 
> I have 2 ethernet devices (and lo) on my machine:
> 
> $ ls /sys/class/net/
> eth0  eth1  lo
> $
> 
> These are symlinks and I can quite quickly find out which each of these
> devices in (based on topology):
> $ ls -l /sys/class/net/
> total 0
> lrwxrwxrwx 1 root root 0 2011-08-10 11:56 eth0 ->
> ../../devices/pci0000:00/0000:00:19.0/net/eth0
> lrwxrwxrwx 1 root root 0 2011-08-10 11:56 eth1 ->
> ../../devices/pci0000:00/0000:00:1c.1/0000:03:00.0/net/eth1
> lrwxrwxrwx 1 root root 0 2011-08-10 11:56 lo -> ../../devices/virtual/net/lo
> $
> 
> I'd think that this contains the information that you have in the config file
> (based on the previous discussion we had) and would allow you to map the bus
> numbering after booting.
> 
> To do this, I think we need to register a class called "vme", I guess in
> vme_init() and add a call to class_device_register in vme_register_bridge and
> a call to class_device_unregister in vme_unregister_bridge.
> 
> Greg: Is that right?

Yes.

> I'm really not convinced that the solution presented in this patch is suitable
> for inclusion upstream.

I agree.  Bus numbers are dynamic and should NEVER be depended on by
userspace to be static and unchanging on reboots.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2011-08-23 22:16 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-10  9:33 [PATCH 0/6] [RESEND] VME framework fixes Manohar Vanga
2011-08-10  9:33 ` [PATCH 1/6] staging: vme: allow explicit assignment of bus numbers Manohar Vanga
2011-08-10 10:02   ` Martyn Welch
2011-08-10 10:41     ` Manohar Vanga
2011-08-10 12:50       ` Martyn Welch
2011-08-23 22:06         ` Greg KH
2011-08-10  9:33 ` [PATCH 2/6] staging: vme: make [alloc|free]_consistent bridge specific Manohar Vanga
2011-08-10 10:04   ` Martyn Welch
2011-08-10 13:24     ` Dan Carpenter
2011-08-10 13:12   ` Joe Perches
2011-08-10 13:34     ` Martyn Welch
2011-08-10 13:51       ` Joe Perches
2011-08-10 13:55         ` Martyn Welch
2011-08-10 14:30           ` Joe Perches
2011-08-10 14:33             ` Martyn Welch
2011-08-10  9:33 ` [PATCH 3/6] staging: vme: keep track of registered buses Manohar Vanga
2011-08-10 10:06   ` Martyn Welch
2011-08-10  9:33 ` [PATCH 4/6] staging: vme: add functions for bridge module refcounting Manohar Vanga
2011-08-10 10:09   ` Martyn Welch
2011-08-10 19:14     ` Emilio G. Cota
2011-08-10  9:33 ` [PATCH 5/6] staging: vme: add struct vme_dev for VME devices Manohar Vanga
2011-08-10 10:14   ` Martyn Welch
2011-08-10 10:33     ` Manohar Vanga
2011-08-10  9:33 ` [PATCH 6/6] staging: vme: make match() driver specific to improve non-VME64x support Manohar Vanga
2011-08-10 10:18   ` Martyn Welch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox