Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi David,

On Tue, 8 Jan 2008 15:18:17 +0100, Jean Delvare wrote:
> On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> > Though I was a bit worried it might have bugs in it, since all I had
> > time to do was code it, and sanity check it with a couple variants of
> > one new-style config.  (I no longer have those monster "build all I2C
> > drivers" configs around.)
> 
> I'll give it good testing, don't worry too much about that.

Hmm, I get a lockup when removing any legacy chip driver or any bus
driver with legacy clients attached. It seems to remove the 1st client
(no longer visible in sysfs) but never goes on with the second one.

Call Trace:
  [<ffffffff802bdbd1>] sysfs_addrm_finish+0x191/0x1e0
  [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
  [<ffffffff802bdc50>] remove_dir+0x30/0x40
  [<ffffffff802bdcb6>] sysfs_remove_dir+0x56/0x70
  [<ffffffff8041bab8>] wait_for_common+0xc8/0x130
  [<ffffffff80227e40>] default_wake_function+0x0/0x10
  [<ffffffff80386e55>] device_del+0x1b5/0x290
  [<ffffffff803ae4ce>] i2c_detach_client+0x4e/0x90
  [<ffffffff882f0071>] :lm90:lm90_detach_client+0x71/0xa0
  [<ffffffff803ae8ff>] detach_all_clients+0x6f/0xb0
  [<ffffffff803ae890>] detach_all_clients+0x0/0xb0
  [<ffffffff8038674d>] device_for_each_child+0x2d/0x60
  [<ffffffff803af315>] i2c_del_adapter+0xa5/0x140
  [<ffffffff8800b285>] :i2c_parport:i2c_parport_detach+0x55/0x110
  [<ffffffff88000796>] :parport:parport_unregister_driver+0x46/0x80
  [<ffffffff8024f001>] sys_delete_module+0x141/0x1e0
  [<ffffffff8020b92e>] system_call+0x7e/0x83

Are you certain that it is safe to remove a device's child while
walking the list of children? device_for_each_child() relies on a
klist_iter, and the documentation of klist_next mentions increasing the
reference count of the "active" list item. I would guess that you can't
remove a list item while its reference count is not zero, and that
would explain the lockup. The original code used list_for_each_safe(),
which explicitly allows the removal of the items while walking the list.

OTOH the header comment of lib/klist.c says:

 *	The entire point is to provide an interface for iterating over a list
 *	that is safe and allows for modification of the list during the
 *	iteration (e.g. insertion and removal), including modification of the
 *	current node on the list.

So it is supposed to work somehow...

-- 
Jean Delvare