BUG in bleeding edge c560f3d

Quoted 13 lines · Dirk Brandewie

Hi All,

There is a bug in the bleeding-edge branch. Using the ondemand governor and acpi_cpufreq scaling driver the system hangs while trying to reboot or trying to offline a cpu manually. The last call where I have tracing turned on is __cpufreq_governor(data, CPUFREQ_GOV_STOP);

[   38.138236] __cpufreq_remove_dev: cpufreq: __cpufreq_remove_dev:
unregistering CPU 1
[   38.146663] __cpufreq_governor: cpufreq: __cpufreq_governor for CPU 0, event 2
[   71.562262] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen

Quoted 4 lines

It looks like the breakage comes from the removal of the sysfs files in the incorrect order. While rebasing my patches onto bleeding edge I found this problem as well. I have a couple of hack patches that workaround the issue when using my driver if anyone would like to see them.

There have been a few fixes since c560f3d, they are in linux-pm.git/linux-next now. Can you please test that tree and see if the problem is still there?

Rafael

Hi All,

There is a bug in the bleeding-edge branch. Using the ondemand governor and acpi_cpufreq scaling driver the system hangs while trying to reboot or trying to offline a cpu manually. The last call where I have tracing turned on is __cpufreq_governor(data, CPUFREQ_GOV_STOP);

[   38.138236] __cpufreq_remove_dev: cpufreq: __cpufreq_remove_dev: 
unregistering CPU 1
[   38.146663] __cpufreq_governor: cpufreq: __cpufreq_governor for CPU 0, event 2
[   71.562262] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen

It looks like the breakage comes from the removal of the sysfs files in the incorrect order. While rebasing my patches onto bleeding edge I found this problem as well. I have a couple of hack patches that workaround the issue when using my driver if anyone would like to see them.

Quoted 19 lines · Rafael J. Wysocki

Quoted 17 lines · Dirk Brandewie

Hi All,

There is a bug in the bleeding-edge branch. Using the ondemand governor and acpi_cpufreq scaling driver the system hangs while trying to reboot or trying to offline a cpu manually. The last call where I have tracing turned on is __cpufreq_governor(data, CPUFREQ_GOV_STOP);

[   38.138236] __cpufreq_remove_dev: cpufreq: __cpufreq_remove_dev:
unregistering CPU 1
[   38.146663] __cpufreq_governor: cpufreq: __cpufreq_governor for CPU 0, event 2
[   71.562262] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen

It looks like the breakage comes from the removal of the sysfs files in the incorrect order. While rebasing my patches onto bleeding edge I found this problem as well. I have a couple of hack patches that workaround the issue when using my driver if anyone would like to see them.

Quoted 3 lines

There have been a few fixes since c560f3d, they are in linux-pm.git/linux-next now. Can you please test that tree and see if the problem is still there?

Rebased a couple of hours ago testing now. ATM it looks like it is related to cpufreq_stats handling of the sysfs files

Quoted 19 lines · Rafael J. Wysocki

Quoted 17 lines · Dirk Brandewie

Hi All,

There is a bug in the bleeding-edge branch. Using the ondemand governor and acpi_cpufreq scaling driver the system hangs while trying to reboot or trying to offline a cpu manually. The last call where I have tracing turned on is __cpufreq_governor(data, CPUFREQ_GOV_STOP);

[   38.138236] __cpufreq_remove_dev: cpufreq: __cpufreq_remove_dev:
unregistering CPU 1
[   38.146663] __cpufreq_governor: cpufreq: __cpufreq_governor for CPU 0, event 2
[   71.562262] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen

It looks like the breakage comes from the removal of the sysfs files in the incorrect order. While rebasing my patches onto bleeding edge I found this problem as well. I have a couple of hack patches that workaround the issue when using my driver if anyone would like to see them.

Quoted 3 lines

There have been a few fixes since c560f3d, they are in linux-pm.git/linux-next now. Can you please test that tree and see if the problem is still there?

Rebased a couple of hours ago testing now. ATM it looks like it is related to cpufreq_stats handling of the sysfs files

Quoted 2 lines · Dirk Brandewie

Rebased a couple of hours ago testing now. ATM it looks like it is related to cpufreq_stats handling of the sysfs files

This looks like a problem with commit b8eed8a. Viresh?

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines · Dirk Brandewie

Rebased a couple of hours ago testing now. ATM it looks like it is related to cpufreq_stats handling of the sysfs files

This looks like a problem with commit b8eed8a. Viresh?

I had some direct chat with Dirk to understand his problem. He has got lots of minor changes, which by themselves looks incorrect, for ex:
- He is required to cpufreq_frequency_table_put_attr() from his drivers init() code to make it NULL... But it should already have been NULL as per my understanding. I really want him to test his driver without any additional target/set_policy() patches he has.. to see if linux-next works for
him or not.

He will do this testing tomorrow and will let us know of any issues he faces.

We have already tested linux-next and these patches on ARM TC2 and STE u8500 (by Fabio), with reboots and lots of hotplugging.

Though i am still going to review my own patches once more to see if there is scope for improvement.

Quoted 16 lines · Viresh Kumar

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines · Dirk Brandewie

Rebased a couple of hours ago testing now. ATM it looks like it is related to cpufreq_stats handling of the sysfs files

This looks like a problem with commit b8eed8a. Viresh?

I had some direct chat with Dirk to understand his problem. He has got lots of minor changes, which by themselves looks incorrect, for ex:
- He is required to cpufreq_frequency_table_put_attr() from his drivers init() code to make it NULL... But it should already have been NULL as per my understanding. I really want him to test his driver without any additional target/set_policy() patches he has.. to see if linux-next works for
him or not.

Quoted 7 lines

He will do this testing tomorrow and will let us know of any issues he faces.

We have already tested linux-next and these patches on ARM TC2 and STE u8500 (by Fabio), with reboots and lots of hotplugging.

Though i am still going to review my own patches once more to see if there is scope for improvement.

The system on which Dirk is testing his patches has following configuration:
1 Package, 4 cpus, 8 virtual cores...

Though they share their clock line in some way, it is handled by the cpufreq driver
only and for the outside world (cpufreq core), it looks as there are 8 independent cpus,
and so 8 policy structures.

I tried to reproduce the issue at my end by removing extra cpus from my clusters and
just keeping one per cluster alive from DT.

And i *wasn't* able to reproduce the problem. Though i was able to find a small bug/issue
in the current code for which i have following patch (attached too):

@Dirk: Please give this one a try. Atleast on my system with various configurations, i
couldn't see any different this patch has made, but is more logical to me.

commit 9bdd6d47403e696d05953870019e791806f8d6bf

Author: Viresh Kumar <[email protected]>
Date:   Tue Feb 5 12:28:18 2013 +0530

    cpufreq: Don't remove sysfs link for policy->cpu

    "cpufreq" directory in policy->cpu is never created using
sysfs_create_link(),
    but using kobject_init_and_add(). And so we shouldn't call
sysfs_remove_link()
    for policy->cpu(). sysfs stuff for policy->cpu is automatically

removed when we
call kobject_put() for dying policy.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/cpufreq/cpufreq.c +3 -1

index 7aacfbf..9567451 100644
@@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
        cpus = cpumask_weight(data->cpus);
        cpumask_clear_cpu(cpu, data->cpus);
 
-       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
+       if (cpu != data->cpu) {
+               sysfs_remove_link(&dev->kobj, "cpufreq");
+       } else if (cpus > 1) {
                /* first sibling now owns the new sysfs dir */
                cpu_dev = get_cpu_device(cpumask_first(data->cpus));

                sysfs_remove_link(&cpu_dev->kobj, "cpufreq");

drivers/cpufreq/cpufreq.c +0 -1

@@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
        pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
        cpufreq_cpu_put(data);
        unlock_policy_rwsem_write(cpu);
-       sysfs_remove_link(&dev->kobj, "cpufreq");
 
        /* If cpu is last user of policy, free policy */

        if (cpus == 1) {

@Dirk: Please give this one a try. Atleast on my system with various configurations, i
couldn't see any different this patch has made, but is more logical to me.

commit 9bdd6d47403e696d05953870019e791806f8d6bf

Author: Viresh Kumar <[email protected]>
Date:   Tue Feb 5 12:28:18 2013 +0530

     cpufreq: Don't remove sysfs link for policy->cpu

     "cpufreq" directory in policy->cpu is never created using
sysfs_create_link(),
     but using kobject_init_and_add(). And so we shouldn't call
sysfs_remove_link()
     for policy->cpu(). sysfs stuff for policy->cpu is automatically

removed when we
call kobject_put() for dying policy.

Signed-off-by Viresh Kumar <[email protected]>

---
  drivers/cpufreq/cpufreq.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

drivers/cpufreq/cpufreq.c +3 -1

index 7aacfbf..9567451 100644
@@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
         cpus = cpumask_weight(data->cpus);
         cpumask_clear_cpu(cpu, data->cpus);
 
-       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
+       if (cpu != data->cpu) {
+               sysfs_remove_link(&dev->kobj, "cpufreq");
+       } else if (cpus > 1) {
                 /* first sibling now owns the new sysfs dir */
                 cpu_dev = get_cpu_device(cpumask_first(data->cpus));

                 sysfs_remove_link(&cpu_dev->kobj, "cpufreq");

drivers/cpufreq/cpufreq.c +0 -1

@@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
         pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
         cpufreq_cpu_put(data);
         unlock_policy_rwsem_write(cpu);
-       sysfs_remove_link(&dev->kobj, "cpufreq");
 
         /* If cpu is last user of policy, free policy */

         if (cpus == 1) {

Quoted 4 lines · Viresh Kumar

@Dirk: Please give this one a try. Atleast on my system with various configurations, i
couldn't see any different this patch has made, but is more logical to me.

Tested works fine with and without my driver present.

I added it to my patch set with my Tested-by tag it seems gmail decided to swallow it :-(

Please feel to add my Tested-by:

Quoted 45 lines

commit 9bdd6d47403e696d05953870019e791806f8d6bf

Author: Viresh Kumar <[email protected]>
Date:   Tue Feb 5 12:28:18 2013 +0530

     cpufreq: Don't remove sysfs link for policy->cpu

     "cpufreq" directory in policy->cpu is never created using
sysfs_create_link(),
     but using kobject_init_and_add(). And so we shouldn't call
sysfs_remove_link()
     for policy->cpu(). sysfs stuff for policy->cpu is automatically

removed when we
call kobject_put() for dying policy.

Signed-off-by Viresh Kumar <[email protected]>

---
  drivers/cpufreq/cpufreq.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

drivers/cpufreq/cpufreq.c +3 -1

index 7aacfbf..9567451 100644
@@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
         cpus = cpumask_weight(data->cpus);
         cpumask_clear_cpu(cpu, data->cpus);
 
-       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
+       if (cpu != data->cpu) {
+               sysfs_remove_link(&dev->kobj, "cpufreq");
+       } else if (cpus > 1) {
                 /* first sibling now owns the new sysfs dir */
                 cpu_dev = get_cpu_device(cpumask_first(data->cpus));

                 sysfs_remove_link(&cpu_dev->kobj, "cpufreq");

drivers/cpufreq/cpufreq.c +0 -1

@@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
 *dev, struct subsys_interface *sif
         pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
         cpufreq_cpu_put(data);
         unlock_policy_rwsem_write(cpu);
-       sysfs_remove_link(&dev->kobj, "cpufreq");
 
         /* If cpu is last user of policy, free policy */

         if (cpus == 1) {

Quoted 5 lines · Viresh Kumar

@Dirk: Please give this one a try. Atleast on my system with various configurations, i
couldn't see any different this patch has made, but is more logical to me.

Tested works fine with and without my driver present.