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

[Jean Delvare]

Hi David,

On Mon, 12 Mar 2007 08:49:25 -0800, David Brownell wrote:
> More update for new style driver support:  add a remove() method, and
> use it in the relevant code paths.
> 
> 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>
> 
> ---
> UPDATED: address the del_driver() issue, don't add separate interface
> to unregister newstyle drivers, add and use is_newstyle_driver() for
> better internal consistency

I've been converting i2c-parport + lm90 to experiment with the new
model, but I'm hitting a bug in i2c_unregister_device. I converted lm90
to the new-style of i2c device driver, then modified i2c-parport to
instantiate an lm90 device. Loading the drivers work fine, lm90 is
loaded automatically as expected. If I unload lm90 first and
i2c-parport second, it works OK. However, if I unload i2c-parport
first, the lm90 driver's remove function ends up being called
_twice_ for the i2c device, with the consequences you can imagine.

I added a stack dump in i2c_device_remove to figure out what was going
on, and here is what I obtained:

First call:

  [<d98a61f6>] i2c_device_remove+0x76/0x90 [i2c_core]
  [<d98a63a4>] i2c_unregister_device+0x34/0x150 [i2c_core]
  [<c02b247b>] __mutex_lock_interruptible_slowpath+0xfb/0x290
  [<d9a976b5>] i2c_parport_detach+0xf5/0x120 [i2c_parport]
  [<d88651a5>] parport_unregister_driver+0x35/0x60 [parport]
  [<c0136109>] sys_delete_module+0x129/0x140

Second call:

  [<d98a61f6>] i2c_device_remove+0x76/0x90 [i2c_core]
  [<c0235c24>] __device_release_driver+0x94/0xb0
  [<c0235c5d>] device_release_driver+0x1d/0x40
  [<c0234f72>] bus_remove_device+0x52/0x70
  [<c023389c>] device_del+0x5c/0x1f0
  [<c0233a38>] device_unregister+0x8/0x10
  [<d98a6fdc>] i2c_detach_client+0x6c/0xf0 [i2c_core]
  [<d98a61fb>] i2c_device_remove+0x7b/0x90 [i2c_core]
  [<d98a63b5>] i2c_unregister_device+0x45/0x150 [i2c_core]
  [<c02b247b>] __mutex_lock_interruptible_slowpath+0xfb/0x290
  [<d9a976b5>] i2c_parport_detach+0xf5/0x120 [i2c_parport]
  [<d88651a5>] parport_unregister_driver+0x35/0x60 [parport]
  [<c0136109>] sys_delete_module+0x129/0x140

As you can see, i2c_device_remove is indeed called twice, once by this
part of i2c_unregister_device:

	/* unbind driver */
	if (driver) {
		/* REVISIT driver model core would handle this for us, and
		 * once i2c_detach_client() stops updating lists, it should.
		 */
		status = i2c_device_remove(&client->dev);
		if (status < 0) {
			dev_err(&client->dev, "unbind failed (%d)\n", status);
			return;
		}
	}

And a second time by the part right after:

	/* now remove the device, and free its memory */
	status = i2c_detach_client(client);
	if (status < 0)
		dev_err(&adap->dev, "detach failed (%d) for client [%s] "
			"at address 0x%02x\n",
			status, client->name, client->addr);
	else
		put_device(&client->dev);

It's obviously not correct to call i2c_device_remove explicitly and
then have i2c_detach_client call it again, but I am a bit confused on
how to fix it. Why do we need this explicit call to i2c_device_remove
in the first place, as it appears that i2c_detach_client does it just
fine? If we really want to do that, then I guess we need to set
client->dev.driver to NULL between the two blocks so that the second
call to i2c_device_remove takes the quick exit?

Thanks,
-- 
Jean Delvare