[PATCH 0/3] [RESEND v3] VME Driver Changes

[PATCH 0/3] [RESEND v3] VME Driver Changes

[Manohar Vanga]

Hey Martyn,

These are the updated patches with the discussed issues resolved. Let
me know if you spot anything else. A note on the changes:

* staging: vme: add functions for bridge module refcounting

Fixed the parameter imbalance in vme_bridge_[get|put]. They both now
take a struct vme_bridge * as their parameter. I used the vme_bridge *
instead of vme_dev as it more coherent with the names of the functions.


* staging: vme: add struct vme_dev for VME devices

Fixes based on the above changes.


* staging: vme: make match() driver specific to improve non-VME64x
    support

Cleaned up the loop in __vme_register_driver with a note on why there
is no race involved.

Thanks
Manohar

Manohar Vanga (3):
  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 |    2 +
 drivers/staging/vme/bridges/vme_tsi148.c   |    2 +
 drivers/staging/vme/devices/vme_user.c     |   67 +++----
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  305 +++++++++++++++-------------
 drivers/staging/vme/vme.h                  |   55 ++++--
 drivers/staging/vme/vme_api.txt            |   79 +++++---
 drivers/staging/vme/vme_bridge.h           |    6 +-
 8 files changed, 285 insertions(+), 233 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] 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                  |   43 +++++++++++++++++++++++++--
 drivers/staging/vme/vme_bridge.h           |    1 +
 4 files changed, 44 insertions(+), 4 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..f5ff578 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -52,6 +52,32 @@ 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.
+ */
+static int vme_bridge_get(struct vme_bridge *bridge)
+{
+	if (try_module_get(bridge->owner))
+		return 0;
+	return -ENXIO;
+}
+
+/*
+ * Decrements the reference count of a VME bridge
+ *
+ * This function decrements the reference count of the module that controls
+ * the bridge. It must come after a call to vme_bridge_get().
+ */
+static void vme_bridge_put(struct vme_bridge *bridge)
+{
+	module_put(bridge->owner);
+}
+
+/*
  * Find the bridge that the resource is associated with.
  */
 static struct vme_bridge *find_bridge(struct vme_resource *resource)
@@ -1496,13 +1522,19 @@ 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 (driver->probe != NULL)
+	if (driver->probe) {
+		if (vme_bridge_get(bridge))
+			return -ENXIO;
+
 		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		if (retval)
+			vme_bridge_put(bridge);
+	}
 
 	return retval;
 }
@@ -1511,14 +1543,17 @@ 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);
 
-	if (driver->remove != NULL)
+	if (driver->remove)
 		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
 
+	if (driver->probe)
+		vme_bridge_put(bridge);
+
 	return retval;
 }
 
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 1/3] staging: vme: add functions for bridge module refcounting

[Emilio G. Cota]

On Mon, Aug 29, 2011 at 11:02:48 +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.
> 
> 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>

Reviewed-by: Emilio G. Cota <cota@braap.org>

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

[Greg KH]

On Mon, Aug 29, 2011 at 11:02:48AM +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.
> 
> This patch is based on the changes introduced by Emilio G. Cota
> in the patch:
> 
>     https://lkml.org/lkml/2010/10/25/492

Generally do not try to couple a module reference count with a device
count, you are locking code, not data, and we really don't want to lock
code anymore now that we properly handle device reference counts.

> 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                  |   43 +++++++++++++++++++++++++--
>  drivers/staging/vme/vme_bridge.h           |    1 +
>  4 files changed, 44 insertions(+), 4 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..f5ff578 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -52,6 +52,32 @@ 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.
> + */
> +static int vme_bridge_get(struct vme_bridge *bridge)
> +{
> +	if (try_module_get(bridge->owner))
> +		return 0;
> +	return -ENXIO;
> +}
> +
> +/*
> + * Decrements the reference count of a VME bridge
> + *
> + * This function decrements the reference count of the module that controls
> + * the bridge. It must come after a call to vme_bridge_get().
> + */
> +static void vme_bridge_put(struct vme_bridge *bridge)
> +{
> +	module_put(bridge->owner);
> +}
> +
> +/*
>   * Find the bridge that the resource is associated with.
>   */
>  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> @@ -1496,13 +1522,19 @@ 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 (driver->probe != NULL)
> +	if (driver->probe) {
> +		if (vme_bridge_get(bridge))
> +			return -ENXIO;
> +
>  		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> +		if (retval)
> +			vme_bridge_put(bridge);
> +	}
>  
>  	return retval;
>  }
> @@ -1511,14 +1543,17 @@ static int vme_bus_remove(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> -	int retval = -ENODEV;
> +	int retval = 0;

This looks like a bugfix that we should take separate from this overall
patch, right?  Same goes for the function above this.


>  
>  	driver = dev_to_vme_driver(dev);
>  	bridge = dev_to_bridge(dev);
>  
> -	if (driver->remove != NULL)
> +	if (driver->remove)

Was that really needed? :)

>  		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
>  
> +	if (driver->probe)
> +		vme_bridge_put(bridge);
> +
>  	return retval;
>  }
>  
> 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 */

Why?  When you register the device for this bridge, you should properly
handle the module reference counting in the vme core for any open sysfs
files.

Other than that, you shouldn't care about the module reference at all,
right?

What am I missing here?  What is this solving?

confused,

greg k-h

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

[Emilio G. Cota]

On Mon, Aug 29, 2011 at 10:51:04 -0700, Greg KH wrote:
> On Mon, Aug 29, 2011 at 11:02:48AM +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.
> > 
> > This patch is based on the changes introduced by Emilio G. Cota
> > in the patch:
> > 
> >     https://lkml.org/lkml/2010/10/25/492
> 
> Generally do not try to couple a module reference count with a device
> count, you are locking code, not data, and we really don't want to lock
> code anymore now that we properly handle device reference counts.

(snip)

> > 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 */
> 
> Why?  When you register the device for this bridge, you should properly
> handle the module reference counting in the vme core for any open sysfs
> files.
> 
> Other than that, you shouldn't care about the module reference at all,
> right?
> 
> What am I missing here?  What is this solving?
> 
> confused,

AFAIK currently a vme bridge driver can be freely removed with rmmod
even if it has devices under it. The patch avoids this by increasing
the refcount of the bridge module every time a device is registered
under it.

What's the appropriate thing to do when rmmod tries to remove a bridge
module? Shall we remove each bridge and all the child devices, or just
refuse to remove a bridge when it has child devices? Manohar implements
the latter, although incrementing the refcount of the module is admittedly
coarse. He said he'd later change this to implement the refcounting on
each bridge device (and not module).

Hope this helps, thanks for looking at it.

		Emilio

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

[Greg KH]

On Mon, Aug 29, 2011 at 02:40:12PM -0400, Emilio G. Cota wrote:
> On Mon, Aug 29, 2011 at 10:51:04 -0700, Greg KH wrote:
> > On Mon, Aug 29, 2011 at 11:02:48AM +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.
> > > 
> > > This patch is based on the changes introduced by Emilio G. Cota
> > > in the patch:
> > > 
> > >     https://lkml.org/lkml/2010/10/25/492
> > 
> > Generally do not try to couple a module reference count with a device
> > count, you are locking code, not data, and we really don't want to lock
> > code anymore now that we properly handle device reference counts.
> 
> (snip)
> 
> > > 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 */
> > 
> > Why?  When you register the device for this bridge, you should properly
> > handle the module reference counting in the vme core for any open sysfs
> > files.
> > 
> > Other than that, you shouldn't care about the module reference at all,
> > right?
> > 
> > What am I missing here?  What is this solving?
> > 
> > confused,
> 
> AFAIK currently a vme bridge driver can be freely removed with rmmod
> even if it has devices under it. The patch avoids this by increasing
> the refcount of the bridge module every time a device is registered
> under it.
> 
> What's the appropriate thing to do when rmmod tries to remove a bridge
> module?

Let it happen and remove all of the devices under it.

That's the way all other bus drivers in the kernel work, why change this
type of behavior?

thanks,

greg k-h

[PATCH 2/3] 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 f5ff578..19a4ad2 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);
 }
 
 /*
@@ -262,7 +258,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;
@@ -271,7 +267,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;
@@ -418,7 +414,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;
@@ -427,7 +423,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;
@@ -676,7 +672,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;
@@ -687,7 +684,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;
@@ -1020,13 +1017,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;
@@ -1063,11 +1060,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;
@@ -1098,11 +1095,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;
@@ -1125,7 +1122,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;
@@ -1133,7 +1130,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;
@@ -1314,11 +1311,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;
@@ -1366,7 +1363,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;
 
@@ -1379,20 +1376,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;
 	}
@@ -1401,8 +1401,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;
@@ -1412,12 +1412,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);
 }
@@ -1443,32 +1443,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)
@@ -1479,15 +1453,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) {
@@ -1507,7 +1483,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++;
@@ -1522,16 +1498,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 (driver->probe) {
 		if (vme_bridge_get(bridge))
 			return -ENXIO;
 
-		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->probe(vdev);
 		if (retval)
 			vme_bridge_put(bridge);
 	}
@@ -1543,13 +1521,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)
-		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->remove(vdev);
 
 	if (driver->probe)
 		vme_bridge_put(bridge);
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 4155d8c..d442cce 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

Re: [PATCH 2/3] staging: vme: add struct vme_dev for VME devices

[Emilio G. Cota]

On Mon, Aug 29, 2011 at 11:02:49 +0200, 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>

Reviewed-by: Emilio G. Cota <cota@braap.org>

Re: [PATCH 2/3] staging: vme: add struct vme_dev for VME devices

[Greg KH]

On Mon, Aug 29, 2011 at 11:02:49AM +0200, Manohar Vanga wrote:
> --- 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 */

Overall, this is the right way to go, using a vme_dev.

BUT, you should never have a static list of devices, these should be
pointers, not actual structures here, otherwise your reference counting
just got all messed up and is wrong.

And yes, I know you didn't create the code this way, but it needs to be
fixed _before_ you make this kind of a change.

thanks,

greg k-h

Re: [PATCH 2/3] staging: vme: add struct vme_dev for VME devices

[Emilio G. Cota]

On Mon, Aug 29, 2011 at 10:52:41 -0700, Greg KH wrote:
> On Mon, Aug 29, 2011 at 11:02:49AM +0200, Manohar Vanga wrote:
> > --- 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 */
> 
> Overall, this is the right way to go, using a vme_dev.
> 
> BUT, you should never have a static list of devices, these should be
> pointers, not actual structures here, otherwise your reference counting
> just got all messed up and is wrong.
> 
> And yes, I know you didn't create the code this way, but it needs to be
> fixed _before_ you make this kind of a change.

True, I overlooked this while reviewing >_<

Manohar, have a look at vme_unregister_driver_ng() and
__vme_register_driver_bus() in this patch:
 
    http://article.gmane.org/gmane.linux.kernel/1054046

Note that there's no kfree(vme_dev) in those functions. put_device(),
which is also called from device_unregister(), decrements
the underlying kobj's refcount. When this refcount reaches 0,
kobject_release() is called. Eventually dev->release() gets called,
which in this case is set to vme_dev_release_ng(); it's there
(and only there) where vme_dev can be freed.

		Emilio

Re: [PATCH 2/3] staging: vme: add struct vme_dev for VME devices

[Manohar Vanga]

Hey Emilio,

On Mon, Aug 29, 2011 at 07:55:08PM -0400, Emilio G. Cota wrote:
> On Mon, Aug 29, 2011 at 10:52:41 -0700, Greg KH wrote:
> > On Mon, Aug 29, 2011 at 11:02:49AM +0200, Manohar Vanga wrote:
> > > --- 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 */
> > 
> > Overall, this is the right way to go, using a vme_dev.
> > 
> > BUT, you should never have a static list of devices, these should be
> > pointers, not actual structures here, otherwise your reference counting
> > just got all messed up and is wrong.
> > 
> > And yes, I know you didn't create the code this way, but it needs to be
> > fixed _before_ you make this kind of a change.
> 
> True, I overlooked this while reviewing >_<
> 
> Manohar, have a look at vme_unregister_driver_ng() and
> __vme_register_driver_bus() in this patch:
>  
>     http://article.gmane.org/gmane.linux.kernel/1054046

I actually threw the static devices out of the code in patch 3. I didn't
introduce it here as I thought the changes were unrelated (introducing
struct vme_dev across the code and getting rid of that static device
array).

-- 
/manohar

[PATCH 3/3] 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              |  214 ++++++++++++++++---------------
 drivers/staging/vme/vme.h              |   22 +++-
 drivers/staging/vme/vme_api.txt        |   60 +++++-----
 drivers/staging/vme/vme_bridge.h       |    4 -
 6 files changed, 177 insertions(+), 178 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 19a4ad2..2fc00df 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.
@@ -1363,147 +1360,158 @@ 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) {
+		/*
+		 * This cannot cause trouble as we already have vme_buses_lock
+		 * and if the bridge is removed, it will have to go through
+		 * vme_unregister_bridge() to do it (which calls remove() on
+		 * the bridge which in turn tries to acquire vme_buses_lock and
+		 * will have to wait). The probe() called after device
+		 * registration in __vme_register_driver below will also fail
+		 * as the bridge is being removed (since the probe() calls
+		 * vme_bridge_get()).
+		 */
+		err = __vme_register_driver_bus(drv, bridge, ndevs);
+		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);
+	struct vme_driver *vme_drv;
 
-	num = vdev->id.slot;
-	if (num <= 0 || num >= VME_SLOTS_MAX)
-		goto err_dev;
+	vme_drv = container_of(drv, struct vme_driver, driver);
 
-	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)) {
-
-		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;
-	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;
+	driver = dev->platform_data;
 
 	if (driver->probe) {
 		if (vme_bridge_get(bridge))
@@ -1519,14 +1527,12 @@ 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;
+	driver = dev->platform_data;
 
 	if (driver->remove)
 		retval = driver->remove(vdev);
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index d442cce..1033344 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 *);
 
 
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 3/3] staging: vme: make match() driver specific to improve non-VME64x support

[Emilio G. Cota]

Hi Manohar,

Great work! A couple of comments inlined below.

On Mon, Aug 29, 2011 at 11:02:50 +0200, Manohar Vanga wrote:
(snip)
> +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);

Here there seems to be a missing kfree(vdev), like the one after device_unregister()
a few lines earlier.

>  	}
> -	vme_remove_bus(bridge);
> -	return retval;
> +	return err;
>  }
> -EXPORT_SYMBOL(vme_register_bridge);

(snip)

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

Probably you meant if (!err && drv->ndev == 0) here.

In fact one could imagine a driver that has no devices when it's
installed (it might get them later); so I'd remove this check.

> +		err = -ENODEV;
> +	if (err)
> +		vme_unregister_driver(drv);
> +	return err;
>  }
>  EXPORT_SYMBOL(vme_register_driver);


		Emilio

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

[Manohar Vanga]

> Here there seems to be a missing kfree(vdev), like the one after device_unregister()
> a few lines earlier.

> > +	if (!err & (drv->ndev == 0))
> 
> Probably you meant if (!err && drv->ndev == 0) here.

Ah darn. Will fix and resend.

> In fact one could imagine a driver that has no devices when it's
> installed (it might get them later); so I'd remove this check.

Yeah I suppose with VME64x it's possible. Will remove.

-- 
/manohar

[PATCH 0/3] [RESEND v4] VME Framework Fixes

[Manohar Vanga]

Hi Greg, Martyn,

I have reordered the patches to separate the bridge refcounting patch
for now so that we can make some progress on getting the other patches
into the tree. I will rework the refcounting patch and send it later.

Please review these patches and let me know if I missed anything (quite
likely). A note on the various patches follows:

* staging: vme: change static device array to pointers
	This patch changes the static array of devices to an array of
	pointers that are dynamically allocated. Hopefully I understood
	what you meant correctly and the changes are sufficient. I assume
	you were referring to the requirement that all kobjects be allocated
	dynamically and never statically (found in Documentation/kobject.txt).

* staging: vme: add struct vme_dev for VME devices
* staging: vme: make match() driver specific to improve non-VME64x
    support
	Changes based on the moving of the bridge refcounting patch as well
	as changes based on comments from Emilio and Greg in the last resend.

Thanks!
Manohar

Manohar Vanga (3):
  staging: vme: change static device array to pointers
  staging: vme: add struct vme_dev for VME devices
  staging: vme: make match() driver specific to improve non-VME64x
    support

 drivers/staging/vme/devices/vme_user.c |   67 +++-----
 drivers/staging/vme/devices/vme_user.h |    2 +-
 drivers/staging/vme/vme.c              |  274 +++++++++++++++-----------------
 drivers/staging/vme/vme.h              |   55 +++++--
 drivers/staging/vme/vme_api.txt        |   79 ++++++----
 drivers/staging/vme/vme_bridge.h       |    5 -
 6 files changed, 244 insertions(+), 238 deletions(-)

-- 
1.7.4.1

[PATCH 3/3] 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              |  201 +++++++++++++++-----------------
 drivers/staging/vme/vme.h              |   22 +++-
 drivers/staging/vme/vme_api.txt        |   60 +++++-----
 drivers/staging/vme/vme_bridge.h       |    4 -
 6 files changed, 163 insertions(+), 179 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 76e08f3..2c3d47d 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)
 
 /*
  * Find the bridge that the resource is associated with.
@@ -1342,153 +1339,148 @@ static void vme_dev_release(struct device *dev)
 
 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++) {
-		bridge->dev[i] = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
-		if (!bridge->dev[i]) {
-			retval = -ENOMEM;
-			goto err_devalloc;
-		}
-		vdev = bridge->dev[i];
-		memset(vdev, 0, sizeof(struct vme_dev));
+/* - Driver Registration --------------------------------------------------- */
 
+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;
-		vdev->dev.release = vme_dev_release;
-		/*
-		 * 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);
+		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
+			vdev->id.num);
 
-		retval = device_register(&vdev->dev);
-		if (retval)
-			goto err_reg;
-	}
+		err = device_register(&vdev->dev);
+		if (err)
+			goto err_dev_reg;
 
-	return retval;
+		if (vdev->dev.platform_data) {
+			list_add_tail(&vdev->list, &drv->devices);
+			drv->ndev++;
+		} else
+			device_unregister(&vdev->dev);
+	}
+	return 0;
 
-err_reg:
+err_dev_reg:
 	kfree(vdev);
-err_devalloc:
-	while (--i >= 0) {
-		vdev = bridge->dev[i];
+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) {
+		/*
+		 * This cannot cause trouble as we already have vme_buses_lock
+		 * and if the bridge is removed, it will have to go through
+		 * vme_unregister_bridge() to do it (which calls remove() on
+		 * the bridge which in turn tries to acquire vme_buses_lock and
+		 * will have to wait). The probe() called after device
+		 * registration in __vme_register_driver below will also fail
+		 * as the bridge is being removed (since the probe() calls
+		 * vme_bridge_get()).
+		 */
+		err = __vme_register_driver_bus(drv, bridge, ndevs);
+		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;
+	INIT_LIST_HEAD(&drv->devices);
+
+	err = driver_register(&drv->driver);
+	if (err)
+		return err;
+
+	err = __vme_register_driver(drv, ndevs);
+	if (err)
+		vme_unregister_driver(drv);
 
-	return driver_register(&drv->driver);
+	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;
-
-	if (driver->bind_table == NULL) {
-		dev_err(dev, "Bind table NULL\n");
-		goto err_table;
-	}
+	struct vme_driver *vme_drv;
 
-	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_driver *driver;
-	struct vme_dev *vdev;
 	int retval = -ENODEV;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
+	driver = dev->platform_data;
 
 	if (driver->probe != NULL)
 		retval = driver->probe(vdev);
@@ -1498,12 +1490,11 @@ static int vme_bus_probe(struct device *dev)
 
 static int vme_bus_remove(struct device *dev)
 {
-	struct vme_driver *driver;
-	struct vme_dev *vdev;
 	int retval = -ENODEV;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
+	driver = dev->platform_data;
 
 	if (driver->remove != NULL)
 		retval = driver->remove(vdev);
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index d442cce..1033344 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 *);
 
 
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 6dd2f4c..99c0f97 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
  */
@@ -114,9 +113,6 @@ struct vme_bridge {
 	void *driver_priv;	/* Private pointer for the bridge driver */
 	struct list_head bus_list; /* list of VME buses */
 
-	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 3/3] staging: vme: make match() driver specific to improve non-VME64x support

[Emilio G. Cota]

This was hard to review. There are references to functions that
are not committed in Greg's tree yet ("staging" tree @ git.kernel.org).

I assume this patch was applied before you wrote the v4 patchset:

	https://lkml.org/lkml/2011/8/12/107

On Wed, Aug 31, 2011 at 12:05:46 +0200, Manohar Vanga wrote:
(snip)
> 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).

The mention to refcounting seems outdated. As I stated in my reply
to v0, we should just safely remove devices under the bus when
vme_unregister_bus() is called.

> ---
>  drivers/staging/vme/devices/vme_user.c |   53 +++-----
>  drivers/staging/vme/devices/vme_user.h |    2 +-
>  drivers/staging/vme/vme.c              |  201 +++++++++++++++-----------------
>  drivers/staging/vme/vme.h              |   22 +++-
>  drivers/staging/vme/vme_api.txt        |   60 +++++-----
>  drivers/staging/vme/vme_bridge.h       |    4 -
>  6 files changed, 163 insertions(+), 179 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 76e08f3..2c3d47d 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)

The function was added in patch 2. In general, you should not add code
that then gets removed, unless there's a good reason. In this case just
add either the function or the macro in patch 2, and stick to it.

>  
>  /*
>   * Find the bridge that the resource is associated with.
> @@ -1342,153 +1339,148 @@ static void vme_dev_release(struct device *dev)
>  
>  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);

vme_remove_bus is in the separate patch:
   https://lkml.org/lkml/2011/8/12/107

+static void vme_remove_bus(struct vme_bridge *bridge)
+{
+	mutex_lock(&vme_buses_lock);
+	vme_bus_numbers &= ~(1 << bridge->num);
+	list_del(&bridge->bus_list);
+	mutex_unlock(&vme_buses_lock);
+}

It's quite unfortunate that we cannot easily see the old vs the
new vme_unregister_bridge in this patch. I paste here for clarity:

> -void vme_unregister_bridge(struct vme_bridge *bridge)
> {
> -	int i;
> -	struct vme_dev *vdev;
> -
> -
> -	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		vdev = bridge->dev[i];
> -		device_unregister(&vdev->dev);
> - 	}
> 	vme_remove_bus(bridge);
> }

So we're essentially leaving the devices there, even though the
bridge they're under will be removed. This doesn't seem right.
btw with the removal of the array of vme_dev's from struct vme_bridge,
the bridge cannot know which devices are under it.

We have to bear in mind that the drv->devices list needs to be
updated when devices come and go; possibly a bridge->devices list
could also be kept.

Helpers around device_register and _unregister may simplify the lists'
housekeeping.

>  
> -	/* 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++) {
> -		bridge->dev[i] = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
> -		if (!bridge->dev[i]) {
> -			retval = -ENOMEM;
> -			goto err_devalloc;
> -		}
> -		vdev = bridge->dev[i];
> -		memset(vdev, 0, sizeof(struct vme_dev));
> +/* - Driver Registration --------------------------------------------------- */
>  
> +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;
> -		vdev->dev.release = vme_dev_release;
> -		/*
> -		 * 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);
> +		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
> +			vdev->id.num);
>  
> -		retval = device_register(&vdev->dev);
> -		if (retval)
> -			goto err_reg;
> -	}
> +		err = device_register(&vdev->dev);
> +		if (err)
> +			goto err_dev_reg;
>  
> -	return retval;
> +		if (vdev->dev.platform_data) {
> +			list_add_tail(&vdev->list, &drv->devices);
> +			drv->ndev++;

Ok, so drv->ndev can only increase. In case a device is removed (when
a bus driver is removed) this may need to be decreased, which isn't
done in the corresponding list_del() calls (I've marked them).

In fact I wonder whether it is useful at all to have drv->ndev. What's
its purpose?

> +		} else
> +			device_unregister(&vdev->dev);
> +	}
> +	return 0;
>  
> -err_reg:
> +err_dev_reg:
>  	kfree(vdev);
> -err_devalloc:
> -	while (--i >= 0) {
> -		vdev = bridge->dev[i];
> +err_alloc:
> +	list_for_each_entry_safe(vdev, tmp, &drv->devices, list) {
> +		list_del(&vdev->list);

missing drv->ndev-- ?

>  		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) {
> +		/*
> +		 * This cannot cause trouble as we already have vme_buses_lock
> +		 * and if the bridge is removed, it will have to go through
> +		 * vme_unregister_bridge() to do it (which calls remove() on
> +		 * the bridge which in turn tries to acquire vme_buses_lock and
> +		 * will have to wait). The probe() called after device
> +		 * registration in __vme_register_driver below will also fail
> +		 * as the bridge is being removed (since the probe() calls
> +		 * vme_bridge_get()).
> +		 */
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		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;
> +	INIT_LIST_HEAD(&drv->devices);
> +
> +	err = driver_register(&drv->driver);
> +	if (err)
> +		return err;
> +
> +	err = __vme_register_driver(drv, ndevs);
> +	if (err)
> +		vme_unregister_driver(drv);
>  
> -	return driver_register(&drv->driver);
> +	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);

missing drv->ndev-- ?

		Emilio

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

[Manohar Vanga]

Hey Emilio,

On Wed, Aug 31, 2011 at 05:20:47PM -0400, Emilio G. Cota wrote:
> This was hard to review. There are references to functions that
> are not committed in Greg's tree yet ("staging" tree @ git.kernel.org).
> 
> I assume this patch was applied before you wrote the v4 patchset:
> 
> 	https://lkml.org/lkml/2011/8/12/107
>

I believe Greg has acked this patch (I received a confirmation mail from him).

> On Wed, Aug 31, 2011 at 12:05:46 +0200, Manohar Vanga wrote:
> (snip)
> > 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).
> 
> The mention to refcounting seems outdated. As I stated in my reply
> to v0, we should just safely remove devices under the bus when
> vme_unregister_bus() is called.

Ah right need to reword that.

> > -void vme_unregister_bridge(struct vme_bridge *bridge)
> > {
> > -	int i;
> > -	struct vme_dev *vdev;
> > -
> > -
> > -	for (i = 0; i < VME_SLOTS_MAX; i++) {
> > -		vdev = bridge->dev[i];
> > -		device_unregister(&vdev->dev);
> > - 	}
> > 	vme_remove_bus(bridge);
> > }
> 
> So we're essentially leaving the devices there, even though the
> bridge they're under will be removed. This doesn't seem right.
> btw with the removal of the array of vme_dev's from struct vme_bridge,
> the bridge cannot know which devices are under it.
> 
> We have to bear in mind that the drv->devices list needs to be
> updated when devices come and go; possibly a bridge->devices list
> could also be kept.
> 
> Helpers around device_register and _unregister may simplify the lists'
> housekeeping.

I was going to add a separate patch for this but I'll just integrate into this
one (makes more sense anyway).

And yes, I also noticed that the bridge no longer has track of its devices and
bridges will need to keep a list of them.

> > -	return retval;
> > +		if (vdev->dev.platform_data) {
> > +			list_add_tail(&vdev->list, &drv->devices);
> > +			drv->ndev++;
> 
> Ok, so drv->ndev can only increase. In case a device is removed (when
> a bus driver is removed) this may need to be decreased, which isn't
> done in the corresponding list_del() calls (I've marked them).
> 
> In fact I wonder whether it is useful at all to have drv->ndev. What's
> its purpose?

I'm not sure why I added that now...
It can be removed.

-- 
/manohar

[PATCH 0/3] [RESEND v5] VME Framework Fixes

[Manohar Vanga]

Hi Greg, Martyn, Emilio,

This is the resend of the patches with additional code added to patch 3 to
remove devices in the case that a bridge driver is removed. (I don't know how
common it is to keep resending patchsets so I apologize for any noise I may
be causing).

Thanks
Manohar

Manohar Vanga (3):
  staging: vme: change static device array to pointers
  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 |    3 +
 drivers/staging/vme/bridges/vme_tsi148.c   |    3 +
 drivers/staging/vme/devices/vme_user.c     |   67 +++----
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  283 ++++++++++++++--------------
 drivers/staging/vme/vme.h                  |   56 ++++--
 drivers/staging/vme/vme_api.txt            |   79 +++++---
 drivers/staging/vme/vme_bridge.h           |    6 +-
 8 files changed, 263 insertions(+), 236 deletions(-)

-- 
1.7.4.1

[PATCH 3/3] 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.

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/bridges/vme_ca91cx42.c |    3 +
 drivers/staging/vme/bridges/vme_tsi148.c   |    3 +
 drivers/staging/vme/devices/vme_user.c     |   53 +++-----
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  206 ++++++++++++++--------------
 drivers/staging/vme/vme.h                  |   23 +++-
 drivers/staging/vme/vme_api.txt            |   60 ++++----
 drivers/staging/vme/vme_bridge.h           |    5 +-
 8 files changed, 180 insertions(+), 175 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 0e4feac..5b3dcb5 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1774,6 +1774,9 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	lm->monitors = 4;
 	list_add_tail(&lm->list, &ca91cx42_bridge->lm_resources);
 
+	/* Initialize the list for bridge devices */
+	INIT_LIST_HEAD(&ca91cx42_bridge->devices);
+
 	ca91cx42_bridge->slave_get = ca91cx42_slave_get;
 	ca91cx42_bridge->slave_set = ca91cx42_slave_set;
 	ca91cx42_bridge->master_get = ca91cx42_master_get;
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index 6c1167c..ea13116 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2448,6 +2448,9 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	lm->monitors = 4;
 	list_add_tail(&lm->list, &tsi148_bridge->lm_resources);
 
+	/* Initialize the list for bridge devices */
+	INIT_LIST_HEAD(&tsi148_bridge->devices);
+
 	tsi148_bridge->slave_get = tsi148_slave_get;
 	tsi148_bridge->slave_set = tsi148_slave_set;
 	tsi148_bridge->master_get = tsi148_master_get;
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 76e08f3..cc55c35 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -1329,8 +1329,16 @@ static int vme_add_bus(struct vme_bridge *bridge)
 
 static void vme_remove_bus(struct vme_bridge *bridge)
 {
+	struct vme_dev *vdev;
+	struct vme_dev *tmp;
+
 	mutex_lock(&vme_buses_lock);
 	vme_bus_numbers &= ~(1 << bridge->num);
+	list_for_each_entry_safe(vdev, tmp, &bridge->devices, bridge_list) {
+		list_del(&vdev->drv_list);
+		list_del(&vdev->bridge_list);
+		device_unregister(&vdev->dev);
+	}
 	list_del(&bridge->bus_list);
 	mutex_unlock(&vme_buses_lock);
 }
@@ -1342,153 +1350,150 @@ static void vme_dev_release(struct device *dev)
 
 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++) {
-		bridge->dev[i] = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
-		if (!bridge->dev[i]) {
-			retval = -ENOMEM;
-			goto err_devalloc;
-		}
-		vdev = bridge->dev[i];
-		memset(vdev, 0, sizeof(struct vme_dev));
+/* - Driver Registration --------------------------------------------------- */
 
+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;
-		vdev->dev.release = vme_dev_release;
-		/*
-		 * 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);
+		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
+			vdev->id.num);
 
-		retval = device_register(&vdev->dev);
-		if (retval)
-			goto err_reg;
-	}
+		err = device_register(&vdev->dev);
+		if (err)
+			goto err_dev_reg;
 
-	return retval;
+		if (vdev->dev.platform_data) {
+			list_add_tail(&vdev->drv_list, &drv->devices);
+			list_add_tail(&vdev->bridge_list, &bridge->devices);
+		} else
+			device_unregister(&vdev->dev);
+	}
+	return 0;
 
-err_reg:
+err_dev_reg:
 	kfree(vdev);
-err_devalloc:
-	while (--i >= 0) {
-		vdev = bridge->dev[i];
+err_alloc:
+	list_for_each_entry_safe(vdev, tmp, &drv->devices, drv_list) {
+		list_del(&vdev->drv_list);
+		list_del(&vdev->bridge_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) {
+		/*
+		 * This cannot cause trouble as we already have vme_buses_lock
+		 * and if the bridge is removed, it will have to go through
+		 * vme_unregister_bridge() to do it (which calls remove() on
+		 * the bridge which in turn tries to acquire vme_buses_lock and
+		 * will have to wait). The probe() called after device
+		 * registration in __vme_register_driver below will also fail
+		 * as the bridge is being removed (since the probe() calls
+		 * vme_bridge_get()).
+		 */
+		err = __vme_register_driver_bus(drv, bridge, ndevs);
+		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;
+	INIT_LIST_HEAD(&drv->devices);
+
+	err = driver_register(&drv->driver);
+	if (err)
+		return err;
 
-	return driver_register(&drv->driver);
+	err = __vme_register_driver(drv, ndevs);
+	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, drv_list) {
+		list_del(&dev->drv_list);
+		list_del(&dev->bridge_list);
+		device_unregister(&dev->dev);
+	}
 	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);
+	struct vme_driver *vme_drv;
 
-	num = vdev->id.slot;
-	if (num <= 0 || num >= VME_SLOTS_MAX)
-		goto err_dev;
-
-	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_driver *driver;
-	struct vme_dev *vdev;
 	int retval = -ENODEV;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
+	driver = dev->platform_data;
 
 	if (driver->probe != NULL)
 		retval = driver->probe(vdev);
@@ -1498,12 +1503,11 @@ static int vme_bus_probe(struct device *dev)
 
 static int vme_bus_remove(struct device *dev)
 {
-	struct vme_driver *driver;
-	struct vme_dev *vdev;
 	int retval = -ENODEV;
+	struct vme_driver *driver;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
+	driver = dev->platform_data;
 
 	if (driver->remove != NULL)
 		retval = driver->remove(vdev);
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index d442cce..95224d7 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,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
+ * @drv_list: List of devices (per driver)
+ * @bridge_list: List of devices (per bridge)
  */
 struct vme_dev {
 	struct vme_device_id id;
 	struct vme_bridge *bridge;
 	struct device dev;
+	struct list_head drv_list;
+	struct list_head bridge_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;
 };
 
 void *vme_alloc_consistent(struct vme_resource *, size_t, dma_addr_t *);
@@ -179,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 *);
 
 
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 6dd2f4c..c2deda2 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
  */
@@ -108,15 +107,13 @@ struct vme_bridge {
 	struct list_head lm_resources;
 
 	struct list_head vme_errors;	/* List for errors generated on VME */
+	struct list_head devices;	/* List of devices on this 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 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 3/3] staging: vme: make match() driver specific to improve non-VME64x support

[Emilio G. Cota]

Patches 1 and 2 are hunky dory. This one still needs a bit of work,
but not much really. See below.

All in all this is good stuff, keep it up!

		Emilio

On Thu, Sep 01, 2011 at 11:15:26 +0200, 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.
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/bridges/vme_ca91cx42.c |    3 +
>  drivers/staging/vme/bridges/vme_tsi148.c   |    3 +
>  drivers/staging/vme/devices/vme_user.c     |   53 +++-----
>  drivers/staging/vme/devices/vme_user.h     |    2 +-
>  drivers/staging/vme/vme.c                  |  206 ++++++++++++++--------------
>  drivers/staging/vme/vme.h                  |   23 +++-
>  drivers/staging/vme/vme_api.txt            |   60 ++++----
>  drivers/staging/vme/vme_bridge.h           |    5 +-
>  8 files changed, 180 insertions(+), 175 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index 0e4feac..5b3dcb5 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1774,6 +1774,9 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	lm->monitors = 4;
>  	list_add_tail(&lm->list, &ca91cx42_bridge->lm_resources);
>  
> +	/* Initialize the list for bridge devices */
> +	INIT_LIST_HEAD(&ca91cx42_bridge->devices);
> +
>  	ca91cx42_bridge->slave_get = ca91cx42_slave_get;
>  	ca91cx42_bridge->slave_set = ca91cx42_slave_set;
>  	ca91cx42_bridge->master_get = ca91cx42_master_get;
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index 6c1167c..ea13116 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2448,6 +2448,9 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	lm->monitors = 4;
>  	list_add_tail(&lm->list, &tsi148_bridge->lm_resources);
>  
> +	/* Initialize the list for bridge devices */
> +	INIT_LIST_HEAD(&tsi148_bridge->devices);
> +

Probably it makes more sense to handle this in vme.c:vme_add_bus(), since
the particular bridge drivers do not manage at all the device list.

>  	tsi148_bridge->slave_get = tsi148_slave_get;
>  	tsi148_bridge->slave_set = tsi148_slave_set;
>  	tsi148_bridge->master_get = tsi148_master_get;
> 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

this could be another patch, but duh..

>  /*
>   * VMEbus Master Window Configuration Structure
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 76e08f3..cc55c35 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1329,8 +1329,16 @@ static int vme_add_bus(struct vme_bridge *bridge)
>  
>  static void vme_remove_bus(struct vme_bridge *bridge)
>  {
> +	struct vme_dev *vdev;
> +	struct vme_dev *tmp;
> +
>  	mutex_lock(&vme_buses_lock);
>  	vme_bus_numbers &= ~(1 << bridge->num);
> +	list_for_each_entry_safe(vdev, tmp, &bridge->devices, bridge_list) {
> +		list_del(&vdev->drv_list);
> +		list_del(&vdev->bridge_list);
> +		device_unregister(&vdev->dev);
> +	}

I like this. All the management of both lists is now correctly done with
vme_buses_lock held--except, perhaps, the initialisation of the bridge list,
but that's ok.

>  	list_del(&bridge->bus_list);
>  	mutex_unlock(&vme_buses_lock);
>  }
> @@ -1342,153 +1350,150 @@ static void vme_dev_release(struct device *dev)
>  
>  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);

Consider sending a subsequent patch cleaning up functions like these.
But don't do it in this patch; this patch, if anything, needs to go
on a diet.

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

ditto

> -	/* 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++) {
> -		bridge->dev[i] = kzalloc(sizeof(struct vme_dev), GFP_KERNEL);
> -		if (!bridge->dev[i]) {
> -			retval = -ENOMEM;
> -			goto err_devalloc;
> -		}
> -		vdev = bridge->dev[i];
> -		memset(vdev, 0, sizeof(struct vme_dev));
> +/* - Driver Registration --------------------------------------------------- */

I know you're keeping this comment from what's already in the file,
but personally I simply find it distracting.

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

Now the slot field has lots its meaning, and AFAICT its use, see below.

>  		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;
> -		vdev->dev.release = vme_dev_release;
> -		/*
> -		 * 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);
> +		dev_set_name(&vdev->dev, "%s.%u-%u", drv->name, vdev->id.bus,
> +			vdev->id.num);
>  
> -		retval = device_register(&vdev->dev);
> -		if (retval)
> -			goto err_reg;
> -	}
> +		err = device_register(&vdev->dev);
> +		if (err)
> +			goto err_dev_reg;
>  
> -	return retval;
> +		if (vdev->dev.platform_data) {
> +			list_add_tail(&vdev->drv_list, &drv->devices);
> +			list_add_tail(&vdev->bridge_list, &bridge->devices);

buses_lock is held upon calling the function. Good!

> +		} else
> +			device_unregister(&vdev->dev);
> +	}
> +	return 0;
>  
> -err_reg:
> +err_dev_reg:

Leaving the previous label would be better, it's easier to review.

>  	kfree(vdev);
> -err_devalloc:
> -	while (--i >= 0) {
> -		vdev = bridge->dev[i];
> +err_alloc:

ditto

> +	list_for_each_entry_safe(vdev, tmp, &drv->devices, drv_list) {
> +		list_del(&vdev->drv_list);
> +		list_del(&vdev->bridge_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) {
> +		/*
> +		 * This cannot cause trouble as we already have vme_buses_lock
> +		 * and if the bridge is removed, it will have to go through
> +		 * vme_unregister_bridge() to do it (which calls remove() on
> +		 * the bridge which in turn tries to acquire vme_buses_lock and
> +		 * will have to wait). The probe() called after device
> +		 * registration in __vme_register_driver below will also fail
> +		 * as the bridge is being removed (since the probe() calls
> +		 * vme_bridge_get()).
> +		 */
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		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;
> +	INIT_LIST_HEAD(&drv->devices);
> +
> +	err = driver_register(&drv->driver);
> +	if (err)
> +		return err;
>  
> -	return driver_register(&drv->driver);
> +	err = __vme_register_driver(drv, ndevs);
> +	if (err)
> +		vme_unregister_driver(drv);

If __vme_register_driver() fails, we can be sure the created devices
(and their corresponding lists) have been cleaned up before the
function returned the failure. So here it seems clearer to call
unregister_driver().

> +
> +	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, drv_list) {
> +		list_del(&dev->drv_list);
> +		list_del(&dev->bridge_list);
> +		device_unregister(&dev->dev);
> +	}

All code operating on both lists is protected by vme_buses_lock, except the
one in this function, which seems dangerous. vme_unregister_driver may race
with vme_unregister_bridge. We need to acquire the lock here too.

>  	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);
> +	struct vme_driver *vme_drv;
>  
> -	num = vdev->id.slot;
> -	if (num <= 0 || num >= VME_SLOTS_MAX)
> -		goto err_dev;
> -
> -	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_driver *driver;
> -	struct vme_dev *vdev;
>  	int retval = -ENODEV;
> +	struct vme_driver *driver;
> +	struct vme_dev *vdev = dev_to_vme_dev(dev);
>  
> -	driver = dev_to_vme_driver(dev);
> -	vdev = dev_to_vme_dev(dev);
> +	driver = dev->platform_data;
>  
>  	if (driver->probe != NULL)
>  		retval = driver->probe(vdev);
> @@ -1498,12 +1503,11 @@ static int vme_bus_probe(struct device *dev)
>  
>  static int vme_bus_remove(struct device *dev)
>  {
> -	struct vme_driver *driver;
> -	struct vme_dev *vdev;
>  	int retval = -ENODEV;
> +	struct vme_driver *driver;
> +	struct vme_dev *vdev = dev_to_vme_dev(dev);
>  
> -	driver = dev_to_vme_driver(dev);
> -	vdev = dev_to_vme_dev(dev);
> +	driver = dev->platform_data;
>  
>  	if (driver->remove != NULL)
>  		retval = driver->remove(vdev);
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index d442cce..95224d7 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;

As I mentioned earlier, AFAICT the slot field is meaningless now.
Consider submitting a subsequent patch that removes it.

>  };
> @@ -106,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
> + * @drv_list: List of devices (per driver)
> + * @bridge_list: List of devices (per bridge)
>   */
>  struct vme_dev {
>  	struct vme_device_id id;
>  	struct vme_bridge *bridge;
>  	struct device dev;
> +	struct list_head drv_list;
> +	struct list_head bridge_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;
>  };
>  
>  void *vme_alloc_consistent(struct vme_resource *, size_t, dma_addr_t *);
> @@ -179,7 +190,7 @@ void vme_lm_free(struct vme_resource *);
>  
>  int vme_slot_get(struct vme_dev *);

AFAICT this is an exported symbol that after this patch has no callers
and no meaning. Consider submitting a subsequent patch that removes it,
possibly together with the removal of struct vme_device_id.slot.
btw remember to update the documentation, I'm sure I'd forget.

> -int vme_register_driver(struct vme_driver *);
> +int vme_register_driver(struct vme_driver *, unsigned int);
>  void vme_unregister_driver(struct vme_driver *);
>  
>  
> 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 6dd2f4c..c2deda2 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

So this should in fact be part of the "slot cleanup" patch.

>  /*
>   * Resource structures
>   */
> @@ -108,15 +107,13 @@ struct vme_bridge {
>  	struct list_head lm_resources;
>  
>  	struct list_head vme_errors;	/* List for errors generated on VME */
> +	struct list_head devices;	/* List of devices on this bridge */

As you can see the locking of this list and that on each bridge got
a bit complex. This is OK, locks tend to spread, but it's important
to be aware of it. Perhaps adding a comment somewhere (possibly
but not necessarily here) would avoid future bugs/deadlocks.

>  
>  	/* 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 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 3/3] staging: vme: make match() driver specific to improve non-VME64x support

[Manohar Vanga]

Hey Emilio,

> > +	/* Initialize the list for bridge devices */
> > +	INIT_LIST_HEAD(&tsi148_bridge->devices);
> > +
> 
> Probably it makes more sense to handle this in vme.c:vme_add_bus(), since
> the particular bridge drivers do not manage at all the device list.

I thought of doing this but decided to go the other way for some forgotten
reason. I think it was the fact that there would be a gap between allocation
and initialization that bothered me.

Anyway, I've changed it and it is now done in vme_add_bus().

> > -#define USER_BUS_MAX                  1
> > +#define VME_USER_BUS_MAX	1
> 
> this could be another patch, but duh..

Done.

> >  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);
> 
> Consider sending a subsequent patch cleaning up functions like these.
> But don't do it in this patch; this patch, if anything, needs to go
> on a diet.
> 
> > -	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);
> 
> ditto

I'm not sure I understood this entirely. This replaces the old function
with the new one. There isn't any cleanup here. Or did I understand something
wrongly?

> > +/* - Driver Registration --------------------------------------------------- */
> 
> I know you're keeping this comment from what's already in the file,
> but personally I simply find it distracting.

Well there are others as well, so I've left it there for now.

> > +		} else
> > +			device_unregister(&vdev->dev);
> > +	}
> > +	return 0;
> >  
> > -err_reg:
> > +err_dev_reg:
> 
> Leaving the previous label would be better, it's easier to review.
> 
> >  	kfree(vdev);
> > -err_devalloc:
> > -	while (--i >= 0) {
> > -		vdev = bridge->dev[i];
> > +err_alloc:
> 
> ditto

Done.

> > -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;
> > +	INIT_LIST_HEAD(&drv->devices);
> > +
> > +	err = driver_register(&drv->driver);
> > +	if (err)
> > +		return err;
> >  
> > -	return driver_register(&drv->driver);
> > +	err = __vme_register_driver(drv, ndevs);
> > +	if (err)
> > +		vme_unregister_driver(drv);
> 
> If __vme_register_driver() fails, we can be sure the created devices
> (and their corresponding lists) have been cleaned up before the
> function returned the failure. So here it seems clearer to call
> unregister_driver().

Agreed. Fixed in the resend.

> >  void vme_unregister_driver(struct vme_driver *drv)
> >  {
> > +	struct vme_dev *dev, *dev_tmp;
> > +
> > +	list_for_each_entry_safe(dev, dev_tmp, &drv->devices, drv_list) {
> > +		list_del(&dev->drv_list);
> > +		list_del(&dev->bridge_list);
> > +		device_unregister(&dev->dev);
> > +	}
> 
> All code operating on both lists is protected by vme_buses_lock, except the
> one in this function, which seems dangerous. vme_unregister_driver may race
> with vme_unregister_bridge. We need to acquire the lock here too.

Fixed.

> >  struct vme_device_id {
> > +	int num;
> >  	int bus;
> >  	int slot;
> 
> As I mentioned earlier, AFAICT the slot field is meaningless now.
> Consider submitting a subsequent patch that removes it.

Done. See resend.

> >  int vme_slot_get(struct vme_dev *);
> 
> AFAICT this is an exported symbol that after this patch has no callers
> and no meaning. Consider submitting a subsequent patch that removes it,
> possibly together with the removal of struct vme_device_id.slot.
> btw remember to update the documentation, I'm sure I'd forget.

This returns the geographical location of the bridge device. Would this
be useful for VME64x crates? I see it isn't used anywhere so I can't imagine
when it might be needed. Maybe Martyn can clarify?

-- 
/manohar