next-20130206 cpufreq - WARN in sysfs_add_one

next-20130206 cpufreq - WARN in sysfs_add_one

[Valdis Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
do so if the offender isn't obvious...

[    2.567662] netconsole: network logging started
[    2.581661] ------------[ cut here ]------------
[    2.581670] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0x91/0xa5()
[    2.581673] Hardware name: Latitude E6500
[    2.581676] sysfs: cannot create duplicate filename '/devices/system/cpu/cpufreq/ondemand'
[    2.581678] Modules linked in:
[    2.581684] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc6-next-20130206 #57
[    2.581687] Call Trace:
[    2.581693]  [<ffffffff811713d6>] ? sysfs_add_one+0x91/0xa5
[    2.581700]  [<ffffffff8102ecdc>] warn_slowpath_common+0x7c/0x96
[    2.581706]  [<ffffffff8102ed3d>] warn_slowpath_fmt+0x47/0x49
[    2.581711]  [<ffffffff811713d6>] sysfs_add_one+0x91/0xa5
[    2.581716]  [<ffffffff81171521>] create_dir+0x6b/0xa4
[    2.581721]  [<ffffffff81171787>] sysfs_create_subdir+0x17/0x19
[    2.581727]  [<ffffffff81172e76>] internal_create_group+0xd4/0x1df
[    2.581732]  [<ffffffff81172f8f>] sysfs_create_group+0xe/0x10
[    2.581739]  [<ffffffff813e033a>] cpufreq_governor_dbs+0x209/0x475
[    2.581744]  [<ffffffff813df187>] od_cpufreq_governor_dbs+0x15/0x17
[    2.581750]  [<ffffffff813dd6ab>] __cpufreq_governor+0x61/0xb5
[    2.581755]  [<ffffffff813dd803>] __cpufreq_set_policy+0x104/0x140
[    2.581761]  [<ffffffff81609303>] cpufreq_add_dev_interface+0x217/0x261
[    2.581767]  [<ffffffff813de5ed>] ? cpufreq_update_policy+0x133/0x133
[    2.581773]  [<ffffffff813dddc9>] cpufreq_add_dev+0x222/0x2d7
[    2.581780]  [<ffffffff812dc7da>] subsys_interface_register+0xa3/0xd7
[    2.581786]  [<ffffffff8160e6c9>] ? _raw_spin_unlock_irqrestore+0x72/0x80
[    2.581792]  [<ffffffff813dd1ae>] cpufreq_register_driver+0x9c/0x13b
[    2.581799]  [<ffffffff81d778ed>] ? cpufreq_gov_dbs_init+0x2c/0x2c
[    2.581805]  [<ffffffff81d77968>] acpi_cpufreq_init+0x7b/0x185
[    2.581811]  [<ffffffff8100023a>] do_one_initcall+0x7a/0x130
[    2.581817]  [<ffffffff81d4ddfb>] kernel_init_freeable+0x15e/0x1df
[    2.581822]  [<ffffffff81d4d6f2>] ? do_early_param+0x88/0x88
[    2.581828]  [<ffffffff815f9568>] ? rest_init+0x12c/0x12c
[    2.581833]  [<ffffffff815f9571>] kernel_init+0x9/0xd1
[    2.581839]  [<ffffffff81614bec>] ret_from_fork+0x7c/0xb0
[    2.581844]  [<ffffffff815f9568>] ? rest_init+0x12c/0x12c
[    2.581847] ---[ end trace a0eea8e0db3a3905 ]---
[    2.585909] ALSA device list:
[    2.585913]   #0: HDA Intel at 0xf6fdc000 irq 48
[    2.590781] Freeing unused kernel memory: 852k freed


[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Rafael J. Wysocki]

On Wednesday, February 06, 2013 12:44:35 PM Valdis Kletnieks wrote:
> Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
> do so if the offender isn't obvious...

I suppose this is 73bf0fc "cpufreq: Don't remove sysfs link for policy->cpu".

Can you test the linux-pm.git/pm-cpufreq branch alone, please, and see
if that's this one (top-most commit)?

Even if it is, it is a bug fix, so we'll need one fix more on top of it,
but can you please confirm?

Rafael


> [    2.567662] netconsole: network logging started
> [    2.581661] ------------[ cut here ]------------
> [    2.581670] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0x91/0xa5()
> [    2.581673] Hardware name: Latitude E6500
> [    2.581676] sysfs: cannot create duplicate filename '/devices/system/cpu/cpufreq/ondemand'
> [    2.581678] Modules linked in:
> [    2.581684] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc6-next-20130206 #57
> [    2.581687] Call Trace:
> [    2.581693]  [<ffffffff811713d6>] ? sysfs_add_one+0x91/0xa5
> [    2.581700]  [<ffffffff8102ecdc>] warn_slowpath_common+0x7c/0x96
> [    2.581706]  [<ffffffff8102ed3d>] warn_slowpath_fmt+0x47/0x49
> [    2.581711]  [<ffffffff811713d6>] sysfs_add_one+0x91/0xa5
> [    2.581716]  [<ffffffff81171521>] create_dir+0x6b/0xa4
> [    2.581721]  [<ffffffff81171787>] sysfs_create_subdir+0x17/0x19
> [    2.581727]  [<ffffffff81172e76>] internal_create_group+0xd4/0x1df
> [    2.581732]  [<ffffffff81172f8f>] sysfs_create_group+0xe/0x10
> [    2.581739]  [<ffffffff813e033a>] cpufreq_governor_dbs+0x209/0x475
> [    2.581744]  [<ffffffff813df187>] od_cpufreq_governor_dbs+0x15/0x17
> [    2.581750]  [<ffffffff813dd6ab>] __cpufreq_governor+0x61/0xb5
> [    2.581755]  [<ffffffff813dd803>] __cpufreq_set_policy+0x104/0x140
> [    2.581761]  [<ffffffff81609303>] cpufreq_add_dev_interface+0x217/0x261
> [    2.581767]  [<ffffffff813de5ed>] ? cpufreq_update_policy+0x133/0x133
> [    2.581773]  [<ffffffff813dddc9>] cpufreq_add_dev+0x222/0x2d7
> [    2.581780]  [<ffffffff812dc7da>] subsys_interface_register+0xa3/0xd7
> [    2.581786]  [<ffffffff8160e6c9>] ? _raw_spin_unlock_irqrestore+0x72/0x80
> [    2.581792]  [<ffffffff813dd1ae>] cpufreq_register_driver+0x9c/0x13b
> [    2.581799]  [<ffffffff81d778ed>] ? cpufreq_gov_dbs_init+0x2c/0x2c
> [    2.581805]  [<ffffffff81d77968>] acpi_cpufreq_init+0x7b/0x185
> [    2.581811]  [<ffffffff8100023a>] do_one_initcall+0x7a/0x130
> [    2.581817]  [<ffffffff81d4ddfb>] kernel_init_freeable+0x15e/0x1df
> [    2.581822]  [<ffffffff81d4d6f2>] ? do_early_param+0x88/0x88
> [    2.581828]  [<ffffffff815f9568>] ? rest_init+0x12c/0x12c
> [    2.581833]  [<ffffffff815f9571>] kernel_init+0x9/0xd1
> [    2.581839]  [<ffffffff81614bec>] ret_from_fork+0x7c/0xb0
> [    2.581844]  [<ffffffff815f9568>] ? rest_init+0x12c/0x12c
> [    2.581847] ---[ end trace a0eea8e0db3a3905 ]---
> [    2.585909] ALSA device list:
> [    2.585913]   #0: HDA Intel at 0xf6fdc000 irq 48
> [    2.590781] Freeing unused kernel memory: 852k freed
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Wed, 06 Feb 2013 22:24:39 +0100, "Rafael J. Wysocki" said:
> On Wednesday, February 06, 2013 12:44:35 PM Valdis Kletnieks wrote:
> > Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
> > do so if the offender isn't obvious...
>
> I suppose this is 73bf0fc "cpufreq: Don't remove sysfs link for policy->cpu".
>
> Can you test the linux-pm.git/pm-cpufreq branch alone, please, and see
> if that's this one (top-most commit)?

Color me mystified.  I can't find that commit by either number or
the description:

[/usr/src/linux-next] git log | egrep -i 'cpufreq.*remove.*link'
[/usr/src/linux-next] git log | grep -i 73bf0fc
[/usr/src/linux-next]

Was that a very recent commit that may have gotten pushed too late
for the next-20130206 merge?







[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Rafael J. Wysocki]

On Wednesday, February 06, 2013 07:55:58 PM Valdis.Kletnieks@vt.edu wrote:
> On Wed, 06 Feb 2013 22:24:39 +0100, "Rafael J. Wysocki" said:
> > On Wednesday, February 06, 2013 12:44:35 PM Valdis Kletnieks wrote:
> > > Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
> > > do so if the offender isn't obvious...
> >
> > I suppose this is 73bf0fc "cpufreq: Don't remove sysfs link for policy->cpu".
> >
> > Can you test the linux-pm.git/pm-cpufreq branch alone, please, and see
> > if that's this one (top-most commit)?
> 
> Color me mystified.  I can't find that commit by either number or
> the description:
> 
> [/usr/src/linux-next] git log | egrep -i 'cpufreq.*remove.*link'
> [/usr/src/linux-next] git log | grep -i 73bf0fc
> [/usr/src/linux-next]
> 
> Was that a very recent commit that may have gotten pushed too late
> for the next-20130206 merge?

Well, sorry, next-20130206 didn't include that commit.

Can you please test the linux-pm.git/pm-cpufreq branch, then?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Rafael J. Wysocki]

On Thursday, February 07, 2013 02:12:17 AM Rafael J. Wysocki wrote:
> On Wednesday, February 06, 2013 07:55:58 PM Valdis.Kletnieks@vt.edu wrote:
> > On Wed, 06 Feb 2013 22:24:39 +0100, "Rafael J. Wysocki" said:
> > > On Wednesday, February 06, 2013 12:44:35 PM Valdis Kletnieks wrote:
> > > > Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
> > > > do so if the offender isn't obvious...
> > >
> > > I suppose this is 73bf0fc "cpufreq: Don't remove sysfs link for policy->cpu".
> > >
> > > Can you test the linux-pm.git/pm-cpufreq branch alone, please, and see
> > > if that's this one (top-most commit)?
> > 
> > Color me mystified.  I can't find that commit by either number or
> > the description:
> > 
> > [/usr/src/linux-next] git log | egrep -i 'cpufreq.*remove.*link'
> > [/usr/src/linux-next] git log | grep -i 73bf0fc
> > [/usr/src/linux-next]
> > 
> > Was that a very recent commit that may have gotten pushed too late
> > for the next-20130206 merge?
> 
> Well, sorry, next-20130206 didn't include that commit.
> 
> Can you please test the linux-pm.git/pm-cpufreq branch, then?

To be precise, I mean the pm-cpufreq branch of the tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

which is based on v3.8-rc6.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Viresh Kumar]

On Thu, Feb 7, 2013 at 2:54 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, February 06, 2013 12:44:35 PM Valdis Kletnieks wrote:
>> Seen in dmesg.  next-20130128 was OK. Haven't done a bisect, but can
>> do so if the offender isn't obvious...
>
> I suppose this is 73bf0fc "cpufreq: Don't remove sysfs link for policy->cpu".

Not really. :)

Hi Valdis,

First of all i want to confirm something about your system. I am sure it is a
multi-policy system (or multi cluster system). i.e. there are more than one
clock line for different cpus ? And so multiple struct policy exist
simultaneously.

Because this crash can only come on those.

Anyway, i have tested and pushed a fix here:

http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-valdis

Please test it.

For others, the patch is:

commit 007dda326f1b1415846671d7fcfbd520f4f16151
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Thu Feb 7 12:51:27 2013 +0530

    cpufreq: governors: Fix WARN_ON() for multi-policy platforms

    On multi-policy systems there is a single instance of governor for both the
    policies (if same governor is chosen for both policies). With the
code update
    from following patches:

    8eeed09 cpufreq: governors: Get rid of dbs_data->enable field
    b394058 cpufreq: governors: Reset tunables only for
cpufreq_unregister_governor()

    We are creating/removing sysfs directory of governor for for every call to
    GOV_START and STOP. This would fail for multi-policy system as there is a
    per-policy call to START/STOP.

    This patch reuses the governor->initialized variable to detect
total users of
    governor.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          |  6 ++++--
 drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ccc598a..3b941a1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1567,8 +1567,10 @@ static int __cpufreq_governor(struct
cpufreq_policy *policy,
                                                policy->cpu, event);
        ret = policy->governor->governor(policy, event);

-       if (!policy->governor->initialized && (event == CPUFREQ_GOV_START))
-               policy->governor->initialized = 1;
+       if (event == CPUFREQ_GOV_START)
+               policy->governor->initialized++;
+       else if (event == CPUFREQ_GOV_STOP)
+               policy->governor->initialized--;

        /* we keep one module reference alive for
                        each CPU governed by this CPU */
diff --git a/drivers/cpufreq/cpufreq_governor.c
b/drivers/cpufreq/cpufreq_governor.c
index e4a306c..5a76086 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -247,11 +247,13 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
                                             dbs_data->gov_dbs_timer);
                }

-               rc = sysfs_create_group(cpufreq_global_kobject,
-                               dbs_data->attr_group);
-               if (rc) {
-                       mutex_unlock(&dbs_data->mutex);
-                       return rc;
+               if (!policy->governor->initialized) {
+                       rc = sysfs_create_group(cpufreq_global_kobject,
+                                       dbs_data->attr_group);
+                       if (rc) {
+                               mutex_unlock(&dbs_data->mutex);
+                               return rc;
+                       }
                }

                /*
@@ -262,13 +264,15 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
                        cs_dbs_info->down_skip = 0;
                        cs_dbs_info->enable = 1;
                        cs_dbs_info->requested_freq = policy->cur;
-                       cpufreq_register_notifier(cs_ops->notifier_block,
-                                       CPUFREQ_TRANSITION_NOTIFIER);

-                       if (!policy->governor->initialized)
+                       if (!policy->governor->initialized) {
+
cpufreq_register_notifier(cs_ops->notifier_block,
+                                               CPUFREQ_TRANSITION_NOTIFIER);
+
                                dbs_data->min_sampling_rate =
                                        MIN_SAMPLING_RATE_RATIO *
                                        jiffies_to_usecs(10);
+                       }
                } else {
                        od_dbs_info->rate_mult = 1;
                        od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -311,11 +315,13 @@ unlock:
                mutex_lock(&dbs_data->mutex);
                mutex_destroy(&cpu_cdbs->timer_mutex);

-               sysfs_remove_group(cpufreq_global_kobject,
-                               dbs_data->attr_group);
-               if (dbs_data->governor == GOV_CONSERVATIVE)
-                       cpufreq_unregister_notifier(cs_ops->notifier_block,
-                                       CPUFREQ_TRANSITION_NOTIFIER);
+               if (policy->governor->initialized == 1) {
+                       sysfs_remove_group(cpufreq_global_kobject,
+                                       dbs_data->attr_group);
+                       if (dbs_data->governor == GOV_CONSERVATIVE)
+
cpufreq_unregister_notifier(cs_ops->notifier_block,
+                                               CPUFREQ_TRANSITION_NOTIFIER);
+               }
                mutex_unlock(&dbs_data->mutex);

                break;

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Thu, 07 Feb 2013 13:11:52 +0530, Viresh Kumar said:

> First of all i want to confirm something about your system. I am sure it is a
> multi-policy system (or multi cluster system). i.e. there are more than one
> clock line for different cpus ? And so multiple struct policy exist
> simultaneously.

Hmm.. it's a bog-standard Dell Latitude E6500 laptop, with a single
Core2 Duo P8700 CPU (one die, 2 cores, no HT). It's apparently able
to clock both cores at different speeds (one core running busy at 2540mhz
and the other idling at 800mhz), if that's what you mean by multiple
clock lines.

In any case, next-20130206 complained, and with this patch added I see
nothing in dmesg and cpufreq is acting properly on both cores, so:

Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

(btw - I had to hand-apply your patch, as it showed up white-space
damaged.  Three lines wrapped, and tabs converted to spaces).





[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: next-20130206 cpufreq - WARN in sysfs_add_one

[Viresh Kumar]

On 8 February 2013 00:48,  <Valdis.Kletnieks@vt.edu> wrote:
> On Thu, 07 Feb 2013 13:11:52 +0530, Viresh Kumar said:
>
>> First of all i want to confirm something about your system. I am sure it is a
>> multi-policy system (or multi cluster system). i.e. there are more than one
>> clock line for different cpus ? And so multiple struct policy exist
>> simultaneously.
>
> Hmm.. it's a bog-standard Dell Latitude E6500 laptop, with a single
> Core2 Duo P8700 CPU (one die, 2 cores, no HT). It's apparently able
> to clock both cores at different speeds (one core running busy at 2540mhz
> and the other idling at 800mhz), if that's what you mean by multiple
> clock lines.

Perfect!! So, when the cpus can manage different freqs, we have multiple
struct policies for them per cpu.

> In any case, next-20130206 complained, and with this patch added I see
> nothing in dmesg and cpufreq is acting properly on both cores, so:
>
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

Thanks.

> (btw - I had to hand-apply your patch, as it showed up white-space
> damaged.  Three lines wrapped, and tabs converted to spaces).

I know that and feeling bad for you :(

I gave you this because of my mails issue :)

http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-valdis