| Message ID | 1483174.NsuFf8aJk6@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 25 October 2013 15:21, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Friday, October 25, 2013 11:28:02 AM Rafael J. Wysocki wrote: >> On Friday, October 25, 2013 10:02:22 AM Linus Torvalds wrote: >> > This particular cpufreq issue may be triggered by the fact that >> > acpi-cpufreq isn't actually in use (pstate is). Or it might be some >> > generic cpufreq/sysfs bug. Rafael, Greg, ideas? >> >> I *think* that this indeed is related to acpi-cpufreq being unused. That said, >> we've been fixing sysfs-related bugs in cpufreq recently and we may have >> overlooked something. I agree.. Recently I have tested few other cpufreq drivers for module insert/removal along with governors insertion/removal... So that part must be okay.. > Well, if the ACPI cpufreq driver is not registered, the exit function of the > module shouldn't try to unregister it, so I have the appended patch (untested) > to fix that particular thing. > > Rafael > > > --- > drivers/cpufreq/acpi-cpufreq.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c > +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c > @@ -982,6 +982,8 @@ static void __exit acpi_cpufreq_boost_ex > } > } > > +static bool driver_registered; > + > static int __init acpi_cpufreq_init(void) > { > int ret; > @@ -1021,10 +1023,12 @@ static int __init acpi_cpufreq_init(void > #endif > > ret = cpufreq_register_driver(&acpi_cpufreq_driver); > - if (ret) > + if (ret) { > free_acpi_perf_data(); > - else > + } else { > acpi_cpufreq_boost_init(); > + driver_registered = true; > + } > > return ret; > } > @@ -1032,6 +1036,8 @@ static int __init acpi_cpufreq_init(void > static void __exit acpi_cpufreq_exit(void) > { > pr_debug("acpi_cpufreq_exit\n"); > + if (!driver_registered) > + return; > > acpi_cpufreq_boost_exit(); Looks like the right solution here. But this kind of issues look to be somewhat generic, doesn't they? And probably most of the drivers would be struggling with such issues.. They are working because we normally have something like this in core unregister parts: int cpufreq_unregister_driver(struct cpufreq_driver *driver) { ... if (!cpufreq_driver || (driver != cpufreq_driver)) return -EINVAL; .... So, even in this case if we could actually check return value of cpufreq_unregister_driver() and then do the other stuff, then we wouldn't require this extra variable.. But the problem is the order in which things happen. Would this be a big problem if we do unregister first and then acpi_cpufreq_boost_exit(), based on what unregister returned? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c @@ -982,6 +982,8 @@ static void __exit acpi_cpufreq_boost_ex } } +static bool driver_registered; + static int __init acpi_cpufreq_init(void) { int ret; @@ -1021,10 +1023,12 @@ static int __init acpi_cpufreq_init(void #endif ret = cpufreq_register_driver(&acpi_cpufreq_driver); - if (ret) + if (ret) { free_acpi_perf_data(); - else + } else { acpi_cpufreq_boost_init(); + driver_registered = true; + } return ret; } @@ -1032,6 +1036,8 @@ static int __init acpi_cpufreq_init(void static void __exit acpi_cpufreq_exit(void) { pr_debug("acpi_cpufreq_exit\n"); + if (!driver_registered) + return; acpi_cpufreq_boost_exit();