[PATCH 0/8] VME Driver Fixes

[PATCH 0/8] VME Driver Fixes

[Manohar Vanga]

A little background: I work at CERN in the device drivers section and
we have a lot of VME crates we use in the accelerator controls. We have
been using an in-house driver for some time now and want to slowly port
our existing device drivers over to the kernel driver.

This set of patches adds multiple changes to the VME driver that fix
various issues, mostly dealing with the device driver model. Some of
these are based on the changes made by Emilio G. Cota in a previous
LKML thread at:

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

Thanks and best regards,
Manohar Vanga

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

Manohar Vanga (7):
  staging: vme_user: change kmalloc+memset to kzalloc
  staging: vme: make [alloc|free]_consistent bridge specific
  staging: vme: keep track of registered buses
  staging: vme: add functions for bridge module refcounting
  staging: vme: rename *_slot_get to *_get_slot
  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 |   54 ++++-
 drivers/staging/vme/bridges/vme_tsi148.c   |   53 ++++-
 drivers/staging/vme/devices/vme_user.c     |   63 ++++-
 drivers/staging/vme/devices/vme_user.h     |    2 +-
 drivers/staging/vme/vme.c                  |  414 ++++++++++++++++------------
 drivers/staging/vme/vme.h                  |   56 +++-
 drivers/staging/vme/vme_api.txt            |    2 +-
 drivers/staging/vme/vme_bridge.h           |   19 +-
 8 files changed, 435 insertions(+), 228 deletions(-)

-- 
1.7.4.1

[PATCH 1/8] staging: vme_user: change kmalloc+memset to kzalloc

[Manohar Vanga]

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/devices/vme_user.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 91d2cc7..3cbeb2a 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -651,7 +651,7 @@ static int __init vme_user_init(void)
 
 
 	/* Dynamically create the bind table based on module parameters */
-	ids = kmalloc(sizeof(struct vme_device_id) * (bus_num + 1), GFP_KERNEL);
+	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);
@@ -659,8 +659,6 @@ static int __init vme_user_init(void)
 		goto err_id;
 	}
 
-	memset(ids, 0, (sizeof(struct vme_device_id) * (bus_num + 1)));
-
 	for (i = 0; i < bus_num; i++) {
 		ids[i].bus = bus[i];
 		/*
-- 
1.7.4.1

Re: [PATCH 1/8] staging: vme_user: change kmalloc+memset to kzalloc

[Martyn Welch]

On 01/08/11 11:20, Manohar Vanga wrote:
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

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

> ---
>  drivers/staging/vme/devices/vme_user.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index 91d2cc7..3cbeb2a 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -651,7 +651,7 @@ static int __init vme_user_init(void)
>  
>  
>  	/* Dynamically create the bind table based on module parameters */
> -	ids = kmalloc(sizeof(struct vme_device_id) * (bus_num + 1), GFP_KERNEL);
> +	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);
> @@ -659,8 +659,6 @@ static int __init vme_user_init(void)
>  		goto err_id;
>  	}
>  
> -	memset(ids, 0, (sizeof(struct vme_device_id) * (bus_num + 1)));
> -
>  	for (i = 0; i < bus_num; i++) {
>  		ids[i].bus = bus[i];
>  		/*


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

Re: [PATCH 1/8] staging: vme_user: change kmalloc+memset to kzalloc

[Martyn Welch]

On 01/08/11 11:52, Martyn Welch wrote:
> On 01/08/11 11:20, Manohar Vanga wrote:
>> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> 
> Acked-by: Martyn Welch <martyn.welch@ge.com>
> 

Greg, please can you apply this patch.

As it's the first in the series, deals with a separate issue from the rest of
the patch series it would mean one less patch for Manohar to resend.

Martyn

>> ---
>>  drivers/staging/vme/devices/vme_user.c |    4 +---
>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
>> index 91d2cc7..3cbeb2a 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -651,7 +651,7 @@ static int __init vme_user_init(void)
>>  
>>  
>>  	/* Dynamically create the bind table based on module parameters */
>> -	ids = kmalloc(sizeof(struct vme_device_id) * (bus_num + 1), GFP_KERNEL);
>> +	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);
>> @@ -659,8 +659,6 @@ static int __init vme_user_init(void)
>>  		goto err_id;
>>  	}
>>  
>> -	memset(ids, 0, (sizeof(struct vme_device_id) * (bus_num + 1)));
>> -
>>  	for (i = 0; i < bus_num; i++) {
>>  		ids[i].bus = bus[i];
>>  		/*
> 
> 


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

[PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Manohar Vanga]

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

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

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

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

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

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

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

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Dan Carpenter]

On Mon, Aug 01, 2011 at 12:20:47PM +0200, Manohar Vanga wrote:
> -static int vme_alloc_bus_num(void)
> +/* call with vme_bus_num_mtx held */

Why move the lock outside the function?

> +static int __vme_alloc_bus_num(int bus_num)
>  {

regards,
dan carpenter

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Manohar Vanga]

On Mon, Aug 01, 2011 at 02:10:00PM +0300, Dan Carpenter wrote:
> On Mon, Aug 01, 2011 at 12:20:47PM +0200, Manohar Vanga wrote:
> > -static int vme_alloc_bus_num(void)
> > +/* call with vme_bus_num_mtx held */
> 
> Why move the lock outside the function?
> 
> > +static int __vme_alloc_bus_num(int bus_num)
> >  {
> 
> regards,
> dan carpenter
> 

It just makes the code in __vme_add_bus cleaner when you have failures.
I can merge __vme_add_bus and __vme_alloc_bus_num into one.

--
/manohar

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Martyn Welch]

On 01/08/11 11:20, Manohar Vanga wrote:
> From: Emilio G. Cota <cota@braap.org>
> 
> In a configuration with several bridges, each bridge is
> assigned a certain bus number depending on the order in which
> bridge modules are loaded. This can complicate multi-bridge
> installations because the bus numbers will depend on the load
> order of the bridges.
> 

I assume this is in a system where there are more than one type of bridge
(i.e. a ca91c042 and tsi148).

I'm not sure that passing a list of bus numbers to each driver is the correct
way to resolve this. This issue also exists for hard drives and ethernet
devices as well.

The network subsystem either has or uses a mechanism to allow udev rules to
rename the devices (this seems to be done by MAC address on my system).

I believe drive ordering is resolved to some extent with UUIDs. Persistent
device naming can be provided by using udev rules to create symlinks (such as
"/dev/cdrom" etc), with the drives are determined by system topology.

I'm wondering whether re-ordering the bus numbering based on system topology
using udev rules (where persistent bus numbering is required), or bind based
on system topology and not need persistent numbering at all).

Martyn

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


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

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Manohar Vanga]

Hi Martyn,

> I assume this is in a system where there are more than one type of bridge
> (i.e. a ca91c042 and tsi148).

Yes. This is referring to multiple bridges on a single PCI bus (in the case of
the existing bridges). In practice we have only a single bridge of a single
type. We can however have multiple types of bridges (eg. 1 TSI148 and 1 Universe).
This solution is specifically for such situations.

> I'm not sure that passing a list of bus numbers to each driver is the correct
> way to resolve this. This issue also exists for hard drives and ethernet
> devices as well.

This solution won't help the cases where multiple bridges of the same type are
present (as we cannot control probe order) however in the case of multiple
bridges of differing types, it helps in deterministically assigning numbers to
the bridge.

The array was created simply for the reason that the code allows for multiple
bridges of the same kind. In practice we have only one of a specific kind.

> The network subsystem either has or uses a mechanism to allow udev rules to
> rename the devices (this seems to be done by MAC address on my system).
> 
> I believe drive ordering is resolved to some extent with UUIDs. Persistent
> device naming can be provided by using udev rules to create symlinks (such as
> "/dev/cdrom" etc), with the drives are determined by system topology.
> 
> I'm wondering whether re-ordering the bus numbering based on system topology
> using udev rules (where persistent bus numbering is required), or bind based
> on system topology and not need persistent numbering at all).

The problem here is that it is kernel space that requires a consistent
numbering so creating symlinks is not a useful solution for this. (unless the
driver loader script queries each device for its UUID at startup which seems
a little like a roundabout way).

The VME drivers require a list of bus numbers that it passes to the VME
driver so it knows which buses/bridges to probe on. We need to pass this to
the drivers during load time. Are you recommending we change this to UUID's?

Also, if we replace a card entirely (eg. in the case of a malfunction), the
UUID changes and we need to update a lot of local files to maintain the
automated startup (Please correct me if I'm wrong here). This would become a
mess to maintain really quickly.

In this patch, we don't solve the issue of multiple bridges of the same kind
but we solve the automation issue in cases of different types of bridge.
We already maintain a database with configuration options for each crate
and a bus number can be easily added. This won't change in the case of card
replacement.

--
/manohar

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Martyn Welch]

On 01/08/11 15:31, Manohar Vanga wrote:
> Hi Martyn,
> 
>> I assume this is in a system where there are more than one type of bridge
>> (i.e. a ca91c042 and tsi148).
> 
> Yes. This is referring to multiple bridges on a single PCI bus (in the case of
> the existing bridges). In practice we have only a single bridge of a single
> type. We can however have multiple types of bridges (eg. 1 TSI148 and 1 Universe).
> This solution is specifically for such situations.
> 
>> I'm not sure that passing a list of bus numbers to each driver is the correct
>> way to resolve this. This issue also exists for hard drives and ethernet
>> devices as well.
> 
> This solution won't help the cases where multiple bridges of the same type are
> present (as we cannot control probe order) however in the case of multiple
> bridges of differing types, it helps in deterministically assigning numbers to
> the bridge.
> 
> The array was created simply for the reason that the code allows for multiple
> bridges of the same kind. In practice we have only one of a specific kind.
> 
>> The network subsystem either has or uses a mechanism to allow udev rules to
>> rename the devices (this seems to be done by MAC address on my system).
>>
>> I believe drive ordering is resolved to some extent with UUIDs. Persistent
>> device naming can be provided by using udev rules to create symlinks (such as
>> "/dev/cdrom" etc), with the drives are determined by system topology.
>>
>> I'm wondering whether re-ordering the bus numbering based on system topology
>> using udev rules (where persistent bus numbering is required), or bind based
>> on system topology and not need persistent numbering at all).
> 
> The problem here is that it is kernel space that requires a consistent
> numbering so creating symlinks is not a useful solution for this. (unless the
> driver loader script queries each device for its UUID at startup which seems
> a little like a roundabout way).
> 

No I'm thinking more along the lines of how the persistent device names seem
to be created. For example, on my system udev is used to create a symlink
called "/dev/cdrom" using this udev rule:

SUBSYSTEM=="block", ENV{ID_CDROM}=="?*",
ENV{ID_PATH}=="pci-0000:00:1f.2-scsi-1:0:0:0", SYMLINK+="cdrom",
ENV{GENERATED}="1"

The bit that might be more useful to us is the ENV{ID_PATH}, the
"pci-0000:00:1f.2" bit in particular. If I'm not mistaken, this is sufficient
to identify a PCI device (such as a VME bridge)


> The VME drivers require a list of bus numbers that it passes to the VME
> driver so it knows which buses/bridges to probe on. We need to pass this to
> the drivers during load time. Are you recommending we change this to UUID's?
> 

I'm thinking of the topology that is used in the above example to uniquely
identify a specific SATA device.

We could either provide the above paths, or rename buses similar to how
network interfaces are renamed (hence why I mentioned them as well). Of course
renaming the buses has the disadvantage that the correct names will only be in
place once the board has got to user space, so the drivers would have to be
built as modules.

> Also, if we replace a card entirely (eg. in the case of a malfunction), the
> UUID changes and we need to update a lot of local files to maintain the
> automated startup (Please correct me if I'm wrong here). This would become a
> mess to maintain really quickly.
> 

I assume that you replace like with like, so the bridges are found in the same
order (if you have more than one of the same bridge). In this case the
topology will be the same, so you should reliably be able to replace cards.

> In this patch, we don't solve the issue of multiple bridges of the same kind
> but we solve the automation issue in cases of different types of bridge.

As long as you only have one type of each...

I'm wondering whether an approach as above may reliably solve both.

Martyn

> We already maintain a database with configuration options for each crate
> and a bus number can be easily added. This won't change in the case of card
> replacement.
> 
> --
> /manohar


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

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Manohar Vanga]

Hey Martin,

> No I'm thinking more along the lines of how the persistent device names seem
> to be created. For example, on my system udev is used to create a symlink
> called "/dev/cdrom" using this udev rule:
> 
> SUBSYSTEM=="block", ENV{ID_CDROM}=="?*",
> ENV{ID_PATH}=="pci-0000:00:1f.2-scsi-1:0:0:0", SYMLINK+="cdrom",
> ENV{GENERATED}="1"
> 
> The bit that might be more useful to us is the ENV{ID_PATH}, the
> "pci-0000:00:1f.2" bit in particular. If I'm not mistaken, this is sufficient
> to identify a PCI device (such as a VME bridge)

We have cases where we have multiple PCI devices of the same type and we need
a way to access each of them based on physical location. This cannot be solved
in PCI in any way except manually testing and figuring out which slot
corresponds to which device.

Firmware changes and BIOS updates have a chance of changing the PCI ordering.
We have had this problem before and we solve it by mapping the PCI bus and
device ID's to slot numbers (these are manually entered into the database by
looking at the lspci output and manually testing which device is which). These
mappings are stored in a local file at the moment (with plans to move it into
the database). This file looks something like this:

$ cat /etc/crateconfig
1 0 0
2 0 0
3 0 0
4 2 f
5 2 e <-- eg. This is the 1st device (02:0e.0)
6 2 d <--     This is the 2nd device (02:0d.0)
7 0 0
8 0 0
9 0 0
10 0 0
11 0 0
12 1 f
13 1 e
14 1 d

The database entry for the crate then uses these mapped numbers only:

    --> TEWS Technologies GmbH TPCI200, Slot 5
    --> TEWS Technologies GmbH TPCI200, Slot 6

For such device drivers, the loading is done as follows:

1. Get the mapped slot number for the device from the database (eg. 5, 6)
2. Convert it to the bus/device combination using /etc/crateconfig
   (eg. 5 --> 02,0e and 6 --> 02,0d)
3. Pass it to the driver during load time using module parameters
     $ insmod driver.ko dev_id=0,1 buses=02,02 devices=0e, 0d
   The driver checks this with the pci_dev structure passed to it and
   assigns the device number based on this.

This is what would be my suggestion for doing with multiple bridges of the
same kind. This way we can control the changes from firmware through
modparams and a config file like /etc/crateconfig. If the PCI locations
change with BIOS/firmware updates, we can manually change the mappings to
the new ones.

The UUID's do nothing but compress the two separate params (bus, device)
into a single param (the UUID). This has the bigger disadvantage that it
is not human readable. This is a strong requirement since the database
mappings need to be maintained by the admins.

The point I'm trying to make is that the PCI id's cannot be used directly
as they change with firmware and BIOS changes.

> I'm thinking of the topology that is used in the above example to uniquely
> identify a specific SATA device.

SATA devices like hard disks have unique fixed identifiers that can be used
to generate the UUID's. PCI device locations shouldn't be assumed to be
fixed.

> We could either provide the above paths, or rename buses similar to how
> network interfaces are renamed (hence why I mentioned them as well). Of course
> renaming the buses has the disadvantage that the correct names will only be in
> place once the board has got to user space, so the drivers would have to be
> built as modules.

Yes, this would be another contraint.

> I assume that you replace like with like, so the bridges are found in the same
> order (if you have more than one of the same bridge). In this case the
> topology will be the same, so you should reliably be able to replace cards.

Yeah but if firmware/BIOS versions are differing in the cards (eg. updated
revisions of the board), then things could get messy.

--
/manohar

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Martyn Welch]

On 02/08/11 12:54, Manohar Vanga wrote:
> Hey Martin,
> 
>> No I'm thinking more along the lines of how the persistent device names seem
>> to be created. For example, on my system udev is used to create a symlink
>> called "/dev/cdrom" using this udev rule:
>>
>> SUBSYSTEM=="block", ENV{ID_CDROM}=="?*",
>> ENV{ID_PATH}=="pci-0000:00:1f.2-scsi-1:0:0:0", SYMLINK+="cdrom",
>> ENV{GENERATED}="1"
>>
>> The bit that might be more useful to us is the ENV{ID_PATH}, the
>> "pci-0000:00:1f.2" bit in particular. If I'm not mistaken, this is sufficient
>> to identify a PCI device (such as a VME bridge)
> 
> We have cases where we have multiple PCI devices of the same type and we need
> a way to access each of them based on physical location. This cannot be solved
> in PCI in any way except manually testing and figuring out which slot
> corresponds to which device.
> 

I agree - you will always need to work out which PCI device corresponds to
which VME bus.

> Firmware changes and BIOS updates have a chance of changing the PCI ordering.
> We have had this problem before and we solve it by mapping the PCI bus and
> device ID's to slot numbers (these are manually entered into the database by
> looking at the lspci output and manually testing which device is which). These
> mappings are stored in a local file at the moment (with plans to move it into
> the database). This file looks something like this:
> 
> $ cat /etc/crateconfig
> 1 0 0
> 2 0 0
> 3 0 0
> 4 2 f
> 5 2 e <-- eg. This is the 1st device (02:0e.0)
> 6 2 d <--     This is the 2nd device (02:0d.0)
> 7 0 0
> 8 0 0
> 9 0 0
> 10 0 0
> 11 0 0
> 12 1 f
> 13 1 e
> 14 1 d
> 
> The database entry for the crate then uses these mapped numbers only:
> 
>     --> TEWS Technologies GmbH TPCI200, Slot 5
>     --> TEWS Technologies GmbH TPCI200, Slot 6
> 

An I right in thinking that you use IndustryPack VME bridges on TPCI200
carrier cards?

I'm not sure I follow. The above configuration seems to describe how you map
physical slot numbers to devices, not VME bridges to an reasonably arbitrary
VME bus number on a specific host. I assume that 02.0e.0 and 02.0d.0 are 2
bridges on the same host and that the host is acting as a bridge between slots
1-5 and 6-14 that are 2 separate buses in the same crate.

> For such device drivers, the loading is done as follows:
> 
> 1. Get the mapped slot number for the device from the database (eg. 5, 6)
> 2. Convert it to the bus/device combination using /etc/crateconfig
>    (eg. 5 --> 02,0e and 6 --> 02,0d)
> 3. Pass it to the driver during load time using module parameters
>      $ insmod driver.ko dev_id=0,1 buses=02,02 devices=0e, 0d
>    The driver checks this with the pci_dev structure passed to it and
>    assigns the device number based on this.
>

So 0, 1 are the bus numbers?

Where does the 5 & 6 in the part above come into play?

> This is what would be my suggestion for doing with multiple bridges of the
> same kind. This way we can control the changes from firmware through
> modparams and a config file like /etc/crateconfig. If the PCI locations
> change with BIOS/firmware updates, we can manually change the mappings to
> the new ones.
> 

If we are storing them in user space, do we need to push them into kernel
space when we load the VME bridge driver?

> The UUID's do nothing but compress the two separate params (bus, device)
> into a single param (the UUID). This has the bigger disadvantage that it
> is not human readable. This is a strong requirement since the database
> mappings need to be maintained by the admins.
> 

I'm not sure it's any more or less readable than having separate PCI bus and
PCI device numbers in a csv formatted file.

It does do more. Crucially in the udev example I gave, it identifies that the
disk is connected to a controller on PCI. It also identifies that it's on PCI
domain 0.

I'm fairly sure that a this format allows for other buses to provide parameter
strings in varying formats, should someone write a bridge driver for a USB-VME
bridge like this:

http://www.struck.de/sis3150usb.htm

We could still identify the VME bridge being used (even if two such devices
were connected to the same host).

> The point I'm trying to make is that the PCI id's cannot be used directly
> as they change with firmware and BIOS changes.
> 

But you seem to be using them directly in the example above. You pass in the
PCI bus and device number. In fact. you are *just* using the PCI bus and
device numbering, which would not work with the USB-VME bridge above.

>> I'm thinking of the topology that is used in the above example to uniquely
>> identify a specific SATA device.
> 
> SATA devices like hard disks have unique fixed identifiers that can be used
> to generate the UUID's. PCI device locations shouldn't be assumed to be
> fixed.
> 

Which part of the example udev rule I gave is a fixed identifier? It
identifies the CDROM based on system topology, using the PCI bus numbering and
SCSI bus numbering.

For example, for a device sitting on VME, in the A32 address space at 0x30000,
via a PCI-VME bridge on PCI bus 2, device 0d, we could provide something like
this:

vme-a32:30000-pci-0000:02:0d.0

>> We could either provide the above paths, or rename buses similar to how
>> network interfaces are renamed (hence why I mentioned them as well). Of course
>> renaming the buses has the disadvantage that the correct names will only be in
>> place once the board has got to user space, so the drivers would have to be
>> built as modules.
> 
> Yes, this would be another contraint.
> 
>> I assume that you replace like with like, so the bridges are found in the same
>> order (if you have more than one of the same bridge). In this case the
>> topology will be the same, so you should reliably be able to replace cards.
> 
> Yeah but if firmware/BIOS versions are differing in the cards (eg. updated
> revisions of the board), then things could get messy.
> 

I can't see how the approach you laid out above solves this.

Martyn


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

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Martyn Welch]

On 02/08/11 15:57, Martyn Welch wrote:
> 
> Which part of the example udev rule I gave is a fixed identifier? It
> identifies the CDROM based on system topology, using the PCI bus numbering and
> SCSI bus numbering.
> 
> For example, for a device sitting on VME, in the A32 address space at 0x30000,
> via a PCI-VME bridge on PCI bus 2, device 0d, we could provide something like
> this:
> 
> vme-a32:30000-pci-0000:02:0d.0
> 

Hmm, thinking about it, that's not right either.

The VME bridge would be at "pci-0000:02:0d.0". In the current scheme this
would be given a bus number and we should be able to discover from sysfs which
bus numbers are used for which VME bridges. For example, as the usb buses are
enumerated:

$ ls -la /sys/bus/usb/devices/usb*
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb1 ->
../../../devices/pci0000:00/0000:00:1a.7/usb1
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb2 ->
../../../devices/pci0000:00/0000:00:1d.7/usb2
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb3 ->
../../../devices/pci0000:00/0000:00:1a.0/usb3
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb4 ->
../../../devices/pci0000:00/0000:00:1a.1/usb4
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb5 ->
../../../devices/pci0000:00/0000:00:1a.2/usb5
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb6 ->
../../../devices/pci0000:00/0000:00:1d.0/usb6
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb7 ->
../../../devices/pci0000:00/0000:00:1d.1/usb7
lrwxrwxrwx 1 root root 0 2011-08-03 09:29 /sys/bus/usb/devices/usb8 ->
../../../devices/pci0000:00/0000:00:1d.2/usb8
$

Whilst it would be nice to be able to change the bus numbering, I don't think
that passing numbers into the bridge driver at load time is the correct approach.

Martyn

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

Re: [PATCH 2/8] staging: vme: allow explicit assignment of bus numbers

[Manohar Vanga]

Hey Martyn,

> Whilst it would be nice to be able to change the bus numbering, I don't think
> that passing numbers into the bridge driver at load time is the correct approach.

Depends on the problem we are trying to solve :P

This patch fixes the issue with multiple bridges of different types in a simple way
(which is currently an issue for us).

The UUID method would be more "complete" and solve the issues with multiple bridges
of the same type but I think we can apply this one till we have an implementation
for it. What do you think?

Thanks!

--
/manohar

[PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Manohar Vanga]

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

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

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

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Dan Carpenter]

On Mon, Aug 01, 2011 at 12:20:48PM +0200, Manohar Vanga wrote:
>  	if (bridge->parent == NULL) {
>  		printk(KERN_ERR "Dev entry NULL\n");

The user wouldn't know which driver is printing the error.

>  		return NULL;
>  	}
> -	pdev = container_of(bridge->parent, struct pci_dev, dev);
>  
> -	return pci_alloc_consistent(pdev, size, dma);
> +	if (bridge->alloc_consistent == NULL) {
> +		printk(KERN_ERR "alloc_consistent not supported by bridge\n");

Could probably be improved as well.

> +		return NULL;
> +	}
> +

regards,
dan carpenter

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Manohar Vanga]

Hi Dan,

> The user wouldn't know which driver is printing the error.
> ...
> Could probably be improved as well.

Thanks! I have added the bridge name to the output here.

P.S. Should I send this patch immediately or wait till I have some
feedback on the others and resend them all at once?

--
/manohar

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Martyn Welch]

On 01/08/11 11:20, Manohar Vanga wrote:
> Make PCI dependent functions ([alloc|free]_consistent() in
> 'vme.c') bridge specific. By removing the dependency of the
> VME bridge framework on PCI, this patch allows for addition of
> non-PCI based VME bridges.
> 

I like the approach, I think I agree with Dan, I'd rather see the locking
inside the function for now.

Martyn

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


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

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Manohar Vanga]

> I like the approach, I think I agree with Dan, I'd rather see the locking
> inside the function for now.

Great! I'll put the locking into the function and resend the patch.

--
/manohar

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Manohar Vanga]

Hi Martyn,

> > Make PCI dependent functions ([alloc|free]_consistent() in
> > 'vme.c') bridge specific. By removing the dependency of the
> > VME bridge framework on PCI, this patch allows for addition of
> > non-PCI based VME bridges.
> > 
> 
> I like the approach, I think I agree with Dan, I'd rather see the locking
> inside the function for now.

Just noticed that the locking that Dan was referring to and the one you agree
with in the previous patch ([PATCH 2/8] staging: vme: allow explicit assignment
of bus numbers) and not this one. Which one are you referring to here?

--
/manohar

Re: [PATCH 3/8] staging: vme: make [alloc|free]_consistent bridge specific

[Martyn Welch]

On 01/08/11 15:00, Manohar Vanga wrote:
> Hi Martyn,
> 
>>> Make PCI dependent functions ([alloc|free]_consistent() in
>>> 'vme.c') bridge specific. By removing the dependency of the
>>> VME bridge framework on PCI, this patch allows for addition of
>>> non-PCI based VME bridges.
>>>
>>
>> I like the approach, I think I agree with Dan, I'd rather see the locking
>> inside the function for now.
> 
> Just noticed that the locking that Dan was referring to and the one you agree
> with in the previous patch ([PATCH 2/8] staging: vme: allow explicit assignment
> of bus numbers) and not this one. Which one are you referring to here?
> 

Whoops. Sorry, Dan's comment about the error printing, not the locking.

Martyn

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

[PATCH 4/8] staging: vme: keep track of registered buses

[Manohar Vanga]

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

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

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

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

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

[PATCH 5/8] 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()'.

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/devices/vme_user.c     |   20 +++++++++++
 drivers/staging/vme/vme.c                  |   48 ++++++++++++++++++++++++++++
 drivers/staging/vme/vme.h                  |    2 +
 drivers/staging/vme/vme_bridge.h           |    1 +
 6 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 15a0b19..2f9c22b 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1680,6 +1680,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ca91cx42_bridge->parent = &pdev->dev;
 	strcpy(ca91cx42_bridge->name, driver_name);
 
+	ca91cx42_bridge->owner = THIS_MODULE;
+
 	/* Setup IRQ */
 	retval = ca91cx42_irq_init(ca91cx42_bridge);
 	if (retval != 0) {
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index 5c147d6..9db89dc 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2320,6 +2320,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	tsi148_bridge->parent = &pdev->dev;
 	strcpy(tsi148_bridge->name, driver_name);
 
+	tsi148_bridge->owner = THIS_MODULE;
+
 	/* Setup IRQ */
 	retval = tsi148_irq_init(tsi148_bridge);
 	if (retval != 0) {
diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 3cbeb2a..7ed4a5c 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -118,6 +118,8 @@ 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_bridge *vme_user_bus;		/* Pointer to bridge */
+
 
 static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
 					MASTER_MINOR,	MASTER_MINOR,
@@ -824,10 +826,25 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
 		}
 	}
 
+	/*
+	 * Increment reference count of bridge module. We don't want things
+	 * to fall over in case the bridge module is removed while the vme_user
+	 * driver is still loaded.
+	 */
+	vme_user_bus = vme_bridge_get(cur_bus);
+	if (!vme_user_bus) {
+		printk(KERN_ERR "%s: vme_bridge_get failed\n", driver_name);
+		err = -EINVAL;
+		goto err_bridge_get;
+	}
+
 	return 0;
 
 	/* Ensure counter set correcty to destroy all sysfs devices */
 	i = VME_DEVS;
+err_bridge_get:
+	for (i = 0; i < VME_DEVS; i++)
+		device_destroy(vme_user_sysfs_class, MKDEV(VME_MAJOR, i));
 err_sysfs:
 	while (i > 0) {
 		i--;
@@ -892,6 +909,9 @@ static int __devexit vme_user_remove(struct device *dev, int cur_bus,
 	/* Unregiser the major and minor device numbers */
 	unregister_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS);
 
+	/* decrement bridge module reference */
+	vme_bridge_put(vme_user_bus);
+
 	return 0;
 }
 
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 11e28ca..759ec2b 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -52,6 +52,54 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
 }
 
 /*
+ * Increments the reference count of a VME bridge.
+ *
+ * If found, a pointer to the bridge is returned and the reference count
+ * of the module that owns it is incremented.
+ *
+ * On success, it can be assumed that the bridge won't be removed until
+ * the corresponding call to vme_put_bridge().
+ *
+ * Normally drivers should call vme_get_bridge() on a successfull .probe,
+ * and vme_put_bridge() when releasing the device, e.g. in .remove.
+ */
+struct vme_bridge *vme_bridge_get(unsigned int bus_id)
+{
+	struct vme_bridge *bridge;
+	struct vme_bridge *retp = NULL;
+
+	mutex_lock(&vme_buses_lock);
+	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
+		if (bridge->num == bus_id) {
+			if (!bridge->owner)
+				dev_warn(bridge->parent,
+					"bridge->owner not set\n");
+			else if (try_module_get(bridge->owner))
+				retp = bridge;
+			break;
+		}
+	}
+	mutex_unlock(&vme_buses_lock);
+	return retp;
+}
+EXPORT_SYMBOL(vme_bridge_get);
+
+/*
+ * Decrements the reference count of a bridge
+ *
+ * This function decrements the reference count of the module that controls
+ * the bridge. It must come after a call to vme_get_bridge().
+ */
+void vme_bridge_put(struct vme_bridge *bridge)
+{
+	if (!bridge->owner)
+		dev_warn(bridge->parent, "bridge->owner not set\n");
+	else
+		module_put(bridge->owner);
+}
+EXPORT_SYMBOL(vme_bridge_put);
+
+/*
  * Find the bridge that the resource is associated with.
  */
 static struct vme_bridge *find_bridge(struct vme_resource *resource)
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 3ccb497..8fb35e2 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -167,6 +167,8 @@ int vme_slot_get(struct device *);
 int vme_register_driver(struct vme_driver *);
 void vme_unregister_driver(struct vme_driver *);
 
+struct vme_bridge *vme_bridge_get(unsigned int bus_id);
+void vme_bridge_put(struct vme_bridge *bridge);
 
 #endif /* _VME_H_ */
 
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index 8959670..ef751a4 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -113,6 +113,7 @@ struct vme_bridge {
 	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
 	void *driver_priv;	/* Private pointer for the bridge driver */
 	struct list_head bus_list; /* list of VME buses */
+	struct module *owner;	/* module that owns the bridge */
 
 	struct device dev[VME_SLOTS_MAX];	/* Device registered with
 						 * device model on VME bus
-- 
1.7.4.1

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

[Martyn Welch]

On 01/08/11 11:20, 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()'.
> 
> 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/devices/vme_user.c     |   20 +++++++++++
>  drivers/staging/vme/vme.c                  |   48 ++++++++++++++++++++++++++++
>  drivers/staging/vme/vme.h                  |    2 +
>  drivers/staging/vme/vme_bridge.h           |    1 +
>  6 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index 15a0b19..2f9c22b 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1680,6 +1680,8 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ca91cx42_bridge->parent = &pdev->dev;
>  	strcpy(ca91cx42_bridge->name, driver_name);
>  
> +	ca91cx42_bridge->owner = THIS_MODULE;
> +
>  	/* Setup IRQ */
>  	retval = ca91cx42_irq_init(ca91cx42_bridge);
>  	if (retval != 0) {
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index 5c147d6..9db89dc 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2320,6 +2320,8 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	tsi148_bridge->parent = &pdev->dev;
>  	strcpy(tsi148_bridge->name, driver_name);
>  
> +	tsi148_bridge->owner = THIS_MODULE;
> +
>  	/* Setup IRQ */
>  	retval = tsi148_irq_init(tsi148_bridge);
>  	if (retval != 0) {
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index 3cbeb2a..7ed4a5c 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -118,6 +118,8 @@ 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_bridge *vme_user_bus;		/* Pointer to bridge */
> +
>  
>  static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
>  					MASTER_MINOR,	MASTER_MINOR,
> @@ -824,10 +826,25 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
>  		}
>  	}
>  
> +	/*
> +	 * Increment reference count of bridge module. We don't want things
> +	 * to fall over in case the bridge module is removed while the vme_user
> +	 * driver is still loaded.
> +	 */
> +	vme_user_bus = vme_bridge_get(cur_bus);
> +	if (!vme_user_bus) {
> +		printk(KERN_ERR "%s: vme_bridge_get failed\n", driver_name);
> +		err = -EINVAL;
> +		goto err_bridge_get;
> +	}
> +

Can we not do this inside vme_master_request, vme_slave_request, etc?

I.e. Add reference count when resources are given out.

>  	return 0;
>  
>  	/* Ensure counter set correcty to destroy all sysfs devices */
>  	i = VME_DEVS;
> +err_bridge_get:
> +	for (i = 0; i < VME_DEVS; i++)
> +		device_destroy(vme_user_sysfs_class, MKDEV(VME_MAJOR, i));
>  err_sysfs:
>  	while (i > 0) {
>  		i--;
> @@ -892,6 +909,9 @@ static int __devexit vme_user_remove(struct device *dev, int cur_bus,
>  	/* Unregiser the major and minor device numbers */
>  	unregister_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS);
>  
> +	/* decrement bridge module reference */
> +	vme_bridge_put(vme_user_bus);
> +

This would then be done in the vme_*_free routines.

Martyn

>  	return 0;
>  }
>  
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 11e28ca..759ec2b 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -52,6 +52,54 @@ static struct vme_bridge *dev_to_bridge(struct device *dev)
>  }
>  
>  /*
> + * Increments the reference count of a VME bridge.
> + *
> + * If found, a pointer to the bridge is returned and the reference count
> + * of the module that owns it is incremented.
> + *
> + * On success, it can be assumed that the bridge won't be removed until
> + * the corresponding call to vme_put_bridge().
> + *
> + * Normally drivers should call vme_get_bridge() on a successfull .probe,
> + * and vme_put_bridge() when releasing the device, e.g. in .remove.
> + */
> +struct vme_bridge *vme_bridge_get(unsigned int bus_id)
> +{
> +	struct vme_bridge *bridge;
> +	struct vme_bridge *retp = NULL;
> +
> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		if (bridge->num == bus_id) {
> +			if (!bridge->owner)
> +				dev_warn(bridge->parent,
> +					"bridge->owner not set\n");
> +			else if (try_module_get(bridge->owner))
> +				retp = bridge;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&vme_buses_lock);
> +	return retp;
> +}
> +EXPORT_SYMBOL(vme_bridge_get);
> +
> +/*
> + * Decrements the reference count of a bridge
> + *
> + * This function decrements the reference count of the module that controls
> + * the bridge. It must come after a call to vme_get_bridge().
> + */
> +void vme_bridge_put(struct vme_bridge *bridge)
> +{
> +	if (!bridge->owner)
> +		dev_warn(bridge->parent, "bridge->owner not set\n");
> +	else
> +		module_put(bridge->owner);
> +}
> +EXPORT_SYMBOL(vme_bridge_put);
> +
> +/*
>   * Find the bridge that the resource is associated with.
>   */
>  static struct vme_bridge *find_bridge(struct vme_resource *resource)
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 3ccb497..8fb35e2 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -167,6 +167,8 @@ int vme_slot_get(struct device *);
>  int vme_register_driver(struct vme_driver *);
>  void vme_unregister_driver(struct vme_driver *);
>  
> +struct vme_bridge *vme_bridge_get(unsigned int bus_id);
> +void vme_bridge_put(struct vme_bridge *bridge);
>  
>  #endif /* _VME_H_ */
>  
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index 8959670..ef751a4 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -113,6 +113,7 @@ struct vme_bridge {
>  	struct device *parent;	/* Parent device (eg. pdev->dev for PCI) */
>  	void *driver_priv;	/* Private pointer for the bridge driver */
>  	struct list_head bus_list; /* list of VME buses */
> +	struct module *owner;	/* module that owns the bridge */
>  
>  	struct device dev[VME_SLOTS_MAX];	/* Device registered with
>  						 * device model on VME bus


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

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

[Manohar Vanga]

Hey Martyn,

> Can we not do this inside vme_master_request, vme_slave_request, etc?
> 
> I.e. Add reference count when resources are given out.
> 
> This would then be done in the vme_*_free routines.

I agree this would be much better. I will change this and resend :)

Thanks!
--
/manohar

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

[Emilio G. Cota]

On Wed, Aug 03, 2011 at 16:06:30 +0200, Manohar Vanga wrote:
> Hey Martyn,
> 
> > Can we not do this inside vme_master_request, vme_slave_request, etc?
> > 
> > I.e. Add reference count when resources are given out.
> > 
> > This would then be done in the vme_*_free routines.
> 
> I agree this would be much better. I will change this and resend :)

To me it seems better to keep this as is, to be
used by .probe and .release methods, in the same way
usb_get_dev() and usb_put_dev() are used by usb drivers.

Hiding refcounts under the resource accessors will just
complicate things unnecessarily. Modifying refcounts
should always be explicit unless there's a very good
reason against it.

		Emilio 

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

[Martyn Welch]

On 03/08/11 16:23, Emilio G. Cota wrote:
> On Wed, Aug 03, 2011 at 16:06:30 +0200, Manohar Vanga wrote:
>> Hey Martyn,
>>
>>> Can we not do this inside vme_master_request, vme_slave_request, etc?
>>>
>>> I.e. Add reference count when resources are given out.
>>>
>>> This would then be done in the vme_*_free routines.
>>
>> I agree this would be much better. I will change this and resend :)
> 
> To me it seems better to keep this as is, to be
> used by .probe and .release methods, in the same way
> usb_get_dev() and usb_put_dev() are used by usb drivers.
> 

The description of the functions in usb.c suggests, to me at least, that these
functions are the equivalent to the vme_*_request functions in the vme code.
Being called when a function binds to an interface and when it is finished
with it.

Martyn

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

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

[Emilio G. Cota]

On Thu, Aug 04, 2011 at 08:23:48 +0100, Martyn Welch wrote:
> On 03/08/11 16:23, Emilio G. Cota wrote:
> > On Wed, Aug 03, 2011 at 16:06:30 +0200, Manohar Vanga wrote:
> >>> Can we not do this inside vme_master_request, vme_slave_request, etc?
> >>> I.e. Add reference count when resources are given out.
> >>> This would then be done in the vme_*_free routines.
> >>
> >> I agree this would be much better. I will change this and resend :)
> > 
> > To me it seems better to keep this as is, to be
> > used by .probe and .release methods, in the same way
> > usb_get_dev() and usb_put_dev() are used by usb drivers.
> 
> The description of the functions in usb.c suggests, to me at least, that these
> functions are the equivalent to the vme_*_request functions in the vme code.
> Being called when a function binds to an interface and when it is finished
> with it.

Which functions? usb_get_dev and usb_put_dev? These _only_
in/de-crement the refcounts. They're called by .probe and
.release methods of usb drivers. The "resources" or "services"
the callers acquire from the usb bridge is orthogonal.

The point is to separate incrementing the refcounts from other
functionality. We have currently five "request" functions:
vme_{master,slave,dma,irq,lm}_request. The average driver
would call two or three of these for each device. This would
result in a fairly large refcount for the bridge, that would
tell us nothing about how many users (devices) are hanging in
there--"yeah, we have lots" is not good enough.

So no, usb_get_dev and usb_put_dev are not equivalent to our
vme_*_request functions. The get/put functions only operate
on the refcounts, to mark the dependency of a struct device
on another device (the bridge) so that the parent cannot
be removed. This is the way things are done in other much
more mature subsystems, I don't see why we should do it
differently.

		Emilio

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

[Martyn Welch]

On 04/08/11 17:34, Emilio G. Cota wrote:
> On Thu, Aug 04, 2011 at 08:23:48 +0100, Martyn Welch wrote:
>> On 03/08/11 16:23, Emilio G. Cota wrote:
>>> On Wed, Aug 03, 2011 at 16:06:30 +0200, Manohar Vanga wrote:
>>>>> Can we not do this inside vme_master_request, vme_slave_request, etc?
>>>>> I.e. Add reference count when resources are given out.
>>>>> This would then be done in the vme_*_free routines.
>>>>
>>>> I agree this would be much better. I will change this and resend :)
>>>
>>> To me it seems better to keep this as is, to be
>>> used by .probe and .release methods, in the same way
>>> usb_get_dev() and usb_put_dev() are used by usb drivers.
>>
>> The description of the functions in usb.c suggests, to me at least, that these
>> functions are the equivalent to the vme_*_request functions in the vme code.
>> Being called when a function binds to an interface and when it is finished
>> with it.
> 
> Which functions? usb_get_dev and usb_put_dev? These _only_
> in/de-crement the refcounts. They're called by .probe and
> .release methods of usb drivers. The "resources" or "services"
> the callers acquire from the usb bridge is orthogonal.
> 
> The point is to separate incrementing the refcounts from other
> functionality. We have currently five "request" functions:
> vme_{master,slave,dma,irq,lm}_request. The average driver
> would call two or three of these for each device. This would
> result in a fairly large refcount for the bridge, that would
> tell us nothing about how many users (devices) are hanging in
> there--"yeah, we have lots" is not good enough.
> 

Actually, it would give you a good indication of how many of the resources
provided by each VME bridge chip were used. I don't see the refcount
accurately reflecting the number of users as being important, more as a means
of tracking which bridges have resources that are being used (and therefore
can't be removed).

> So no, usb_get_dev and usb_put_dev are not equivalent to our
> vme_*_request functions. The get/put functions only operate
> on the refcounts, to mark the dependency of a struct device
> on another device (the bridge) so that the parent cannot
> be removed. This is the way things are done in other much
> more mature subsystems, I don't see why we should do it
> differently.
> 

The USB bus and VME bus are very different entities.

Martyn

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

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

[Manohar Vanga]

Hey Martin,

> Actually, it would give you a good indication of how many of the resources
> provided by each VME bridge chip were used. I don't see the refcount
> accurately reflecting the number of users as being important, more as a means
> of tracking which bridges have resources that are being used (and therefore
> can't be removed).

On second thought, I think I agree with Emilio that the _module_ refcount should
reflect the number of users (other modules) using the bridge module. The refcount
of resources should probably stay separate from the refcount of module usage. We
can add resource refcounting as well but I don't see a need for it at the moment.

vme_bridge_get() and vme_bridge_put() in this case should refer to the reference
count of the bridge module (not the resources). What do you think?

Thanks!

--
/manohar

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

[Martyn Welch]

On 05/08/11 10:04, Manohar Vanga wrote:
> Hey Martin,
> 
>> Actually, it would give you a good indication of how many of the resources
>> provided by each VME bridge chip were used. I don't see the refcount
>> accurately reflecting the number of users as being important, more as a means
>> of tracking which bridges have resources that are being used (and therefore
>> can't be removed).
> 
> On second thought, I think I agree with Emilio that the _module_ refcount should
> reflect the number of users (other modules) using the bridge module. The refcount
> of resources should probably stay separate from the refcount of module usage. We
> can add resource refcounting as well but I don't see a need for it at the moment.
> 
> vme_bridge_get() and vme_bridge_put() in this case should refer to the reference
> count of the bridge module (not the resources). What do you think?
> 

I think that by refcounting the resources being used we will know whether a
bridge module is being used or not, thus whether it can be unloaded or not. By
reference counting the use of resources we minimise the chance of poorly
written drivers using resources, but not registering the fact that they are in
fact using a VME bridge.

Martyn

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

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

[Emilio G. Cota]

On Fri, Aug 05, 2011 at 10:24:55 +0100, Martyn Welch wrote:
> I think that by refcounting the resources being used we
> will know whether a bridge module is being used or not,
> thus whether it can be unloaded or not.

But the granularity is wrong; if you want to know whether the
bridge is being used, just keep track of the devices *which
want to make known* that they're under the bridge.

> By reference counting the use of resources we minimise the
> chance of poorly written drivers using resources, but not
> registering the fact that they are in fact using a VME bridge.

A driver leaking a resource will then leave a bogus refcount
on the bus driver--a clear case of self-inflicted pain.

The argument of "poorly written drivers" does not apply;
I would expect all the merged drivers to be "good quality"
only, that's why we want to be upstream and why we put effort
in reviewing. Your point is well-intentioned, but in
practice we'd be shooting ourselves in the foot, potentially
ending up with an unremovable vme bridge module--which is
worse than a driver leaking a resource.

Refcounting must be kept simple & stupid; doing it behind the
backs of the drivers we're trying to protect is a mistake.

		Emilio

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

[Martyn Welch]

On 05/08/11 18:47, Emilio G. Cota wrote:
> On Fri, Aug 05, 2011 at 10:24:55 +0100, Martyn Welch wrote:
>> I think that by refcounting the resources being used we
>> will know whether a bridge module is being used or not,
>> thus whether it can be unloaded or not.
> 
> But the granularity is wrong; if you want to know whether the
> bridge is being used, just keep track of the devices *which
> want to make known* that they're under the bridge.
> 

I disagree, it shouldn't be up to the driver to determine whether it's use of
the bus should be known by the system. The system should know when the bus is
being used.

>> By reference counting the use of resources we minimise the
>> chance of poorly written drivers using resources, but not
>> registering the fact that they are in fact using a VME bridge.
> 
> A driver leaking a resource will then leave a bogus refcount
> on the bus driver--a clear case of self-inflicted pain.
> 

As opposed to adding a function that *only* refcounts and therefore requires
every driver to make extra explicit function call just to keep the refcounts
up-to-date.

> The argument of "poorly written drivers" does not apply;
> I would expect all the merged drivers to be "good quality"
> only, that's why we want to be upstream and why we put effort
> in reviewing. Your point is well-intentioned, but in
> practice we'd be shooting ourselves in the foot, potentially
> ending up with an unremovable vme bridge module--which is
> worse than a driver leaking a resource.
> 

Which wouldn't happen because all the upstreamed drivers are "good quality". I
might add that failing to free a resource will mean that it won't be
re-allocated, so having visibility of the number of resources having been
allocated would be advantageous.

> Refcounting must be kept simple & stupid; doing it behind the
> backs of the drivers we're trying to protect is a mistake.
> 

I just simply disagree. Forcing each driver to specifically maintain the
refcount is just stupid when an alternative is possible.

Martyn

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

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

[Emilio G. Cota]

On Mon, Aug 08, 2011 at 09:01:46 +0100, Martyn Welch wrote:
> On 05/08/11 18:47, Emilio G. Cota wrote:
> > A driver leaking a resource will then leave a bogus refcount
> > Refcounting must be kept simple & stupid; doing it behind the
> > backs of the drivers we're trying to protect is a mistake.
> 
> I just simply disagree. Forcing each driver to specifically maintain the
> refcount is just stupid when an alternative is possible.

Martyn, no one in the kernel is doing what you propose, for
good reason. Look at USB, PCI, RapidIO.  They all provide get
and put functions to be called from probe and release.

Doing otherwise is a bug. If a driver needs a resource *not
necessarily at .probe time*, it increments the refcount
then (in .probe) to make sure that the parent driver won't
be removed. IOW if you increase the refcount in the bridge
driver only when a resource is requested you're doomed,
because when the non-probe request arrives, the bridge driver
may have been removed already.. And the kernel gently reminds
you of this fact in the form of an oops.

There is a technical reason why we want drivers to explicitly
manage refcounts in the device hierarchy. It's the sane thing
to do.

		Emilio

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

[Martyn Welch]

On 08/08/11 10:14, Emilio G. Cota wrote:
> On Mon, Aug 08, 2011 at 09:01:46 +0100, Martyn Welch wrote:
>> On 05/08/11 18:47, Emilio G. Cota wrote:
>>> A driver leaking a resource will then leave a bogus refcount
>>> Refcounting must be kept simple & stupid; doing it behind the
>>> backs of the drivers we're trying to protect is a mistake.
>>
>> I just simply disagree. Forcing each driver to specifically maintain the
>> refcount is just stupid when an alternative is possible.
> 
> Martyn, no one in the kernel is doing what you propose, for
> good reason. Look at USB, PCI, RapidIO.  They all provide get
> and put functions to be called from probe and release.
> 

Really, which functions are these for PCI?

> Doing otherwise is a bug. If a driver needs a resource *not
> necessarily at .probe time*, it increments the refcount
> then (in .probe) to make sure that the parent driver won't
> be removed. IOW if you increase the refcount in the bridge
> driver only when a resource is requested you're doomed,
> because when the non-probe request arrives, the bridge driver
> may have been removed already.. And the kernel gently reminds
> you of this fact in the form of an oops.
> 

The probe is going to need to request the resources to access the hardware it
is designed to control. Hence it will be incrementing the refcount exactly
when it needs to.

Martyn

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

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

[Martyn Welch]

On 08/08/11 11:11, Emilio G. Cota wrote:
> On Mon, Aug 08, 2011 at 10:26:50 +0100, Martyn Welch wrote:
>> On 08/08/11 10:14, Emilio G. Cota wrote:
>>> Martyn, no one in the kernel is doing what you propose, for
>>> good reason. Look at USB, PCI, RapidIO.  They all provide get
>>> and put functions to be called from probe and release.
>>
>> Really, which functions are these for PCI?
> 
> pci_get_dev/put in drivers/pci/pci-driver.c.
> 

Which isn't explicitly used by the vast majority of PCI drivers. In fact the
instances which I did find where this was used by a PCI device driver, it
appears to be using the old-style PCI probing.

This is taken care of automatically for drivers using the "newer" PCI driver
registration model as part of the probe.

>>> Doing otherwise is a bug. If a driver needs a resource *not
>>> necessarily at .probe time*, it increments the refcount
>>> then (in .probe) to make sure that the parent driver won't
>>> be removed. IOW if you increase the refcount in the bridge
>>> driver only when a resource is requested you're doomed,
>>> because when the non-probe request arrives, the bridge driver
>>> may have been removed already.. And the kernel gently reminds
>>> you of this fact in the form of an oops.
>>
>> The probe is going to need to request the resources to access the hardware it
>> is designed to control. Hence it will be incrementing the refcount exactly
>> when it needs to.
> 
> We cannot presume (impose) what all VME drivers will do; that's
> up to them. Imagine for instance I want to write a driver that
> allows me to operate on the location monitors, all orchestrated
> via sysfs.  Probe would be almost empty, resources would come
> and go at a later time.

You'd need to request the location monitor to use it. If you didn't request it
at probe time, then removed the bridge, when you went to request the resource
at run time it would fail. It would be up to your driver to deal with that
gracefully.

> If the bridge driver is removed when
> I haven't registered any location monitors, I'll get an oops
> when I try to register a block of them. Not pretty.

No, your driver will be told the resource isn't available by vme_lm_request(),
it would return null. It would then be up to you as the driver writer to
handle that gracefully. In fact, just as you'd have to do if the location
monitor was already being used by a different VME device driver which had
positioned them where it needed them.

> Remember that we're writing a bus driver here, we shouldn't
> get on the way of the drivers for the devices on the bus, no
> matter how crazy they are.

Actually we're providing resource management and a bridge independent VME API
for VME device drivers as part of a bus specific core, much as USB and PCI do.
The VME bridge drivers bind under it, the VME device drivers bind on top of it.

Crazy probably equates to not portable across different VME bridges, so no I
don't agree. I'm in favour of providing as much flexibility to VME device
driver authors as possible, without impacting the flexibility of the user to
use the drivers on top of different VME bridges.

> IMHO in order to make sure we're on
> the right track we must look at other bus drivers. And these
> other drivers just keep things simple and stupid, which is
> flexible, safe and "Obviously Correct(tm)".

Though I don't think the approach you are advocating is simple for the VME
device driver writer (new style PCI drivers don't as a rule directly manage
refcounting, in the same way your approach would add complexity for the
individual VME device drivers); it's not safe (I deem it unsafe to expect
every VME device driver writer to manage the refcounting in their drivers
explicitly, just as it appears is the case for PCI devices) and as a result
not "Obviously Correct(tm)".

Martyn

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

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

[Emilio G. Cota]

On Mon, Aug 08, 2011 at 12:06:37 +0100, Martyn Welch wrote:
> On 08/08/11 11:11, Emilio G. Cota wrote:
> > On Mon, Aug 08, 2011 at 10:26:50 +0100, Martyn Welch wrote:
> >> On 08/08/11 10:14, Emilio G. Cota wrote:
> >>> Martyn, no one in the kernel is doing what you propose, for
> >>> good reason. Look at USB, PCI, RapidIO.  They all provide get
> >>> and put functions to be called from probe and release.
> >>
> >> Really, which functions are these for PCI?
> > 
> > pci_get_dev/put in drivers/pci/pci-driver.c.
> > 
> 
> Which isn't explicitly used by the vast majority of PCI drivers. In fact the
> instances which I did find where this was used by a PCI device driver, it
> appears to be using the old-style PCI probing.

> This is taken care of automatically for drivers using the "newer" PCI driver
> registration model as part of the probe.

And I don't care. The new model is not doing that nor what you suggest
anyway, AFAIK it's not incrementing any refcounts for devices--perhaps
because it cannot be built as a module? I dunno.

Look at other subsystems that usually sit under PCI, like USB
or RapidIO.

> No, your driver will be told the resource isn't available by vme_lm_request(),
> it would return null. It would then be up to you as the driver writer to
> handle that gracefully. In fact, just as you'd have to do if the location
> monitor was already being used by a different VME device driver which had
> positioned them where it needed them.

Ok, we may not oops, but this is unnecessary pain for the driver--and
probably not what the driver wanted.

> > Remember that we're writing a bus driver here, we shouldn't
> > get on the way of the drivers for the devices on the bus, no
> > matter how crazy they are.
> 
> Actually we're providing resource management and a bridge independent VME API
> for VME device drivers as part of a bus specific core, much as USB and PCI do.
> The VME bridge drivers bind under it, the VME device drivers bind on top of it.
> 
> Crazy probably equates to not portable across different VME bridges

?? Crazy means crazy. ftrace is crazy, linux-rt is crazy. And
they're great.

> , so no I don't agree. I'm in favour of providing as much flexibility to VME device
> driver authors as possible, without impacting the flexibility of the user to
> use the drivers on top of different VME bridges.

This is nonsense.

> > IMHO in order to make sure we're on
> > the right track we must look at other bus drivers. And these
> > other drivers just keep things simple and stupid, which is
> > flexible, safe and "Obviously Correct(tm)".
> 
> Though I don't think the approach you are advocating is simple for the VME
> device driver writer (new style PCI drivers don't as a rule directly manage
> refcounting, in the same way your approach would add complexity for the
> individual VME device drivers); it's not safe (I deem it unsafe to expect
> every VME device driver writer to manage the refcounting in their drivers
> explicitly, just as it appears is the case for PCI devices) and as a result
> not "Obviously Correct(tm)".

By your definition there are lots of drivers in the kernel
that are unsafe..

I'm tired of your non-arguments. I'm just trying to persuade you
to do what everyone else is doing, with technical reasons. To me
(and to everybody else in this list, I'd imagine) refcounting
should be explicit. You're going against what I perceive are
well-established pratices in the kernel. I can't understand it.

		Emilio

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

[Greg KH]

On Mon, Aug 08, 2011 at 01:22:16PM -0400, Emilio G. Cota wrote:
> On Mon, Aug 08, 2011 at 12:06:37 +0100, Martyn Welch wrote:
> > On 08/08/11 11:11, Emilio G. Cota wrote:
> > > On Mon, Aug 08, 2011 at 10:26:50 +0100, Martyn Welch wrote:
> > >> On 08/08/11 10:14, Emilio G. Cota wrote:
> > >>> Martyn, no one in the kernel is doing what you propose, for
> > >>> good reason. Look at USB, PCI, RapidIO.  They all provide get
> > >>> and put functions to be called from probe and release.
> > >>
> > >> Really, which functions are these for PCI?
> > > 
> > > pci_get_dev/put in drivers/pci/pci-driver.c.
> > > 
> > 
> > Which isn't explicitly used by the vast majority of PCI drivers. In fact the
> > instances which I did find where this was used by a PCI device driver, it
> > appears to be using the old-style PCI probing.
> 
> > This is taken care of automatically for drivers using the "newer" PCI driver
> > registration model as part of the probe.

All PCI drivers are using that model, it is not "new" at all (5+ years
old now.)

The fact is, if you grab a pointer to a device, and save it somewhere,
you HAVE TO grab a reference to that object at the same time.

To not do so is a bug, and must be fixed.

It's that simple.

greg k-h

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

[Martyn Welch]

On 08/08/11 18:22, Emilio G. Cota wrote:
> On Mon, Aug 08, 2011 at 12:06:37 +0100, Martyn Welch wrote:
>> On 08/08/11 11:11, Emilio G. Cota wrote:
>>> On Mon, Aug 08, 2011 at 10:26:50 +0100, Martyn Welch wrote:
>>>> On 08/08/11 10:14, Emilio G. Cota wrote:
>>>>> Martyn, no one in the kernel is doing what you propose, for
>>>>> good reason. Look at USB, PCI, RapidIO.  They all provide get
>>>>> and put functions to be called from probe and release.
>>>>
>>>> Really, which functions are these for PCI?
>>>
>>> pci_get_dev/put in drivers/pci/pci-driver.c.
>>>
>>
>> Which isn't explicitly used by the vast majority of PCI drivers. In fact the
>> instances which I did find where this was used by a PCI device driver, it
>> appears to be using the old-style PCI probing.
> 
>> This is taken care of automatically for drivers using the "newer" PCI driver
>> registration model as part of the probe.
> 
> And I don't care. The new model is not doing that nor what you suggest
> anyway, AFAIK it's not incrementing any refcounts for devices--perhaps
> because it cannot be built as a module? I dunno.
> 
> Look at other subsystems that usually sit under PCI, like USB
> or RapidIO.
> 

Just did.

RapidIO, defines rio_dev_put() in rio-driver.c, used in the rio_device_probe
function, so handled much like it is in PCI. Can't see any explicit
refcounting in the rionet driver.

USB, interestingly here the functions are defined as usb_get_dev() and
usb_put_dev() (not usb_dev_get() and usb_dev_put()). Here the situation is a
little different, the refcounting is more explicit, I assume because of the
hotplug and/or hierarchical nature of the USB bus (maybe Greg would be kind
enough to explain the rationale?)

So, out of the 3 bus types you have used to demonstrate that refcounts should
always be handled explicitly, 2 of the 3 (at least to me) appear to implicitly
handling refcounting.

>> No, your driver will be told the resource isn't available by vme_lm_request(),
>> it would return null. It would then be up to you as the driver writer to
>> handle that gracefully. In fact, just as you'd have to do if the location
>> monitor was already being used by a different VME device driver which had
>> positioned them where it needed them.
> 
> Ok, we may not oops, but this is unnecessary pain for the driver--and
> probably not what the driver wanted.
> 

The location monitor is a single, (location) configurable resource. If it's
being used for one purpose in one location by a driver, it can't be
repositioned and used for another purpose by another driver without adversely
affecting the operation of the first.

>>> Remember that we're writing a bus driver here, we shouldn't
>>> get on the way of the drivers for the devices on the bus, no
>>> matter how crazy they are.
>>
>> Actually we're providing resource management and a bridge independent VME API
>> for VME device drivers as part of a bus specific core, much as USB and PCI do.
>> The VME bridge drivers bind under it, the VME device drivers bind on top of it.
>>
>> Crazy probably equates to not portable across different VME bridges
> 
> ?? Crazy means crazy. ftrace is crazy, linux-rt is crazy. And
> they're great.
> 

I'm sure they are great, not sure I'd describe them as crazy though.

>> , so no I don't agree. I'm in favour of providing as much flexibility to VME device
>> driver authors as possible, without impacting the flexibility of the user to
>> use the drivers on top of different VME bridges.
> 
> This is nonsense.
> 
>>> IMHO in order to make sure we're on
>>> the right track we must look at other bus drivers. And these
>>> other drivers just keep things simple and stupid, which is
>>> flexible, safe and "Obviously Correct(tm)".
>>
>> Though I don't think the approach you are advocating is simple for the VME
>> device driver writer (new style PCI drivers don't as a rule directly manage
>> refcounting, in the same way your approach would add complexity for the
>> individual VME device drivers); it's not safe (I deem it unsafe to expect
>> every VME device driver writer to manage the refcounting in their drivers
>> explicitly, just as it appears is the case for PCI devices) and as a result
>> not "Obviously Correct(tm)".
> 
> By your definition there are lots of drivers in the kernel
> that are unsafe..
> 

It appears to me that both PCI and RapidIO (both buses that you have tried to
use to defend your position) don't seem to me to by-and-large expect drivers
to explicitly manage refcounting. That leaves USB, which I'd argue is a very
different bus to VME.

> I'm tired of your non-arguments. I'm just trying to persuade you
> to do what everyone else is doing, with technical reasons. To me
> (and to everybody else in this list, I'd imagine) refcounting
> should be explicit. You're going against what I perceive are
> well-established pratices in the kernel. I can't understand it.

Which I have yet to see you convincingly backup. I see a large amount of bluff
about oppses and how buses apparently deal with refcounting. I also know I
have had a number of commits to the VME code by a number of others (which is a
matter of public record, you can look at the commits in git) that haven't
complained about how the bus code is structured.

You have proposed a completely different structure. Manohar has proposed some
changes and I am trying to work with him to find a solution that satisfies
both of us. I'm not going to make any claims about others views, simply
because as they haven't aired them, I don't currently actually know what they are.

Martyn

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

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

[Emilio G. Cota]

On Tue, Aug 09, 2011 at 10:00:09 +0100, Martyn Welch wrote:
> So, out of the 3 bus types you have used to demonstrate that refcounts should
> always be handled explicitly, 2 of the 3 (at least to me) appear to implicitly
> handling refcounting.
(snip)
> It appears to me that both PCI and RapidIO (both buses that you have tried to
> use to defend your position) don't seem to me to by-and-large expect drivers
> to explicitly manage refcounting.

Well if that's the impression you get, perhaps you need to
actually read the code:

In drivers/rapidio/rio-driver.c:

/**
 * rio_dev_get - Increments the reference count of the RIO device structure
 *
 * @rdev: RIO device being referenced
 *
 * Each live reference to a device should be refcounted.
 *
 * Drivers for RIO devices should normally record such references in
 * their probe() methods, when they bind to a device, and release
 * them by calling rio_dev_put(), in their disconnect() methods.
 */
struct rio_dev *rio_dev_get(struct rio_dev *rdev)
{
        if (rdev)
                get_device(&rdev->dev);

        return rdev;
}

You can find a similar function (with almost the very same description)
for usb and pci.

> That leaves USB, which I'd argue is a very different bus to VME.

>From the device model's viewpoint, VME is not special in any way,
something you don't seem to understand.

> > I'm tired of your non-arguments. I'm just trying to persuade you
> > to do what everyone else is doing, with technical reasons. To me
> > (and to everybody else in this list, I'd imagine) refcounting
> > should be explicit. You're going against what I perceive are
> > well-established pratices in the kernel. I can't understand it.
> 
> Which I have yet to see you convincingly backup. I see a
> large amount of bluff about oppses and how buses apparently
> deal with refcounting.  I also know I have had a number of
> commits to the VME code by a number of others (which is a
> matter of public record, you can look at the commits in git)
> that haven't complained about how the bus code is structured.

You're in denial. Manohar's patch fixes a very obvious bug,
which is fixed in the same way other buses deal with this.

> I also know I have had a number of commits to the VME code
> by a number of others (which is a matter of public record,
> you can look at the commits in git) that haven't complained
> about how the bus code is structured.

The fact that other people didn't notice this bug is irrelevant.

> You have proposed a completely different structure. Manohar has proposed some
> changes and I am trying to work with him to find a solution that satisfies
> both of us.

??? I just want you to ack Manohar's patch.

		Emilio

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

[Martyn Welch]

On 09/08/11 20:19, Emilio G. Cota wrote:
> On Tue, Aug 09, 2011 at 10:00:09 +0100, Martyn Welch wrote:
>> So, out of the 3 bus types you have used to demonstrate that refcounts should
>> always be handled explicitly, 2 of the 3 (at least to me) appear to implicitly
>> handling refcounting.
> (snip)
>> It appears to me that both PCI and RapidIO (both buses that you have tried to
>> use to defend your position) don't seem to me to by-and-large expect drivers
>> to explicitly manage refcounting.
> 
> Well if that's the impression you get, perhaps you need to
> actually read the code:
> 
> In drivers/rapidio/rio-driver.c:
> 
> /**
>  * rio_dev_get - Increments the reference count of the RIO device structure
>  *
>  * @rdev: RIO device being referenced
>  *
>  * Each live reference to a device should be refcounted.
>  *
>  * Drivers for RIO devices should normally record such references in
>  * their probe() methods, when they bind to a device, and release
>  * them by calling rio_dev_put(), in their disconnect() methods.
>  */
> struct rio_dev *rio_dev_get(struct rio_dev *rdev)
> {
>         if (rdev)
>                 get_device(&rdev->dev);
> 
>         return rdev;
> }
> 
> You can find a similar function (with almost the very same description)
> for usb and pci.
> 

And I think you need to go and do a grep of the code and find out where those
functions are actually used, rather than blindly relying on the comment.

>> That leaves USB, which I'd argue is a very different bus to VME.
> 
> From the device model's viewpoint, VME is not special in any way,
> something you don't seem to understand.
> 
>>> I'm tired of your non-arguments. I'm just trying to persuade you
>>> to do what everyone else is doing, with technical reasons. To me
>>> (and to everybody else in this list, I'd imagine) refcounting
>>> should be explicit. You're going against what I perceive are
>>> well-established pratices in the kernel. I can't understand it.
>>
>> Which I have yet to see you convincingly backup. I see a
>> large amount of bluff about oppses and how buses apparently
>> deal with refcounting.  I also know I have had a number of
>> commits to the VME code by a number of others (which is a
>> matter of public record, you can look at the commits in git)
>> that haven't complained about how the bus code is structured.
> 
> You're in denial. Manohar's patch fixes a very obvious bug,
> which is fixed in the same way other buses deal with this.
> 

Go grep the code.

>> I also know I have had a number of commits to the VME code
>> by a number of others (which is a matter of public record,
>> you can look at the commits in git) that haven't complained
>> about how the bus code is structured.
> 
> The fact that other people didn't notice this bug is irrelevant.
> 

Suitable bug fixes are welcome.

>> You have proposed a completely different structure. Manohar has proposed some
>> changes and I am trying to work with him to find a solution that satisfies
>> both of us.
> 
> ??? I just want you to ack Manohar's patch.
> 

I won't be in it's current form. I'm looking forward to seeing Manohar's
revised patch series.

Martyn

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

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

[Emilio G. Cota]

On Wed, Aug 10, 2011 at 08:39:07 +0100, Martyn Welch wrote:
> And I think you need to go and do a grep of the code and find out where those
> functions are actually used, rather than blindly relying on the comment.
(snip)
> Go grep the code.

/me greps once again..

RapidIO: there are no rapidIO drivers upstream, only switches
and rionet, which does not manage RapidIO devices (it just sends
Ethernet packets on top of RapidIO's messaging).  So obviously
there aren't any callers.

PCI: pci_dev_get() referenced in 60 files.  Another way of
explicitly incrementing the refcount of a pci device is with
pci_get_device(), which searches in the device list for a
particular one by its vendor/device ID.  This function is
referenced in 127 files.

USB: usb_get_dev() referenced in 75 files.

There are also lots of direct calls to get_device() from .probe
methods of devices not tied to a particular bus.

Guess that was enough grepping.

> Suitable bug fixes are welcome.

I sent a fix (as part of an admittedly large patchset) in
Nov 2010[1], ie 9 months ago, you were sick at the time and
told me you'd have a look at the changes later[2], which
unfortunately never happened, even after pinging you off-list.

Anyway let's forget that, Manohar's patches are what matters now.

		Emilio

[1] http://thread.gmane.org/gmane.linux.kernel/1054034
[2] http://thread.gmane.org/gmane.linux.kernel/1054034/focus=9819

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

[Martyn Welch]

On 10/08/11 10:15, Emilio G. Cota wrote:
> On Wed, Aug 10, 2011 at 08:39:07 +0100, Martyn Welch wrote:
>> And I think you need to go and do a grep of the code and find out where those
>> functions are actually used, rather than blindly relying on the comment.
> (snip)
>> Go grep the code.
> 
> /me greps once again..
> 
> RapidIO: there are no rapidIO drivers upstream, only switches
> and rionet, which does not manage RapidIO devices (it just sends
> Ethernet packets on top of RapidIO's messaging).  So obviously
> there aren't any callers.
> 

Yes there are. In rio-driver.c for a start:

/**
 *  rio_device_probe - Tell if a RIO device structure has a matching RIO
device id structure
 *  @dev: the RIO device structure to match against
 *
 * return 0 and set rio_dev->driver when drv claims rio_dev, else error
 */
static int rio_device_probe(struct device *dev)
{
        struct rio_driver *rdrv = to_rio_driver(dev->driver);
        struct rio_dev *rdev = to_rio_dev(dev);
        int error = -ENODEV;
        const struct rio_device_id *id;

        if (!rdev->driver && rdrv->probe) {
                if (!rdrv->id_table)
                        return error;
                id = rio_match_device(rdrv->id_table, rdev);
                rio_dev_get(rdev);
                if (id)
                        error = rdrv->probe(rdev, id);
                if (error >= 0) {
                        rdev->driver = rdrv;
                        error = 0;
                } else
                        rio_dev_put(rdev);
        }
        return error;
}

Doing what Manohar suggested and I have agreed with.

> PCI: pci_dev_get() referenced in 60 files.  Another way of
> explicitly incrementing the refcount of a pci device is with
> pci_get_device(), which searches in the device list for a
> particular one by its vendor/device ID.  This function is
> referenced in 127 files.
> 

Again, in pci-driver.c:

static int pci_device_probe(struct device * dev)
{
        int error = 0;
        struct pci_driver *drv;
        struct pci_dev *pci_dev;

        drv = to_pci_driver(dev->driver);
        pci_dev = to_pci_dev(dev);
        pci_dev_get(pci_dev);
        error = __pci_device_probe(drv, pci_dev);
        if (error)
                pci_dev_put(pci_dev);

        return error;
}

Again, doing as Manohar suggested.


For those drivers using pci_get_device() - P313 of the Linux Device Drivers
3rd Ed, under "Old-style PCI Probing". Not a mechanism currently supported for
the VME bus.

> USB: usb_get_dev() referenced in 75 files.
> 

I've already explained that I don't think the USB bus is good for comparison
due to very real differences in bus topology and use.

> There are also lots of direct calls to get_device() from .probe
> methods of devices not tied to a particular bus.
> 

Which may therefore have nothing to do with a bus.

> Guess that was enough grepping.
> 

It seems not quite.

>> Suitable bug fixes are welcome.
> 
> I sent a fix (as part of an admittedly large patchset) in
> Nov 2010[1], ie 9 months ago, you were sick at the time and
> told me you'd have a look at the changes later[2], which
> unfortunately never happened, even after pinging you off-list.
> 

Some of those patches were applied:

http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/gregkh/staging-2.6.git;a=history;f=drivers/staging/vme;hb=HEAD

The remaining patches completely changed the VME bus model. I said at the time
I wasn't overly happy with the model you had put forward. Take this as my review:

- I am not happy with the proposed alternative model.

Sorry for not getting back to you sooner. I'm afraid my TODO list sometimes
ends up acting as a stack rather than a FIFO.

> Anyway let's forget that, Manohar's patches are what matters now.
> 

Will do.

Martyn

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

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

[Emilio G. Cota]

On Wed, Aug 10, 2011 at 10:50:59 +0100, Martyn Welch wrote:
> Again, in pci-driver.c:
> 
> static int pci_device_probe(struct device * dev)
> {
>         int error = 0;
>         struct pci_driver *drv;
>         struct pci_dev *pci_dev;
> 
>         drv = to_pci_driver(dev->driver);
>         pci_dev = to_pci_dev(dev);
>         pci_dev_get(pci_dev);
>         error = __pci_device_probe(drv, pci_dev);
>         if (error)
>                 pci_dev_put(pci_dev);
> 
>         return error;
> }
> 
> Again, doing as Manohar suggested.

Well in fact I just realised our discussion has been completely
misdirected.

I read the original patch again, and it does something different
to what these _get_dev functions are doing. The patch increments
the refcount of the _bridge_; the get_dev() calls increment the
functions of the _device_ itself, not the bridge, which wouldn't
protect the device against the bridge being removed.

The closest thing I saw in the kernel was usb_get_intf(),
which is what I had in mind when I wrote the original patchset
back in the day:

http://lxr.free-electrons.com/ident?i=usb_get_intf

Sorry for overlooking this (and the subsequent noise), it's
been a while since then.

		Emilio

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

[Manohar Vanga]

Hey Martyn,

> I think that by refcounting the resources being used we will know whether a
> bridge module is being used or not, thus whether it can be unloaded or not. By
> reference counting the use of resources we minimise the chance of poorly
> written drivers using resources, but not registering the fact that they are in
> fact using a VME bridge.

In the end, this discussion is about what we want the bride module reference count
to represent. I see your point here that it would be really useful to know what
resources have been allocated. I am just wondering whether the module refcount is
a good place to give information on allocated resources (rather than the bridge
module refcount).

I am not really an expert in these matters but would something like a sysfs file
be a cleaner approach to providing information on allocated resources within the
driver?

With this approach, I am also thinking about cases where resources are not allocated
within the probe call. This can cause issues if the bridge module is removed after
a successful probe but before the resources are allocated. This would be a direct
bug :-/

If we really don't want explicit module refcounting by drivers, can we perhaps use
the return value of the probe to automatically do this? eg. in vme_bus_probe() like
below:

    int ret = 0;
    ...
    vme_bridge_get(bridge);
    if (driver->probe)
            ret = driver->probe(vdev);
    if (ret)
        vme_bridge_put(bridge);
    return ret;

Just a thought. Feel free to shoot it down if you think it's the incorrect
approach :P

Thanks!

--
/manohar

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

[Martyn Welch]

On 09/08/11 14:24, Manohar Vanga wrote:
> Hey Martyn,
> 
>> I think that by refcounting the resources being used we will know whether a
>> bridge module is being used or not, thus whether it can be unloaded or not. By
>> reference counting the use of resources we minimise the chance of poorly
>> written drivers using resources, but not registering the fact that they are in
>> fact using a VME bridge.
> 
> In the end, this discussion is about what we want the bride module reference count
> to represent. I see your point here that it would be really useful to know what
> resources have been allocated. I am just wondering whether the module refcount is
> a good place to give information on allocated resources (rather than the bridge
> module refcount).
> 
> I am not really an expert in these matters but would something like a sysfs file
> be a cleaner approach to providing information on allocated resources within the
> driver?
> 

That would probably be a better idea.

> With this approach, I am also thinking about cases where resources are not allocated
> within the probe call. This can cause issues if the bridge module is removed after
> a successful probe but before the resources are allocated. This would be a direct
> bug :-/
> 
> If we really don't want explicit module refcounting by drivers, can we perhaps use
> the return value of the probe to automatically do this? eg. in vme_bus_probe() like
> below:
> 
>     int ret = 0;
>     ...
>     vme_bridge_get(bridge);
>     if (driver->probe)
>             ret = driver->probe(vdev);
>     if (ret)
>         vme_bridge_put(bridge);
>     return ret;
> 
> Just a thought. Feel free to shoot it down if you think it's the incorrect
> approach :P
> 

After looking at the PCI and RapidIO subsystems, I think this is probably the
correct approach. I guess the only quiestion then is at which point is
vme_bridge_put() called assuming the probe is successful. I guess at module
unload time, though I haven't checked in the PCI and RapidIO code.

(Thank you for your patience)

Martyn

> Thanks!
> 
> --
> /manohar


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

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

[Manohar Vanga]

Hey Martyn,

> > I am not really an expert in these matters but would something like a sysfs file
> > be a cleaner approach to providing information on allocated resources within the
> > driver?
> 
> That would probably be a better idea.

Great! We'll add a sysfs file for this :)

> >     int ret = 0;
> >     ...
> >     vme_bridge_get(bridge);
> >     if (driver->probe)
> >             ret = driver->probe(vdev);
> >     if (ret)
> >         vme_bridge_put(bridge);
> >     return ret;
> 
> After looking at the PCI and RapidIO subsystems, I think this is probably the
> correct approach. I guess the only quiestion then is at which point is
> vme_bridge_put() called assuming the probe is successful. I guess at module
> unload time, though I haven't checked in the PCI and RapidIO code.

vme_bus_remove()? If it's incorrect, we can always change this later.

If we are agreed on this, let me know and I will make the changes and
resend the patches today.

> (Thank you for your patience)

(no problem)!

Thanks!

--
/manohar

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

[Martyn Welch]

On 09/08/11 15:35, Manohar Vanga wrote:
> Hey Martyn,
> 
>>> I am not really an expert in these matters but would something like a sysfs file
>>> be a cleaner approach to providing information on allocated resources within the
>>> driver?
>>
>> That would probably be a better idea.
> 
> Great! We'll add a sysfs file for this :)
> 

I'm thinking either one for each resource type (slave, master, lm, dma) would
be great. Though it might also be nice to be able to track how each resource
is being used (though I guess that would be quite a bit of work).

>>>     int ret = 0;
>>>     ...
>>>     vme_bridge_get(bridge);
>>>     if (driver->probe)
>>>             ret = driver->probe(vdev);
>>>     if (ret)
>>>         vme_bridge_put(bridge);
>>>     return ret;
>>
>> After looking at the PCI and RapidIO subsystems, I think this is probably the
>> correct approach. I guess the only quiestion then is at which point is
>> vme_bridge_put() called assuming the probe is successful. I guess at module
>> unload time, though I haven't checked in the PCI and RapidIO code.
> 
> vme_bus_remove()? If it's incorrect, we can always change this later.
> 

Yes, that seems to follow how it's done in RapidIO.

> If we are agreed on this, let me know and I will make the changes and
> resend the patches today.
> 

RapidIO has rio_dev_get and rio_dev_put functions that are called in
rio_device_probe and rio_device_remove. This is equivalent to vme_bus_probe
and vme_bus_remove in the VME code. That seems to be a very appropriate
solution and I'd be happy with that.

Martyn

>> (Thank you for your patience)
> 
> (no problem)!
> 
> Thanks!
> 
> --
> /manohar


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

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

[Emilio G. Cota]

On Tue, Aug 09, 2011 at 15:26:09 +0100, Martyn Welch wrote:
> On 09/08/11 14:24, Manohar Vanga wrote:
> > If we really don't want explicit module refcounting by drivers, can we perhaps use
> > the return value of the probe to automatically do this? eg. in vme_bus_probe() like
> > below:
> > 
> >     int ret = 0;
> >     ...
> >     vme_bridge_get(bridge);
> >     if (driver->probe)
> >             ret = driver->probe(vdev);
> >     if (ret)
> >         vme_bridge_put(bridge);
> >     return ret;
> > 
> > Just a thought. Feel free to shoot it down if you think it's the incorrect
> > approach :P
> > 
> 
> After looking at the PCI and RapidIO subsystems, I think this is probably the
> correct approach. I guess the only quiestion then is at which point is
> vme_bridge_put() called assuming the probe is successful. I guess at module
> unload time, though I haven't checked in the PCI and RapidIO code.

This works as long as all devices are tied to the driver
loading; i.e. when the devices can only be removed when the
driver is removed.  A device might go away while the driver
still manages other devices of the same kin; this is why
particular drivers explicitly manage the get/put in
.probe/.release.

In vme we may one day have hotplug (VME64x supports it IIRC),
but we're not quite there yet. One can remove devices from
sysfs anytime though.

I still think the original patch is the sane way to go.

		Emilio

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

[Manohar Vanga]

Hey Emilio,

> This works as long as all devices are tied to the driver
> loading; i.e. when the devices can only be removed when the
> driver is removed.  A device might go away while the driver
> still manages other devices of the same kin; this is why
> particular drivers explicitly manage the get/put in
> .probe/.release.

I have still left the exports for the refcounting functions
intact so border case drivers are free to use these explicitly
(albeit on top of the automatic refcounting in .probe and
.remove).

> In vme we may one day have hotplug (VME64x supports it IIRC),
> but we're not quite there yet. One can remove devices from
> sysfs anytime though.

Won't this event trigger the .remove function? If it does, it
shouldn't be a problem.

> I still think the original patch is the sane way to go.

I think this approach is a good middle ground between both
arguments. If we really run into major issues with this
(which I somewhat doubt will happen), we can always fix
things up. It's not immutable!

Thanks!

--
/manohar

[PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Manohar Vanga]

Functions following the naming format of *_get and *_put are used
for reference counting. Rename the slot_get functions to get_slot
to prevent such confusion in the meaning.

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/bridges/vme_ca91cx42.c |    8 ++++----
 drivers/staging/vme/bridges/vme_tsi148.c   |    6 +++---
 drivers/staging/vme/vme.c                  |   12 ++++++------
 drivers/staging/vme/vme.h                  |    2 +-
 drivers/staging/vme/vme_api.txt            |    2 +-
 drivers/staging/vme/vme_bridge.h           |    2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
index 2f9c22b..dff64e1 100644
--- a/drivers/staging/vme/bridges/vme_ca91cx42.c
+++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
@@ -1490,7 +1490,7 @@ static int ca91cx42_lm_detach(struct vme_lm_resource *lm, int monitor)
 	return 0;
 }
 
-static int ca91cx42_slot_get(struct vme_bridge *ca91cx42_bridge)
+static int ca91cx42_get_slot(struct vme_bridge *ca91cx42_bridge)
 {
 	u32 slot = 0;
 	struct ca91cx42_driver *bridge;
@@ -1551,7 +1551,7 @@ static int ca91cx42_crcsr_init(struct vme_bridge *ca91cx42_bridge,
 
 	bridge = ca91cx42_bridge->driver_priv;
 
-	slot = ca91cx42_slot_get(ca91cx42_bridge);
+	slot = ca91cx42_get_slot(ca91cx42_bridge);
 
 	/* Write CSR Base Address if slot ID is supplied as a module param */
 	if (geoid)
@@ -1799,7 +1799,7 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ca91cx42_bridge->lm_get = ca91cx42_lm_get;
 	ca91cx42_bridge->lm_attach = ca91cx42_lm_attach;
 	ca91cx42_bridge->lm_detach = ca91cx42_lm_detach;
-	ca91cx42_bridge->slot_get = ca91cx42_slot_get;
+	ca91cx42_bridge->get_slot = ca91cx42_get_slot;
 	ca91cx42_bridge->alloc_consistent = ca91cx42_alloc_consistent;
 	ca91cx42_bridge->free_consistent = ca91cx42_free_consistent;
 
@@ -1807,7 +1807,7 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
 		(data & CA91CX42_MISC_CTL_SYSCON) ? "" : " not");
 	dev_info(&pdev->dev, "Slot ID is %d\n",
-		ca91cx42_slot_get(ca91cx42_bridge));
+		ca91cx42_get_slot(ca91cx42_bridge));
 
 	if (ca91cx42_crcsr_init(ca91cx42_bridge, pdev))
 		dev_err(&pdev->dev, "CR/CSR configuration failed.\n");
diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
index 9db89dc..922902b 100644
--- a/drivers/staging/vme/bridges/vme_tsi148.c
+++ b/drivers/staging/vme/bridges/vme_tsi148.c
@@ -2106,7 +2106,7 @@ static int tsi148_lm_detach(struct vme_lm_resource *lm, int monitor)
 /*
  * Determine Geographical Addressing
  */
-static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
+static int tsi148_get_slot(struct vme_bridge *tsi148_bridge)
 {
 	u32 slot = 0;
 	struct tsi148_driver *bridge;
@@ -2191,7 +2191,7 @@ static int tsi148_crcsr_init(struct vme_bridge *tsi148_bridge,
 	cbar = ioread32be(bridge->base + TSI148_CBAR);
 	cbar = (cbar & TSI148_CRCSR_CBAR_M)>>3;
 
-	vstat = tsi148_slot_get(tsi148_bridge);
+	vstat = tsi148_get_slot(tsi148_bridge);
 
 	if (cbar != vstat) {
 		cbar = vstat;
@@ -2474,7 +2474,7 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	tsi148_bridge->lm_get = tsi148_lm_get;
 	tsi148_bridge->lm_attach = tsi148_lm_attach;
 	tsi148_bridge->lm_detach = tsi148_lm_detach;
-	tsi148_bridge->slot_get = tsi148_slot_get;
+	tsi148_bridge->get_slot = tsi148_get_slot;
 	tsi148_bridge->alloc_consistent = tsi148_alloc_consistent;
 	tsi148_bridge->free_consistent = tsi148_free_consistent;
 
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 759ec2b..81a33dc 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -1332,7 +1332,7 @@ void vme_lm_free(struct vme_resource *resource)
 }
 EXPORT_SYMBOL(vme_lm_free);
 
-int vme_slot_get(struct device *bus)
+int vme_get_slot(struct device *bus)
 {
 	struct vme_bridge *bridge;
 
@@ -1342,14 +1342,14 @@ int vme_slot_get(struct device *bus)
 		return -EINVAL;
 	}
 
-	if (bridge->slot_get == NULL) {
-		printk(KERN_WARNING "vme_slot_get not supported\n");
+	if (bridge->get_slot == NULL) {
+		printk(KERN_WARNING "vme_get_slot not supported\n");
 		return -EINVAL;
 	}
 
-	return bridge->slot_get(bridge);
+	return bridge->get_slot(bridge);
 }
-EXPORT_SYMBOL(vme_slot_get);
+EXPORT_SYMBOL(vme_get_slot);
 
 
 /* - Bridge Registration --------------------------------------------------- */
@@ -1549,7 +1549,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_get_slot(dev)))
 				return 1;
 		}
 		i++;
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 8fb35e2..bda6fdf 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -162,7 +162,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_get_slot(struct device *);
 
 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..e0152c5 100644
--- a/drivers/staging/vme/vme_api.txt
+++ b/drivers/staging/vme/vme_api.txt
@@ -380,4 +380,4 @@ Slot Detection
 
 This function returns the slot ID of the provided bridge.
 
-	int vme_slot_get(struct device *dev);
+	int vme_get_slot(struct device *dev);
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index ef751a4..5f70848 100644
--- a/drivers/staging/vme/vme_bridge.h
+++ b/drivers/staging/vme/vme_bridge.h
@@ -164,7 +164,7 @@ struct vme_bridge {
 	int (*lm_detach) (struct vme_lm_resource *, int);
 
 	/* CR/CSR space functions */
-	int (*slot_get) (struct vme_bridge *);
+	int (*get_slot) (struct vme_bridge *);
 
 	/* Bridge parent interface */
 	void *(*alloc_consistent)(struct device *dev, size_t size,
-- 
1.7.4.1

Re: [PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Martyn Welch]

On 01/08/11 11:20, Manohar Vanga wrote:
> Functions following the naming format of *_get and *_put are used
> for reference counting. Rename the slot_get functions to get_slot
> to prevent such confusion in the meaning.
> 

If your doing vme_slot_get, then there's also vme_lm_get, vme_master_get and
vme_slave_get.

Doing just this would then lead to more inconsistency in the naming and
wouldn't even get rid of all the functions using the *_get naming convention.

I'm not sure this change is worth it.

Martyn

> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/bridges/vme_ca91cx42.c |    8 ++++----
>  drivers/staging/vme/bridges/vme_tsi148.c   |    6 +++---
>  drivers/staging/vme/vme.c                  |   12 ++++++------
>  drivers/staging/vme/vme.h                  |    2 +-
>  drivers/staging/vme/vme_api.txt            |    2 +-
>  drivers/staging/vme/vme_bridge.h           |    2 +-
>  6 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/vme/bridges/vme_ca91cx42.c b/drivers/staging/vme/bridges/vme_ca91cx42.c
> index 2f9c22b..dff64e1 100644
> --- a/drivers/staging/vme/bridges/vme_ca91cx42.c
> +++ b/drivers/staging/vme/bridges/vme_ca91cx42.c
> @@ -1490,7 +1490,7 @@ static int ca91cx42_lm_detach(struct vme_lm_resource *lm, int monitor)
>  	return 0;
>  }
>  
> -static int ca91cx42_slot_get(struct vme_bridge *ca91cx42_bridge)
> +static int ca91cx42_get_slot(struct vme_bridge *ca91cx42_bridge)
>  {
>  	u32 slot = 0;
>  	struct ca91cx42_driver *bridge;
> @@ -1551,7 +1551,7 @@ static int ca91cx42_crcsr_init(struct vme_bridge *ca91cx42_bridge,
>  
>  	bridge = ca91cx42_bridge->driver_priv;
>  
> -	slot = ca91cx42_slot_get(ca91cx42_bridge);
> +	slot = ca91cx42_get_slot(ca91cx42_bridge);
>  
>  	/* Write CSR Base Address if slot ID is supplied as a module param */
>  	if (geoid)
> @@ -1799,7 +1799,7 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	ca91cx42_bridge->lm_get = ca91cx42_lm_get;
>  	ca91cx42_bridge->lm_attach = ca91cx42_lm_attach;
>  	ca91cx42_bridge->lm_detach = ca91cx42_lm_detach;
> -	ca91cx42_bridge->slot_get = ca91cx42_slot_get;
> +	ca91cx42_bridge->get_slot = ca91cx42_get_slot;
>  	ca91cx42_bridge->alloc_consistent = ca91cx42_alloc_consistent;
>  	ca91cx42_bridge->free_consistent = ca91cx42_free_consistent;
>  
> @@ -1807,7 +1807,7 @@ static int ca91cx42_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	dev_info(&pdev->dev, "Board is%s the VME system controller\n",
>  		(data & CA91CX42_MISC_CTL_SYSCON) ? "" : " not");
>  	dev_info(&pdev->dev, "Slot ID is %d\n",
> -		ca91cx42_slot_get(ca91cx42_bridge));
> +		ca91cx42_get_slot(ca91cx42_bridge));
>  
>  	if (ca91cx42_crcsr_init(ca91cx42_bridge, pdev))
>  		dev_err(&pdev->dev, "CR/CSR configuration failed.\n");
> diff --git a/drivers/staging/vme/bridges/vme_tsi148.c b/drivers/staging/vme/bridges/vme_tsi148.c
> index 9db89dc..922902b 100644
> --- a/drivers/staging/vme/bridges/vme_tsi148.c
> +++ b/drivers/staging/vme/bridges/vme_tsi148.c
> @@ -2106,7 +2106,7 @@ static int tsi148_lm_detach(struct vme_lm_resource *lm, int monitor)
>  /*
>   * Determine Geographical Addressing
>   */
> -static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
> +static int tsi148_get_slot(struct vme_bridge *tsi148_bridge)
>  {
>  	u32 slot = 0;
>  	struct tsi148_driver *bridge;
> @@ -2191,7 +2191,7 @@ static int tsi148_crcsr_init(struct vme_bridge *tsi148_bridge,
>  	cbar = ioread32be(bridge->base + TSI148_CBAR);
>  	cbar = (cbar & TSI148_CRCSR_CBAR_M)>>3;
>  
> -	vstat = tsi148_slot_get(tsi148_bridge);
> +	vstat = tsi148_get_slot(tsi148_bridge);
>  
>  	if (cbar != vstat) {
>  		cbar = vstat;
> @@ -2474,7 +2474,7 @@ static int tsi148_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	tsi148_bridge->lm_get = tsi148_lm_get;

>  	tsi148_bridge->lm_attach = tsi148_lm_attach;
>  	tsi148_bridge->lm_detach = tsi148_lm_detach;
> -	tsi148_bridge->slot_get = tsi148_slot_get;
> +	tsi148_bridge->get_slot = tsi148_get_slot;
>  	tsi148_bridge->alloc_consistent = tsi148_alloc_consistent;
>  	tsi148_bridge->free_consistent = tsi148_free_consistent;
>  
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 759ec2b..81a33dc 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1332,7 +1332,7 @@ void vme_lm_free(struct vme_resource *resource)
>  }
>  EXPORT_SYMBOL(vme_lm_free);
>  
> -int vme_slot_get(struct device *bus)
> +int vme_get_slot(struct device *bus)
>  {
>  	struct vme_bridge *bridge;
>  
> @@ -1342,14 +1342,14 @@ int vme_slot_get(struct device *bus)
>  		return -EINVAL;
>  	}
>  
> -	if (bridge->slot_get == NULL) {
> -		printk(KERN_WARNING "vme_slot_get not supported\n");
> +	if (bridge->get_slot == NULL) {
> +		printk(KERN_WARNING "vme_get_slot not supported\n");
>  		return -EINVAL;
>  	}
>  
> -	return bridge->slot_get(bridge);
> +	return bridge->get_slot(bridge);
>  }
> -EXPORT_SYMBOL(vme_slot_get);
> +EXPORT_SYMBOL(vme_get_slot);
>  
>  
>  /* - Bridge Registration --------------------------------------------------- */
> @@ -1549,7 +1549,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_get_slot(dev)))
>  				return 1;
>  		}
>  		i++;
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index 8fb35e2..bda6fdf 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -162,7 +162,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_get_slot(struct device *);
>  
>  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..e0152c5 100644
> --- a/drivers/staging/vme/vme_api.txt
> +++ b/drivers/staging/vme/vme_api.txt
> @@ -380,4 +380,4 @@ Slot Detection
>  
>  This function returns the slot ID of the provided bridge.
>  
> -	int vme_slot_get(struct device *dev);
> +	int vme_get_slot(struct device *dev);
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index ef751a4..5f70848 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -164,7 +164,7 @@ struct vme_bridge {
>  	int (*lm_detach) (struct vme_lm_resource *, int);
>  
>  	/* CR/CSR space functions */
> -	int (*slot_get) (struct vme_bridge *);
> +	int (*get_slot) (struct vme_bridge *);
>  
>  	/* Bridge parent interface */
>  	void *(*alloc_consistent)(struct device *dev, size_t size,


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

Re: [PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Manohar Vanga]

Hi Martyn,

> If your doing vme_slot_get, then there's also vme_lm_get, vme_master_get and
> vme_slave_get.
> 
> Doing just this would then lead to more inconsistency in the naming and
> wouldn't even get rid of all the functions using the *_get naming convention.

I can simply change those as well and resend :)

> I'm not sure this change is worth it.

I thought it would be worth changing this especially with the addition of bridge
refcounting functions (which have the *_get/_set convention).

--
/manohar

Re: [PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Martyn Welch]

On 01/08/11 13:31, Manohar Vanga wrote:
> Hi Martyn,
> 
>> If your doing vme_slot_get, then there's also vme_lm_get, vme_master_get and
>> vme_slave_get.
>>
>> Doing just this would then lead to more inconsistency in the naming and
>> wouldn't even get rid of all the functions using the *_get naming convention.
> 
> I can simply change those as well and resend :)
> 
>> I'm not sure this change is worth it.
> 
> I thought it would be worth changing this especially with the addition of bridge
> refcounting functions (which have the *_get/_set convention).
> 

The naming of the API is reasonably consistent at the moment. It looks like we
aren't going to need to export vme_bus_get and vme_bus_put based on the
conversation on patch 5. In addition to this, I've discovered that the
functions for USB are actually called usb_get_dev and usb_put_dev, so the
*_put/get convention isn't very strictly adhered to. I'd prefer to keep the
API as is.

Martyn

> --
> /manohar


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

Re: [PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Greg KH]

On Tue, Aug 09, 2011 at 04:18:24PM +0100, Martyn Welch wrote:
> On 01/08/11 13:31, Manohar Vanga wrote:
> > Hi Martyn,
> > 
> >> If your doing vme_slot_get, then there's also vme_lm_get, vme_master_get and
> >> vme_slave_get.
> >>
> >> Doing just this would then lead to more inconsistency in the naming and
> >> wouldn't even get rid of all the functions using the *_get naming convention.
> > 
> > I can simply change those as well and resend :)
> > 
> >> I'm not sure this change is worth it.
> > 
> > I thought it would be worth changing this especially with the addition of bridge
> > refcounting functions (which have the *_get/_set convention).
> > 
> 
> The naming of the API is reasonably consistent at the moment. It looks like we
> aren't going to need to export vme_bus_get and vme_bus_put based on the
> conversation on patch 5. In addition to this, I've discovered that the
> functions for USB are actually called usb_get_dev and usb_put_dev, so the
> *_put/get convention isn't very strictly adhered to. I'd prefer to keep the
> API as is.

It's not that strict, so I wouldn't worry about changing the names, as
it's obvious what is happening.

greg k-h

Re: [PATCH 6/8] staging: vme: rename *_slot_get to *_get_slot

[Manohar Vanga]

Hey Martyn,

> The naming of the API is reasonably consistent at the moment. It looks like we
> aren't going to need to export vme_bus_get and vme_bus_put based on the
> conversation on patch 5. In addition to this, I've discovered that the
> functions for USB are actually called usb_get_dev and usb_put_dev, so the
> *_put/get convention isn't very strictly adhered to. I'd prefer to keep the
> API as is.

I guess it's not as strict. I'm dropping this one from the resend.

Thanks!

--
/manohar

[PATCH 7/8] 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 |   16 ++---
 drivers/staging/vme/vme.c              |  120 +++++++++++++------------------
 drivers/staging/vme/vme.h              |   37 +++++++---
 drivers/staging/vme/vme_bridge.h       |    5 +-
 4 files changed, 86 insertions(+), 92 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 7ed4a5c..aff4f92 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 struct vme_bridge *vme_user_bus;		/* Pointer to bridge */
 
@@ -137,8 +137,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,
@@ -691,8 +691,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];
@@ -704,7 +703,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++) {
@@ -831,7 +830,7 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
 	 * to fall over in case the bridge module is removed while the vme_user
 	 * driver is still loaded.
 	 */
-	vme_user_bus = vme_bridge_get(cur_bus);
+	vme_user_bus = vme_bridge_get(vdev->id.bus);
 	if (!vme_user_bus) {
 		printk(KERN_ERR "%s: vme_bridge_get failed\n", driver_name);
 		err = -EINVAL;
@@ -882,8 +881,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 81a33dc..5220d43 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);
 }
 
 /*
@@ -280,7 +276,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;
@@ -289,7 +285,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;
@@ -436,7 +432,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;
@@ -445,7 +441,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;
@@ -694,7 +690,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;
@@ -705,7 +702,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;
@@ -1038,13 +1035,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;
@@ -1081,11 +1078,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;
@@ -1116,11 +1113,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;
@@ -1143,7 +1140,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;
@@ -1151,7 +1148,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;
@@ -1332,11 +1329,11 @@ void vme_lm_free(struct vme_resource *resource)
 }
 EXPORT_SYMBOL(vme_lm_free);
 
-int vme_get_slot(struct device *bus)
+int vme_get_slot(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;
@@ -1408,7 +1405,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;
 
@@ -1421,20 +1418,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;
 	}
@@ -1443,8 +1443,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;
@@ -1454,12 +1454,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);
 }
@@ -1485,32 +1485,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)
@@ -1521,15 +1495,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) {
@@ -1549,7 +1525,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_get_slot(dev)))
+				(num == vme_get_slot(vdev)))
 				return 1;
 		}
 		i++;
@@ -1564,13 +1540,15 @@ static int vme_bus_probe(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = -ENODEV;
 
 	driver = dev_to_vme_driver(dev);
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 
 	if (driver->probe != NULL)
-		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->probe(vdev);
 
 	return retval;
 }
@@ -1579,13 +1557,15 @@ static int vme_bus_remove(struct device *dev)
 {
 	struct vme_bridge *bridge;
 	struct vme_driver *driver;
+	struct vme_dev *vdev;
 	int retval = -ENODEV;
 
 	driver = dev_to_vme_driver(dev);
-	bridge = dev_to_bridge(dev);
+	vdev = dev_to_vme_dev(dev);
+	bridge = vdev->bridge;
 
 	if (driver->remove != NULL)
-		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
+		retval = driver->remove(vdev);
 
 	return retval;
 }
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index bda6fdf..6e37939 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -93,17 +93,34 @@ extern struct bus_type vme_bus_type;
 #define VME_SLOT_CURRENT	-1
 #define VME_SLOT_ALL		-2
 
+/**
+ * VME device identifier structure
+ * @bus: The bus ID of the bus the device is on
+ * @slot: The slot this device is plugged into
+ */
 struct vme_device_id {
 	int bus;
 	int slot;
 };
 
+/**
+ * Structure representing a VME device
+ * @id: The ID of the device (currently the bus and slot number)
+ * @bridge: Pointer to the bridge device this device is on
+ * @dev: Internal device structure
+ */
+struct vme_dev {
+	struct vme_device_id id;
+	struct vme_bridge *bridge;
+	struct device dev;
+};
+
 struct vme_driver {
 	struct list_head node;
 	const char *name;
 	const struct vme_device_id *bind_table;
-	int (*probe)  (struct device *, int, int);
-	int (*remove) (struct device *, int, int);
+	int (*probe)  (struct vme_dev *);
+	int (*remove) (struct vme_dev *);
 	void (*shutdown) (void);
 	struct device_driver    driver;
 };
@@ -114,7 +131,7 @@ void vme_free_consistent(struct vme_resource *, size_t,  void *,
 
 size_t vme_get_size(struct vme_resource *);
 
-struct vme_resource *vme_slave_request(struct device *, vme_address_t,
+struct vme_resource *vme_slave_request(struct vme_dev *, vme_address_t,
 	vme_cycle_t);
 int vme_slave_set(struct vme_resource *, int, unsigned long long,
 	unsigned long long, dma_addr_t, vme_address_t, vme_cycle_t);
@@ -122,7 +139,7 @@ int vme_slave_get(struct vme_resource *, int *, unsigned long long *,
 	unsigned long long *, dma_addr_t *, vme_address_t *, vme_cycle_t *);
 void vme_slave_free(struct vme_resource *);
 
-struct vme_resource *vme_master_request(struct device *, vme_address_t,
+struct vme_resource *vme_master_request(struct vme_dev *, vme_address_t,
 	vme_cycle_t, vme_width_t);
 int vme_master_set(struct vme_resource *, int, unsigned long long,
 	unsigned long long, vme_address_t, vme_cycle_t, vme_width_t);
@@ -134,7 +151,7 @@ unsigned int vme_master_rmw(struct vme_resource *, unsigned int, unsigned int,
 	unsigned int, loff_t);
 void vme_master_free(struct vme_resource *);
 
-struct vme_resource *vme_dma_request(struct device *, vme_dma_route_t);
+struct vme_resource *vme_dma_request(struct vme_dev *, vme_dma_route_t);
 struct vme_dma_list *vme_new_dma_list(struct vme_resource *);
 struct vme_dma_attr *vme_dma_pattern_attribute(u32, vme_pattern_t);
 struct vme_dma_attr *vme_dma_pci_attribute(dma_addr_t);
@@ -147,12 +164,12 @@ int vme_dma_list_exec(struct vme_dma_list *);
 int vme_dma_list_free(struct vme_dma_list *);
 int vme_dma_free(struct vme_resource *);
 
-int vme_irq_request(struct device *, int, int,
+int vme_irq_request(struct vme_dev *, int, int,
 	void (*callback)(int, int, void *), void *);
-void vme_irq_free(struct device *, int, int);
-int vme_irq_generate(struct device *, int, int);
+void vme_irq_free(struct vme_dev *, int, int);
+int vme_irq_generate(struct vme_dev *, int, int);
 
-struct vme_resource * vme_lm_request(struct device *);
+struct vme_resource * vme_lm_request(struct vme_dev *);
 int vme_lm_count(struct vme_resource *);
 int vme_lm_set(struct vme_resource *, unsigned long long, vme_address_t,
 	vme_cycle_t);
@@ -162,7 +179,7 @@ int vme_lm_attach(struct vme_resource *, int, void (*callback)(int));
 int vme_lm_detach(struct vme_resource *, int);
 void vme_lm_free(struct vme_resource *);
 
-int vme_get_slot(struct device *);
+int vme_get_slot(struct vme_dev *);
 
 int vme_register_driver(struct vme_driver *);
 void vme_unregister_driver(struct vme_driver *);
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index 5f70848..e5ea767 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 7/8] staging: vme: add struct vme_dev for VME devices

[Martyn Welch]

On 01/08/11 11:20, 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).
> 

Given the proposed changes made to earlier patches in the series, I'm going to
reserve judgement on this patch for now (it'll clearly change a bit...)

Martyn

> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/devices/vme_user.c |   16 ++---
>  drivers/staging/vme/vme.c              |  120 +++++++++++++------------------
>  drivers/staging/vme/vme.h              |   37 +++++++---
>  drivers/staging/vme/vme_bridge.h       |    5 +-
>  4 files changed, 86 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index 7ed4a5c..aff4f92 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 struct vme_bridge *vme_user_bus;		/* Pointer to bridge */
>  
> @@ -137,8 +137,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,
> @@ -691,8 +691,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];
> @@ -704,7 +703,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++) {
> @@ -831,7 +830,7 @@ static int __devinit vme_user_probe(struct device *dev, int cur_bus,
>  	 * to fall over in case the bridge module is removed while the vme_user
>  	 * driver is still loaded.
>  	 */
> -	vme_user_bus = vme_bridge_get(cur_bus);
> +	vme_user_bus = vme_bridge_get(vdev->id.bus);
>  	if (!vme_user_bus) {
>  		printk(KERN_ERR "%s: vme_bridge_get failed\n", driver_name);
>  		err = -EINVAL;
> @@ -882,8 +881,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 81a33dc..5220d43 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);
>  }
>  
>  /*
> @@ -280,7 +276,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;
> @@ -289,7 +285,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;
> @@ -436,7 +432,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;
> @@ -445,7 +441,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;
> @@ -694,7 +690,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;
> @@ -705,7 +702,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;
> @@ -1038,13 +1035,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;
> @@ -1081,11 +1078,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;
> @@ -1116,11 +1113,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;
> @@ -1143,7 +1140,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;
> @@ -1151,7 +1148,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;
> @@ -1332,11 +1329,11 @@ void vme_lm_free(struct vme_resource *resource)
>  }
>  EXPORT_SYMBOL(vme_lm_free);
>  
> -int vme_get_slot(struct device *bus)
> +int vme_get_slot(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;
> @@ -1408,7 +1405,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;
>  
> @@ -1421,20 +1418,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;
>  	}
> @@ -1443,8 +1443,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;
> @@ -1454,12 +1454,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);
>  }
> @@ -1485,32 +1485,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)
> @@ -1521,15 +1495,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) {
> @@ -1549,7 +1525,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_get_slot(dev)))
> +				(num == vme_get_slot(vdev)))
>  				return 1;
>  		}
>  		i++;
> @@ -1564,13 +1540,15 @@ static int vme_bus_probe(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> +	struct vme_dev *vdev;
>  	int retval = -ENODEV;
>  
>  	driver = dev_to_vme_driver(dev);
> -	bridge = dev_to_bridge(dev);
> +	vdev = dev_to_vme_dev(dev);
> +	bridge = vdev->bridge;
>  
>  	if (driver->probe != NULL)
> -		retval = driver->probe(dev, bridge->num, vme_calc_slot(dev));
> +		retval = driver->probe(vdev);
>  
>  	return retval;
>  }
> @@ -1579,13 +1557,15 @@ static int vme_bus_remove(struct device *dev)
>  {
>  	struct vme_bridge *bridge;
>  	struct vme_driver *driver;
> +	struct vme_dev *vdev;
>  	int retval = -ENODEV;
>  
>  	driver = dev_to_vme_driver(dev);
> -	bridge = dev_to_bridge(dev);
> +	vdev = dev_to_vme_dev(dev);
> +	bridge = vdev->bridge;
>  
>  	if (driver->remove != NULL)
> -		retval = driver->remove(dev, bridge->num, vme_calc_slot(dev));
> +		retval = driver->remove(vdev);
>  
>  	return retval;
>  }
> diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
> index bda6fdf..6e37939 100644
> --- a/drivers/staging/vme/vme.h
> +++ b/drivers/staging/vme/vme.h
> @@ -93,17 +93,34 @@ extern struct bus_type vme_bus_type;
>  #define VME_SLOT_CURRENT	-1
>  #define VME_SLOT_ALL		-2
>  
> +/**
> + * VME device identifier structure
> + * @bus: The bus ID of the bus the device is on
> + * @slot: The slot this device is plugged into
> + */
>  struct vme_device_id {
>  	int bus;
>  	int slot;
>  };
>  
> +/**
> + * Structure representing a VME device
> + * @id: The ID of the device (currently the bus and slot number)
> + * @bridge: Pointer to the bridge device this device is on
> + * @dev: Internal device structure
> + */
> +struct vme_dev {
> +	struct vme_device_id id;
> +	struct vme_bridge *bridge;
> +	struct device dev;
> +};
> +
>  struct vme_driver {
>  	struct list_head node;
>  	const char *name;
>  	const struct vme_device_id *bind_table;
> -	int (*probe)  (struct device *, int, int);
> -	int (*remove) (struct device *, int, int);
> +	int (*probe)  (struct vme_dev *);
> +	int (*remove) (struct vme_dev *);
>  	void (*shutdown) (void);
>  	struct device_driver    driver;
>  };
> @@ -114,7 +131,7 @@ void vme_free_consistent(struct vme_resource *, size_t,  void *,
>  
>  size_t vme_get_size(struct vme_resource *);
>  
> -struct vme_resource *vme_slave_request(struct device *, vme_address_t,
> +struct vme_resource *vme_slave_request(struct vme_dev *, vme_address_t,
>  	vme_cycle_t);
>  int vme_slave_set(struct vme_resource *, int, unsigned long long,
>  	unsigned long long, dma_addr_t, vme_address_t, vme_cycle_t);
> @@ -122,7 +139,7 @@ int vme_slave_get(struct vme_resource *, int *, unsigned long long *,
>  	unsigned long long *, dma_addr_t *, vme_address_t *, vme_cycle_t *);
>  void vme_slave_free(struct vme_resource *);
>  
> -struct vme_resource *vme_master_request(struct device *, vme_address_t,
> +struct vme_resource *vme_master_request(struct vme_dev *, vme_address_t,
>  	vme_cycle_t, vme_width_t);
>  int vme_master_set(struct vme_resource *, int, unsigned long long,
>  	unsigned long long, vme_address_t, vme_cycle_t, vme_width_t);
> @@ -134,7 +151,7 @@ unsigned int vme_master_rmw(struct vme_resource *, unsigned int, unsigned int,
>  	unsigned int, loff_t);
>  void vme_master_free(struct vme_resource *);
>  
> -struct vme_resource *vme_dma_request(struct device *, vme_dma_route_t);
> +struct vme_resource *vme_dma_request(struct vme_dev *, vme_dma_route_t);
>  struct vme_dma_list *vme_new_dma_list(struct vme_resource *);
>  struct vme_dma_attr *vme_dma_pattern_attribute(u32, vme_pattern_t);
>  struct vme_dma_attr *vme_dma_pci_attribute(dma_addr_t);
> @@ -147,12 +164,12 @@ int vme_dma_list_exec(struct vme_dma_list *);
>  int vme_dma_list_free(struct vme_dma_list *);
>  int vme_dma_free(struct vme_resource *);
>  
> -int vme_irq_request(struct device *, int, int,
> +int vme_irq_request(struct vme_dev *, int, int,
>  	void (*callback)(int, int, void *), void *);
> -void vme_irq_free(struct device *, int, int);
> -int vme_irq_generate(struct device *, int, int);
> +void vme_irq_free(struct vme_dev *, int, int);
> +int vme_irq_generate(struct vme_dev *, int, int);
>  
> -struct vme_resource * vme_lm_request(struct device *);
> +struct vme_resource * vme_lm_request(struct vme_dev *);
>  int vme_lm_count(struct vme_resource *);
>  int vme_lm_set(struct vme_resource *, unsigned long long, vme_address_t,
>  	vme_cycle_t);
> @@ -162,7 +179,7 @@ int vme_lm_attach(struct vme_resource *, int, void (*callback)(int));
>  int vme_lm_detach(struct vme_resource *, int);
>  void vme_lm_free(struct vme_resource *);
>  
> -int vme_get_slot(struct device *);
> +int vme_get_slot(struct vme_dev *);
>  
>  int vme_register_driver(struct vme_driver *);
>  void vme_unregister_driver(struct vme_driver *);
> diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
> index 5f70848..e5ea767 100644
> --- a/drivers/staging/vme/vme_bridge.h
> +++ b/drivers/staging/vme/vme_bridge.h
> @@ -115,9 +115,8 @@ struct vme_bridge {
>  	struct list_head bus_list; /* list of VME buses */
>  	struct module *owner;	/* module that owns the bridge */
>  
> -	struct device dev[VME_SLOTS_MAX];	/* Device registered with
> -						 * device model on VME bus
> -						 */
> +	struct vme_dev dev[VME_SLOTS_MAX];	/* Device registered
> +						 * on VME bus */
>  
>  	/* Interrupt callbacks */
>  	struct vme_irq irq[7];


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

[PATCH 8/8] 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 |   25 +++-
 drivers/staging/vme/devices/vme_user.h |    2 +-
 drivers/staging/vme/vme.c              |  218 +++++++++++++++-----------------
 drivers/staging/vme/vme.h              |   19 ++-
 drivers/staging/vme/vme_bridge.h       |    4 -
 5 files changed, 139 insertions(+), 129 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index aff4f92..1218992 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:
@@ -137,6 +137,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 *);
 
@@ -622,6 +623,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),
 };
@@ -645,10 +647,10 @@ static int __init vme_user_init(void)
 	/* Let's start by supporting one bus, we can support more than one
 	 * in future revisions if that ever becomes necessary.
 	 */
-	if (bus_num > USER_BUS_MAX) {
+	if (bus_num > VME_USER_BUS_MAX) {
 		printk(KERN_ERR "%s: Driver only able to handle %d buses\n",
-			driver_name, USER_BUS_MAX);
-		bus_num = USER_BUS_MAX;
+			driver_name, VME_USER_BUS_MAX);
+		bus_num = VME_USER_BUS_MAX;
 	}
 
 
@@ -673,7 +675,13 @@ static int __init vme_user_init(void)
 
 	vme_user_driver.bind_table = ids;
 
-	retval = vme_register_driver(&vme_user_driver);
+	/*
+	 * Here we just register the maximum number of devices we can and
+	 * leave vme_user_match() to allow only 1 to go through to probe().
+	 * This way, if we later want to allow multiple user access devices,
+	 * we just change the code in vme_user_match().
+	 */
+	retval = vme_register_driver(&vme_user_driver, VME_MAX_SLOTS);
 	if (retval != 0)
 		goto err_reg;
 
@@ -686,6 +694,13 @@ err_nocard:
 	return retval;
 }
 
+static int vme_user_match(struct vme_dev *vdev)
+{
+	if (vdev->id.num >= VME_USER_BUS_MAX)
+		return 0;
+	return 1;
+}
+
 /*
  * In this simple access driver, the old behaviour is being preserved as much
  * as practical. We will therefore reserve the buffers and request the images
diff --git a/drivers/staging/vme/devices/vme_user.h b/drivers/staging/vme/devices/vme_user.h
index 24bf4e5..d85a1e9 100644
--- a/drivers/staging/vme/devices/vme_user.h
+++ b/drivers/staging/vme/devices/vme_user.h
@@ -1,7 +1,7 @@
 #ifndef _VME_USER_H_
 #define _VME_USER_H_
 
-#define USER_BUS_MAX                  1
+#define VME_USER_BUS_MAX	1
 
 /*
  * VMEbus Master Window Configuration Structure
diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index 5220d43..05166c3 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.
@@ -1405,169 +1402,162 @@ 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);
+		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:
-	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) {
+		if (!try_module_get(bridge->owner))
+			continue;
+		mutex_unlock(&vme_buses_lock);
+		err = __vme_register_driver_bus(drv, bridge, ndevs);
+		mutex_lock(&vme_buses_lock);
+		module_put(bridge->owner);
+		if (err)
+			break;
 	}
-	vme_remove_bus(bridge);
+	mutex_unlock(&vme_buses_lock);
+	return err;
 }
-EXPORT_SYMBOL(vme_unregister_bridge);
 
-
-/* - Driver Registration --------------------------------------------------- */
-
-int vme_register_driver(struct vme_driver *drv)
+int vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
 {
+	int err;
+
 	drv->driver.name = drv->name;
 	drv->driver.bus = &vme_bus_type;
-
-	return driver_register(&drv->driver);
+	INIT_LIST_HEAD(&drv->devices);
+
+	err = driver_register(&drv->driver);
+	if (err)
+		return err;
+
+	err = __vme_register_driver(drv, ndevs);
+	if (!err & (drv->ndev == 0))
+		err = -ENODEV;
+	if (err)
+		vme_unregister_driver(drv);
+	return err;
 }
 EXPORT_SYMBOL(vme_register_driver);
 
 void vme_unregister_driver(struct vme_driver *drv)
 {
+	struct vme_dev *dev, *dev_tmp;
+
+	list_for_each_entry_safe(dev, dev_tmp, &drv->devices, list) {
+		device_unregister(&dev->dev);
+		list_del(&dev->list);
+	}
 	driver_unregister(&drv->driver);
 }
 EXPORT_SYMBOL(vme_unregister_driver);
 
 /* - Bus Registration ------------------------------------------------------ */
 
-static struct vme_driver *dev_to_vme_driver(struct device *dev)
-{
-	if (dev->driver == NULL)
-		printk(KERN_ERR "Bugger dev->driver is NULL\n");
-
-	return container_of(dev->driver, struct vme_driver, driver);
-}
-
 static int vme_bus_match(struct device *dev, struct device_driver *drv)
 {
-	struct vme_dev *vdev;
-	struct vme_bridge *bridge;
-	struct vme_driver *driver;
-	int i, num;
-
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
-	driver = container_of(drv, struct vme_driver, driver);
-
-	num = vdev->id.slot;
-	if (num <= 0 || num >= VME_SLOTS_MAX)
-		goto err_dev;
-
-	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)) {
+	struct vme_driver *vme_drv;
 
-		if (bridge->num == driver->bind_table[i].bus) {
-			if (num == driver->bind_table[i].slot)
-				return 1;
+	vme_drv = container_of(drv, struct vme_driver, driver);
 
-			if (driver->bind_table[i].slot == VME_SLOT_ALL)
-				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_CURRENT) &&
-				(num == vme_get_slot(vdev)))
-				return 1;
-		}
-		i++;
+		if (vme_drv->match && vme_drv->match(vdev))
+			return 1;
+		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 = -ENODEV;
-
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	if (driver->probe != NULL)
-		retval = driver->probe(vdev);
-
-	return retval;
+	driver = dev->platform_data;
+	if (driver->probe)
+		return driver->probe(vdev);
+	return 0;
 }
 
 static int vme_bus_remove(struct device *dev)
 {
-	struct vme_bridge *bridge;
 	struct vme_driver *driver;
-	struct vme_dev *vdev;
-	int retval = -ENODEV;
+	struct vme_dev *vdev = dev_to_vme_dev(dev);
 
-	driver = dev_to_vme_driver(dev);
-	vdev = dev_to_vme_dev(dev);
-	bridge = vdev->bridge;
-
-	if (driver->remove != NULL)
-		retval = driver->remove(vdev);
-
-	return retval;
+	driver = dev->platform_data;
+	if (driver->remove)
+		return driver->remove(vdev);
+	return 0;
 }
 
 struct bus_type vme_bus_type = {
diff --git a/drivers/staging/vme/vme.h b/drivers/staging/vme/vme.h
index 6e37939..ce158b5 100644
--- a/drivers/staging/vme/vme.h
+++ b/drivers/staging/vme/vme.h
@@ -90,15 +90,19 @@ extern struct bus_type vme_bus_type;
 
 /* VME_MAX_BRIDGES comes from the type of vme_bus_numbers */
 #define VME_MAX_BRIDGES		(sizeof(unsigned int)*8)
+#define VME_MAX_SLOTS		32
+
 #define VME_SLOT_CURRENT	-1
 #define VME_SLOT_ALL		-2
 
 /**
  * VME device identifier structure
+ * @num: The device ID (ranges from 0 to N-1 for N devices)
  * @bus: The bus ID of the bus the device is on
  * @slot: The slot this device is plugged into
  */
 struct vme_device_id {
+	int num;
 	int bus;
 	int slot;
 };
@@ -108,21 +112,26 @@ struct vme_device_id {
  * @id: The ID of the device (currently the bus and slot number)
  * @bridge: Pointer to the bridge device this device is on
  * @dev: Internal device structure
+ * @list: List of devices (per driver)
  */
 struct vme_dev {
 	struct vme_device_id id;
 	struct vme_bridge *bridge;
 	struct device dev;
+	struct list_head list;
 };
 
 struct vme_driver {
 	struct list_head node;
 	const char *name;
 	const struct vme_device_id *bind_table;
-	int (*probe)  (struct vme_dev *);
-	int (*remove) (struct vme_dev *);
-	void (*shutdown) (void);
-	struct device_driver    driver;
+	int (*match)(struct vme_dev *);
+	int (*probe)(struct vme_dev *);
+	int (*remove)(struct vme_dev *);
+	void (*shutdown)(void);
+	struct device_driver driver;
+	struct list_head devices;
+	unsigned int ndev;
 };
 
 void *vme_alloc_consistent(struct vme_resource *, size_t, dma_addr_t *);
@@ -181,7 +190,7 @@ void vme_lm_free(struct vme_resource *);
 
 int vme_get_slot(struct vme_dev *);
 
-int vme_register_driver(struct vme_driver *);
+int vme_register_driver(struct vme_driver *, unsigned int);
 void vme_unregister_driver(struct vme_driver *);
 
 struct vme_bridge *vme_bridge_get(unsigned int bus_id);
diff --git a/drivers/staging/vme/vme_bridge.h b/drivers/staging/vme/vme_bridge.h
index e5ea767..460591c 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 8/8] staging: vme: make match() driver specific to improve non-VME64x support

[Martyn Welch]

On 01/08/11 11:20, 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).
> 

I haven't got any hardware old enough to hand to play with how ISA devices get
bound into the device model. I was thinking that we may be able to do
vme-$bus_id.$device_id, but this would require the $device_id to be unique per
bus, which isn't an issue on buses such as PCI (or I guess USB) where such
enumeration is built into the spec. With the exception of VME64x, which I
guess we should really be treating as a special case (I guess most cards in
actual use don't follow this standard), we are much closer to the ISA bus. I
assume that $driver_name-$bus_id.$device_id is roughly in line with how ISA
would do this (IIRC, ther would only ever be one ISA bus on a board, so the
bus entry wouldn't be needed, but we clearly need this)?

Martyn

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

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

[Manohar Vanga]

Hey Martyn,

> I haven't got any hardware old enough to hand to play with how ISA devices get
> bound into the device model.

Neither do I! I studied the code in drivers/base/isa.c to understand how it is
being done. More specifically, take a look at isa_register_driver() and
isa_bus_match() and you will see the similarities with this patch :)

> I was thinking that we may be able to do vme-$bus_id.$device_id, but this
> would require the $device_id to be unique per bus, which isn't an issue on
> buses such as PCI (or I guess USB) where such enumeration is built into the
> spec. With the exception of VME64x, which I guess we should really be
> treating as a special case (I guess most cards in actual use don't follow
> this standard), we are much closer to the ISA bus.

Exactly. This is great for VME64x but unfortunately makes the rest of the
cases a bit constrained.

> I assume that $driver_name-$bus_id.$device_id is roughly in line with how ISA
> would do this (IIRC, ther would only ever be one ISA bus on a board, so the
> bus entry wouldn't be needed, but we clearly need this)?

Yup. (Most of this code was taken from drivers/base/isa.c)

Thanks!

--
/manohar

Re: [PATCH 0/8] VME Driver Fixes

[Martyn Welch]

On 01/08/11 11:20, Manohar Vanga wrote:
> A little background: I work at CERN in the device drivers section and
> we have a lot of VME crates we use in the accelerator controls. We have
> been using an in-house driver for some time now and want to slowly port
> our existing device drivers over to the kernel driver.
> 
> This set of patches adds multiple changes to the VME driver that fix
> various issues, mostly dealing with the device driver model. 

Please can you update the vme_api.txt document as well to fully reflect the
proposed changes.

Martyn

> Some of
> these are based on the changes made by Emilio G. Cota in a previous
> LKML thread at:
> 
>     https://lkml.org/lkml/2010/10/25/480
> 



> Thanks and best regards,
> Manohar Vanga
> 
> Emilio G. Cota (1):
>   staging: vme: allow explicit assignment of bus numbers
> 
> Manohar Vanga (7):
>   staging: vme_user: change kmalloc+memset to kzalloc
>   staging: vme: make [alloc|free]_consistent bridge specific
>   staging: vme: keep track of registered buses
>   staging: vme: add functions for bridge module refcounting
>   staging: vme: rename *_slot_get to *_get_slot
>   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 |   54 ++++-
>  drivers/staging/vme/bridges/vme_tsi148.c   |   53 ++++-
>  drivers/staging/vme/devices/vme_user.c     |   63 ++++-
>  drivers/staging/vme/devices/vme_user.h     |    2 +-
>  drivers/staging/vme/vme.c                  |  414 ++++++++++++++++------------
>  drivers/staging/vme/vme.h                  |   56 +++-
>  drivers/staging/vme/vme_api.txt            |    2 +-
>  drivers/staging/vme/vme_bridge.h           |   19 +-
>  8 files changed, 435 insertions(+), 228 deletions(-)
> 


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

Re: [PATCH 0/8] VME Driver Fixes

[Manohar Vanga]

Hi Martyn,

> Please can you update the vme_api.txt document as well to fully reflect the
> proposed changes.

Yikes forgot to do this. I'll modify and resend :)

--
/manohar

Re: [PATCH 0/8] VME Driver Fixes

[Greg KH]

On Mon, Aug 01, 2011 at 12:20:45PM +0200, Manohar Vanga wrote:
> A little background: I work at CERN in the device drivers section and
> we have a lot of VME crates we use in the accelerator controls. We have
> been using an in-house driver for some time now and want to slowly port
> our existing device drivers over to the kernel driver.
> 
> This set of patches adds multiple changes to the VME driver that fix
> various issues, mostly dealing with the device driver model. Some of
> these are based on the changes made by Emilio G. Cota in a previous
> LKML thread at:
> 
>     https://lkml.org/lkml/2010/10/25/480

I only applied the first patch in this series.

thanks,

greg k-h