Re: [2.5.50, ACPI] link error

Re: [2.5.50, ACPI] link error

[Adrian Bunk]

On Wed, Dec 04, 2002 at 12:41:19PM +0100, Pavel Machek wrote:

> Hi!

Hi Pavel!

> > When compiling 2.5.50 with CONFIG_ACPI_SLEEP=y
> 
> Enable SWSUSP... Then investigate why it is possible to select
> ACPI_SLEEP but not SWSUSP...

In drivers/acpi/Config.in in 2.5.44:

<--   snip  -->

...
if [ "$CONFIG_ACPI_HT_ONLY" != "y" ]; then
   bool     '  Sleep States' CONFIG_ACPI_SLEEP
...
   define_bool CONFIG_ACPI_SLEEP $CONFIG_SOFTWARE_SUSPEND
...
fi
...

<--  snip  -->


This double definition was at least confusing, a simple

  dep_bool '  Sleep States' CONFIG_ACPI_SLEEP $CONFIG_SOFTWARE_SUSPEND

would have expressed it better.


The transition to the new kconfig converted this confusing Config.in 
snipped to the following code in drivers/acpi/Kconfig:

<--  snip  -->

...
config ACPI_SLEEP
        bool "Sleep States"
        depends on X86 && ACPI && !ACPI_HT_ONLY
        default SOFTWARE_SUSPEND
...

<--  snip  -->


The following patch makes it work the way it was intended:

--- linux-2.5.50/drivers/acpi/Kconfig.old	2002-12-04 14:24:14.000000000 +0100
+++ linux-2.5.50/drivers/acpi/Kconfig	2002-12-04 14:27:17.000000000 +0100
@@ -58,7 +58,7 @@
 
 config ACPI_SLEEP
 	bool "Sleep States"
-	depends on X86 && ACPI && !ACPI_HT_ONLY
+	depends on X86 && ACPI && !ACPI_HT_ONLY && SOFTWARE_SUSPEND
 	default SOFTWARE_SUSPEND
 	---help---
 	  This option adds support for ACPI suspend states. 


Another thing that seems to be suboptimal is that the question for 
SOFTWARE_SUSPEND comes _after_ the question for ACPI_SLEEP, IOW someone 
doing a "make config" to configure his kernel doesn't have a chance to 
select ACPI_SLEEP. To fix this (and it seems to be logical) the 
SOFTWARE_SUSPEND question should be moved to the "Power management 
options" menu. The patch below tries to solve this and it makes ACPI 
dependant on PM as the comments in PM indicate:
- move "Software Suspend" to the "Power management options" menu above
  the ACPI entry
- let ACPI depend on PM

Any comments on this patch?

> 							Pavel

cu
Adrian


--- linux-2.5.50/arch/i386/Kconfig.old	2002-12-04 14:47:03.000000000 +0100
+++ linux-2.5.50/arch/i386/Kconfig	2002-12-04 14:43:48.000000000 +0100
@@ -789,8 +789,6 @@
 
 menu "Power management options (ACPI, APM)"
 
-source "drivers/acpi/Kconfig"
-
 config PM
 	bool "Power Management support"
 	---help---
@@ -811,6 +809,37 @@
 	  will issue the hlt instruction if nothing is to be done, thereby
 	  sending the processor to sleep and saving power.
 
+config SOFTWARE_SUSPEND
+        bool "Software Suspend (EXPERIMENTAL)"
+        depends on EXPERIMENTAL && PM
+        ---help---
+          Enable the possibilty of suspendig machine. It doesn't need APM.
+          You may suspend your machine by 'swsusp' or 'shutdown -z <time>'
+          (patch for sysvinit needed).
+
+          It creates an image which is saved in your active swaps. By the next
+          booting the, pass 'resume=/path/to/your/swap/file' and kernel will
+          detect the saved image, restore the memory from
+          it and then it continues to run as before you've suspended.
+          If you don't want the previous state to continue use the 'noresume'
+          kernel option. However note that your partitions will be fsck'd and
+          you must re-mkswap your swap partitions/files.
+
+          Right now you may boot without resuming and then later resume but
+          in meantime you cannot use those swap partitions/files which were
+          involved in suspending. Also in this case there is a risk that buffers
+          on disk won't match with saved ones.
+
+          SMP is supported ``as-is''. There's a code for it but doesn't work.
+          There have been problems reported relating SCSI.
+
+          This option is about getting stable. However there is still some
+          absence of features.
+
+          For more information take a look at Documentation/swsusp.txt.
+
+source "drivers/acpi/Kconfig"
+
 config APM
 	tristate "Advanced Power Management BIOS support"
 	depends on PM
@@ -1516,35 +1545,6 @@
 
 menu "Kernel hacking"
 
-config SOFTWARE_SUSPEND
-	bool "Software Suspend (EXPERIMENTAL)"
-	depends on EXPERIMENTAL && PM
-	---help---
-	  Enable the possibilty of suspendig machine. It doesn't need APM.
-	  You may suspend your machine by 'swsusp' or 'shutdown -z <time>' 
-	  (patch for sysvinit needed). 
-
-	  It creates an image which is saved in your active swaps. By the next
-	  booting the, pass 'resume=/path/to/your/swap/file' and kernel will 
-	  detect the saved image, restore the memory from
-	  it and then it continues to run as before you've suspended.
-	  If you don't want the previous state to continue use the 'noresume'
-	  kernel option. However note that your partitions will be fsck'd and
-	  you must re-mkswap your swap partitions/files.
-
-	  Right now you may boot without resuming and then later resume but
-	  in meantime you cannot use those swap partitions/files which were
-	  involved in suspending. Also in this case there is a risk that buffers
-	  on disk won't match with saved ones.
-
-	  SMP is supported ``as-is''. There's a code for it but doesn't work.
-	  There have been problems reported relating SCSI.
-
-	  This option is about getting stable. However there is still some
-	  absence of features.
-
-	  For more information take a look at Documentation/swsusp.txt.
-
 config DEBUG_KERNEL
 	bool "Kernel debugging"
 	help
--- linux-2.5.50/drivers/acpi/Kconfig.old	2002-12-04 14:39:24.000000000 +0100
+++ linux-2.5.50/drivers/acpi/Kconfig	2002-12-04 14:42:57.000000000 +0100
@@ -3,6 +3,7 @@
 #
 
 menu "ACPI Support"
+	depends on PM
 
 config ACPI
 	bool "ACPI Support" if X86

Re: [2.5.50, ACPI] link error

[Roman Zippel]

Hi,

On Wed, 4 Dec 2002, Adrian Bunk wrote:

> In drivers/acpi/Config.in in 2.5.44:
> 
> <--   snip  -->
> 
> ...
> if [ "$CONFIG_ACPI_HT_ONLY" != "y" ]; then
>    bool     '  Sleep States' CONFIG_ACPI_SLEEP
> ...
>    define_bool CONFIG_ACPI_SLEEP $CONFIG_SOFTWARE_SUSPEND
> ...
> fi
> ...
> 
> <--  snip  -->
> 
> 
> This double definition was at least confusing, a simple
> 
>   dep_bool '  Sleep States' CONFIG_ACPI_SLEEP $CONFIG_SOFTWARE_SUSPEND
> 
> would have expressed it better.

It's not really the same and the first probably heavily confuses xconfig.
If CONFIG_SOFTWARE_SUSPEND can do without CONFIG_ACPI_SLEEP, the second 
version should work fine.

>  config ACPI_SLEEP
>  	bool "Sleep States"
> -	depends on X86 && ACPI && !ACPI_HT_ONLY
> +	depends on X86 && ACPI && !ACPI_HT_ONLY && SOFTWARE_SUSPEND
>  	default SOFTWARE_SUSPEND
>  	---help---
>  	  This option adds support for ACPI suspend states. 

The default is probably not needed anymore.

> Another thing that seems to be suboptimal is that the question for 
> SOFTWARE_SUSPEND comes _after_ the question for ACPI_SLEEP, IOW someone 
> doing a "make config" to configure his kernel doesn't have a chance to 
> select ACPI_SLEEP.

That was only true with the old "make config". :)

> To fix this (and it seems to be logical) the 
> SOFTWARE_SUSPEND question should be moved to the "Power management 
> options" menu. The patch below tries to solve this and it makes ACPI 
> dependant on PM as the comments in PM indicate:
> - move "Software Suspend" to the "Power management options" menu above
>   the ACPI entry
> - let ACPI depend on PM

Is ACPI really only a PM thingie?

Anyway, I had a cleaned up drivers/acpi/Kconfig lying around. The ACPI 
config file was not really easy to convert automatically. I integrated 
your change from above, it would be nice if some could check if every 
still works as it should.

bye, Roman

--- linux/drivers/acpi/Kconfig	31 Oct 2002 13:26:48 -0000	1.1.1.1
+++ linux/drivers/acpi/Kconfig	4 Dec 2002 20:12:43 -0000
@@ -35,9 +35,11 @@ config ACPI
 	  available at:
 	  <http://www.acpi.info>
 
+if ACPI
+
 config ACPI_HT_ONLY
 	bool "CPU Enumeration Only"
-	depends on X86 && ACPI && X86_LOCAL_APIC
+	depends on X86_LOCAL_APIC
 	---help---
 	  This option enables limited ACPI support -- just enough to 
 	  enumerate processors from the ACPI Multiple APIC Description 
@@ -51,15 +53,12 @@ config ACPI_HT_ONLY
 	  There is no command-line option to disable this, but the kernel
 	  will fall back to the MPS table if the MADT is not present.
 
-config ACPI_BOOT
-	bool
-	depends on IA64 && (!IA64_HP_SIM || IA64_SGI_SN) || X86 && ACPI && !ACPI_HT_ONLY || X86 && ACPI
-	default y
+
+if X86 && !ACPI_HT_ONLY || IA64 && !IA64_HP_SIM
 
 config ACPI_SLEEP
 	bool "Sleep States"
-	depends on X86 && ACPI && !ACPI_HT_ONLY
-	default SOFTWARE_SUSPEND
+	depends on X86 && SOFTWARE_SUSPEND
 	---help---
 	  This option adds support for ACPI suspend states. 
 
@@ -78,7 +77,7 @@ config ACPI_SLEEP
 
 config ACPI_AC
 	tristate "AC Adapter"
-	depends on X86 && ACPI && !ACPI_HT_ONLY
+	depends on X86
 	help
 	  This driver adds support for the AC Adapter object, which indicates
 	  whether a system is on AC, or not.  Typically, only mobile systems 
@@ -86,7 +85,7 @@ config ACPI_AC
 
 config ACPI_BATTERY
 	tristate "Battery"
-	depends on X86 && ACPI && !ACPI_HT_ONLY
+	depends on X86
 	help
 	  This driver adds support for battery information through
 	  /proc/acpi/battery. If you have a mobile system with a battery, 
@@ -94,7 +93,6 @@ config ACPI_BATTERY
 
 config ACPI_BUTTON
 	tristate "Button"
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	help
 	  This driver registers for events based on buttons, such as the
 	  power, sleep, and lid switch.  In the future, a daemon will read
@@ -104,14 +102,12 @@ config ACPI_BUTTON
 
 config ACPI_FAN
 	tristate "Fan"
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	help
 	  This driver adds support for ACPI fan devices, allowing user-mode 
 	  applications to perform basic fan control (on, off, status).
 
 config ACPI_PROCESSOR
 	tristate "Processor"
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	help
 	  This driver installs ACPI as the idle handler for Linux, and uses
 	  ACPI C2 and C3 processor states to save power, on systems that
@@ -119,7 +115,7 @@ config ACPI_PROCESSOR
 
 config ACPI_PROCESSOR_PERF
 	bool "Processor Performance States"
-	depends on X86 && ACPI && !ACPI_HT_ONLY && ACPI_PROCESSOR && CPU_FREQ
+	depends on X86 && ACPI_PROCESSOR && CPU_FREQ
 	help
 	  This driver adds support for CPU frequency scaling, if this is supported
 	  by the hardware and the BIOS. If you are compiling for a mobile system,
@@ -135,12 +131,12 @@ config ACPI_THERMAL
 	  may be damaged without it.
 
 config ACPI_NUMA
-	bool "NUMA support" if NUMA && (IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY)
-	default y if IA64 && IA64_SGI_SN
+	bool "NUMA support"
+	depends on NUMA
 
 config ACPI_TOSHIBA
 	tristate "Toshiba Laptop Extras"
-	depends on X86 && ACPI && !ACPI_HT_ONLY
+	depends on X86
 	---help---
 	  This driver adds support for access to certain system settings
 	  on "legacy free" Toshiba laptops.  These laptops can be recognized by
@@ -166,7 +162,6 @@ config ACPI_TOSHIBA
 
 config ACPI_DEBUG
 	bool "Debug Statements"
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	help
 	  The ACPI driver can optionally report errors with a great deal
 	  of verbosity. Saying Y enables these statements. This will increase
@@ -174,17 +169,15 @@ config ACPI_DEBUG
 
 config ACPI_BUS
 	bool
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	default y
 
 config ACPI_INTERPRETER
 	bool
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	default y
 
 config ACPI_EC
 	bool
-	depends on X86 && ACPI && !ACPI_HT_ONLY
+	depends on X86
 	default y
 	help
 	  This driver is required on some systems for the proper operation of
@@ -193,26 +186,35 @@ config ACPI_EC
 
 config ACPI_POWER
 	bool
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	default y
 
 config ACPI_PCI
 	bool
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	default PCI
 
 config ACPI_SYSTEM
 	bool
-	depends on IA64 && !IA64_HP_SIM || X86 && ACPI && !ACPI_HT_ONLY
 	default y
 	help
 	  This driver will enable your system to shut down using ACPI, and
 	  dump your ACPI DSDT table using /proc/acpi/dsdt.
 
+endif
+
+config ACPI_NUMA
+	depends IA64 && IA64_SGI_SN
+	default y
+
 config ACPI_EFI
 	bool
 	depends on IA64 && (!IA64_HP_SIM || IA64_SGI_SN)
 	default y
+
+config ACPI_BOOT
+	bool
+	default y
+
+endif
 
 endmenu
 

Re: [2.5.50, ACPI] link error

[Pavel Machek]

Hi!

> > > Doesn't that imply your fix is broken to begin with?
> > 
> > ACPI/S4 support needs swsusp. ACPI/S3 needs big part of
> > swsusp. Splitting CONFIG_ACPI_SLEEP to S3 and S4 part seems like
> > overdesign to me, OTOH if you do the work it is okay with me.
> 
> You broke the design. S3 support was developed long before swsusp was in 
> the kernel, and completely indpendent of it. It should have remained that 
> way. 
> 
> S3 support is a subset of what is need for S4 support. 

That's not true. acpi_wakeup.S is nasty piece of code, needed for S3
but not for S4. Big part of driver support is only needed for S3.

> swsusp is an implementation of S4 support. In theory, there could be 
> multiple implementations that all use the same core (saving/restoring 
> state). 

There were patches for S4bios floating around, but it never really
worked, IIRC.

> There could also be different power management schemes that use swsusp as 
> an implementation for suspend-to-disk. But, that's another tangent. 
> 
> CONFIG_ACPI_SLEEP should give you S3 support, and the ACPI side of S4 
> support. 

What's ACPI side of S4 good for when you can not do S4?

> The comment in the config option should tell the user that they 
> must choose a suspend implementation (e.g. CONFIG_SUSPEND, which should 
> prolly be CONFIG_SWAP_SUSPEND) in order to get complete S4 support. (The 
> ACPI side can make an empty call to swsusp if no implementation is 
> selected). 

S3 needs process stopper from kernel/suspend.c. I did not want to have
#ifdefs all over suspend.c...

> Some time ago, I made a BK repo for suspend support. I axed it, since no 
> one ever used it. But, it's back again, and I'll be integrating your 
> patches and try to dedicate a few extra cycles to resolving some of the 
> issues. I'll send an announcement to the list once I've integrated your 
> patches. 

I probably will not persuade you to make it CVS, right? [Sorry, I'm
not going to touch bitkeeper.]
								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: [2.5.50, ACPI] link error

[Patrick Mochel]

> > S3 support is a subset of what is need for S4 support. 
> 
> That's not true. acpi_wakeup.S is nasty piece of code, needed for S3
> but not for S4. Big part of driver support is only needed for S3.

Ok, acpi_wakeup.S is only for S3.

As for drivers, I'm dubious of swsusp's handling of device and driver
support. A suspend cycle is supposed to leave devices in the same state
they were in before the cycle.  So, you need suspend and resume hooks in
the drivers, even for S4 support, to capture and restore context in the
devices themselves. Then again, I've no proof that swsusp doesn't get 
everything right as is.

> > CONFIG_ACPI_SLEEP should give you S3 support, and the ACPI side of S4 
> > support. 
> 
> What's ACPI side of S4 good for when you can not do S4?

To not litter the code with #ifdefs. 

> > The comment in the config option should tell the user that they 
> > must choose a suspend implementation (e.g. CONFIG_SUSPEND, which should 
> > prolly be CONFIG_SWAP_SUSPEND) in order to get complete S4 support. (The 
> > ACPI side can make an empty call to swsusp if no implementation is 
> > selected). 
> 
> S3 needs process stopper from kernel/suspend.c. I did not want to have
> #ifdefs all over suspend.c...

Then break it up into separate files in a separate directory.

> > Some time ago, I made a BK repo for suspend support. I axed it, since no 
> > one ever used it. But, it's back again, and I'll be integrating your 
> > patches and try to dedicate a few extra cycles to resolving some of the 
> > issues. I'll send an announcement to the list once I've integrated your 
> > patches. 
> 
> I probably will not persuade you to make it CVS, right? [Sorry, I'm
> not going to touch bitkeeper.]

I know, and that's fine. I won't touch CVS again, unless there's a hefty 
sum and a lot of good beer involved. (Or, after I've consumed a lot of 
good beer). Patches can be made from the repo, most easily after merging 
to a new kernel version.

	-pat

Re: [2.5.50, ACPI] link error

[Pavel Machek]

Hi!

> > > S3 support is a subset of what is need for S4 support. 
> > 
> > That's not true. acpi_wakeup.S is nasty piece of code, needed for S3
> > but not for S4. Big part of driver support is only needed for S3.
> 
> Ok, acpi_wakeup.S is only for S3.
> 
> As for drivers, I'm dubious of swsusp's handling of device and driver
> support. A suspend cycle is supposed to leave devices in the same state

Some devices do not really have a state (timer -- it needs to be set
to 1kHz, but that's it), and they do not need support for S4 but need
it for S3.

> > > The comment in the config option should tell the user that they 
> > > must choose a suspend implementation (e.g. CONFIG_SUSPEND, which should 
> > > prolly be CONFIG_SWAP_SUSPEND) in order to get complete S4 support. (The 
> > > ACPI side can make an empty call to swsusp if no implementation is 
> > > selected). 
> > 
> > S3 needs process stopper from kernel/suspend.c. I did not want to have
> > #ifdefs all over suspend.c...
> 
> Then break it up into separate files in a separate directory.

Uff, having kernel/suspend/freezer.c and kernel/suspend/disk.c would
seem very ugly to me, and freezer is pretty short in fact... I do not
think we want separate directory for suspend.

> > > Some time ago, I made a BK repo for suspend support. I axed it, since no 
> > > one ever used it. But, it's back again, and I'll be integrating your 
> > > patches and try to dedicate a few extra cycles to resolving some of the 
> > > issues. I'll send an announcement to the list once I've integrated your 
> > > patches. 
> > 
> > I probably will not persuade you to make it CVS, right? [Sorry, I'm
> > not going to touch bitkeeper.]
> 
> I know, and that's fine. I won't touch CVS again, unless there's a hefty 
> sum and a lot of good beer involved. (Or, after I've consumed a lot of 
> good beer). Patches can be made from the repo, most easily after merging 
> to a new kernel version.

:-) there should be some good beer around here ;-).

I'd like to keep it simple for now. I feel alone developing sleep on
2.5, and it is easier for me not to ave to test different
configurations. So I think ACPI_SLEEP requiring SOFTWARE_SUSPEND is
okay for now (code bloat is not too bad). If you are joining and will
work on ACPI_SLEEP && !SOFTWARE_SUSPEND, you can easily catch
non-compilations and similar mistakes, and it will be okay to separate
the two.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: [2.5.50, ACPI] link error

[Jeff Garzik]

Pavel Machek wrote:
> S3 needs process stopper from kernel/suspend.c. I did not want to have
> #ifdefs all over suspend.c...


Then move it to a different file, that may be shared between various 
CONFIG_xxx features.