[PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[Faik Uygur]

Hi,

This patch uses idr_get_new to allocate a bus id while registering
a new adapter.


--- a/drivers/i2c/i2c-core.c	Sat May 15 23:19:11 2004
+++ b/drivers/i2c/i2c-core.c	Sat May 15 23:19:11 2004
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/idr.h>
 #include <linux/seq_file.h>
 #include <asm/uaccess.h>
 
@@ -35,6 +36,7 @@
 static LIST_HEAD(adapters);
 static LIST_HEAD(drivers);
 static DECLARE_MUTEX(core_lists);
+static DEFINE_IDR(i2c_adapter_idr);
 
 int i2c_device_probe(struct device *dev)
 {
@@ -113,13 +115,17 @@
  */
 int i2c_add_adapter(struct i2c_adapter *adap)
 {
-	static int nr = 0;
+	int id;
 	struct list_head   *item;
 	struct i2c_driver  *driver;
 
+	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
+		return -ENOMEM;
+
 	down(&core_lists);
 
-	adap->nr = nr++;
+	id = idr_get_new(&i2c_adapter_idr, NULL);
+	adap->nr =  id & MAX_ID_MASK;
 	init_MUTEX(&adap->bus_lock);
 	init_MUTEX(&adap->clist_lock);
 	list_add_tail(&adap->list,&adapters);
@@ -207,6 +213,9 @@
 	/* wait for sysfs to drop all references */
 	wait_for_completion(&adap->dev_released);
 	wait_for_completion(&adap->class_dev_released);
+
+	/* free dynamically allocated bus id */
+	idr_remove(&i2c_adapter_idr, adap->nr);
 
 	dev_dbg(&adap->dev, "adapter unregistered\n");
 

Re: [PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[Andrew Morton]

Faik Uygur <faikuygur@tnn.net> wrote:
>
> Hi,
> 
> This patch uses idr_get_new to allocate a bus id while registering
> a new adapter.
> 

The IDR interface is a bit cumbersome.  Even though you called
idr_pre_get(), there's no guarantee that the memory which it preallocated
is still present when you call idr_get_new().

It's easily fixed though:


>  int i2c_add_adapter(struct i2c_adapter *adap)
>  {
> -	static int nr = 0;
> +	int id;
>  	struct list_head   *item;
>  	struct i2c_driver  *driver;
>  
> +	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
> +		return -ENOMEM;
> +
>  	down(&core_lists);
>  
> -	adap->nr = nr++;
> +	id = idr_get_new(&i2c_adapter_idr, NULL);
> +	adap->nr =  id & MAX_ID_MASK;
>  	init_MUTEX(&adap->bus_lock);
>  	init_MUTEX(&adap->clist_lock);
>  	list_add_tail(&adap->list,&adapters);
> @@ -207,6 +213,9 @@
>  	/* wait for sysfs to drop all references */
>  	wait_for_completion(&adap->dev_released);
>  	wait_for_completion(&adap->class_dev_released);
> +
> +	/* free dynamically allocated bus id */
> +	idr_remove(&i2c_adapter_idr, adap->nr);
>  
>  	dev_dbg(&adap->dev, "adapter unregistered\n");

Just move the idr_pre_get() to after the down().  That way you know nobody
else will steal the preallocation.

Is the kernel likely to ever have so many bus IDs that we actually need
this patch?  Or do you specifically want first-fit-from-zero for some
reason?

Re: [PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[Faik Uygur]

On Sat, 15 May 2004, Andrew Morton wrote:

> The IDR interface is a bit cumbersome.  Even though you called
> idr_pre_get(), there's no guarantee that the memory which it preallocated
> is still present when you call idr_get_new().

Thanks for the correction.

> Is the kernel likely to ever have so many bus IDs that we actually need
> this patch?  Or do you specifically want first-fit-from-zero for some
> reason?

Actually there is no special need for this. It is just what i think would
be the expected behaviour. There was a thread two weeks ago about this issue:

http://marc.theaimsgroup.com/?l=linux-kernel&m=108370586601550&w=2

here is the updated patch:

diff -Nru a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
--- a/drivers/i2c/i2c-core.c	Sun May 16 11:55:29 2004
+++ b/drivers/i2c/i2c-core.c	Sun May 16 11:55:29 2004
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/idr.h>
 #include <linux/seq_file.h>
 #include <asm/uaccess.h>
 
@@ -35,6 +36,7 @@
 static LIST_HEAD(adapters);
 static LIST_HEAD(drivers);
 static DECLARE_MUTEX(core_lists);
+static DEFINE_IDR(i2c_adapter_idr);
 
 int i2c_device_probe(struct device *dev)
 {
@@ -113,13 +115,19 @@
  */
 int i2c_add_adapter(struct i2c_adapter *adap)
 {
-	static int nr = 0;
+	int id, res = 0;
 	struct list_head   *item;
 	struct i2c_driver  *driver;
 
 	down(&core_lists);
 
-	adap->nr = nr++;
+	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) {
+		res = -ENOMEM;
+		goto out_unlock;
+	}
+
+	id = idr_get_new(&i2c_adapter_idr, NULL);
+	adap->nr =  id & MAX_ID_MASK;
 	init_MUTEX(&adap->bus_lock);
 	init_MUTEX(&adap->clist_lock);
 	list_add_tail(&adap->list,&adapters);
@@ -151,10 +159,12 @@
 			/* We ignore the return code; if it fails, too bad */
 			driver->attach_adapter(adap);
 	}
-	up(&core_lists);
 
 	dev_dbg(&adap->dev, "registered as adapter #%d\n", adap->nr);
-	return 0;
+
+ out_unlock:
+	up(&core_lists);
+	return res;
 }
 
 
@@ -207,6 +217,9 @@
 	/* wait for sysfs to drop all references */
 	wait_for_completion(&adap->dev_released);
 	wait_for_completion(&adap->class_dev_released);
+
+	/* free dynamically allocated bus id */
+	idr_remove(&i2c_adapter_idr, adap->nr);
 
 	dev_dbg(&adap->dev, "adapter unregistered\n");

Re: [PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[Andrew Morton]

Faik Uygur <faikuygur@tnn.net> wrote:
>
>  > Is the kernel likely to ever have so many bus IDs that we actually need
>  > this patch?  Or do you specifically want first-fit-from-zero for some
>  > reason?
> 
>  Actually there is no special need for this. It is just what i think would
>  be the expected behaviour. There was a thread two weeks ago about this issue:
> 
>  http://marc.theaimsgroup.com/?l=linux-kernel&m=108370586601550&w=2
> 
>  here is the updated patch:

Looks good to me, thanks.

fyi, the IDR implementation in -mm doesn't mangle the top eight bits of the
idr_get_new() return value, so that masking will be able to go away.

And you'll then be able to support 2^31-1 i2c adapters...

Re: [PATCH] use idr_get_new to allocate a bus id in drivers/i2c/i2c-core.c

[Greg KH]

On Sun, May 16, 2004 at 12:13:13PM +0300, Faik Uygur wrote:
> On Sat, 15 May 2004, Andrew Morton wrote:
> 
> > The IDR interface is a bit cumbersome.  Even though you called
> > idr_pre_get(), there's no guarantee that the memory which it preallocated
> > is still present when you call idr_get_new().
> 
> Thanks for the correction.
> 
> > Is the kernel likely to ever have so many bus IDs that we actually need
> > this patch?  Or do you specifically want first-fit-from-zero for some
> > reason?
> 
> Actually there is no special need for this. It is just what i think would
> be the expected behaviour. There was a thread two weeks ago about this issue:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=108370586601550&w=2
> 
> here is the updated patch:

Nice, thanks, I've applied this to my trees.  I tried doing this same
patch a while ago, but missed the masking off the upper bits trick, and
was wondering why my results were so strange...

thanks again,

greg k-h