Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Alexey Dobriyan]

Steps to reproduce:

	# modprobe p4-clockmod
	$ cd /sys/devices/system/cpu/cpu0/cpufreq/
	# rmmod p4-clockmod
	$ cat stats/time_in_state
	Segmentation fault

p4-clockmod: P4/Xeon(TM) CPU On-Demand Clock Modulation available
BUG: unable to handle kernel paging request at virtual address 6b6b6b6f
 printing eip:
c01906b4
*pde = 00000000
Oops: 0000 [#1]
PREEMPT
Modules linked in: speedstep_lib ohci_hcd af_packet e1000 ehci_hcd uhci_hcd usbcore xfs
CPU:    0
EIP:    0060:[<c01906b4>]    Not tainted VLI
EFLAGS: 00010202   (2.6.21-rc4-5851fadce8824d5d4b8fd02c22ae098401f6489e #2)
EIP is at sysfs_open_file+0xd3/0x259
eax: 6b6b6b6b   ebx: 00000000   ecx: f371b000   edx: 00000000
esi: c03a7330   edi: f35c8408   ebp: f371c9b0   esp: f371bee0
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process cat (pid: 6876, ti=f371b000 task=c192f550 task.ti=f371b000)
Stack: f3713b2c c0393400 00000000 f371c9b0 00000000 f3713b2c c01905e1 c0158575
       c18d3208 f71c4cd8 00000000 f371c9b0 00008000 f371bf38 ffffff9c c0158702
       f371c9b0 00000000 00000000 c0158744 00000000 f371bf38 f71c4cd8 c18d3208
Call Trace:
 [<c01905e1>] sysfs_open_file+0x0/0x259
 [<c0158575>] __dentry_open+0xec/0x1ec
 [<c0158702>] nameidata_to_filp+0x31/0x3a
 [<c0158744>] do_filp_open+0x39/0x40
 [<c0158478>] get_unused_fd+0xa1/0xb2
 [<c02d4cde>] _spin_unlock+0x25/0x3b
 [<c0158478>] get_unused_fd+0xa1/0xb2
 [<c0158785>] do_sys_open+0x3a/0x6d
 [<c01587f3>] sys_open+0x1c/0x20
 [<c0103e72>] sysenter_past_esp+0x5f/0x99
 =======================
Code: 00 f0 ff ff 8b 40 08 a8 08 0f 85 95 01 00 00 bb ed ff ff ff 8b 44 24 08 85 c0 0f 84 f8 00 00 00 8b 47 28 85 c0 0f 84 34 01 00 00 <8b> 40 04 85 c0 0f 84 29 01 00 00 8b 58 04 85 db 0f 84 0f 01 00
EIP: [<c01906b4>] sysfs_open_file+0xd3/0x259 SS:ESP 0068:f371bee0
Slab corruption: start=f35c83bc, len=256
Redzone: 0x5a2cf071/0x5a2cf071.
Last user: [<c0181d54>](load_elf_binary+0x161b/0x1ac7)
060: 6b 6b 6b 6b 6c 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
Prev obj: start=f35c82b0, len=256
Redzone: 0x170fc2a5/0x170fc2a5.
Last user: [<c0180758>](load_elf_binary+0x1f/0x1ac7)
000: 7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00
010: 02 00 03 00 01 00 00 00 b0 85 04 08 34 00 00 00
Next obj: start=f35c84c8, len=256
Redzone: 0x170fc2a5/0x170fc2a5.
Last user: [<c01807ea>](load_elf_binary+0xb1/0x1ac7)
000: 06 00 00 00 34 00 00 00 34 80 04 08 34 80 04 08
010: 00 01 00 00 00 01 00 00 05 00 00 00 04 00 00 00

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Greg KH]

On Mon, Mar 19, 2007 at 06:30:13PM +0300, Alexey Dobriyan wrote:
> Steps to reproduce:
> 
> 	# modprobe p4-clockmod
> 	$ cd /sys/devices/system/cpu/cpu0/cpufreq/
> 	# rmmod p4-clockmod
> 	$ cat stats/time_in_state
> 	Segmentation fault

Has this always happened?  Or is it new?

thanks,

greg k-h

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Alexey Dobriyan]

On Mon, Mar 19, 2007 at 01:41:25PM -0700, Greg KH wrote:
> On Mon, Mar 19, 2007 at 06:30:13PM +0300, Alexey Dobriyan wrote:
> > Steps to reproduce:
> >
> > 	# modprobe p4-clockmod
> > 	$ cd /sys/devices/system/cpu/cpu0/cpufreq/
> > 	# rmmod p4-clockmod
> > 	$ cat stats/time_in_state
> > 	Segmentation fault
>
> Has this always happened?  Or is it new?

I've checked 2.6.17 and up and it happens too.

Some .config peculiarities:

	CONFIG_CPU_FREQ=y
	CONFIG_CPU_FREQ_STAT=y
	CONFIG_X86_P4_CLOCKMOD=m

After modprobe/rmmod cpufreq/stats directory appears but doesn't get
removed. Should it?

/sys/devices/system/cpu/cpu0/cpufreq $ ls -lR
.:
total 0
drwxr-xr-x 2 root root 0 2007-03-20 12:59 stats

./stats:
total 0
-r--r--r-- 1 root root 4096 2007-03-20 12:59 time_in_state
-r--r--r-- 1 root root 4096 2007-03-20 12:59 total_trans

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Greg KH]

On Tue, Mar 20, 2007 at 01:06:34PM +0300, Alexey Dobriyan wrote:
> On Mon, Mar 19, 2007 at 01:41:25PM -0700, Greg KH wrote:
> > On Mon, Mar 19, 2007 at 06:30:13PM +0300, Alexey Dobriyan wrote:
> > > Steps to reproduce:
> > >
> > > 	# modprobe p4-clockmod
> > > 	$ cd /sys/devices/system/cpu/cpu0/cpufreq/
> > > 	# rmmod p4-clockmod
> > > 	$ cat stats/time_in_state
> > > 	Segmentation fault
> >
> > Has this always happened?  Or is it new?
> 
> I've checked 2.6.17 and up and it happens too.
> 
> Some .config peculiarities:
> 
> 	CONFIG_CPU_FREQ=y
> 	CONFIG_CPU_FREQ_STAT=y
> 	CONFIG_X86_P4_CLOCKMOD=m
> 
> After modprobe/rmmod cpufreq/stats directory appears but doesn't get
> removed. Should it?

Yes.

Well, one can argue that those stats should never be in sysfs at all
anyway, I mean come on, a histogram in sysfs?  That's, not ok.

thanks,

greg k-h

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Dave Jones]

On Wed, Mar 21, 2007 at 08:07:53PM -0700, Greg KH wrote:
 
 > > After modprobe/rmmod cpufreq/stats directory appears but doesn't get
 > > removed. Should it?
 > Well, one can argue that those stats should never be in sysfs at all
 > anyway, I mean come on, a histogram in sysfs?  That's, not ok.

Meh, it's only a cheesy debug thing, so it's not really that big a deal imo.
it could probably move to debugfs (we didn't have that when it was merged iirc)
I doubt anyone really cares enough to bother though I wouldn't
be averse to a patch.
To the best of my knowledge, nothing in userspace is relying on that stuff
being present (it'd have to cope with it not being there anyway given its
optional).

	Dave

-- 
http://www.codemonkey.org.uk

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Greg KH]

On Wed, Mar 21, 2007 at 11:51:04PM -0400, Dave Jones wrote:
> On Wed, Mar 21, 2007 at 08:07:53PM -0700, Greg KH wrote:
>  
>  > > After modprobe/rmmod cpufreq/stats directory appears but doesn't get
>  > > removed. Should it?
>  > Well, one can argue that those stats should never be in sysfs at all
>  > anyway, I mean come on, a histogram in sysfs?  That's, not ok.
> 
> Meh, it's only a cheesy debug thing, so it's not really that big a deal imo.
> it could probably move to debugfs (we didn't have that when it was merged iirc)
> I doubt anyone really cares enough to bother though I wouldn't
> be averse to a patch.

Yeah, I realize this, I'm not trying to find fault, sorry if it came
across that way.  And yes, it should move to debugfs some day, but if it
does, I'll loose my "what not to put in sysfs" example I use in
presentations :)

thanks,

greg k-h

Re: Oops after cd /sys/.../cpufreq/; rmmod; cat stats/time_in_state

[Andrew Morton]

On Wed, 21 Mar 2007 21:00:06 -0700 Greg KH <gregkh@suse.de> wrote:

> On Wed, Mar 21, 2007 at 11:51:04PM -0400, Dave Jones wrote:
> > On Wed, Mar 21, 2007 at 08:07:53PM -0700, Greg KH wrote:
> >  
> >  > > After modprobe/rmmod cpufreq/stats directory appears but doesn't get
> >  > > removed. Should it?
> >  > Well, one can argue that those stats should never be in sysfs at all
> >  > anyway, I mean come on, a histogram in sysfs?  That's, not ok.
> > 
> > Meh, it's only a cheesy debug thing, so it's not really that big a deal imo.
> > it could probably move to debugfs (we didn't have that when it was merged iirc)
> > I doubt anyone really cares enough to bother though I wouldn't
> > be averse to a patch.
> 
> Yeah, I realize this, I'm not trying to find fault, sorry if it came
> across that way.  And yes, it should move to debugfs some day, but if it
> does, I'll loose my "what not to put in sysfs" example I use in
> presentations :)
> 

I ain't picky, but as a short-term thing it'd be kinda nice if it didn't
oops the kernel.

[PATCH] fix cpufreq_stats attrs removal

[Mattia Dongili]

On Wed, Mar 21, 2007 at 08:10:42PM -0800, Andrew Morton wrote:
> On Wed, 21 Mar 2007 21:00:06 -0700 Greg KH <gregkh@suse.de> wrote:
> 
> > On Wed, Mar 21, 2007 at 11:51:04PM -0400, Dave Jones wrote:
> > > On Wed, Mar 21, 2007 at 08:07:53PM -0700, Greg KH wrote:
> > >  
> > >  > > After modprobe/rmmod cpufreq/stats directory appears but doesn't get
> > >  > > removed. Should it?
> > >  > Well, one can argue that those stats should never be in sysfs at all
> > >  > anyway, I mean come on, a histogram in sysfs?  That's, not ok.
> > > 
> > > Meh, it's only a cheesy debug thing, so it's not really that big a deal imo.
> > > it could probably move to debugfs (we didn't have that when it was merged iirc)
> > > I doubt anyone really cares enough to bother though I wouldn't
> > > be averse to a patch.
> > 
> > Yeah, I realize this, I'm not trying to find fault, sorry if it came
> > across that way.  And yes, it should move to debugfs some day, but if it
> > does, I'll loose my "what not to put in sysfs" example I use in
> > presentations :)
> > 
> 
> I ain't picky, but as a short-term thing it'd be kinda nice if it didn't
> oops the kernel.

There are other symptoms to this same bug:

1. unload p4-clockmod: /sys/.../cpu0/cpufreq is removed all together
2. load p4-clockmod: /sys/.../cpu0/cpufreq appears but no 'stats' subdir
   (yes, cpufreq_stats is loaded)
3. rmmod cpufreq_stats: Ooops!

Call Trace:
 [<c0183f5b>] remove_dir+0x33/0xc4
 [<c0184fca>] remove_files+0x1a/0x28
 [<c018503b>] sysfs_remove_group+0x63/0x71
 [<f898c38d>] cpufreq_stat_cpu_callback+0x51/0x8a [cpufreq_stats]
 [<f898c477>] cpufreq_stats_exit+0x47/0x4b [cpufreq_stats]
 [<c012f145>] sys_delete_module+0x190/0x1b7
 [<c0140073>] do_wp_page+0x231/0x3e7
 [<c0102e17>] syscall_call+0x7/0xb

The problem is cpufreq_stats doesn't know when a cpufreq driver is
removed and doesn't cleanup. I guess this affects any setup with
cpufreq_stats.
The attached patch seems to solve both symptoms and yes... it's quite
invasive as it introduce one more cpufreq policy notification (REMOVED).

BTW: the patch is against .21-rc4-mm1 but applies with some fuzz to
2.6.20 too

diff -rup linux-2.6.20/drivers/cpufreq/cpufreq.c linux-2.6.20.dirty/drivers/cpufreq/cpufreq.c
--- linux-2.6.20/drivers/cpufreq/cpufreq.c	2007-03-22 17:00:38.000000000 +0100
+++ linux-2.6.20.dirty/drivers/cpufreq/cpufreq.c	2007-03-22 16:51:09.000000000 +0100
@@ -989,6 +989,10 @@ static int __cpufreq_remove_dev (struct 
 
 	unlock_policy_rwsem_write(cpu);
 
+	/* notify of policy cancellation */
+	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+			CPUFREQ_REMOVE, data);
+
 	kobject_unregister(&data->kobj);
 
 	kobject_put(&data->kobj);
diff -rup linux-2.6.20/drivers/cpufreq/cpufreq_stats.c linux-2.6.20.dirty/drivers/cpufreq/cpufreq_stats.c
--- linux-2.6.20/drivers/cpufreq/cpufreq_stats.c	2007-03-22 17:00:38.000000000 +0100
+++ linux-2.6.20.dirty/drivers/cpufreq/cpufreq_stats.c	2007-03-22 17:06:24.000000000 +0100
@@ -257,18 +257,23 @@ static int
 cpufreq_stat_notifier_policy (struct notifier_block *nb, unsigned long val,
 		void *data)
 {
-	int ret;
+	int ret = 0;
 	struct cpufreq_policy *policy = data;
 	struct cpufreq_frequency_table *table;
 	unsigned int cpu = policy->cpu;
-	if (val != CPUFREQ_NOTIFY)
-		return 0;
-	table = cpufreq_frequency_get_table(cpu);
-	if (!table)
-		return 0;
-	if ((ret = cpufreq_stats_create_table(policy, table)))
-		return ret;
-	return 0;
+	switch (val) {
+	case CPUFREQ_NOTIFY:
+		table = cpufreq_frequency_get_table(cpu);
+		if (!table)
+			break;
+		ret = cpufreq_stats_create_table(policy, table);
+		break;
+
+	case CPUFREQ_REMOVE:
+		cpufreq_stats_free_table(cpu);
+		break;
+	}
+	return ret;
 }
 
 static int
@@ -371,8 +376,7 @@ __exit cpufreq_stats_exit(void)
 			CPUFREQ_TRANSITION_NOTIFIER);
 	unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
 	for_each_online_cpu(cpu) {
-		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier,
-						CPU_DEAD, (void *)(long)cpu);
+		cpufreq_stats_free_table(cpu);
 	}
 }
 
--- linux-2.6.20/include/linux/cpufreq.h	2007-03-22 17:00:47.000000000 +0100
+++ linux-2.6.20.dirty/include/linux/cpufreq.h	2007-03-22 16:10:37.000000000 +0100
@@ -96,6 +96,7 @@ struct cpufreq_policy {
 #define CPUFREQ_ADJUST		(0)
 #define CPUFREQ_INCOMPATIBLE	(1)
 #define CPUFREQ_NOTIFY		(2)
+#define CPUFREQ_REMOVE		(3)
 
 #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
 #define CPUFREQ_SHARED_TYPE_HW	 (1) /* HW does needed coordination */

Re: [PATCH] fix cpufreq_stats attrs removal

[Dave Jones]

On Thu, Mar 22, 2007 at 06:02:01PM +0100, Mattia Dongili wrote:

 > The problem is cpufreq_stats doesn't know when a cpufreq driver is
 > removed and doesn't cleanup. I guess this affects any setup with
 > cpufreq_stats.
 > The attached patch seems to solve both symptoms and yes... it's quite
 > invasive as it introduce one more cpufreq policy notification (REMOVED).
 > 
 > BTW: the patch is against .21-rc4-mm1 but applies with some fuzz to
 > 2.6.20 too

Alternatively, as it's just debug functionality, we could just
mark it unsafe for rmmod, which is nastier, but a lot less invasive
to the non-debug code.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] fix cpufreq_stats attrs removal

[Mattia Dongili]

On Thu, Mar 22, 2007 at 06:02:01PM +0100, Mattia Dongili wrote:

forgot... (should I resend the whole thing?)

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

Re: [PATCH] fix cpufreq_stats attrs removal

[Dave Jones]

On Thu, Mar 22, 2007 at 06:43:21PM +0100, Mattia Dongili wrote:
 > On Thu, Mar 22, 2007 at 06:02:01PM +0100, Mattia Dongili wrote:
 > 
 > forgot... (should I resend the whole thing?)
 > 
 > Signed-off-by: Mattia Dongili <malattia@linux.it>

Nah, I'll cut-n-paste.
Ignore my other mail btw, I wasn't thinking straight.
Marking the stats module unsafe for removal won't prevent
the race when we remove _other_ modules :-)

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] fix cpufreq_stats attrs removal

[Alexey Dobriyan]

On Thu, Mar 22, 2007 at 06:02:01PM +0100, Mattia Dongili wrote:
> On Wed, Mar 21, 2007 at 08:10:42PM -0800, Andrew Morton wrote:
> > I ain't picky, but as a short-term thing it'd be kinda nice if it didn't
> > oops the kernel.
> 
> There are other symptoms to this same bug:
> 
> 1. unload p4-clockmod: /sys/.../cpu0/cpufreq is removed all together
> 2. load p4-clockmod: /sys/.../cpu0/cpufreq appears but no 'stats' subdir
>    (yes, cpufreq_stats is loaded)
> 3. rmmod cpufreq_stats: Ooops!
> 
> Call Trace:
>  [<c0183f5b>] remove_dir+0x33/0xc4
>  [<c0184fca>] remove_files+0x1a/0x28
>  [<c018503b>] sysfs_remove_group+0x63/0x71
>  [<f898c38d>] cpufreq_stat_cpu_callback+0x51/0x8a [cpufreq_stats]
>  [<f898c477>] cpufreq_stats_exit+0x47/0x4b [cpufreq_stats]
>  [<c012f145>] sys_delete_module+0x190/0x1b7
>  [<c0140073>] do_wp_page+0x231/0x3e7
>  [<c0102e17>] syscall_call+0x7/0xb
> 
> The problem is cpufreq_stats doesn't know when a cpufreq driver is
> removed and doesn't cleanup. I guess this affects any setup with
> cpufreq_stats.
> The attached patch seems to solve both symptoms and yes... it's quite
> invasive as it introduce one more cpufreq policy notification (REMOVED).
> 
> BTW: the patch is against .21-rc4-mm1 but applies with some fuzz to
> 2.6.20 too

Also, it doesn't work.

/sys/*/cpufreq/stats dir stays if you cd into it _before_ rmmod.

	# modprobe p4-clockmod
	$ /sys/devices/system/cpu/cpu0/cpufreq/stats
	# rmmod p4-clockmod
	$ cat time_in_state

p4-clockmod: P4/Xeon(TM) CPU On-Demand Clock Modulation available
BUG: unable to handle kernel paging request at virtual address 6b6b6b6f
 printing eip:
c01955e1
*pde = 00000000
Oops: 0000 [#1]
last sysfs file: devices/system/cpu/cpu0/cpufreq/stats/time_in_state
Modules linked in: speedstep_lib ohci_hcd af_packet e1000 ehci_hcd uhci_hcd usbcore
CPU:    0
EIP:    0060:[<c01955e1>]    Not tainted VLI
EFLAGS: 00010202   (2.6.21-rc5-mm1 #2)
EIP is at sysfs_open_file+0xb6/0x26f
eax: 6b6b6b6b   ebx: c03b3cb0   ecx: 00000000   edx: f732772c
esi: 00000000   edi: c0681400   ebp: f36fad10   esp: f3595ee0
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process cat (pid: 7545, ti=f3594000 task=f3773510 task.ti=f3594000)
Stack: 00001000 f376f26c f732772c f376f26c f36fad10 f376f26c f3595f38 c019552b 
       c015ced8 c18d3190 f3746a18 f36fad10 00008000 f3595f38 ffffff9c c015d052 
       f36fad10 00000000 00000000 c015d094 00000000 f3595f38 f3746a18 c18d3190 
Call Trace:
 [<c019552b>] sysfs_open_file+0x0/0x26f
 [<c015ced8>] __dentry_open+0xa4/0x191
 [<c015d052>] nameidata_to_filp+0x31/0x3a
 [<c015d094>] do_filp_open+0x39/0x40
 [<c015ce23>] get_unused_fd+0xa1/0xb2
 [<c02db41f>] _spin_unlock+0x14/0x1c
 [<c015ce23>] get_unused_fd+0xa1/0xb2
 [<c015d0d5>] do_sys_open+0x3a/0x6d
 [<c015d143>] sys_open+0x1c/0x20
 [<c0103c96>] sysenter_past_esp+0x5f/0x99
 =======================
INFO: lockdep is turned off.
Code: 0f 85 24 01 00 00 8b 43 04 85 c0 74 0f 83 38 02 0f 84 c4 01 00 00 ff 80 80 01 00 00 8b 54 24 08 8b 42 28 85 c0 0f 84 62 01 00 00 <8b> 40 04 85 c0 0f 84 57 01 00 00 8b 40 04 89 44 24 0c 8b 74 24 
EIP: [<c01955e1>] sysfs_open_file+0xb6/0x26f SS:ESP 0068:f3595ee0
Slab corruption: start=f73276e0, len=256
Redzone: 0x5a2cf071/0x5a2cf071.
Last user: [<c0185810>](load_elf_binary+0xa79/0x1a19)
060: 6b 6b 6b 6b 6c 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
Prev obj: start=f73275d4, len=256
Redzone: 0x5a2cf071/0x5a2cf071.
Last user: [<c01864fb>](load_elf_binary+0x1764/0x1a19)
000: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
010: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
Next obj: start=f73277ec, len=256
Redzone: 0x5a2cf071/0x5a2cf071.
Last user: [<c01864fb>](load_elf_binary+0x1764/0x1a19)
000: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
010: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b

> --- linux-2.6.20/drivers/cpufreq/cpufreq.c	2007-03-22 17:00:38.000000000 +0100
> +++ linux-2.6.20.dirty/drivers/cpufreq/cpufreq.c	2007-03-22 16:51:09.000000000 +0100
> @@ -989,6 +989,10 @@ static int __cpufreq_remove_dev (struct 
>  
>  	unlock_policy_rwsem_write(cpu);
>  
> +	/* notify of policy cancellation */
> +	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> +			CPUFREQ_REMOVE, data);
> +
>  	kobject_unregister(&data->kobj);
>  
>  	kobject_put(&data->kobj);
> diff -rup linux-2.6.20/drivers/cpufreq/cpufreq_stats.c linux-2.6.20.dirty/drivers/cpufreq/cpufreq_stats.c
> --- linux-2.6.20/drivers/cpufreq/cpufreq_stats.c	2007-03-22 17:00:38.000000000 +0100
> +++ linux-2.6.20.dirty/drivers/cpufreq/cpufreq_stats.c	2007-03-22 17:06:24.000000000 +0100
> @@ -257,18 +257,23 @@ static int
>  cpufreq_stat_notifier_policy (struct notifier_block *nb, unsigned long val,
>  		void *data)
>  {
> -	int ret;
> +	int ret = 0;
>  	struct cpufreq_policy *policy = data;
>  	struct cpufreq_frequency_table *table;
>  	unsigned int cpu = policy->cpu;
> -	if (val != CPUFREQ_NOTIFY)
> -		return 0;
> -	table = cpufreq_frequency_get_table(cpu);
> -	if (!table)
> -		return 0;
> -	if ((ret = cpufreq_stats_create_table(policy, table)))
> -		return ret;
> -	return 0;
> +	switch (val) {
> +	case CPUFREQ_NOTIFY:
> +		table = cpufreq_frequency_get_table(cpu);
> +		if (!table)
> +			break;
> +		ret = cpufreq_stats_create_table(policy, table);
> +		break;
> +
> +	case CPUFREQ_REMOVE:
> +		cpufreq_stats_free_table(cpu);
> +		break;
> +	}
> +	return ret;
>  }
>  
>  static int
> @@ -371,8 +376,7 @@ __exit cpufreq_stats_exit(void)
>  			CPUFREQ_TRANSITION_NOTIFIER);
>  	unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
>  	for_each_online_cpu(cpu) {
> -		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier,
> -						CPU_DEAD, (void *)(long)cpu);
> +		cpufreq_stats_free_table(cpu);
>  	}
>  }
>  
> --- linux-2.6.20/include/linux/cpufreq.h	2007-03-22 17:00:47.000000000 +0100
> +++ linux-2.6.20.dirty/include/linux/cpufreq.h	2007-03-22 16:10:37.000000000 +0100
> @@ -96,6 +96,7 @@ struct cpufreq_policy {
>  #define CPUFREQ_ADJUST		(0)
>  #define CPUFREQ_INCOMPATIBLE	(1)
>  #define CPUFREQ_NOTIFY		(2)
> +#define CPUFREQ_REMOVE		(3)
>  
>  #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
>  #define CPUFREQ_SHARED_TYPE_HW	 (1) /* HW does needed coordination */

Re: [PATCH] fix cpufreq_stats attrs removal

[Mattia Dongili]

On Fri, Mar 30, 2007 at 12:09:03PM +0400, Alexey Dobriyan wrote:
> On Thu, Mar 22, 2007 at 06:02:01PM +0100, Mattia Dongili wrote:
> > On Wed, Mar 21, 2007 at 08:10:42PM -0800, Andrew Morton wrote:
> > > I ain't picky, but as a short-term thing it'd be kinda nice if it didn't
> > > oops the kernel.
> > 
> > There are other symptoms to this same bug:
> > 
> > 1. unload p4-clockmod: /sys/.../cpu0/cpufreq is removed all together
> > 2. load p4-clockmod: /sys/.../cpu0/cpufreq appears but no 'stats' subdir
> >    (yes, cpufreq_stats is loaded)
> > 3. rmmod cpufreq_stats: Ooops!
> > 
> > Call Trace:
> >  [<c0183f5b>] remove_dir+0x33/0xc4
> >  [<c0184fca>] remove_files+0x1a/0x28
> >  [<c018503b>] sysfs_remove_group+0x63/0x71
> >  [<f898c38d>] cpufreq_stat_cpu_callback+0x51/0x8a [cpufreq_stats]
> >  [<f898c477>] cpufreq_stats_exit+0x47/0x4b [cpufreq_stats]
> >  [<c012f145>] sys_delete_module+0x190/0x1b7
> >  [<c0140073>] do_wp_page+0x231/0x3e7
> >  [<c0102e17>] syscall_call+0x7/0xb
> > 
> > The problem is cpufreq_stats doesn't know when a cpufreq driver is
> > removed and doesn't cleanup. I guess this affects any setup with
> > cpufreq_stats.
> > The attached patch seems to solve both symptoms and yes... it's quite
> > invasive as it introduce one more cpufreq policy notification (REMOVED).
> > 
> > BTW: the patch is against .21-rc4-mm1 but applies with some fuzz to
> > 2.6.20 too
> 
> Also, it doesn't work.

hmmm... odd. I did test that case. Also, using .21-rc5-mm3 (which
has that patch applied) and acpi-cpufreq (I don't have that P4 handy
now) I actually hit a WARN_ON in kref_get(), but no oops:

# rmmod acpi-cpufreq

[ 1839.895632] acpi-cpufreq: acpi_cpufreq_exit
[ 1839.895637] cpufreq-core: unregistering driver acpi-cpufreq
[ 1839.895642] cpufreq-core: unregistering CPU 0
[ 1839.895649] cpufreq-core: __cpufreq_governor for CPU 0, event 2
[ 1839.895661] cpufreq-stats: removing tables
[ 1839.895714] cpufreq-core: last reference is dropped
[ 1839.895716] cpufreq-core: waiting for dropping of refcount
[ 1839.895718] cpufreq-core: wait complete
[ 1839.895720] acpi-cpufreq: acpi_cpufreq_cpu_exit
[ 1839.895723] freq-table: clearing show_table for cpu 0
[ 1839.895726] cpufreq-core: unregistering CPU 1
[ 1839.895728] cpufreq-core: __cpufreq_governor for CPU 1, event 2
[ 1839.895731] cpufreq-stats: removing tables    <------- dprintk on CPUFREQ_REMOVE
[ 1839.895741] cpufreq-core: last reference is dropped
[ 1839.895744] cpufreq-core: waiting for dropping of refcount
[ 1839.895746] cpufreq-core: wait complete
[ 1839.895747] acpi-cpufreq: acpi_cpufreq_cpu_exit
[ 1839.895749] freq-table: clearing show_table for cpu 1

# cat time_in_state

[ 1853.932509] BUG: at lib/kref.c:32 kref_get()
[ 1853.932567]  [<c0104b7a>] show_trace_log_lvl+0x1a/0x30
[ 1853.932676]  [<c0105705>] show_trace+0x12/0x14
[ 1853.932778]  [<c010575e>] dump_stack+0x16/0x18
[ 1853.932880]  [<c01dcfba>] kref_get+0x37/0x40
[ 1853.932986]  [<c01dc415>] kobject_get+0x15/0x1b
[ 1853.933087]  [<c019ff0b>] sysfs_open_file+0x40/0x209
[ 1853.933189]  [<c01693bc>] __dentry_open+0xc2/0x179
[ 1853.933293]  [<c01694f5>] nameidata_to_filp+0x27/0x38
[ 1853.933395]  [<c0169539>] do_filp_open+0x33/0x3b
[ 1853.933498]  [<c0169584>] do_sys_open+0x43/0xc7
[ 1853.933600]  [<c0169640>] sys_open+0x1c/0x1e
[ 1853.933700]  [<c0103d24>] sysenter_past_esp+0x5d/0x81
[ 1853.933799]  =======================
[ 1853.934330] cpufreq-core: last reference is dropped

-- 
mattia
:wq!