From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Delvare Subject: Re: i2c-remove-redundant-i2c_client-list.patch Date: Tue, 8 Jan 2008 21:50:42 +0100 Message-ID: <20080108215042.534e32fa@hyperion.delvare> References: <20071216052308.A0FB11668D7@adsl-69-226-248-13.dsl.pltn13.pacbell.net> <20080108151817.35e05c6c@hyperion.delvare> <20080108172001.33de6afc@hyperion.delvare> <200801081112.46972.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <200801081112.46972.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Errors-To: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org To: David Brownell Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org List-Id: linux-i2c@vger.kernel.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: > > [] 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. > > [] schedule_timeout+0x95/0xd0 > > This looks like stack garbage... What makes you think so? A second stack trace shows it as well. > > > [] remove_dir+0x30/0x40 > > [] sysfs_remove_dir+0x56/0x70 > > [] wait_for_common+0xc8/0x130 > > [] default_wake_function+0x0/0x10 > > [] device_del+0x1b5/0x290 > > [] i2c_detach_client+0x4e/0x90 > > [] :lm90:lm90_detach_client+0x71/0xa0 > > [] detach_all_clients+0x6f/0xb0 > > [] detach_all_clients+0x0/0xb0 > > [] device_for_each_child+0x2d/0x60 > > [] i2c_del_adapter+0xa5/0x140 > > [] :i2c_parport:i2c_parport_detach+0x55/0x110 > > [] :parport:parport_unregister_driver+0x46/0x80 > > [] sys_delete_module+0x141/0x1e0 > > [] 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