Message-ID: <20080331220906.GA28171@kroah.com>
Date:	Mon, 31 Mar 2008 15:09:06 -0700
From:	Greg KH <greg@...ah.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Kay Sievers <kay.sievers@...y.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Bj?rn Steinbrink <B.Steinbrink@....de>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Johannes Berg <johannes@...solutions.net>,
	Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH] evdev: Release eventual input device grabs when
	getting disconnected

On Mon, Mar 31, 2008 at 04:57:36PM -0400, Dmitry Torokhov wrote:
> On Mon, Mar 31, 2008 at 01:42:21PM -0700, Greg KH wrote:
> > On Mon, Mar 31, 2008 at 02:01:20PM -0400, Dmitry Torokhov wrote:
> > > On Mon, Mar 31, 2008 at 10:28:13AM -0700, Greg KH wrote:
> > > > On Mon, Mar 31, 2008 at 02:15:39AM -0400, Dmitry Torokhov wrote:
> > > > > Hi Linus,
> > > > > 
> > > > > On Sunday 30 March 2008, Linus Torvalds wrote:
> > > > > > 
> > > > > > On Sun, 30 Mar 2008, Bj?rn Steinbrink wrote:
> > > > > > >
> > > > > > > I can't reproduce the bug on my UP box and currently can't afford
> > > > > > > crashing my SMP box (all the oopses seem to come from SMP kernels, so I
> > > > > > > guess it needs SMP to crash), so while this doesn't show any new
> > > > > > > problems, I can't tell whether it actually fixes anything. Testers
> > > > > > > welcome!
> > > > > > 
> > > > > > Ok, I applied this because I will do an -rc8 today or tomorrow, but I 
> > > > > > really really hope somebody can figure out what made this all start to 
> > > > > > trigger. It does smell like some core device layer change, because we do 
> > > > > > not seem to have a lot of changes since 2.6.24 in evdev.c and input.c that 
> > > > > > seem relevant.
> > > > > > 
> > > > > > Greg, are there any refcounting changes that would cause the input devices 
> > > > > > to be free'd earlier or something?
> > > > > > 
> > > > > 
> > > > > The following commit changed lifetime runes on kobjects breaking input:
> > > > > 
> > > > > commit 0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806
> > > > > Author: Kay Sievers <kay.sievers@...y.org>
> > > > > Date:   Wed Dec 19 01:40:42 2007 +0100
> > > > > 
> > > > >     Kobject: auto-cleanup on final unref
> > > > > 
> > > > >     We save the current state in the object itself, so we can do proper
> > > > >     cleanup when the last reference is dropped.
> > > > > 
> > > > >     If the initial reference is dropped, the object will be removed from
> > > > >     sysfs if needed, if an "add" event was sent, "remove" will be send, and
> > > > >     the allocated resources are released.
> > > > > 
> > > > >     This allows us to clean up some driver core usage as well as allowing us
> > > > >     to do other such changes to the rest of the kernel.
> > > > > 
> > > > >     Signed-off-by: Kay Sievers <kay.sievers@...y.org>
> > > > >     Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>
> > > > > 
> > > > > Before we dropped reference to kobject's parent only when child kobject
> > > > > was released (in kobject_cleanup). The changeset above moves the release
> > > > > to kobject_del() which is way too early in my opinion. The kobject is only
> > > > > marked for deletion at that time, not really deleted.
> > > > 
> > > > It was "deleted" from sysfs, and should have never been used again by
> > > > any callers.  If the reference count was dropped to zero with this call,
> > > > it would be cleaned up as well, it seems that you were assuming that it
> > > > would not be?  Perhaps you just need to grab another reference as this
> > > > would have caused you problems without this change anyway, but without
> > > > slab debugging, you never saw it.
> > > > 
> > > 
> > > Greg, please look at the change again. Before kobject_put(kobj->parent)
> > > was done in kobject_cleanup() and so the parent would only be freed when
> > > all its children are gone. Now parent is deleted early, even if its
> > > children are still referenced by other users. This is lifetime rule
> > > change and should really be announced as such.
> > 
> > Ugh, this was done because of scsi, they required that if you really
> > were deleting the parent, you wanted it gone.
> > 
> 
> Gone from sysfs or gone from memory?
> 
> > > If this change it intentional and is here to stay then I will just grab
> > > the references myself, although I wonder what else might be broken by
> > > it.
> > 
> > Yes, if you need those references, you are going to have to hold on for
> > them, the kobject layer will not keep them around.  It now is a "does
> > what you ask for" type model :)
> > 
> > I fail to see where this affects the input code though, in glancing at
> > it, it looks like you are doing things properly.  Kay, any thoughts
> > here, I think you looked at the kobject input layer interaction in the
> > past.
> >
> 
> Ok, I really liked the old behavior better, but if it is to stay then
> we need something like this (not for inclusion yet as mousedev and joydev
> need to be adjusted as well):

Yes, that's the proper behavior anyway, as you are passing off a pointer
to a device, you need to keep the reference to that device around until
you are finished with it.

I'm amazed that this wasn't causing a problem before the kobject change,
as this should have been needed there as well.  Would running with slab
debugging cause it to hit then?

> Signed-off-by: Dmitry Torokhov <dtor@...l.ru>

Acked-by: Greg Kroah-Hartman <gregkh@...e.de>

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists