Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Andrew Morton]

It looks like the cpuidle patches are doing incorrect things
with their kobject protocol.


Begin forwarded message:

Date: Thu, 22 Mar 2007 09:24:38 -0700
From: "Miles Lane" <miles.lane@gmail.com>
To: "Andrew Morton" <akpm@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>, linux-wireless@vger.kernel.org
Subject: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f


I cannot reproduce the BUG with your ml.bz2 patch applied.
I am seeing this with both 2.6.21-rc4-mm1 + hotfixes, and with
2.6.21-rc4 + ml.bz2:

Mar 22 09:10:35 FractalPath kernel: ACPI: CPU0 (power states: C1[C1]
C2[C2] C3[C3])
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0x40/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0xd4/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0x168/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: cpuidle: using driver acpi_idle

Not sure if this is a problem.  Also, the first time I booted the ml.bz2
build, it hung.  I don't have netconsole or a serial debugging system
set up, so I have no idea what the problem was.  The second boot,
ipw2200 loaded with no errors, but NetworkManager wouldn't
connect until I removed and reinserted the module.

All the best,
           Miles

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Fri, 2007-03-23 at 01:52 +0800, Andrew Morton wrote:
> 
> It looks like the cpuidle patches are doing incorrect things
> with their kobject protocol.
Looks the goal to force kobject to be allocated dynamically is to
release the memory of the kobject. But in the cpuidle case, we don't
want to free the memory as it might be used soon. I saw a lot of similar
staff too, like 'cpu_devices' in arch/i386/kernel/topology.c.


Thanks,
Shaohua
> Begin forwarded message:
> 
> Date: Thu, 22 Mar 2007 09:24:38 -0700
> From: "Miles Lane" <miles.lane@gmail.com>
> To: "Andrew Morton" <akpm@linux-foundation.org>
> Cc: LKML <linux-kernel@vger.kernel.org>,
> linux-wireless@vger.kernel.org
> Subject: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle
> kernel paging request at virtual address 6b6b6ceb -- EIP is at
> module_put+0x7/0x1f
> 
> 
> I cannot reproduce the BUG with your ml.bz2 patch applied.
> I am seeing this with both 2.6.21-rc4-mm1 + hotfixes, and with
> 2.6.21-rc4 + ml.bz2:
> 
> Mar 22 09:10:35 FractalPath kernel: ACPI: CPU0 (power states: C1[C1]
> C2[C2] C3[C3])
> Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
> per_cpu__cpuidle_devices+0x40/0x558 is not dynamically allocated.
> Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
> per_cpu__cpuidle_devices+0xd4/0x558 is not dynamically allocated.
> Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
> per_cpu__cpuidle_devices+0x168/0x558 is not dynamically allocated.
> Mar 22 09:10:35 FractalPath kernel: cpuidle: using driver acpi_idle
> 
> Not sure if this is a problem.  Also, the first time I booted the
> ml.bz2
> build, it hung.  I don't have netconsole or a serial debugging system
> set up, so I have no idea what the problem was.  The second boot,
> ipw2200 loaded with no errors, but NetworkManager wouldn't
> connect until I removed and reinserted the module.
> 
> All the best,
>            Miles
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Fri, Mar 23, 2007 at 10:04:26AM +0800, Shaohua Li wrote:
> On Fri, 2007-03-23 at 01:52 +0800, Andrew Morton wrote:
> > 
> > It looks like the cpuidle patches are doing incorrect things
> > with their kobject protocol.
> Looks the goal to force kobject to be allocated dynamically is to
> release the memory of the kobject. But in the cpuidle case, we don't
> want to free the memory as it might be used soon.

What do you mean "used soon"?  Is this a time critical thing?

> I saw a lot of similar staff too, like 'cpu_devices' in
> arch/i386/kernel/topology.c.

The point is that kobjects should be created dynamically as they can be
referenced by different threads at different times, so you need to let
it manage the reference counting logic for you.  Don't create them
statically, it's almost always wrong.

Note, one valid use of static kobject is in struct bus_type and in some
class definitions.

So this should probably be fixed.

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Thu, 2007-03-22 at 22:16 -0700, Greg KH wrote:
> On Fri, Mar 23, 2007 at 10:04:26AM +0800, Shaohua Li wrote:
> > On Fri, 2007-03-23 at 01:52 +0800, Andrew Morton wrote:
> > > 
> > > It looks like the cpuidle patches are doing incorrect things
> > > with their kobject protocol.
> > Looks the goal to force kobject to be allocated dynamically is to
> > release the memory of the kobject. But in the cpuidle case, we don't
> > want to free the memory as it might be used soon.
> 
> What do you mean "used soon"?  Is this a time critical thing?
> 
> > I saw a lot of similar staff too, like 'cpu_devices' in
> > arch/i386/kernel/topology.c.
> 
> The point is that kobjects should be created dynamically as they can be
> referenced by different threads at different times, so you need to let
> it manage the reference counting logic for you.  Don't create them
> statically, it's almost always wrong.
> 
> Note, one valid use of static kobject is in struct bus_type and in some
> class definitions.
> 
> So this should probably be fixed.
Ok, below patch should fix the issue.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Index: rc4-mm1/drivers/acpi/processor_idle.c
===================================================================
--- rc4-mm1.orig/drivers/acpi/processor_idle.c	2007-03-23 08:58:51.000000000 +0800
+++ rc4-mm1/drivers/acpi/processor_idle.c	2007-03-28 09:23:24.000000000 +0800
@@ -623,7 +623,7 @@ int acpi_processor_cst_has_changed(struc
 		return -ENODEV;
 
 	acpi_processor_get_power_info(pr);
-	return cpuidle_force_redetect(&per_cpu(cpuidle_devices, pr->id));
+	return cpuidle_force_redetect(per_cpu(cpuidle_devices, pr->id));
 }
 
 /* proc interface */
Index: rc4-mm1/drivers/cpuidle/cpuidle.c
===================================================================
--- rc4-mm1.orig/drivers/cpuidle/cpuidle.c	2007-03-23 09:52:48.000000000 +0800
+++ rc4-mm1/drivers/cpuidle/cpuidle.c	2007-03-28 09:22:41.000000000 +0800
@@ -18,7 +18,7 @@
 
 #include "cpuidle.h"
 
-DEFINE_PER_CPU(struct cpuidle_device, cpuidle_devices);
+DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
 EXPORT_PER_CPU_SYMBOL_GPL(cpuidle_devices);
 
 DEFINE_MUTEX(cpuidle_lock);
@@ -34,7 +34,7 @@ static void (*pm_idle_old)(void);
  */
 static void cpuidle_idle_call(void)
 {
-	struct cpuidle_device *dev = &__get_cpu_var(cpuidle_devices);
+	struct cpuidle_device *dev = __get_cpu_var(cpuidle_devices);
 
 	struct cpuidle_state *target_state;
 	int next_state;
@@ -117,7 +117,7 @@ static int cpuidle_add_device(struct sys
 	int cpu = sys_dev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 
 	mutex_lock(&cpuidle_lock);
 	if (cpu_is_offline(cpu)) {
@@ -125,6 +125,16 @@ static int cpuidle_add_device(struct sys
 		return 0;
 	}
 
+	if (!dev) {
+		dev = kzalloc(sizeof(struct cpuidle_device), GFP_KERNEL);
+		if (!dev) {
+			mutex_unlock(&cpuidle_lock);
+			return -ENOMEM;
+		}
+		init_completion(&dev->kobj_unregister);
+		per_cpu(cpuidle_devices, cpu) = dev;
+	}
+
 	if (dev->status & CPUIDLE_STATUS_DETECTED) {
 		mutex_unlock(&cpuidle_lock);
 		return 0;
@@ -153,7 +163,7 @@ static int __cpuidle_remove_device(struc
 {
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, sys_dev->id);
+	dev = per_cpu(cpuidle_devices, sys_dev->id);
 
 	if (!(dev->status & CPUIDLE_STATUS_DETECTED)) {
 		return 0;
@@ -166,6 +176,9 @@ static int __cpuidle_remove_device(struc
 		cpuidle_detach_driver(dev);
 	cpuidle_remove_sysfs(sys_dev);
 	list_del(&dev->device_list);
+	wait_for_completion(&dev->kobj_unregister);
+	per_cpu(cpuidle_devices, sys_dev->id) = NULL;
+	kfree(dev);
 
 	return 0;
 }
Index: rc4-mm1/drivers/cpuidle/sysfs.c
===================================================================
--- rc4-mm1.orig/drivers/cpuidle/sysfs.c	2007-03-23 09:34:01.000000000 +0800
+++ rc4-mm1/drivers/cpuidle/sysfs.c	2007-03-28 11:45:56.000000000 +0800
@@ -210,8 +210,16 @@ static struct sysfs_ops cpuidle_sysfs_op
 	.store = cpuidle_store,
 };
 
+static void cpuidle_sysfs_release(struct kobject *kobj)
+{
+	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
+
+	complete(&dev->kobj_unregister);
+}
+
 static struct kobj_type ktype_cpuidle = {
 	.sysfs_ops = &cpuidle_sysfs_ops,
+	.release = cpuidle_sysfs_release,
 };
 
 struct cpuidle_state_attr {
@@ -265,9 +273,15 @@ static struct sysfs_ops cpuidle_state_sy
 	.show = cpuidle_state_show,
 };
 
+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}
+
 static struct kobj_type ktype_state_cpuidle = {
 	.sysfs_ops = &cpuidle_state_sysfs_ops,
 	.default_attrs = cpuidle_state_default_attrs,
+	.release = cpuidle_state_sysfs_release,
 };
 
 /**
@@ -319,7 +333,7 @@ int cpuidle_add_sysfs(struct sys_device 
 	int cpu = sysdev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 	dev->kobj.parent = &sysdev->kobj;
 	dev->kobj.ktype = &ktype_cpuidle;
 	kobject_set_name(&dev->kobj, "%s", "cpuidle");
@@ -335,6 +349,6 @@ void cpuidle_remove_sysfs(struct sys_dev
 	int cpu = sysdev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 	kobject_unregister(&dev->kobj);
 }
Index: rc4-mm1/include/linux/cpuidle.h
===================================================================
--- rc4-mm1.orig/include/linux/cpuidle.h	2007-03-23 09:55:13.000000000 +0800
+++ rc4-mm1/include/linux/cpuidle.h	2007-03-28 11:32:56.000000000 +0800
@@ -89,9 +89,7 @@ struct cpuidle_device {
 	void			*governor_data;
 };
 
-#define to_cpuidle_device(n) container_of(n, struct cpuidle_device, kobj);
-
-DECLARE_PER_CPU(struct cpuidle_device, cpuidle_devices);
+DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
 
 /* Device Status Flags */
 #define CPUIDLE_STATUS_DETECTED		 (0x1)

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

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!!!

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.

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Andrew Morton]

On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <greg@kroah.com> 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.

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

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@kroah.com> 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...

So, where do I need to put this information so that people stop trying
to be smarter than the kernel...

ugh.

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Andrew Morton]

On Tue, 27 Mar 2007 21:51:24 -0700 Greg KH <greg@kroah.com> wrote:

> So, where do I need to put this information so that people stop trying
> to be smarter than the kernel...

Documentation/driver-model/driver.txt?

And a pointer to a sample properly-written driver would help.

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

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@kroah.com> 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?

Thanks,
Shaohua

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Tue, Mar 27, 2007 at 10:09:04PM -0700, Andrew Morton wrote:
> On Tue, 27 Mar 2007 21:51:24 -0700 Greg KH <greg@kroah.com> wrote:
> 
> > So, where do I need to put this information so that people stop trying
> > to be smarter than the kernel...
> 
> Documentation/driver-model/driver.txt?
> 
> And a pointer to a sample properly-written driver would help.

Well, this is not something that a driver will ever have to do, it's
only people writing a driver subsystem, or messing with sysfs in
"different" ways.

But yes, I will do that, it is really needed...

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

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@kroah.com> 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?

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

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@kroah.com> 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

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > 
> > 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[];

Ick, don't do that...

> 	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.

Why not just use the "normal" parent/child relationship with the
kobjects like the rest of the kernel does?

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > 
> > > 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[];
> 
> Ick, don't do that...
why?
> > 	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.
> 
> Why not just use the "normal" parent/child relationship with the
> kobjects like the rest of the kernel does?
I still didn't get the reason why we couldn't do this in the way of my
patch. As I said, there isn't risk to use 'freed memory'. I can make the
'struct foo' a pointer, but this will mess the cpuidle driver.

Thanks,
Shaohua

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Wed, 2007-03-28 at 14:49 +0800, Shaohua Li wrote:
> On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > 
> > > > 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[];
> > 
> > Ick, don't do that...
> why?
Greg, can you share why don't do that? Or I can assume the patch is ok
to push to len?

> > > 	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.
> > 
> > Why not just use the "normal" parent/child relationship with the
> > kobjects like the rest of the kernel does?
> I still didn't get the reason why we couldn't do this in the way of my
> patch. As I said, there isn't risk to use 'freed memory'. I can make the
> 'struct foo' a pointer, but this will mess the cpuidle driver.
> 
> Thanks,
> Shaohua

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Wed, Mar 28, 2007 at 02:49:12PM +0800, Shaohua Li wrote:
> On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > 
> > > > 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[];
> > 
> > Ick, don't do that...
> why?
> > > 	kobject kobjb

Because you have multiple kobjects in the same object.

It's just that simple, the lifetime rules for such a thing is almost
impossible to track properly.  Don't do it!

> > > }
> > > 
> > > 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.
> > 
> > Why not just use the "normal" parent/child relationship with the
> > kobjects like the rest of the kernel does?
> I still didn't get the reason why we couldn't do this in the way of my
> patch. As I said, there isn't risk to use 'freed memory'. I can make the
> 'struct foo' a pointer, but this will mess the cpuidle driver.

Again, the main point is you can not have more than one reference count
for the same structure.  It just does not work at all.

So please, fix the code, it is broken.

And yes, I know of other places in the kernel (scsi stack...) that
violate this, but that only means that they are wrong, not that it is an
excuse for you to do it also.

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Thu, 2007-03-29 at 22:18 -0700, Greg KH wrote:
> On Wed, Mar 28, 2007 at 02:49:12PM +0800, Shaohua Li wrote:
> > On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > > 
> > > > > 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[];
> > > 
> > > Ick, don't do that...
> > why?
> > > > 	kobject kobjb
> 
> Because you have multiple kobjects in the same object.
> 
> It's just that simple, the lifetime rules for such a thing is almost
> impossible to track properly.  Don't do it!
> 
> > > > }
> > > > 
> > > > 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.
> > > 
> > > Why not just use the "normal" parent/child relationship with the
> > > kobjects like the rest of the kernel does?
> > I still didn't get the reason why we couldn't do this in the way of my
> > patch. As I said, there isn't risk to use 'freed memory'. I can make the
> > 'struct foo' a pointer, but this will mess the cpuidle driver.
> 
> Again, the main point is you can not have more than one reference count
> for the same structure.  It just does not work at all.
> 
> So please, fix the code, it is broken.
> 
> And yes, I know of other places in the kernel (scsi stack...) that
> violate this, but that only means that they are wrong, not that it is an
> excuse for you to do it also.
We don't use the kobject to track the reference count. But anyway, below
patch should make you happy.


Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Index: rc4-mm1/drivers/acpi/processor_idle.c
===================================================================
--- rc4-mm1.orig/drivers/acpi/processor_idle.c	2007-03-23 08:58:51.000000000 +0800
+++ rc4-mm1/drivers/acpi/processor_idle.c	2007-03-28 09:23:24.000000000 +0800
@@ -623,7 +623,7 @@ int acpi_processor_cst_has_changed(struc
 		return -ENODEV;
 
 	acpi_processor_get_power_info(pr);
-	return cpuidle_force_redetect(&per_cpu(cpuidle_devices, pr->id));
+	return cpuidle_force_redetect(per_cpu(cpuidle_devices, pr->id));
 }
 
 /* proc interface */
Index: rc4-mm1/drivers/cpuidle/cpuidle.c
===================================================================
--- rc4-mm1.orig/drivers/cpuidle/cpuidle.c	2007-03-23 09:52:48.000000000 +0800
+++ rc4-mm1/drivers/cpuidle/cpuidle.c	2007-03-28 09:22:41.000000000 +0800
@@ -18,7 +18,7 @@
 
 #include "cpuidle.h"
 
-DEFINE_PER_CPU(struct cpuidle_device, cpuidle_devices);
+DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
 EXPORT_PER_CPU_SYMBOL_GPL(cpuidle_devices);
 
 DEFINE_MUTEX(cpuidle_lock);
@@ -34,7 +34,7 @@ static void (*pm_idle_old)(void);
  */
 static void cpuidle_idle_call(void)
 {
-	struct cpuidle_device *dev = &__get_cpu_var(cpuidle_devices);
+	struct cpuidle_device *dev = __get_cpu_var(cpuidle_devices);
 
 	struct cpuidle_state *target_state;
 	int next_state;
@@ -117,7 +117,7 @@ static int cpuidle_add_device(struct sys
 	int cpu = sys_dev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 
 	mutex_lock(&cpuidle_lock);
 	if (cpu_is_offline(cpu)) {
@@ -125,6 +125,16 @@ static int cpuidle_add_device(struct sys
 		return 0;
 	}
 
+	if (!dev) {
+		dev = kzalloc(sizeof(struct cpuidle_device), GFP_KERNEL);
+		if (!dev) {
+			mutex_unlock(&cpuidle_lock);
+			return -ENOMEM;
+		}
+		init_completion(&dev->kobj_unregister);
+		per_cpu(cpuidle_devices, cpu) = dev;
+	}
+
 	if (dev->status & CPUIDLE_STATUS_DETECTED) {
 		mutex_unlock(&cpuidle_lock);
 		return 0;
@@ -153,7 +163,7 @@ static int __cpuidle_remove_device(struc
 {
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, sys_dev->id);
+	dev = per_cpu(cpuidle_devices, sys_dev->id);
 
 	if (!(dev->status & CPUIDLE_STATUS_DETECTED)) {
 		return 0;
@@ -166,6 +176,9 @@ static int __cpuidle_remove_device(struc
 		cpuidle_detach_driver(dev);
 	cpuidle_remove_sysfs(sys_dev);
 	list_del(&dev->device_list);
+	wait_for_completion(&dev->kobj_unregister);
+	per_cpu(cpuidle_devices, sys_dev->id) = NULL;
+	kfree(dev);
 
 	return 0;
 }
Index: rc4-mm1/drivers/cpuidle/sysfs.c
===================================================================
--- rc4-mm1.orig/drivers/cpuidle/sysfs.c	2007-03-23 09:34:01.000000000 +0800
+++ rc4-mm1/drivers/cpuidle/sysfs.c	2007-03-30 14:22:50.000000000 +0800
@@ -210,8 +210,16 @@ static struct sysfs_ops cpuidle_sysfs_op
 	.store = cpuidle_store,
 };
 
+static void cpuidle_sysfs_release(struct kobject *kobj)
+{
+	struct cpuidle_device *dev = kobj_to_cpuidledev(kobj);
+
+	complete(&dev->kobj_unregister);
+}
+
 static struct kobj_type ktype_cpuidle = {
 	.sysfs_ops = &cpuidle_sysfs_ops,
+	.release = cpuidle_sysfs_release,
 };
 
 struct cpuidle_state_attr {
@@ -246,7 +254,8 @@ static struct attribute *cpuidle_state_d
 	NULL
 };
 
-#define kobj_to_state(k) container_of(k, struct cpuidle_state, kobj)
+#define kobj_to_state_obj(k) container_of(k, struct cpuidle_state_kobj, kobj)
+#define kobj_to_state(k) (kobj_to_state_obj(k)->state)
 #define attr_to_stateattr(a) container_of(a, struct cpuidle_state_attr, attr)
 static ssize_t cpuidle_state_show(struct kobject * kobj,
 	struct attribute * attr ,char * buf)
@@ -265,11 +274,27 @@ static struct sysfs_ops cpuidle_state_sy
 	.show = cpuidle_state_show,
 };
 
+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	struct cpuidle_state_kobj *state_obj = kobj_to_state_obj(kobj);
+
+	complete(&state_obj->kobj_unregister);
+}
+
 static struct kobj_type ktype_state_cpuidle = {
 	.sysfs_ops = &cpuidle_state_sysfs_ops,
 	.default_attrs = cpuidle_state_default_attrs,
+	.release = cpuidle_state_sysfs_release,
 };
 
+static void inline cpuidle_free_state_kobj(struct cpuidle_device *device, int i)
+{
+	kobject_unregister(&device->kobjs[i]->kobj);
+	wait_for_completion(&device->kobjs[i]->kobj_unregister);
+	kfree(device->kobjs[i]);
+	device->kobjs[i] = NULL;
+}
+
 /**
  * cpuidle_add_driver_sysfs - adds driver-specific sysfs attributes
  * @device: the target device
@@ -277,24 +302,32 @@ static struct kobj_type ktype_state_cpui
 int cpuidle_add_driver_sysfs(struct cpuidle_device *device)
 {
 	int i, ret;
-	struct cpuidle_state *state;
+	struct cpuidle_state_kobj *kobj;
 
 	/* state statistics */
 	for (i = 0; i < device->state_count; i++) {
-		state = &device->states[i];
-		state->kobj.parent = &device->kobj;
-		state->kobj.ktype = &ktype_state_cpuidle;
-		kobject_set_name(&state->kobj, "state%d", i);
-		ret = kobject_register(&state->kobj);
-		if (ret)
+		kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL);
+		if (!kobj)
+			goto error_state;
+		kobj->state = &device->states[i];
+		init_completion(&kobj->kobj_unregister);
+
+		kobj->kobj.parent = &device->kobj;
+		kobj->kobj.ktype = &ktype_state_cpuidle;
+		kobject_set_name(&kobj->kobj, "state%d", i);
+		ret = kobject_register(&kobj->kobj);
+		if (ret) {
+			kfree(kobj);
 			goto error_state;
+		}
+		device->kobjs[i] = kobj;
 	}
 
 	return 0;
 
 error_state:
 	for (i = i - 1; i >= 0; i--)
-		kobject_unregister(&device->states[i].kobj);
+		cpuidle_free_state_kobj(device, i);
 	return ret;
 }
 
@@ -307,7 +340,7 @@ void cpuidle_remove_driver_sysfs(struct 
 	int i;
 
 	for (i = 0; i < device->state_count; i++)
-		kobject_unregister(&device->states[i].kobj);
+		cpuidle_free_state_kobj(device, i);
 }
 
 /**
@@ -319,7 +352,7 @@ int cpuidle_add_sysfs(struct sys_device 
 	int cpu = sysdev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 	dev->kobj.parent = &sysdev->kobj;
 	dev->kobj.ktype = &ktype_cpuidle;
 	kobject_set_name(&dev->kobj, "%s", "cpuidle");
@@ -335,6 +368,6 @@ void cpuidle_remove_sysfs(struct sys_dev
 	int cpu = sysdev->id;
 	struct cpuidle_device *dev;
 
-	dev = &per_cpu(cpuidle_devices, cpu);
+	dev = per_cpu(cpuidle_devices, cpu);
 	kobject_unregister(&dev->kobj);
 }
Index: rc4-mm1/include/linux/cpuidle.h
===================================================================
--- rc4-mm1.orig/include/linux/cpuidle.h	2007-03-23 09:55:13.000000000 +0800
+++ rc4-mm1/include/linux/cpuidle.h	2007-03-30 14:05:28.000000000 +0800
@@ -41,8 +41,6 @@ struct cpuidle_state {
 
 	int (*enter)	(struct cpuidle_device *dev,
 			 struct cpuidle_state *state);
-
-	struct kobject	kobj;
 };
 
 /* Idle State Flags */
@@ -74,6 +72,12 @@ cpuidle_set_statedata(struct cpuidle_sta
 	state->driver_data = data;
 }
 
+struct cpuidle_state_kobj {
+	struct cpuidle_state *state;
+	struct completion kobj_unregister;
+	struct kobject kobj;
+};
+
 struct cpuidle_device {
 	unsigned int		status;
 	int			cpu;
@@ -81,6 +85,7 @@ struct cpuidle_device {
 	int			last_residency;
 	int			state_count;
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
+	struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
 	struct cpuidle_state	*last_state;
 
 	struct list_head 	device_list;
@@ -89,9 +94,7 @@ struct cpuidle_device {
 	void			*governor_data;
 };
 
-#define to_cpuidle_device(n) container_of(n, struct cpuidle_device, kobj);
-
-DECLARE_PER_CPU(struct cpuidle_device, cpuidle_devices);
+DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
 
 /* Device Status Flags */
 #define CPUIDLE_STATUS_DETECTED		 (0x1)

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Fri, Mar 30, 2007 at 02:33:44PM +0800, Shaohua Li wrote:
> On Thu, 2007-03-29 at 22:18 -0700, Greg KH wrote:
> > On Wed, Mar 28, 2007 at 02:49:12PM +0800, Shaohua Li wrote:
> > > On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > > > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > > > 
> > > > > > 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[];
> > > > 
> > > > Ick, don't do that...
> > > why?
> > > > > 	kobject kobjb
> > 
> > Because you have multiple kobjects in the same object.
> > 
> > It's just that simple, the lifetime rules for such a thing is almost
> > impossible to track properly.  Don't do it!
> > 
> > > > > }
> > > > > 
> > > > > 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.
> > > > 
> > > > Why not just use the "normal" parent/child relationship with the
> > > > kobjects like the rest of the kernel does?
> > > I still didn't get the reason why we couldn't do this in the way of my
> > > patch. As I said, there isn't risk to use 'freed memory'. I can make the
> > > 'struct foo' a pointer, but this will mess the cpuidle driver.
> > 
> > Again, the main point is you can not have more than one reference count
> > for the same structure.  It just does not work at all.
> > 
> > So please, fix the code, it is broken.
> > 
> > And yes, I know of other places in the kernel (scsi stack...) that
> > violate this, but that only means that they are wrong, not that it is an
> > excuse for you to do it also.
> We don't use the kobject to track the reference count.

Then what do you use it for?

And it doesn't matter if you use it or not, the reference count _is_
used by the kobject and sysfs code, so you have to be aware of it.

> But anyway, below patch should make you happy.

I'm still confused as to why you are creating these "extra" kobjects.
What are they used for?

But yes, it is "better" in that you are abiding by the reference count
rules properly, although your completion handling seems like it might
get into a deadlock, but I'll trust you that it works properly :)

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Fri, 2007-03-30 at 00:05 -0700, Greg KH wrote:
> On Fri, Mar 30, 2007 at 02:33:44PM +0800, Shaohua Li wrote:
> > On Thu, 2007-03-29 at 22:18 -0700, Greg KH wrote:
> > > On Wed, Mar 28, 2007 at 02:49:12PM +0800, Shaohua Li wrote:
> > > > On Tue, 2007-03-27 at 22:58 -0700, Greg KH wrote:
> > > > > On Wed, Mar 28, 2007 at 01:39:26PM +0800, Shaohua Li wrote:
> > > > > > On Tue, 2007-03-27 at 22:27 -0700, Greg KH wrote:
> > > > > > > 
> > > > > > > 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[];
> > > > > 
> > > > > Ick, don't do that...
> > > > why?
> > > > > > 	kobject kobjb
> > > 
> > > Because you have multiple kobjects in the same object.
> > > 
> > > It's just that simple, the lifetime rules for such a thing is almost
> > > impossible to track properly.  Don't do it!
> > > 
> > > > > > }
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > Why not just use the "normal" parent/child relationship with the
> > > > > kobjects like the rest of the kernel does?
> > > > I still didn't get the reason why we couldn't do this in the way of my
> > > > patch. As I said, there isn't risk to use 'freed memory'. I can make the
> > > > 'struct foo' a pointer, but this will mess the cpuidle driver.
> > > 
> > > Again, the main point is you can not have more than one reference count
> > > for the same structure.  It just does not work at all.
> > > 
> > > So please, fix the code, it is broken.
> > > 
> > > And yes, I know of other places in the kernel (scsi stack...) that
> > > violate this, but that only means that they are wrong, not that it is an
> > > excuse for you to do it also.
> > We don't use the kobject to track the reference count.
> 
> Then what do you use it for?
> 
> And it doesn't matter if you use it or not, the reference count _is_
> used by the kobject and sysfs code, so you have to be aware of it.
> 
> > But anyway, below patch should make you happy.
> 
> I'm still confused as to why you are creating these "extra" kobjects.
> What are they used for?
It's for each idle state.

> But yes, it is "better" in that you are abiding by the reference count
> rules properly, although your completion handling seems like it might
> get into a deadlock, but I'll trust you that it works properly :)
Yes, it works well in my test.

Thanks,
Shaohua

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Greg KH]

On Fri, Mar 30, 2007 at 03:08:03PM +0800, Shaohua Li wrote:
> On Fri, 2007-03-30 at 00:05 -0700, Greg KH wrote:
> > 
> > I'm still confused as to why you are creating these "extra" kobjects.
> > What are they used for?
> It's for each idle state.

Where are these states in sysfs?  Is this the
/sys/devices/system/cpu/cpu0/cache/indexN directories?

thanks,

greg k-h

Re: Fw: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

[Shaohua Li]

On Fri, 2007-03-30 at 00:51 -0700, Greg KH wrote:
> On Fri, Mar 30, 2007 at 03:08:03PM +0800, Shaohua Li wrote:
> > On Fri, 2007-03-30 at 00:05 -0700, Greg KH wrote:
> > > 
> > > I'm still confused as to why you are creating these "extra" kobjects.
> > > What are they used for?
> > It's for each idle state.
> 
> Where are these states in sysfs?  Is this the
> /sys/devices/system/cpu/cpu0/cache/indexN directories?
Similar with cache info, the sysfs is
/sys/devices/system/cpu/cpu1/cpuidle/statex

Thanks,
Shaohua