On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> On Wed, Mar 28, 2007 at 01:13:56PM +0800, Shaohua Li wrote:
> > On Tue, 2007-03-27 at 21:51 -0700, Greg KH wrote:
> > > On Tue, Mar 27, 2007 at 09:27:19PM -0700, Andrew Morton wrote:
> > > > On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <greg@xxxxxxxxx> wrote:
> > > >
> > > > > On Wed, Mar 28, 2007 at 11:52:33AM +0800, Shaohua Li wrote:
> > > > > > +static void cpuidle_state_sysfs_release(struct kobject *kobj)
> > > > > > +{
> > > > > > + /* Nothing required to do here, just workaround kobject warning*/
> > > > > > +}
> > > > >
> > > > > NO!!!
> > > >
> > > > heh. This happens rather a lot.
> > > >
> > > > > Do people think that I add warnings to the kernel so that people can
> > > > > just work around them by passing empty functions to the driver core?
> > > > >
> > > > > Sometimes I wonder why I even try...
> > > > >
> > > > > Please fix this code, it is wrong.
> > > > >
> > > >
> > > > Is it documented anywhere? I always forget the reasoning so I have to
> > > > cc you each time I see it happening.
> > >
> > > The fact that the kernel itself spits out a message saying:
> > > "Device '%s' does not have a release() function, it is broken
> > > and must be fixed."
> > >
> > > isn't enough? What should I change it to, something like this:
> > >
> > > "Device '%s' does not have a release() function, it is broken
> > > and must be fixed, and if you just provide an empty function to
> > > remove this warning, rabid echidnas will track you down and
> > > puncture your spleen."
> > >
> > >
> > > The reason for this is that the device is a reference counted object,
> > > and you have to free the memory in the release function, otherwise you
> > > can easily be accessing memory that is freed already.
> > >
> > > Now note, this patch used a completion function to wait for the object
> > > to be destroyed, which isn't the nicest. But if you are going to do
> > > this, do it in the release function, like the code did a bit earlier.
> > >
> > > And if you are doing this because you have two kobjects in the same
> > > structure, well, your code is so messed up beyond belief that it needs
> > > to be fixed. And yes scsi developers, I'm pointing at you...
> > The two kobjects will be removed in the maintime. As I wait for one
> > object to be destroyed (the two objects are in one structure, and one
> > object's release will free the memory), we haven't the 'using freed
> > memory' issue here. Do we still need a .release for the kobject you
> > pointed out?
>
> Putting more than one kobject in the same structure is a broken design.
> How can you control the lifetime rules properly if there are two
> reference counts for the same structure? It doesn't work.
>
> If you really need something like this, then just use a pointer to a
> kobject for one of them instead of embedding it. Why do you need two
> different kobjects here?
Our data structure is something like below:
struct foo {
kobject kobja;
}
struct bar {
struct foo foo[];
kobject kobjb
}
kobjb's .release will free struct bar. kobjb is the parent of kobja. if
you have a reference on kobja, then kobjb can't be released too, right?
So we only kobjb provide a .release to free the memory, kobja's .release
isn't required.
Thanks,
Shaohua
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
