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

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

On Tue, 8 Jan 2008 11:12:46 -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > 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. 
> 
> Lockup?  More details ...

There's not much to say. "rmmod lm90" (or "rmmod i2c-parport") never
returns, that's about it.

> > 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
> 
> What's it doing at that line in the call?

(From another run, thus the slightly different address:)

(gdb) list *(sysfs_addrm_finish+0x191)
0xffffffff802bdc01 is in sysfs_addrm_finish (fs/sysfs/sysfs.h:138).
133     }
134
135     static inline void sysfs_put(struct sysfs_dirent *sd)
136     {
137             if (sd && atomic_dec_and_test(&sd->s_count))
138                     release_sysfs_dirent(sd);
139     }
140
141     /*
142      * inode.c
(gdb)

If you need more info, feel free to ask.

> >   [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
> 
> This looks like stack garbage...

What makes you think so? A second stack trace shows it as well.

> 
> >   [<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?
> 
> As you point out below, it's supposed to be safe ...
> 
> 
> > 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...
> 
> Yeah, but then again ... the i2c stack does that oddball stuff with
> complete(&client->released) for legacy drivers, too.  That's always
> been contrary to what the driver model expects, and I never quite
> bothered to sort out the issues.  Maybe this is one of them (sigh).

I see. I doubt I'll be of much help, as I could never figure out what
this completion stuff was for in the first place.

-- 
Jean Delvare