acpi_bus_register_driver() returns -ENODEV after resume

Hi,
I've received some feedback about problems loading the asus_acpi driver back after swsusp resume. I managed to hit that problem on a 2.4.21 + 20030619 ACPI + swsusp1.1-rc2. I don't know yet if it is present in more recent versions, since this behaviour is not common (at least on my machine). Usually, the module can be loaded and unloaded without problems.

Anyway, judging on my quick glance at the code, it would seem that something disables acpi (i.e. sets acpi_disabled to a non-zero value) somewhere during the suspend / resume cycle, which in turn causes acpi_bus_register_driver to return -ENODEV. Is my reasoning correct, or am I missing some codepath? If not, it would signify a bug in ACPI / swsusp interaction. I'll try yet to hit this on a recent kernel.
Best regards,

Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/[email protected]


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

It's obscure, but I think I know the answer. From arch/i386/kernel/setup.c:

#ifdef  CONFIG_ACPI
        int acpi_disabled __initdata = 0;
#else
        int acpi_disabled __initdata = 1;

#endif
EXPORT_SYMBOL(acpi_disabled);

Since it's __initdata, the memory will be freed early in the init process (c.f. init/main.c::init() -> free_initmem()). Anything that uses it later is touching memory that is either unallocated, or allocated by something else. That's a bug.

Also, it doesn't need to be EXPORTed. If it is left as initdata, that's another bug, because the two are semantically disjoint. However, even with the patch below, I'm not sure why you would want to expose the variable (and I've in fact removed that).

Please give it a try and let me know if helps.

Tangential question: Do preprocessor conditionals evaluate to a value? I.e. instead of doing:

#ifdef	CONFIG_ACPI
	int acpi_disabled = 0;
#else
	int acpi_disabled = 1;

#endif

Could we do:

int acpi_disabled = (CONFIG_ACPI == 'y');

..or something?

Thanks,

Pat

===== arch/i386/kernel/setup.c 1.95 vs edited =====

@@ -64,11 +64,10 @@
 EXPORT_SYMBOL_GPL(mmu_cr4_features);
 
 #ifdef	CONFIG_ACPI
-	int acpi_disabled __initdata = 0;
+	int acpi_disabled = 0;
 #else
-	int acpi_disabled __initdata = 1;
+	int acpi_disabled = 1;
 #endif
-EXPORT_SYMBOL(acpi_disabled);
 
 #ifdef	CONFIG_ACPI_BOOT
 	int acpi_irq __initdata = 1;	/* enable IRQ */

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Bah, I just realized that you're using 2.4. The patch may not apply, but it should be straightforward to make the change manually. (Sorry, I don't have a 2.4 tree around).

Regardless, it is a bug if it is present in 2.4, and needs to be fixed.

Pat

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Thus wrote Patrick Mochel:

Sounds reasonable, but... what about modular ACPI? At least acpi_bus_register_driver() uses acpi_disabled, so it would seem to me that every time we're loading an ACPI driver module, acpi_disabled evaluates to something random, as in the aforementioned case?

Additionally, if acpi_disabled is to be available for modules (i.e. modular ACPI), it should rather be exported, shouldn't it?

AFAIK acpi_disabled is independent on CONFIG_ACPI, i.e. CONFIG_ACPI may be defined, but if acpi=off is passed to the kernel command line, acpi_disabled evaluates to 1?
Best regards and a 1:20 am CEST disclaimer: I might be talking nonsenses.

Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/[email protected]


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

acpi_bus_register_driver() is in drivers/acpi/scan.c, which is dependent on CONFIG_ACPI_BUS. That is a bool, so it cannot be a module. That function is exported for modular drivers to use, but it's only code in that function itself that access acpi_disabled (hence no need for it to be EXPORTed).

That's besides the point. acpi_disabled is defined no matter what. I'm only curious if we can make it more concise..

Pat

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

ACPI=y -> #define CONFIG_ACPI 1
ACPI=m -> #define CONFIG_ACPI_MODULE 1
ACPI=n -> #undef CONFIG_ACPI

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Thus wrote Patrick Mochel:

In short: 2.4.23-pre3 vanilla: problem present
2.4.23-pre3 with __initdata and EXPRT_SYMBOL removed: compiles fine, the problem seems to have disappeared.

The patch you made is OK for 2.6, I inline one for 2.4 (the same except for the top line)

@@ -176,11 +176,10 @@
 static u32 disabled_x86_caps[NCAPINTS] __initdata = { 0 };
 
 #ifdef CONFIG_ACPI_INTERPRETER
-       int acpi_disabled __initdata = 0;
+       int acpi_disabled = 0;
 #else
-       int acpi_disabled __initdata = 1;
+       int acpi_disabled = 1;
 #endif
-EXPORT_SYMBOL(acpi_disabled);
 
 #ifdef CONFIG_ACPI_BOOT
        int acpi_ht __initdata = 1;     /* enable HT */

Thanks for help, best regards,

Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/[email protected]


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf