acpi_bus_register_driver() returns -ENODEV after resume

acpi_bus_register_driver() returns -ENODEV after resume

[Karol Kozimor]

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/A@public.gmane.org


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

Re: [ACPI] acpi_bus_register_driver() returns -ENODEV after resume

[Patrick Mochel]

> 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.

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 =====
--- 1.95/arch/i386/kernel/setup.c	Sun Aug 31 16:14:11 2003
+++ edited/arch/i386/kernel/setup.c	Tue Sep  2 15:35:00 2003
@@ -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

Re: [ACPI] acpi_bus_register_driver() returns -ENODEV after resume

[Patrick Mochel]

On Tue, 2 Sep 2003, Patrick Mochel wrote:

> 
> > 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.

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

Re: [ACPI] acpi_bus_register_driver() returns -ENODEV after resume

[Karol Kozimor]

Thus wrote Patrick Mochel:
> 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). 

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?

> 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'); 

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/A@public.gmane.org


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

Re: [ACPI] acpi_bus_register_driver() returns -ENODEV after resume

[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?

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). 

> > 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'); 
> 
> 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?

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

Re: [ACPI] acpi_bus_register_driver() returns -ENODEV after resume

[Matthew Wilcox]

On Tue, Sep 02, 2003 at 03:39:26PM -0700, Patrick Mochel wrote:
> 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? 

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

Re: acpi_bus_register_driver() returns -ENODEV after resume

[Karol Kozimor]

Thus wrote Patrick Mochel:
> 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. 

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)

--- linux-2.4.23-pre3/arch/i386/kernel/setup.c~ 2003-09-04 15:21:03.000000000 +0200
+++ linux-2.4.23-pre3/arch/i386/kernel/setup.c  2003-09-08 23:56:00.000000000 +0200
@@ -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/A@public.gmane.org


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