[PATCH 0/5] [RESEND v2] VME Framework Fixes

[PATCH 0/5] [RESEND v2] VME Framework Fixes

[Manohar Vanga]

Hi Martyn,

This is the updated list of patches. I have removed the bus numbering
patch as we can work on it further before adding it in. Some notes on
the various patches below:

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

Shortened printk messages and used pr_err where lines were too long.

  staging: vme: keep track of registered buses

Made modifications based on the removal of the bus-numbering patch. I
have checked the patch but I might have introduced some bug so a third
person check is a good idea.

  staging: vme: add functions for bridge module refcounting

Changed the return value of the 'vme_bridge_get' function. I will add
device refcounting later,

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

Updated vme_api.txt and cleaned up the patches here and there.

Thanks!

--
/manohar

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 |   26 ++
 drivers/staging/vme/bridges/vme_tsi148.c   |   26 ++
 drivers/staging/vme/devices/vme_user.c     |   67 ++---
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  383 ++++++++++++++++------------
 drivers/staging/vme/vme.h                  |   56 +++-
 drivers/staging/vme/vme_api.txt            |   79 ++++---
 drivers/staging/vme/vme_bridge.h           |   17 +-
 8 files changed, 393 insertions(+), 263 deletions(-)

-- 
1.7.4.1

[PATCH 1/5] staging: vme: make [alloc|free]_consistent bridge specific

[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 5122c13..0e4feac 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1500,6 +1500,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);
@@ -1769,6 +1791,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 9c53951..6c1167c 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2114,6 +2114,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);
@@ -2443,6 +2465,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 c078ce3..d243a4a 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) {
+		pr_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) {
+		pr_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

Re: [PATCH 1/5] staging: vme: make [alloc|free]_consistent bridge specific

[Martyn Welch]

On 12/08/11 11:30, 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>

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 5122c13..0e4feac 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1500,6 +1500,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);
> @@ -1769,6 +1791,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 9c53951..6c1167c 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2114,6 +2114,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);
> @@ -2443,6 +2465,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 c078ce3..d243a4a 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) {
> +		pr_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) {
> +		pr_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

[PATCH 2/5] staging: vme: keep track of registered buses

[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        |   38 +++++++++++++++++++++++---------------
 drivers/staging/vme/vme_bridge.h |    1 +
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index d243a4a..feb2d00 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,27 +1310,32 @@ EXPORT_SYMBOL(vme_slot_get);
 
 /* - Bridge Registration --------------------------------------------------- */
 
-static int vme_alloc_bus_num(void)
+static int vme_add_bus(struct vme_bridge *bridge)
 {
 	int i;
+	int ret = -1;
 
-	mutex_lock(&vme_bus_num_mtx);
+	mutex_lock(&vme_buses_lock);
 	for (i = 0; i < sizeof(vme_bus_numbers) * 8; i++) {
-		if (((vme_bus_numbers >> i) & 0x1) == 0) {
-			vme_bus_numbers |= (0x1 << i);
+		if ((vme_bus_numbers & (1 << i)) == 0) {
+			vme_bus_numbers |= (1 << i);
+			bridge->num = i;
+			list_add_tail(&bridge->bus_list, &vme_bus_list);
+			ret = 0;
 			break;
 		}
 	}
-	mutex_unlock(&vme_bus_num_mtx);
+	mutex_unlock(&vme_buses_lock);
 
-	return i;
+	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)
@@ -1338,7 +1344,9 @@ int vme_register_bridge(struct vme_bridge *bridge)
 	int retval;
 	int i;
 
-	bridge->num = vme_alloc_bus_num();
+	retval = vme_add_bus(bridge);
+	if (retval)
+		return 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
@@ -1370,7 +1378,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);
@@ -1385,7 +1393,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

Re: [PATCH 2/5] staging: vme: keep track of registered buses

[Martyn Welch]

On 12/08/11 11:30, 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>

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

> ---
>  drivers/staging/vme/vme.c        |   38 +++++++++++++++++++++++---------------
>  drivers/staging/vme/vme_bridge.h |    1 +
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index d243a4a..feb2d00 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,27 +1310,32 @@ EXPORT_SYMBOL(vme_slot_get);
>  
>  /* - Bridge Registration --------------------------------------------------- */
>  
> -static int vme_alloc_bus_num(void)
> +static int vme_add_bus(struct vme_bridge *bridge)
>  {
>  	int i;
> +	int ret = -1;
>  
> -	mutex_lock(&vme_bus_num_mtx);
> +	mutex_lock(&vme_buses_lock);
>  	for (i = 0; i < sizeof(vme_bus_numbers) * 8; i++) {
> -		if (((vme_bus_numbers >> i) & 0x1) == 0) {
> -			vme_bus_numbers |= (0x1 << i);
> +		if ((vme_bus_numbers & (1 << i)) == 0) {
> +			vme_bus_numbers |= (1 << i);
> +			bridge->num = i;
> +			list_add_tail(&bridge->bus_list, &vme_bus_list);
> +			ret = 0;
>  			break;
>  		}
>  	}
> -	mutex_unlock(&vme_bus_num_mtx);
> +	mutex_unlock(&vme_buses_lock);
>  
> -	return i;
> +	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)
> @@ -1338,7 +1344,9 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  	int retval;
>  	int i;
>  
> -	bridge->num = vme_alloc_bus_num();
> +	retval = vme_add_bus(bridge);
> +	if (retval)
> +		return 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
> @@ -1370,7 +1378,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);
> @@ -1385,7 +1393,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

[PATCH 3/5] staging: vme: add functions for bridge module refcounting

[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                  |    1 -
 drivers/staging/vme/vme_bridge.h           |    1 +
 5 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 0e4feac..b80cb12 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1673,6 +1673,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 6c1167c..b0b434a 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2312,6 +2312,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 feb2d00..664eae0 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -52,6 +52,48 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
 }
 
 /*
+ * Increments the reference count of a VME bridge.
+ *
+ * On success, it can be assumed that the bridge won't be removed until
+ * the corresponding call to vme_put_bridge(). This along with
+ * vme_bridge_put are automatically called and VME device drivers need
+ * not worry about using this.
+ */
+int vme_bridge_get(unsigned int bus_id)
+{
+	int ret = -1;
+	struct vme_bridge *bridge;
+
+	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))
+				ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&vme_buses_lock);
+	return ret;
+}
+
+/*
+ * 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);
+}
+
+/*
  * Find the bridge that the resource is associated with.
  */
 static struct vme_bridge *find_bridge(struct vme_resource *resource)
@@ -1496,14 +1538,20 @@ static int vme_bus_probe(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
-	int retval = -ENODEV;
+	int retval = 0;
 
 	driver = dev_to_vme_driver(dev);
 	bridge = dev_to_bridge(dev);
 
+	if (vme_bridge_get(bridge->num))
+		return -ENXIO;
+
 	if (driver->probe != NULL)
 		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
 
+	if (driver->probe && retval)
+		vme_bridge_put(bridge);
+
 	return retval;
 }
 
@@ -1511,7 +1559,7 @@ static int vme_bus_remove(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
-	int retval = -ENODEV;
+	int retval = 0;
 
 	driver = dev_to_vme_driver(dev);
 	bridge = dev_to_bridge(dev);
@@ -1519,6 +1567,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 4155d8c..eb00a5e 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -165,6 +165,5 @@ int vme_slot_get(struct device *);
 int vme_register_driver(struct vme_driver *);
 void vme_unregister_driver(struct vme_driver *);
 
-
 #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

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Emilio G. Cota]

On Fri, Aug 12, 2011 at 12:30:49 +0200, 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.
(snip)
> @@ -52,6 +52,48 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
(snip)
> +int vme_bridge_get(unsigned int bus_id)

If it isn't exported, it should be declared as static.

> +{
> +	int ret = -1;

hmm perhaps -ENXIO here would be better, since the outcome
of this function is either that or success.

> +	struct vme_bridge *bridge;
> +
> +	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");

Don't do this; it will throw a false warning if the kernel is
built without module support. Note that in that case

	THIS_MODULE == (struct module *)0.

try_module_get() and module_put() do the right thing for all
possible configs. Trust them.

> +			else if (try_module_get(bridge->owner))
> +				ret = 0;
> +			break;
> +void vme_bridge_put(struct vme_bridge *bridge)

should be static as well

> +{
> +	if (!bridge->owner)
> +		dev_warn(bridge->parent, "bridge->owner not set\n");
> +	else
> +		module_put(bridge->owner);

ditto, remove the warning.

> +
> +/*
>   * Find the bridge that the resource is associated with.
>   */
>  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> @@ -1496,14 +1538,20 @@ static int vme_bus_probe(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> -	int retval = -ENODEV;
> +	int retval = 0;
>  
>  	driver = dev_to_vme_driver(dev);
>  	bridge = dev_to_bridge(dev);
>  
> +	if (vme_bridge_get(bridge->num))
> +		return -ENXIO;
> +
>  	if (driver->probe != NULL)
>  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
>  
> +	if (driver->probe && retval)
> +		vme_bridge_put(bridge);

If the driver doesn't provide a .probe, we would still increment
the refcount of the bridge module. Is that reasonable? I dunno.

If there's no .probe then the device is doing something
weird, and probably either it doesn't have much to do with a
particular bridge (i.e. it manages no "real" devices) or
it'd need to manage its own resources (in which case we could
easily export vme_bridge_get/put.)

Perhaps then the following would be more appropriate,
what do you think?

+	if (driver->probe) {
+		if (vme_bridge_get(bridge->num))
+			return -ENXIO; /* although this could change, see above comment */
+
 		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		if (retval)
+			vme_bridge_put(bridge);
+	}
+
 	return retval;

.. and then remember to do
+ 	if (probe)
+ 		vme_bridge_put(bridge)
in vme_bus_remove(), which in your patch is unconditional (correctly
matching the unconditional get() in vme_bus_probe)

> @@ -1519,6 +1567,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 4155d8c..eb00a5e 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -165,6 +165,5 @@ int vme_slot_get(struct device *);
>  int vme_register_driver(struct vme_driver *);
>  void vme_unregister_driver(struct vme_driver *);
>  
> -
>  #endif /* _VME_H_ */

I'm certainly no checkpatch taliban, but guess you probably
didn't want to send this line change.

Cheers

		Emilio

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Manohar Vanga]

> If the driver doesn't provide a .probe, we would still increment
> the refcount of the bridge module. Is that reasonable? I dunno.
> 
> If there's no .probe then the device is doing something
> weird, and probably either it doesn't have much to do with a
> particular bridge (i.e. it manages no "real" devices) or
> it'd need to manage its own resources (in which case we could
> easily export vme_bridge_get/put.)
> 
> Perhaps then the following would be more appropriate,
> what do you think?
> 
> +	if (driver->probe) {
> +		if (vme_bridge_get(bridge->num))
> +			return -ENXIO; /* although this could change, see above comment */
> +
>  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> +		if (retval)
> +			vme_bridge_put(bridge);
> +	}
> +
>  	return retval;
> 
> .. and then remember to do
> + 	if (probe)
> + 		vme_bridge_put(bridge)
> in vme_bus_remove(), which in your patch is unconditional (correctly
> matching the unconditional get() in vme_bus_probe)

I picked this default behaviour from the PCI driver code (drivers/pci/pci-driver.c):

	static int pci_device_probe(struct device * dev)
	{
		...
		pci_dev_get(pci_dev);
		error = __pci_device_probe(drv, pci_dev);
		if (error)
			pci_dev_put(pci_dev);
	
		return error;
	}

The __pci_device_probe() function checks if probe is present or not.

> I'm certainly no checkpatch taliban, but guess you probably
> didn't want to send this line change.

Gak! Will cleanup and resend.

--
/manohar

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Emilio G. Cota]

On Mon, Aug 15, 2011 at 12:05:03 +0200, Manohar Vanga wrote:
> > If the driver doesn't provide a .probe, we would still increment
> > the refcount of the bridge module. Is that reasonable? I dunno.
(snip)
> I picked this default behaviour from the PCI driver code (drivers/pci/pci-driver.c):
> 
> 	static int pci_device_probe(struct device * dev)
> 	{
> 		...
> 		pci_dev_get(pci_dev);
> 		error = __pci_device_probe(drv, pci_dev);
> 		if (error)
> 			pci_dev_put(pci_dev);
> 	
> 		return error;
> 	}
> 
> The __pci_device_probe() function checks if probe is present or not.

There is a subtle difference here; pci_dev is the _struct pci_device
of the device_ being probed, and pci_dev_get increments its refcount.

In our case vme_bridge_get increments the refcount of the _bridge's
module_; my concern was that if there was no .probe (which would
only happen with a few special drivers), we'd increment the refcount
of the bridge for perhaps no reason.

Anyway we don't have such esoteric drivers yet, so I wouldn't worry
about this too much.

		Emilio

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Martyn Welch]

On 13/08/11 08:47, Emilio G. Cota wrote:
> On Fri, Aug 12, 2011 at 12:30:49 +0200, Manohar Vanga wrote:
>> +	struct vme_bridge *bridge;
>> +
>> +	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");
> 
> Don't do this; it will throw a false warning if the kernel is
> built without module support. Note that in that case
> 
> 	THIS_MODULE == (struct module *)0.
> 
> try_module_get() and module_put() do the right thing for all
> possible configs. Trust them.
> 

I can confirm that this does break when the bridge is compiled into the kernel.

Martyn

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

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Manohar Vanga]

Hey Martyn,

Thanks for testing this out. Sorry for the inactivity last week, I have been
busy with some unrelated work. I will resend the patches by the end of this
week with the correct fixes.

Thanks again!

On Mon, Aug 22, 2011 at 01:24:59PM +0100, Martyn Welch wrote:
> On 13/08/11 08:47, Emilio G. Cota wrote:
> > On Fri, Aug 12, 2011 at 12:30:49 +0200, Manohar Vanga wrote:
> >> +	struct vme_bridge *bridge;
> >> +
> >> +	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");
> > 
> > Don't do this; it will throw a false warning if the kernel is
> > built without module support. Note that in that case
> > 
> > 	THIS_MODULE == (struct module *)0.
> > 
> > try_module_get() and module_put() do the right thing for all
> > possible configs. Trust them.
> > 
> 
> I can confirm that this does break when the bridge is compiled into the kernel.
> 
> Martyn
> 
> -- 
> Martyn Welch (Principal Software Engineer) | Registered in England and
> GE Intelligent Platforms                   | Wales (3828642) at 100
> T +44(0)1327322748                         | Barbirolli Square, Manchester,
> E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

-- 
/manohar

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Emilio G. Cota]

On Fri, Aug 12, 2011 at 12:30:49 +0200, 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()'.
(snip)
> +int vme_bridge_get(unsigned int bus_id)
(snip)
> +void vme_bridge_put(struct vme_bridge *bridge)

Note the input parameter imbalance; in fact this is serious
(see my email on patch 5) because _get() needs to acquire
vme_buses_lock, whereas _put() doesn't. Since a caller with
bridge has bridge->num, but the opposite doesn't hold (num
doesn't give you the bridge without acquiring vme_buses_lock),
it seems reasonable to me to take the bus_id as the input for
both functions, because the requirements on the caller are lower.

But the locking needs to be handled with care, see my reply
to patch 5.

		Emilio

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Martyn Welch]

On 13/08/11 09:54, Emilio G. Cota wrote:
> On Fri, Aug 12, 2011 at 12:30:49 +0200, 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()'.
> (snip)
>> +int vme_bridge_get(unsigned int bus_id)
> (snip)
>> +void vme_bridge_put(struct vme_bridge *bridge)
> 
> Note the input parameter imbalance; in fact this is serious
> (see my email on patch 5) because _get() needs to acquire
> vme_buses_lock, whereas _put() doesn't. Since a caller with
> bridge has bridge->num, but the opposite doesn't hold (num
> doesn't give you the bridge without acquiring vme_buses_lock),
> it seems reasonable to me to take the bus_id as the input for
> both functions, because the requirements on the caller are lower.
> 

Patch 4 makes changes the struct vme_bridge to struct vme_dev. Looking at the
callers we are then effectively doing:

vme_bridge_get(vme_dev.id)

Then in vme_bridge_get(), looping through all the buses to find the one with
the correct id...

We could just pass in the struct vme_dev to both functions.

Martyn

> But the locking needs to be handled with care, see my reply
> to patch 5.
> 
> 		Emilio
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


-- 
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

Re: [PATCH 3/5] staging: vme: add functions for bridge module refcounting

[Martyn Welch]

On 19/08/11 09:32, Martyn Welch wrote:
> On 13/08/11 09:54, Emilio G. Cota wrote:
>> On Fri, Aug 12, 2011 at 12:30:49 +0200, 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()'.
>> (snip)
>>> +int vme_bridge_get(unsigned int bus_id)
>> (snip)
>>> +void vme_bridge_put(struct vme_bridge *bridge)
>>
>> Note the input parameter imbalance; in fact this is serious
>> (see my email on patch 5) because _get() needs to acquire
>> vme_buses_lock, whereas _put() doesn't. Since a caller with
>> bridge has bridge->num, but the opposite doesn't hold (num
>> doesn't give you the bridge without acquiring vme_buses_lock),
>> it seems reasonable to me to take the bus_id as the input for
>> both functions, because the requirements on the caller are lower.
>>
> 
> Patch 4 makes changes the struct vme_bridge to struct vme_dev. Looking at the
> callers we are then effectively doing:
> 

Patch 5 even.

Martyn

> vme_bridge_get(vme_dev.id)
> 
> Then in vme_bridge_get(), looping through all the buses to find the one with
> the correct id...
> 
> We could just pass in the struct vme_dev to both functions.
> 
> Martyn
> 
>> But the locking needs to be handled with care, see my reply
>> to patch 5.
>>
>> 		Emilio
>>
>> _______________________________________________
>> devel mailing list
>> devel@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
> 
> 


-- 
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

[PATCH 4/5] staging: vme: add struct vme_dev for VME devices

[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_api.txt        |   41 ++++++++---
 drivers/staging/vme/vme_bridge.h       |    5 +-
 5 files changed, 115 insertions(+), 102 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 664eae0..ff415fb 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);
 }
 
 /*
@@ -278,7 +274,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;
@@ -287,7 +283,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;
@@ -434,7 +430,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;
@@ -443,7 +439,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;
@@ -692,7 +688,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;
@@ -703,7 +700,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;
@@ -1036,13 +1033,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;
@@ -1079,11 +1076,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;
@@ -1114,11 +1111,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;
@@ -1141,7 +1138,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;
@@ -1149,7 +1146,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;
@@ -1330,11 +1327,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;
@@ -1382,7 +1379,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;
 
@@ -1395,20 +1392,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;
 	}
@@ -1417,8 +1417,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;
@@ -1428,12 +1428,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);
 }
@@ -1459,32 +1459,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)
@@ -1495,15 +1469,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) {
@@ -1523,7 +1499,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++;
@@ -1538,16 +1514,18 @@ static int vme_bus_probe(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = 0;
 
 	driver = dev_to_vme_driver(dev);
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 
 	if (vme_bridge_get(bridge->num))
 		return -ENXIO;
 
 	if (driver->probe != NULL)
-		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->probe(vdev);
 
 	if (driver->probe && retval)
 		vme_bridge_put(bridge);
@@ -1559,13 +1537,15 @@ static int vme_bus_remove(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = 0;
 
 	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 eb00a5e..7269fc0 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -91,17 +91,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;
 };
@@ -112,7 +129,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);
@@ -120,7 +137,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);
@@ -132,7 +149,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);
@@ -145,12 +162,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);
@@ -160,7 +177,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_api.txt b/drivers/staging/vme/vme_api.txt
index 4910e92..03f4813 100644
--- a/drivers/staging/vme/vme_api.txt
+++ b/drivers/staging/vme/vme_api.txt
@@ -20,8 +20,8 @@ registration function. The structure is as follows:
 		struct list_head node;
 		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;
 	};
@@ -33,7 +33,26 @@ to the probe routine.
 
 The arguments of the probe routine are as follows:
 
-	probe(struct device *dev, int bus, int slot);
+	probe(struct vme_dev *dev);
+
+The device structure looks like the following:
+
+	struct vme_dev {
+		struct vme_device_id id;
+		struct vme_bridge *bridge;
+		struct device dev;
+	};
+
+The 'bridge' field contains a pointer to the bridge device. The 'id' field
+contains information useful for the probe function:
+
+	struct vme_device_id {
+		int bus;
+		int slot;
+	};
+
+'bus' is the number of the bus the device being probed is on. 'slot' refers
+to the specific slot on the VME bus.
 
 The '.bind_table' is a pointer to an array of type 'vme_device_id':
 
@@ -71,13 +90,13 @@ specific window or DMA channel (which may be used by a different driver) this
 driver allows a resource to be assigned based on the required attributes of the
 driver in question:
 
-	struct vme_resource * vme_master_request(struct device *dev,
+	struct vme_resource * vme_master_request(struct vme_dev *dev,
 		vme_address_t aspace, vme_cycle_t cycle, vme_width_t width);
 
-	struct vme_resource * vme_slave_request(struct device *dev,
+	struct vme_resource * vme_slave_request(struct vme_dev *dev,
 		vme_address_t aspace, vme_cycle_t cycle);
 
-	struct vme_resource *vme_dma_request(struct device *dev,
+	struct vme_resource *vme_dma_request(struct vme_dev *dev,
 		vme_dma_route_t route);
 
 For slave windows these attributes are split into those of type 'vme_address_t'
@@ -301,10 +320,10 @@ status ID combination. Any given combination can only be assigned a single
 callback function. A void pointer parameter is provided, the value of which is
 passed to the callback function, the use of this pointer is user undefined:
 
-	int vme_irq_request(struct device *dev, int level, int statid,
+	int vme_irq_request(struct vme_dev *dev, int level, int statid,
 		void (*callback)(int, int, void *), void *priv);
 
-	void vme_irq_free(struct device *dev, int level, int statid);
+	void vme_irq_free(struct vme_dev *dev, int level, int statid);
 
 The callback parameters are as follows. Care must be taken in writing a callback
 function, callback functions run in interrupt context:
@@ -318,7 +337,7 @@ Interrupt Generation
 The following function can be used to generate a VME interrupt at a given VME
 level and VME status ID:
 
-	int vme_irq_generate(struct device *dev, int level, int statid);
+	int vme_irq_generate(struct vme_dev *dev, int level, int statid);
 
 
 Location monitors
@@ -334,7 +353,7 @@ Location Monitor Management
 The following functions are provided to request the use of a block of location
 monitors and to free them after they are no longer required:
 
-	struct vme_resource * vme_lm_request(struct device *dev);
+	struct vme_resource * vme_lm_request(struct vme_dev *dev);
 
 	void vme_lm_free(struct vme_resource * res);
 
@@ -380,4 +399,4 @@ Slot Detection
 
 This function returns the slot ID of the provided bridge.
 
-	int vme_slot_get(struct device *dev);
+	int vme_slot_get(struct vme_dev *dev);
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

[PATCH 5/5] staging: vme: make match() driver specific to improve non-VME64x support

[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 |   53 +++-----
 drivers/staging/vme/devices/vme_user.h |    2 +-
 drivers/staging/vme/vme.c              |  219 ++++++++++++++++----------------
 drivers/staging/vme/vme.h              |   22 +++-
 drivers/staging/vme/vme_api.txt        |   60 +++++-----
 drivers/staging/vme/vme_bridge.h       |    4 -
 6 files changed, 178 insertions(+), 182 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index bb33dc2..f85be2c 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),
 };
@@ -628,8 +630,6 @@ static struct vme_driver vme_user_driver = {
 static int __init vme_user_init(void)
 {
 	int retval = 0;
-	int i;
-	struct vme_device_id *ids;
 
 	printk(KERN_INFO "VME User Space Access Driver\n");
 
@@ -643,47 +643,36 @@ 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;
-	}
-
-
-	/* Dynamically create the bind table based on module parameters */
-	ids = kzalloc(sizeof(struct vme_device_id) * (bus_num + 1), GFP_KERNEL);
-	if (ids == NULL) {
-		printk(KERN_ERR "%s: Unable to allocate ID table\n",
-			driver_name);
-		retval = -ENOMEM;
-		goto err_id;
-	}
-
-	for (i = 0; i < bus_num; i++) {
-		ids[i].bus = bus[i];
-		/*
-		 * We register the driver against the slot occupied by *this*
-		 * card, since it's really a low level way of controlling
-		 * the VME bridge
-		 */
-		ids[i].slot = VME_SLOT_CURRENT;
+			driver_name, VME_USER_BUS_MAX);
+		bus_num = VME_USER_BUS_MAX;
 	}
 
-	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;
 
 	return retval;
 
 err_reg:
-	kfree(ids);
-err_id:
 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
@@ -896,8 +885,6 @@ static int __devexit vme_user_remove(struct vme_dev *dev)
 static void __exit vme_user_exit(void)
 {
 	vme_unregister_driver(&vme_user_driver);
-
-	kfree(vme_user_driver.bind_table);
 }
 
 
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 ff415fb..d5807c2 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.
@@ -1379,152 +1376,161 @@ 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;
-	}
+	vme_drv = container_of(drv, struct vme_driver, driver);
 
-	i = 0;
-	while ((driver->bind_table[i].bus != 0) ||
-		(driver->bind_table[i].slot != 0)) {
+	if (dev->platform_data == vme_drv) {
+		struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-		if (bridge->num == driver->bind_table[i].bus) {
-			if (num == driver->bind_table[i].slot)
-				return 1;
+		if (vme_drv->match && vme_drv->match(vdev))
+			return 1;
 
-			if (driver->bind_table[i].slot == VME_SLOT_ALL)
-				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;
-	struct vme_driver *driver;
-	struct vme_dev *vdev;
 	int retval = 0;
-
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
+	struct vme_bridge *bridge = vdev->bridge;
 
 	if (vme_bridge_get(bridge->num))
 		return -ENXIO;
 
-	if (driver->probe != NULL)
+	driver = dev->platform_data;
+	if (driver->probe)
 		retval = driver->probe(vdev);
 
 	if (driver->probe && retval)
@@ -1535,16 +1541,13 @@ static int vme_bus_probe(struct device *dev)
 
 static int vme_bus_remove(struct device *dev)
 {
-	struct vme_bridge *bridge;
-	struct vme_driver *driver;
-	struct vme_dev *vdev;
 	int retval = 0;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
+	struct vme_bridge *bridge = vdev->bridge;
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	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 7269fc0..f67f04d 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -88,15 +88,21 @@ 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_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;
 };
@@ -106,21 +112,25 @@ 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 *);
@@ -179,7 +189,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 *);
 
 #endif /* _VME_H_ */
diff --git a/drivers/staging/vme/vme_api.txt b/drivers/staging/vme/vme_api.txt
index 03f4813..ffe28c3 100644
--- a/drivers/staging/vme/vme_api.txt
+++ b/drivers/staging/vme/vme_api.txt
@@ -18,24 +18,37 @@ registration function. The structure is as follows:
 
 	struct vme_driver {
 		struct list_head node;
-		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;
+		const char *name;
+		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;
 	};
 
-At the minimum, the '.name', '.probe' and '.bind_table' elements of this
-structure should be correctly set. The '.name' element is a pointer to a string
-holding the device driver's name. The '.probe' element should contain a pointer
-to the probe routine.
+At the minimum, the '.name', '.match' and '.probe' elements of this structure
+should be correctly set. The '.name' element is a pointer to a string holding
+the device driver's name.
 
-The arguments of the probe routine are as follows:
+The '.match' function allows controlling the number of devices that need to
+be registered. The match function should return 1 if a device should be
+probed and 0 otherwise. This example match function (from vme_user.c) limits
+the number of devices probed to one:
 
-	probe(struct vme_dev *dev);
+	#define VME_USER_BUS_MAX	1
+	...
+	static int vme_user_match(struct vme_dev *vdev)
+	{
+		if (vdev->id.num >= VME_USER_BUS_MAX)
+			return 0;
+		return 1;
+	}
 
-The device structure looks like the following:
+The '.probe' element should contain a pointer to the probe routine. The
+probe routine is passed a 'struct vme_dev' pointer as an argument. The
+'struct vme_dev' structure looks like the following:
 
 	struct vme_dev {
 		struct vme_device_id id;
@@ -49,25 +62,12 @@ contains information useful for the probe function:
 	struct vme_device_id {
 		int bus;
 		int slot;
+		int num;
 	};
 
-'bus' is the number of the bus the device being probed is on. 'slot' refers
-to the specific slot on the VME bus.
-
-The '.bind_table' is a pointer to an array of type 'vme_device_id':
-
-	struct vme_device_id {
-		int bus;
-		int slot;
-	};
-
-Each structure in this array should provide a bus and slot number where the core
-should probe, using the driver's probe routine, for a device on the specified
-VME bus.
-
-The VME subsystem supports a single VME driver per 'slot'. There are considered
-to be 32 slots per bus, one for each slot-ID as defined in the ANSI/VITA 1-1994
-specification and are analogious to the physical slots on the VME backplane.
+Here, 'bus' is the number of the bus the device being probed is on. 'slot'
+refers to the specific slot on the VME bus. The 'num' field refers to the
+sequential device ID for this specific driver.
 
 A function is also provided to unregister the driver from the VME core and is
 usually called from the device driver's exit routine:
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

Re: [PATCH 5/5] staging: vme: make match() driver specific to improve non-VME64x support

[Emilio G. Cota]

On Fri, Aug 12, 2011 at 12:30:51 +0200, Manohar Vanga wrote:
> +++ b/drivers/staging/vme/vme.c
(snip)
> +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;

hmm have you tested this? It should deadlock, because as in
patch 3 vme_bridge_get() acquires vme_buses_lock.

An alternative is to call here try_module_get() directly on
bridge->owner, which would succeed in preventing it from being
removed (the lock is held 

> +		mutex_unlock(&vme_buses_lock);
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		mutex_lock(&vme_buses_lock);
> +		vme_bridge_put(bridge);

This, interestingly, wouldn't deadlock, because we pass the bridge
directly. See my second message to patch 3.

> +		if (err)
> +			break;
>  	}
> -	vme_remove_bus(bridge);
> +	mutex_unlock(&vme_buses_lock);
> +	return err;
>  }

The whole loop is admittedly complex. IIRC in my original patch
module_get/put were called here directly, and vme_buses_lock
was unlocked before calling __vme_register_driver_bus()
to avoid a deadlock, because within that function the .probe
methods of the driver would likely call vme_bridge_get().

Now that we don't export them, the loop could be simplified to:


> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		if (err)
> +			break;
>  	}
> +	mutex_unlock(&vme_buses_lock);

This cannot race with a bridge being removed. Let's see how:
If the bridge driver is sane, it will call vme_unregister_bridge()
in its .release method. In there vme_remove_bus is called, and
the thread will try to acquire vme_buses_lock, which is already
held by above loop. Coming back to the loop, the try_get_module
call in vme_bus_probe will fail, because the bridge module
is being removed, and as a result all the devices under that
bridge won't be installed--this is what we wanted.

When the loop finishes we unlock vme_buses_lock and the
removal of the bus completes.

That said, I would ONLY take the simplified loop if a comment was
added to explain the above race. And I'd add that comment
near vme_bus_get/put, because if those are exported one
day, the above loop would need be changed accordingly.

		Emilio

Re: [PATCH 0/5] [RESEND v2] VME Framework Fixes

[Greg KH]

On Fri, Aug 12, 2011 at 12:30:46PM +0200, Manohar Vanga wrote:
> Hi Martyn,
> 
> This is the updated list of patches. I have removed the bus numbering
> patch as we can work on it further before adding it in. Some notes on
> the various patches below:
> 
>   staging: vme: make [alloc|free]_consistent bridge specific

I've applied the first 2 only.

Martyn, my vme patch queue is now empty, if I have missed anything I
should have applied, please resend it and let me know.

thanks,

greg k-h

Re: [PATCH 0/5] [RESEND v2] VME Framework Fixes

[Martyn Welch]

On 23/08/11 23:14, Greg KH wrote:
> On Fri, Aug 12, 2011 at 12:30:46PM +0200, Manohar Vanga wrote:
>> Hi Martyn,
>>
>> This is the updated list of patches. I have removed the bus numbering
>> patch as we can work on it further before adding it in. Some notes on
>> the various patches below:
>>
>>   staging: vme: make [alloc|free]_consistent bridge specific
> 
> I've applied the first 2 only.
> 
> Martyn, my vme patch queue is now empty, if I have missed anything I
> should have applied, please resend it and let me know.
> 
> thanks,
> 
> greg k-h

Thanks Greg, I think that's all the patches that have been agreed so far. I
believe that Manohar is working on a few issues that were found in some of the
other patches.

Martyn

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