[i2c] [patch 2.6.21-rc3-git +i2c 2/5] i2c stack can remove()

[Jean Delvare]

Hi David,

On Thu, 8 Mar 2007 21:11:09 -0800, David Brownell wrote:
> More update for new style driver support:  add a remove() method, and
> use it in the relevant code paths.  Also add i2c_unregister_driver()
> for the exclusive use of new-style drivers.
> 
> Again, nothing will use this yet since there's nothing to create devices
> feeding this infrastructure.
> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> 
> ---
>  drivers/i2c/i2c-core.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/i2c.h    |    3 +
>  2 files changed, 99 insertions(+), 4 deletions(-)
> 
> Index: at91/include/linux/i2c.h
> ===================================================================
> --- at91.orig/include/linux/i2c.h	2007-03-08 13:47:48.000000000 -0800
> +++ at91/include/linux/i2c.h	2007-03-08 13:47:49.000000000 -0800
> @@ -130,6 +130,7 @@ struct i2c_driver {
>  	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
>  	 */
>  	int (*probe)(struct i2c_client *);
> +	int (*remove)(struct i2c_client *);
>  
>  	/* driver model interfaces that don't relate to enumeration  */
>  	void (*shutdown)(struct i2c_client *);
> @@ -302,6 +303,8 @@ extern int i2c_add_adapter(struct i2c_ad
>  extern int i2c_del_adapter(struct i2c_adapter *);
>  
>  extern int i2c_register_driver(struct module *, struct i2c_driver *);
> +extern void i2c_unregister_driver(struct i2c_driver *);
> +
>  extern int i2c_del_driver(struct i2c_driver *);
>  
>  static inline int i2c_add_driver(struct i2c_driver *driver)
> Index: at91/drivers/i2c/i2c-core.c
> ===================================================================
> --- at91.orig/drivers/i2c/i2c-core.c	2007-03-08 13:47:48.000000000 -0800
> +++ at91/drivers/i2c/i2c-core.c	2007-03-08 13:47:49.000000000 -0800
> (...)
> +/**
> + * i2c_unregister_driver - unregister non-legacy I2C driver
> + * @driver: the driver being unregistered
> + *
> + * Following the normal driver model conventions, when drivers are
> + * unregistered they receive remove() calls to unbind each i2c_client
> + * that had previously been bound to them by probe().  Those i2c_client
> + * handles must not be used after remove() returns; a different driver
> + * could have been bound to that device after remove() returns.
> + */
> +void i2c_unregister_driver(struct i2c_driver *driver)
> +{
> +	if (!driver->remove) {
> +		printk(KERN_WARNING "i2c-core: legacy driver [%s] "
> +				"can't use i2c_unregister_driver\n",
> +				driver->driver.name);
> +		return;
> +	}
> +	driver_unregister(&driver->driver);
> +}
> +EXPORT_SYMBOL(i2c_unregister_driver);

You have a bug here, you forgot to remove the driver from the i2c-core
internal drivers list. Any subsequent operation on this list will result
in an oops of some sort.

Additionally, I know we discussed this before and I was in favor of
this separate function for unregistering new-style drivers but... A
real world experiment convinced me easily that this wasn't a good idea.
Nobody checks the return value of i2c_del_driver() which is called by
all drivers for now, so people won't notice they are doing something
wrong when porting drivers to the new model. It took me quite some time
to figure it out. So I suggest to get rid of i2c_unregister_driver as
follows:

---
 drivers/i2c/i2c-core.c |   32 ++++++--------------------------
 include/linux/i2c.h    |    1 -
 2 files changed, 6 insertions(+), 27 deletions(-)

--- linux-2.6.21-rc3.orig/drivers/i2c/i2c-core.c	2007-03-09 21:02:37.000000000 +0100
+++ linux-2.6.21-rc3/drivers/i2c/i2c-core.c	2007-03-09 21:47:58.000000000 +0100
@@ -604,29 +604,7 @@ int i2c_register_driver(struct module *o
 EXPORT_SYMBOL(i2c_register_driver);
 
 /**
- * i2c_unregister_driver - unregister non-legacy I2C driver
- * @driver: the driver being unregistered
- *
- * Following the normal driver model conventions, when drivers are
- * unregistered they receive remove() calls to unbind each i2c_client
- * that had previously been bound to them by probe().  Those i2c_client
- * handles must not be used after remove() returns; a different driver
- * could have been bound to that device after remove() returns.
- */
-void i2c_unregister_driver(struct i2c_driver *driver)
-{
-	if (!driver->remove) {
-		printk(KERN_WARNING "i2c-core: legacy driver [%s] "
-				"can't use i2c_unregister_driver\n",
-				driver->driver.name);
-		return;
-	}
-	driver_unregister(&driver->driver);
-}
-EXPORT_SYMBOL(i2c_unregister_driver);
-
-/**
- * i2c_del_driver - unregister legacy I2C driver
+ * i2c_del_driver - unregister I2C driver
  * @driver: the driver being unregistered
  */
 int i2c_del_driver(struct i2c_driver *driver)
@@ -637,15 +615,16 @@ int i2c_del_driver(struct i2c_driver *dr
 
 	int res = 0;
 
-	/* new-style drivers use i2c_unregister_driver() */
+	mutex_lock(&core_lists);
+
+	/* new-style driver? */
 	if (driver->remove)
-		return -EINVAL;
+		goto unregister;
 
 	/* Have a look at each adapter, if clients of this driver are still
 	 * attached. If so, detach them to be able to kill the driver
 	 * afterwards.
 	 */
-	mutex_lock(&core_lists);
 	list_for_each(item1,&adapters) {
 		adap = list_entry(item1, struct i2c_adapter, list);
 		if (driver->detach_adapter) {
@@ -674,6 +653,7 @@ int i2c_del_driver(struct i2c_driver *dr
 		}
 	}
 
+ unregister:
 	driver_unregister(&driver->driver);
 	list_del(&driver->list);
 	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
--- linux-2.6.21-rc3.orig/include/linux/i2c.h	2007-03-09 16:57:41.000000000 +0100
+++ linux-2.6.21-rc3/include/linux/i2c.h	2007-03-09 21:20:04.000000000 +0100
@@ -362,7 +362,6 @@ extern int i2c_del_adapter(struct i2c_ad
 extern int i2c_add_numbered_adapter(struct i2c_adapter *);
 
 extern int i2c_register_driver(struct module *, struct i2c_driver *);
-extern void i2c_unregister_driver(struct i2c_driver *);
 
 extern int i2c_del_driver(struct i2c_driver *);

What do you think? 

-- 
Jean Delvare