On Wed, Oct 27, 2010 at 18:17:11 -0700, Greg KH wrote:
> On Wed, Oct 27, 2010 at 10:46:42AM -0400, Emilio G. Cota wrote:
> > On Wed, Oct 27, 2010 at 11:54:55 +0100, Martyn Welch wrote:
> > > I guess this is an artifact of the current lack of refcounting?
> >
> > No, that's orthogonal to this. This has to do with the way the
> > devices are allocated.
> >
> > > This is definitely an issue, however I don't think masking it by adding
> > > an empty function is the correct way to go.
> >
> > We're not masking anything. The release method is there to free the
> > struct it protects when its refcount goes to zero; however, in this
> > case the struct wasn't allocated dynamically--the 32 devices are
> > stored in struct vme_bridge in an array--and therefore there's
> > nothing to do in .release, since struct vme_bridge is freed
> > elsewhere.
> >
> > While it's true that empty .release methods are usually frowned
> > upon (as stated in Documentation/kobject.txt), due to the way
> > things are done here it actually makes sense to have an
> > empty .release.
>
> FROWNED APON?
>
> Heh, if you add one, as per the documentation there, I get to publicly
> ridicule you for doing so.
>
> So consider this your ridicule, if you ever are thinking you need to
> create an empty release function, YOUR CODE IS WRONG!
>
> Seriously, do you think I just add warnings in there for fun? So that
> you can work around them thinking you know better?
>
> {sigh}
>
> Your implementation is broken, never do this, if you create a kobject,
> you HAVE TO FREE IT in the release function.
>
> I would ask why you are even using a kobject in the first place (hint,
> if you are writing a driver, or even a subsystem, you shouldn't be, use
> 'struct device' instead.)
kobject?
We were talking about a struct device's .release, not about a struct
kobject's .release.
The warning comes from the kobject embedded in struct device, not
from a kobject per se. In fact, let me make it clearer:
NO KOBJECT IS DIRECTLY OPERATED ON.
proof:
$ find staging/vme -name '*.[ch]' | xargs grep -F 'kobj'
brings no results.
In the current implementation, there's no need to do anything(*) in
struct device's .release, because struct device is, as I said
in my message, freed somewhere else.
(*) Of course this is a bug, but it's a known bug, see below.
All struct device's are freed when struct vme_bridge is freed, and
before that happens, device_unregister() is called for each of the
devices:
@@ void vme_unregister_bridge(struct vme_bridge *bridge)
...
for (i = 0; i < VME_SLOTS_MAX; i++) {
dev = &bridge->dev[i];
device_unregister(dev);
}
device_unregister(), as you know, calls device_free() which
then calls kobj_del(), deleting the kobject embedded in struct
device. This call to kobj_del() from device_free() is the one
that throws the warning.
> So consider this a rejection of this patch and implementation :)
I don't like the implementation either, and that's why I sent
another patch that fixes this and some other problems--
see patch 27.
Fact: Anything other than freeing struct device in .release is
a bug. So the warning is very well placed there, no question
about it.
Example: when a VME bridge is removed, then all devices are
mercilessly freed, even if their refcounts aren't 0 because
drivers are controlling them. This is a BUG, and both Martin
and I knew it.
Fact: Right now VME drivers MUST be removed before VME
bridges are removed. We all agree on that this is broken--
bridges shouldn't be allowed to be removed if there are
drivers controlling any of their devices. Patch 27
addresses this among other things.
Why did I sent this patch? With it and with some other patches
I wanted to point out some flaws of the current implementation.
However after your email I think sending patch 27 directly
would have been a wiser option--although now we all probably
understand the code a little bit better.
So please let's focus on the important stuff, which is patch
27.
Emilio, Ridiculous In Chief
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
