LinuxLists.cc - ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Do you have CONFIG_ACPI_SLEEP enabled in your .config?

Andrew Morton wrote:
> On Tue, 25 Sep 2007 08:51:09 +0200 Damien Wyart <[email protected]> wrote:
>
>> Hello,
>>
>> After testing rc8, I noticed that I couldn't power off the computer
>> directly, it only got halted and I had to press the power button
>> manually. Just before displaying "System halted", the following message
>> is displayed:
>>
>> ACPI : PCI interrupt for device 0000:02:08.0 disabled
>>
>> I had to first revert 5a50fe709d527f31169263e36601dd83446d5744 then
>> f216cc3748a3a22c2b99390fddcdafa0583791a2 (handling of Sx states) to
>> recover previous behaviour.
>
> Let's add some cc's.
>
>> Attached are the dmesg for rc7, rc8 and rc8 with the two patches
>> reverted.
>
> "diff -u dmesg.rc8_revert dmesg.rc8" says:
>
> --- dmesg.rc8_revert 2007-09-25 00:31:33.000000000 -0700
> +++ dmesg.rc8 2007-09-25 00:31:30.000000000 -0700
> @@ -1,4 +1,4 @@
> -Linux version 2.6.23-rc8-25092007dw (root@brouette) (gcc version 4.2.1 (Debian 4.2.1-5)) #2 SMP Tue Sep 25 08:31:09 CEST 2007
> +Linux version 2.6.23-rc8-25092007dw (root@brouette) (gcc version 4.2.1 (Debian 4.2.1-5)) #1 SMP Tue Sep 25 07:27:10 CEST 2007
> BIOS-provided physical RAM map:
> BIOS-e820: 0000000000000000 - 00000000000a0000 (usable)
> BIOS-e820: 00000000000f0000 - 0000000000100000 (reserved)
> @@ -72,18 +72,18 @@
> console [tty0] enabled
> Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
> Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
> -Memory: 2074780k/2096592k available (2175k kernel code, 20712k reserved, 815k data, 192k init, 1179088k highmem)
> +Memory: 2074780k/2096592k available (2175k kernel code, 20712k reserved, 816k data, 192k init, 1179088k highmem)
> virtual kernel memory layout:
> fixmap : 0xfff9b000 - 0xfffff000 ( 400 kB)
> pkmap : 0xff800000 - 0xffc00000 (4096 kB)
> vmalloc : 0xf8800000 - 0xff7fe000 ( 111 MB)
> lowmem : 0xc0000000 - 0xf8000000 ( 896 MB)
> .init : 0xc03f0000 - 0xc0420000 ( 192 kB)
> - .data : 0xc031fd5c - 0xc03ebd04 ( 815 kB)
> - .text : 0xc0100000 - 0xc031fd5c (2175 kB)
> + .data : 0xc031fcd4 - 0xc03ebd04 ( 816 kB)
> + .text : 0xc0100000 - 0xc031fcd4 (2175 kB)
> Checking if this processor honours the WP bit even in supervisor mode... Ok.
> SLUB: Genslabs=22, HWalign=64, Order=0-1, MinObjects=4, CPUs=2, Nodes=1
> -Calibrating delay using timer specific routine.. 5987.66 BogoMIPS (lpj=2993833)
> +Calibrating delay using timer specific routine.. 5987.60 BogoMIPS (lpj=2993802)
> Mount-cache hash table entries: 512
> CPU: After generic identify, caps: bfebfbff 00000000 00000000 00000000 0000041d 00000000 00000000 00000000
> monitor/mwait feature present.
> @@ -104,7 +104,7 @@
> Booting processor 1/1 eip 2000
> CPU 1 irqstacks, hard=c0426000 soft=c0424000
> Initializing CPU#1
> -Calibrating delay using timer specific routine.. 5984.18 BogoMIPS (lpj=2992093)
> +Calibrating delay using timer specific routine.. 5984.16 BogoMIPS (lpj=2992084)
> CPU: After generic identify, caps: bfebfbff 00000000 00000000 00000000 0000041d 00000000 00000000 00000000
> monitor/mwait feature present.
> CPU: Trace cache: 12K uops, L1 D cache: 16K
> @@ -116,7 +116,7 @@
> CPU1: Intel P4/Xeon Extended MCE MSRs (12) available
> CPU1: Thermal monitoring enabled
> CPU1: Intel(R) Pentium(R) 4 CPU 3.00GHz stepping 03
> -Total of 2 processors activated (11971.85 BogoMIPS).
> +Total of 2 processors activated (11971.77 BogoMIPS).
> ENABLING IO-APIC IRQs
> ..TIMER: vector=0x31 apic1=0 pin1=2 apic2=-1 pin2=-1
> checking TSC synchronization [CPU#0 -> CPU#1]: passed.
> @@ -279,6 +279,7 @@
> EXT3-fs: mounted filesystem with ordered data mode.
> VFS: Mounted root (ext3 filesystem) readonly.
> Freeing unused kernel memory: 192k freed
> +input: PC Speaker as /devices/platform/pcspkr/input/input4
> sr0: scsi3-mmc drive: 1x/48x cd/rw xa/form2 cdda tray
> Uniform CD-ROM driver Revision: 3.20
> sr 1:0:0:0: Attached scsi CD-ROM sr0
> @@ -287,7 +288,6 @@
> usbcore: registered new interface driver usbfs
> usbcore: registered new interface driver hub
> usbcore: registered new device driver usb
> -input: PC Speaker as /devices/platform/pcspkr/input/input4
> parport_pc 00:0a: reported by Plug and Play ACPI
> parport0: PC-style at 0x378 (0x778), irq 7, using FIFO [PCSPP,TRISTATE,COMPAT,ECP]
> USB Universal Host Controller Interface driver v3.0
> @@ -299,46 +299,42 @@
> usb usb1: configuration #1 chosen from 1 choice
> hub 1-0:1.0: USB hub found
> hub 1-0:1.0: 2 ports detected
> -ACPI: PCI Interrupt 0000:00:1d.1[B] -> GSI 19 (level, low) -> IRQ 20
> -PCI: Setting latency timer of device 0000:00:1d.1 to 64
> -uhci_hcd 0000:00:1d.1: UHCI Host Controller
> -uhci_hcd 0000:00:1d.1: new USB bus registered, assigned bus number 2
> -uhci_hcd 0000:00:1d.1: irq 20, io base 0x0000ff60
> +ACPI: PCI Interrupt 0000:00:1d.7[D] -> GSI 23 (level, low) -> IRQ 20
> +PCI: Setting latency timer of device 0000:00:1d.7 to 64
> +ehci_hcd 0000:00:1d.7: EHCI Host Controller
> +ehci_hcd 0000:00:1d.7: new USB bus registered, assigned bus number 2
> +ehci_hcd 0000:00:1d.7: debug port 1
> +PCI: cache line size of 128 is not supported by device 0000:00:1d.7
> +ehci_hcd 0000:00:1d.7: irq 20, io mem 0xffa80800
> +ehci_hcd 0000:00:1d.7: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004
> usb usb2: configuration #1 chosen from 1 choice
> hub 2-0:1.0: USB hub found
> -hub 2-0:1.0: 2 ports detected
> +hub 2-0:1.0: 8 ports detected
> +ACPI: PCI Interrupt 0000:00:1d.1[B] -> GSI 19 (level, low) -> IRQ 21
> +PCI: Setting latency timer of device 0000:00:1d.1 to 64
> +uhci_hcd 0000:00:1d.1: UHCI Host Controller
> +uhci_hcd 0000:00:1d.1: new USB bus registered, assigned bus number 3
> +uhci_hcd 0000:00:1d.1: irq 21, io base 0x0000ff60
> +usb usb3: configuration #1 chosen from 1 choice
> +hub 3-0:1.0: USB hub found
> +hub 3-0:1.0: 2 ports detected
> ACPI: PCI Interrupt 0000:00:1d.2[C] -> GSI 18 (level, low) -> IRQ 17
> PCI: Setting latency timer of device 0000:00:1d.2 to 64
> uhci_hcd 0000:00:1d.2: UHCI Host Controller
> -uhci_hcd 0000:00:1d.2: new USB bus registered, assigned bus number 3
> +uhci_hcd 0000:00:1d.2: new USB bus registered, assigned bus number 4
> uhci_hcd 0000:00:1d.2: irq 17, io base 0x0000ff40
> -usb usb3: configuration #1 chosen from 1 choice
> -hub 3-0:1.0: USB hub found
> -hub 3-0:1.0: 2 ports detected
> -usb 1-1: new full speed USB device using uhci_hcd and address 2
> +usb usb4: configuration #1 chosen from 1 choice
> +hub 4-0:1.0: USB hub found
> +hub 4-0:1.0: 2 ports detected
> ACPI: PCI Interrupt 0000:00:1d.3[A] -> GSI 16 (level, low) -> IRQ 19
> PCI: Setting latency timer of device 0000:00:1d.3 to 64
> uhci_hcd 0000:00:1d.3: UHCI Host Controller
> -uhci_hcd 0000:00:1d.3: new USB bus registered, assigned bus number 4
> +uhci_hcd 0000:00:1d.3: new USB bus registered, assigned bus number 5
> uhci_hcd 0000:00:1d.3: irq 19, io base 0x0000ff20
> -usb usb4: configuration #1 chosen from 1 choice
> -hub 4-0:1.0: USB hub found
> -hub 4-0:1.0: 2 ports detected
> -ACPI: PCI Interrupt 0000:00:1d.7[D] -> GSI 23 (level, low) -> IRQ 21
> -PCI: Setting latency timer of device 0000:00:1d.7 to 64
> -ehci_hcd 0000:00:1d.7: EHCI Host Controller
> -ehci_hcd 0000:00:1d.7: new USB bus registered, assigned bus number 5
> -ehci_hcd 0000:00:1d.7: debug port 1
> -PCI: cache line size of 128 is not supported by device 0000:00:1d.7
> -ehci_hcd 0000:00:1d.7: irq 21, io mem 0xffa80800
> -ehci_hcd 0000:00:1d.7: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004
> -usb 1-1: configuration #1 chosen from 1 choice
> -usb 1-1: can't set config #1, error -71
> usb usb5: configuration #1 chosen from 1 choice
> hub 5-0:1.0: USB hub found
> -hub 5-0:1.0: 8 ports detected
> -usb 1-1: USB disconnect, address 2
> -usb 1-1: new full speed USB device using uhci_hcd and address 3
> +hub 5-0:1.0: 2 ports detected
> +usb 1-1: new full speed USB device using uhci_hcd and address 2
> usb 1-1: configuration #1 chosen from 1 choice
> Adding 1212896k swap on /dev/sdb7. Priority:-1 extents:1 across:1212896k
> EXT3 FS on sdb2, internal journal
>

2007-09-25 09:08:59

by Torsten Kaiser

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On 9/25/07, Alexey Starikovskiy <[email protected]> wrote:
> Do you have CONFIG_ACPI_SLEEP enabled in your .config?

I'm answering that too, because I suspect that my "2.6.23-rc7-mm1 does
not power off"-error might have the same cause.

No, I do not have CONFIG_ACPI_SLEEP set,
because I do not have CONFIG_PM_SLEEP set,
because I do not want SUSPEND and/or HIBERNATION.

.config from 2.6.23-rc7-mm1 attached.

Torsten

Attachments:

(No filename) (423.00 B)
config.txt (47.22 kB)
Download all attachments

2007-09-25 09:28:10

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Hello,

> On 9/25/07, Alexey Starikovskiy <[email protected]> wrote:
> > Do you have CONFIG_ACPI_SLEEP enabled in your .config?

* Torsten Kaiser <[email protected]> [2007-09-25 11:08]:
> I'm answering that too, because I suspect that my "2.6.23-rc7-mm1 does
> not power off"-error might have the same cause.

> No, I do not have CONFIG_ACPI_SLEEP set,
> because I do not have CONFIG_PM_SLEEP set,
> because I do not want SUSPEND and/or HIBERNATION.

Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
reason (and this worked fine without them in rc7). I do not think these
settings should have changed between rc7 and rc8.

--
Damien Wyart

2007-09-25 09:59:12

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

> > No, I do not have CONFIG_ACPI_SLEEP set,
> > because I do not have CONFIG_PM_SLEEP set,
> > because I do not want SUSPEND and/or HIBERNATION.

> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> reason (and this worked fine without them in rc7). I do not think
> these settings should have changed between rc7 and rc8.

Also, another test I just did: on another computer, rc8 is fine
regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
provide config if needed.

Best,

--
Damien Wyart

2007-09-25 11:02:31

by Daniel Ritz

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

does that one help?

[ as attachment since i'm on webmail ]

ACPI: acpi_sleep_prepare() should not depent on CONFIG_SUSPEND
Signed-off-by: Daniel Ritz <[email protected]>
--
Ist Ihr Browser Vista-kompatibel? Jetzt die neuesten
Browser-Versionen downloaden: http://www.gmx.net/de/go/browser

Attachments:

acpi-sleep.patch (508.00 B)

2007-09-25 11:25:57

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Torsten Kaiser wrote:
> On 9/25/07, Alexey Starikovskiy <[email protected]> wrote:
>> Do you have CONFIG_ACPI_SLEEP enabled in your .config?
>
> I'm answering that too, because I suspect that my "2.6.23-rc7-mm1 does
> not power off"-error might have the same cause.
>
> No, I do not have CONFIG_ACPI_SLEEP set,
> because I do not have CONFIG_PM_SLEEP set,
> because I do not want SUSPEND and/or HIBERNATION.
This is not the reason. SUSPEND is controlled with CONFIG_SUSPEND and
HIBERNATION is controlled with CONFIG_HIBERNATION.
But if you want S5 ACPI sleep state you might want to enable ACPI_SLEEP...
>
> .config from 2.6.23-rc7-mm1 attached.
>
> Torsten
>

2007-09-25 11:29:46

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Daniel, thanks for the patch, but patch for solving your issue is already done.
And it is different from one we having here.

If you feel "patchy" today you may try to remove ACPI_SLEEP from
drivers/acpi/sleep/Makefile in the raw of main.o

Regards,
Alex.

Daniel Ritz wrote:
> does that one help?
>
> [ as attachment since i'm on webmail ]
>
> ACPI: acpi_sleep_prepare() should not depent on CONFIG_SUSPEND
> Signed-off-by: Daniel Ritz <[email protected]>

2007-09-25 11:44:21

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> > > No, I do not have CONFIG_ACPI_SLEEP set,
> > > because I do not have CONFIG_PM_SLEEP set,
> > > because I do not want SUSPEND and/or HIBERNATION.
>
> > Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> > reason (and this worked fine without them in rc7). I do not think
> > these settings should have changed between rc7 and rc8.

Well, we haven't changed much.

> Also, another test I just did: on another computer, rc8 is fine
> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> provide config if needed.

On the box that fails to power off, can you please test -rc8 with these two
commits reverted:

commit 5a50fe709d527f31169263e36601dd83446d5744
ACPI: suspend: consolidate handling of Sx states addendum

commit f216cc3748a3a22c2b99390fddcdafa0583791a2
ACPI: suspend: consolidate handling of Sx states.

and see if it works?

Greetings,
Rafael

2007-09-25 11:49:12

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 13:02, Daniel Ritz wrote:
> does that one help?
>
> [ as attachment since i'm on webmail ]
>
> ACPI: acpi_sleep_prepare() should not depent on CONFIG_SUSPEND
> Signed-off-by: Daniel Ritz <[email protected]>

An alternative fix for this has been sent to Len:

http://marc.info/?l=linux-kernel&m=119052978117735&w=4

Please test.

Greetings,
Rafael

2007-09-25 11:51:24

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> > > > No, I do not have CONFIG_ACPI_SLEEP set,
> > > > because I do not have CONFIG_PM_SLEEP set,
> > > > because I do not want SUSPEND and/or HIBERNATION.
> >
> > > Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> > > reason (and this worked fine without them in rc7). I do not think
> > > these settings should have changed between rc7 and rc8.
>
> Well, we haven't changed much.
>
> > Also, another test I just did: on another computer, rc8 is fine
> > regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> > provide config if needed.
>
> On the box that fails to power off, can you please test -rc8 with these two
> commits reverted:
>
> commit 5a50fe709d527f31169263e36601dd83446d5744
> ACPI: suspend: consolidate handling of Sx states addendum
>
> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> ACPI: suspend: consolidate handling of Sx states.
>
> and see if it works?

If it does, please test the patch from this message

http://marc.info/?l=linux-kernel&m=119052978117735&w=4

on top of vanilla 2.6.23-rc8.

Greetings,
Rafael

2007-09-25 12:02:19

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

> On the box that fails to power off, can you please test -rc8 with these two
> commits reverted:

> commit 5a50fe709d527f31169263e36601dd83446d5744
> ACPI: suspend: consolidate handling of Sx states addendum

> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> ACPI: suspend: consolidate handling of Sx states.

> and see if it works?

I had done these tests in the first place (see my first mail), so yes,
reverting makes the box power off fine.

Will test this evening the patch you pointed in your next message.

--
Damien Wyart

2007-09-25 12:05:51

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>>>> because I do not have CONFIG_PM_SLEEP set,
>>>>> because I do not want SUSPEND and/or HIBERNATION.
>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
>>>> reason (and this worked fine without them in rc7). I do not think
>>>> these settings should have changed between rc7 and rc8.
>> Well, we haven't changed much.
>>
>>> Also, another test I just did: on another computer, rc8 is fine
>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
>>> provide config if needed.
>> On the box that fails to power off, can you please test -rc8 with these two
>> commits reverted:
>>
>> commit 5a50fe709d527f31169263e36601dd83446d5744
>> ACPI: suspend: consolidate handling of Sx states addendum
>>
>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
>> ACPI: suspend: consolidate handling of Sx states.
>>
>> and see if it works?
>
> If it does, please test the patch from this message
>
> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
>
> on top of vanilla 2.6.23-rc8.
You will need one more patch on top of just mentioned one.

Regards,
Alex.
>
> Greetings,
> Rafael
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Attachments:

fix-ACPI_SLEEP_states.patch (2.25 kB)

2007-09-25 12:25:47

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> >> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> >>>>> No, I do not have CONFIG_ACPI_SLEEP set,
> >>>>> because I do not have CONFIG_PM_SLEEP set,
> >>>>> because I do not want SUSPEND and/or HIBERNATION.
> >>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> >>>> reason (and this worked fine without them in rc7). I do not think
> >>>> these settings should have changed between rc7 and rc8.
> >> Well, we haven't changed much.
> >>
> >>> Also, another test I just did: on another computer, rc8 is fine
> >>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> >>> provide config if needed.
> >> On the box that fails to power off, can you please test -rc8 with these two
> >> commits reverted:
> >>
> >> commit 5a50fe709d527f31169263e36601dd83446d5744
> >> ACPI: suspend: consolidate handling of Sx states addendum
> >>
> >> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> >> ACPI: suspend: consolidate handling of Sx states.
> >>
> >> and see if it works?
> >
> > If it does, please test the patch from this message
> >
> > http://marc.info/?l=linux-kernel&m=119052978117735&w=4
> >
> > on top of vanilla 2.6.23-rc8.
> You will need one more patch on top of just mentioned one.

Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?

CONFIG_HIBERNATION needs acpi_target_sleep_state too.

Greetings,
Rafael

2007-09-25 12:54:18

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
>> Rafael J. Wysocki wrote:
>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>>>>>> because I do not have CONFIG_PM_SLEEP set,
>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
>>>>>> reason (and this worked fine without them in rc7). I do not think
>>>>>> these settings should have changed between rc7 and rc8.
>>>> Well, we haven't changed much.
>>>>
>>>>> Also, another test I just did: on another computer, rc8 is fine
>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
>>>>> provide config if needed.
>>>> On the box that fails to power off, can you please test -rc8 with these two
>>>> commits reverted:
>>>>
>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
>>>> ACPI: suspend: consolidate handling of Sx states addendum
>>>>
>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
>>>> ACPI: suspend: consolidate handling of Sx states.
>>>>
>>>> and see if it works?
>>> If it does, please test the patch from this message
>>>
>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
>>>
>>> on top of vanilla 2.6.23-rc8.
>> You will need one more patch on top of just mentioned one.
>
> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
>
> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
Agree, attaching updated patch.

Thanks,
Alex.

Attachments:

fix-ACPI_SLEEP_states.patch (2.23 kB)

2007-09-25 13:12:31

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> >>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> >>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
> >>>>>>> because I do not have CONFIG_PM_SLEEP set,
> >>>>>>> because I do not want SUSPEND and/or HIBERNATION.
> >>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> >>>>>> reason (and this worked fine without them in rc7). I do not think
> >>>>>> these settings should have changed between rc7 and rc8.
> >>>> Well, we haven't changed much.
> >>>>
> >>>>> Also, another test I just did: on another computer, rc8 is fine
> >>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> >>>>> provide config if needed.
> >>>> On the box that fails to power off, can you please test -rc8 with these two
> >>>> commits reverted:
> >>>>
> >>>> commit 5a50fe709d527f31169263e36601dd83446d5744
> >>>> ACPI: suspend: consolidate handling of Sx states addendum
> >>>>
> >>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> >>>> ACPI: suspend: consolidate handling of Sx states.
> >>>>
> >>>> and see if it works?
> >>> If it does, please test the patch from this message
> >>>
> >>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
> >>>
> >>> on top of vanilla 2.6.23-rc8.
> >> You will need one more patch on top of just mentioned one.
> >
> > Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
> >
> > CONFIG_HIBERNATION needs acpi_target_sleep_state too.
> Agree, attaching updated patch.

Well, please use "ifdef CONFIG_PM_SLEEP" instead of
"if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
as you did with the second block.

Greetings,
Rafael

2007-09-25 13:15:42

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
>> Rafael J. Wysocki wrote:
>>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
>>>> Rafael J. Wysocki wrote:
>>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
>>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
>>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
>>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
>>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
>>>>>>>> reason (and this worked fine without them in rc7). I do not think
>>>>>>>> these settings should have changed between rc7 and rc8.
>>>>>> Well, we haven't changed much.
>>>>>>
>>>>>>> Also, another test I just did: on another computer, rc8 is fine
>>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
>>>>>>> provide config if needed.
>>>>>> On the box that fails to power off, can you please test -rc8 with these two
>>>>>> commits reverted:
>>>>>>
>>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
>>>>>> ACPI: suspend: consolidate handling of Sx states addendum
>>>>>>
>>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
>>>>>> ACPI: suspend: consolidate handling of Sx states.
>>>>>>
>>>>>> and see if it works?
>>>>> If it does, please test the patch from this message
>>>>>
>>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
>>>>>
>>>>> on top of vanilla 2.6.23-rc8.
>>>> You will need one more patch on top of just mentioned one.
>>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
>>>
>>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
>> Agree, attaching updated patch.
>
> Well, please use "ifdef CONFIG_PM_SLEEP" instead of
> "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
> as you did with the second block.
I was thinking about that, but it seem to be less clear...
We need this variable only for suspend or hibernation, nothing else.
with pm_sleep it is not visible at all.

Thoughts?

2007-09-25 14:04:34

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 15:15, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
> >>>> Rafael J. Wysocki wrote:
> >>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> >>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> >>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
> >>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
> >>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
> >>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> >>>>>>>> reason (and this worked fine without them in rc7). I do not think
> >>>>>>>> these settings should have changed between rc7 and rc8.
> >>>>>> Well, we haven't changed much.
> >>>>>>
> >>>>>>> Also, another test I just did: on another computer, rc8 is fine
> >>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> >>>>>>> provide config if needed.
> >>>>>> On the box that fails to power off, can you please test -rc8 with these two
> >>>>>> commits reverted:
> >>>>>>
> >>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
> >>>>>> ACPI: suspend: consolidate handling of Sx states addendum
> >>>>>>
> >>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> >>>>>> ACPI: suspend: consolidate handling of Sx states.
> >>>>>>
> >>>>>> and see if it works?
> >>>>> If it does, please test the patch from this message
> >>>>>
> >>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
> >>>>>
> >>>>> on top of vanilla 2.6.23-rc8.
> >>>> You will need one more patch on top of just mentioned one.
> >>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
> >>>
> >>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
> >> Agree, attaching updated patch.
> >
> > Well, please use "ifdef CONFIG_PM_SLEEP" instead of
> > "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
> > as you did with the second block.
> I was thinking about that, but it seem to be less clear...
> We need this variable only for suspend or hibernation, nothing else.
> with pm_sleep it is not visible at all.
>
> Thoughts?

Well, PM_SLEEP is defined as (SUSPEND || HIBERNATION), please have a look
at kernel/power/Kconfig, and it was introduced exactly for the conditions like
this.

IOW, if we want something to be used for anything else than suspend or
hibernation, it shouldn't be defined under PM_SLEEP.

Greetings,
Rafael

2007-09-25 14:19:50

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 15:15, Alexey Starikovskiy wrote:
>> Rafael J. Wysocki wrote:
>>> On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
>>>> Rafael J. Wysocki wrote:
>>>>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
>>>>>> Rafael J. Wysocki wrote:
>>>>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
>>>>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
>>>>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>>>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
>>>>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
>>>>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
>>>>>>>>>> reason (and this worked fine without them in rc7). I do not think
>>>>>>>>>> these settings should have changed between rc7 and rc8.
>>>>>>>> Well, we haven't changed much.
>>>>>>>>
>>>>>>>>> Also, another test I just did: on another computer, rc8 is fine
>>>>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
>>>>>>>>> provide config if needed.
>>>>>>>> On the box that fails to power off, can you please test -rc8 with these two
>>>>>>>> commits reverted:
>>>>>>>>
>>>>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
>>>>>>>> ACPI: suspend: consolidate handling of Sx states addendum
>>>>>>>>
>>>>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
>>>>>>>> ACPI: suspend: consolidate handling of Sx states.
>>>>>>>>
>>>>>>>> and see if it works?
>>>>>>> If it does, please test the patch from this message
>>>>>>>
>>>>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
>>>>>>>
>>>>>>> on top of vanilla 2.6.23-rc8.
>>>>>> You will need one more patch on top of just mentioned one.
>>>>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
>>>>>
>>>>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
>>>> Agree, attaching updated patch.
>>> Well, please use "ifdef CONFIG_PM_SLEEP" instead of
>>> "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
>>> as you did with the second block.
>> I was thinking about that, but it seem to be less clear...
>> We need this variable only for suspend or hibernation, nothing else.
>> with pm_sleep it is not visible at all.
>>
>> Thoughts?
>
> Well, PM_SLEEP is defined as (SUSPEND || HIBERNATION), please have a look
> at kernel/power/Kconfig, and it was introduced exactly for the conditions like
> this.
I've seen this then I wrote the patch :) See my point, it is not clear,
that PM_SLEEP is equivalent to SUSPEND || HIBERNATION, one needs to
grep Kconfig files to find that -- it means that code becomes less readable,
and I would like to avoid that.
>
> IOW, if we want something to be used for anything else than suspend or
> hibernation, it shouldn't be defined under PM_SLEEP.
Agree, but we should distinguish there it is better to use PM_SLEEP,
and there it is better to use (SUSPEND || HIBERNATION) just to be more expressive...

Regards,
Alex.

2007-09-25 14:38:13

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 16:19, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 25 September 2007 15:15, Alexey Starikovskiy wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
> >>>> Rafael J. Wysocki wrote:
> >>>>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
> >>>>>> Rafael J. Wysocki wrote:
> >>>>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> >>>>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> >>>>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
> >>>>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
> >>>>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
> >>>>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> >>>>>>>>>> reason (and this worked fine without them in rc7). I do not think
> >>>>>>>>>> these settings should have changed between rc7 and rc8.
> >>>>>>>> Well, we haven't changed much.
> >>>>>>>>
> >>>>>>>>> Also, another test I just did: on another computer, rc8 is fine
> >>>>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> >>>>>>>>> provide config if needed.
> >>>>>>>> On the box that fails to power off, can you please test -rc8 with these two
> >>>>>>>> commits reverted:
> >>>>>>>>
> >>>>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
> >>>>>>>> ACPI: suspend: consolidate handling of Sx states addendum
> >>>>>>>>
> >>>>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> >>>>>>>> ACPI: suspend: consolidate handling of Sx states.
> >>>>>>>>
> >>>>>>>> and see if it works?
> >>>>>>> If it does, please test the patch from this message
> >>>>>>>
> >>>>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
> >>>>>>>
> >>>>>>> on top of vanilla 2.6.23-rc8.
> >>>>>> You will need one more patch on top of just mentioned one.
> >>>>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
> >>>>>
> >>>>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
> >>>> Agree, attaching updated patch.
> >>> Well, please use "ifdef CONFIG_PM_SLEEP" instead of
> >>> "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
> >>> as you did with the second block.
> >> I was thinking about that, but it seem to be less clear...
> >> We need this variable only for suspend or hibernation, nothing else.
> >> with pm_sleep it is not visible at all.
> >>
> >> Thoughts?
> >
> > Well, PM_SLEEP is defined as (SUSPEND || HIBERNATION), please have a look
> > at kernel/power/Kconfig, and it was introduced exactly for the conditions like
> > this.
> I've seen this then I wrote the patch :) See my point, it is not clear,
> that PM_SLEEP is equivalent to SUSPEND || HIBERNATION, one needs to
> grep Kconfig files to find that -- it means that code becomes less readable,
> and I would like to avoid that.

I see your point. Still, you are using PM_SLEEP in the same file, so someone
reading the code for the first time will have to find out what it is anyway.

OTOH, the only function of PM_SLEEP is to be a replacement for
(SUSPEND || HIBERNATION). It has no other meaning whatsoever.

[Well, sorry, I couldn't invent a better name.]

> > IOW, if we want something to be used for anything else than suspend or
> > hibernation, it shouldn't be defined under PM_SLEEP.
> Agree, but we should distinguish there it is better to use PM_SLEEP,
> and there it is better to use (SUSPEND || HIBERNATION) just to be more expressive...

Well, since PM_SLEEP is used as (SUSPEND || HIBERNATION) everywhere else,
I think that it would actually be confusing not to use it here. :-)

Greetings,
Rafael

2007-09-25 14:45:38

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 16:19, Alexey Starikovskiy wrote:
>> Rafael J. Wysocki wrote:
>>> On Tuesday, 25 September 2007 15:15, Alexey Starikovskiy wrote:
>>>> Rafael J. Wysocki wrote:
>>>>> On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
>>>>>> Rafael J. Wysocki wrote:
>>>>>>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
>>>>>>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
>>>>>>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>>>>>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
>>>>>>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
>>>>>>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
>>>>>>>>>>>> reason (and this worked fine without them in rc7). I do not think
>>>>>>>>>>>> these settings should have changed between rc7 and rc8.
>>>>>>>>>> Well, we haven't changed much.
>>>>>>>>>>
>>>>>>>>>>> Also, another test I just did: on another computer, rc8 is fine
>>>>>>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
>>>>>>>>>>> provide config if needed.
>>>>>>>>>> On the box that fails to power off, can you please test -rc8 with these two
>>>>>>>>>> commits reverted:
>>>>>>>>>>
>>>>>>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
>>>>>>>>>> ACPI: suspend: consolidate handling of Sx states addendum
>>>>>>>>>>
>>>>>>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
>>>>>>>>>> ACPI: suspend: consolidate handling of Sx states.
>>>>>>>>>>
>>>>>>>>>> and see if it works?
>>>>>>>>> If it does, please test the patch from this message
>>>>>>>>>
>>>>>>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
>>>>>>>>>
>>>>>>>>> on top of vanilla 2.6.23-rc8.
>>>>>>>> You will need one more patch on top of just mentioned one.
>>>>>>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
>>>>>>>
>>>>>>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
>>>>>> Agree, attaching updated patch.
>>>>> Well, please use "ifdef CONFIG_PM_SLEEP" instead of
>>>>> "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
>>>>> as you did with the second block.
>>>> I was thinking about that, but it seem to be less clear...
>>>> We need this variable only for suspend or hibernation, nothing else.
>>>> with pm_sleep it is not visible at all.
>>>>
>>>> Thoughts?
>>> Well, PM_SLEEP is defined as (SUSPEND || HIBERNATION), please have a look
>>> at kernel/power/Kconfig, and it was introduced exactly for the conditions like
>>> this.
>> I've seen this then I wrote the patch :) See my point, it is not clear,
>> that PM_SLEEP is equivalent to SUSPEND || HIBERNATION, one needs to
>> grep Kconfig files to find that -- it means that code becomes less readable,
>> and I would like to avoid that.
>
> I see your point. Still, you are using PM_SLEEP in the same file, so someone
> reading the code for the first time will have to find out what it is anyway.
In the second place it depends on header file using PM_SLEEP, so it makes sense.
>
> OTOH, the only function of PM_SLEEP is to be a replacement for
> (SUSPEND || HIBERNATION). It has no other meaning whatsoever.
>
> [Well, sorry, I couldn't invent a better name.]
>
>>> IOW, if we want something to be used for anything else than suspend or
>>> hibernation, it shouldn't be defined under PM_SLEEP.
>> Agree, but we should distinguish there it is better to use PM_SLEEP,
>> and there it is better to use (SUSPEND || HIBERNATION) just to be more expressive...
>
> Well, since PM_SLEEP is used as (SUSPEND || HIBERNATION) everywhere else,
> I think that it would actually be confusing not to use it here. :-)
Ok, patch is here.

Regards,
Alex.

Attachments:

fix-ACPI_SLEEP_states.patch (2.03 kB)

2007-09-25 15:07:19

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

> Will test this evening the patch you pointed in your next message.

Ok, with both patches (including the very latest one from Alexey ---
tried the "stylistically correct" one), machine halts fine again. Thanks
to all for having looked at this!

--
Damien Wyart

2007-09-25 15:40:58

by Linus Torvalds

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tue, 25 Sep 2007 08:51:09 +0200 Damien Wyart <[email protected]> wrote:
>
> Hello,
>
> After testing rc8, I noticed that I couldn't power off the computer
> directly, it only got halted and I had to press the power button
> manually. Just before displaying "System halted", the following message
> is displayed:
>
> ACPI : PCI interrupt for device 0000:02:08.0 disabled
>
> I had to first revert 5a50fe709d527f31169263e36601dd83446d5744 then
> f216cc3748a3a22c2b99390fddcdafa0583791a2 (handling of Sx states) to
> recover previous behaviour.

Hmm. Those things *do* seem to be suspicious.

For example, those commits seem to move code that used to be inside
CONFIG_PM (which pretty much *everybody* has) to be inside
CONFIG_ACPI_SLEEP (which is a totally different thing, and depends on
whether the user asked for suspend support or not!

Damien - does it work if you ask for SUSPEND or HIBERNATION support?

Len - why are you guys moving stuff into CONFIG_PM_SLEEP? I know you seem
to think that absolutely *everybody* should always support suspend and
hibernation, but the fact is, not everybody does. And it's a totally
separate thing for normal ACPI CPU runstate support that people have used
to manage a *running* CPU (and shutting it down).

Linus

2007-09-25 15:41:22

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 16:45, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 25 September 2007 16:19, Alexey Starikovskiy wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Tuesday, 25 September 2007 15:15, Alexey Starikovskiy wrote:
> >>>> Rafael J. Wysocki wrote:
> >>>>> On Tuesday, 25 September 2007 14:53, Alexey Starikovskiy wrote:
> >>>>>> Rafael J. Wysocki wrote:
> >>>>>>> On Tuesday, 25 September 2007 14:05, Alexey Starikovskiy wrote:
> >>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>> On Tuesday, 25 September 2007 13:45, Rafael J. Wysocki wrote:
> >>>>>>>>>> On Tuesday, 25 September 2007 11:58, Damien Wyart wrote:
> >>>>>>>>>>>>> No, I do not have CONFIG_ACPI_SLEEP set,
> >>>>>>>>>>>>> because I do not have CONFIG_PM_SLEEP set,
> >>>>>>>>>>>>> because I do not want SUSPEND and/or HIBERNATION.
> >>>>>>>>>>>> Same answer from my side: I do not have CONFIG_ACPI_SLEEP for the same
> >>>>>>>>>>>> reason (and this worked fine without them in rc7). I do not think
> >>>>>>>>>>>> these settings should have changed between rc7 and rc8.
> >>>>>>>>>> Well, we haven't changed much.
> >>>>>>>>>>
> >>>>>>>>>>> Also, another test I just did: on another computer, rc8 is fine
> >>>>>>>>>>> regarding ACPI power off, even if CONFIG_ACPI_SLEEP is not set. I can
> >>>>>>>>>>> provide config if needed.
> >>>>>>>>>> On the box that fails to power off, can you please test -rc8 with these two
> >>>>>>>>>> commits reverted:
> >>>>>>>>>>
> >>>>>>>>>> commit 5a50fe709d527f31169263e36601dd83446d5744
> >>>>>>>>>> ACPI: suspend: consolidate handling of Sx states addendum
> >>>>>>>>>>
> >>>>>>>>>> commit f216cc3748a3a22c2b99390fddcdafa0583791a2
> >>>>>>>>>> ACPI: suspend: consolidate handling of Sx states.
> >>>>>>>>>>
> >>>>>>>>>> and see if it works?
> >>>>>>>>> If it does, please test the patch from this message
> >>>>>>>>>
> >>>>>>>>> http://marc.info/?l=linux-kernel&m=119052978117735&w=4
> >>>>>>>>>
> >>>>>>>>> on top of vanilla 2.6.23-rc8.
> >>>>>>>> You will need one more patch on top of just mentioned one.
> >>>>>>> Hm, why did you put acpi_target_sleep_state under CONFIG_SUSPEND?
> >>>>>>>
> >>>>>>> CONFIG_HIBERNATION needs acpi_target_sleep_state too.
> >>>>>> Agree, attaching updated patch.
> >>>>> Well, please use "ifdef CONFIG_PM_SLEEP" instead of
> >>>>> "if defined(CONFIG_SUSPEND)||defined(CONFIG_HIBERNATION)",
> >>>>> as you did with the second block.
> >>>> I was thinking about that, but it seem to be less clear...
> >>>> We need this variable only for suspend or hibernation, nothing else.
> >>>> with pm_sleep it is not visible at all.
> >>>>
> >>>> Thoughts?
> >>> Well, PM_SLEEP is defined as (SUSPEND || HIBERNATION), please have a look
> >>> at kernel/power/Kconfig, and it was introduced exactly for the conditions like
> >>> this.
> >> I've seen this then I wrote the patch :) See my point, it is not clear,
> >> that PM_SLEEP is equivalent to SUSPEND || HIBERNATION, one needs to
> >> grep Kconfig files to find that -- it means that code becomes less readable,
> >> and I would like to avoid that.
> >
> > I see your point. Still, you are using PM_SLEEP in the same file, so someone
> > reading the code for the first time will have to find out what it is anyway.
> In the second place it depends on header file using PM_SLEEP, so it makes sense.
> >
> > OTOH, the only function of PM_SLEEP is to be a replacement for
> > (SUSPEND || HIBERNATION). It has no other meaning whatsoever.
> >
> > [Well, sorry, I couldn't invent a better name.]
> >
> >>> IOW, if we want something to be used for anything else than suspend or
> >>> hibernation, it shouldn't be defined under PM_SLEEP.
> >> Agree, but we should distinguish there it is better to use PM_SLEEP,
> >> and there it is better to use (SUSPEND || HIBERNATION) just to be more expressive...
> >
> > Well, since PM_SLEEP is used as (SUSPEND || HIBERNATION) everywhere else,
> > I think that it would actually be confusing not to use it here. :-)
> Ok, patch is here.

Acked-by: Rafael J. Wysocki <[email protected]>

and thanks for fixing this.

2007-09-25 15:46:18

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tuesday, 25 September 2007 17:40, Linus Torvalds wrote:
>
> On Tue, 25 Sep 2007 08:51:09 +0200 Damien Wyart <[email protected]> wrote:
> >
> > Hello,
> >
> > After testing rc8, I noticed that I couldn't power off the computer
> > directly, it only got halted and I had to press the power button
> > manually. Just before displaying "System halted", the following message
> > is displayed:
> >
> > ACPI : PCI interrupt for device 0000:02:08.0 disabled
> >
> > I had to first revert 5a50fe709d527f31169263e36601dd83446d5744 then
> > f216cc3748a3a22c2b99390fddcdafa0583791a2 (handling of Sx states) to
> > recover previous behaviour.
>
> Hmm. Those things *do* seem to be suspicious.
>
> For example, those commits seem to move code that used to be inside
> CONFIG_PM (which pretty much *everybody* has) to be inside
> CONFIG_ACPI_SLEEP (which is a totally different thing, and depends on
> whether the user asked for suspend support or not!
>
> Damien - does it work if you ask for SUSPEND or HIBERNATION support?
>
> Len - why are you guys moving stuff into CONFIG_PM_SLEEP? I know you seem
> to think that absolutely *everybody* should always support suspend and
> hibernation, but the fact is, not everybody does. And it's a totally
> separate thing for normal ACPI CPU runstate support that people have used
> to manage a *running* CPU (and shutting it down).

This was a mistake and fixes have already been posted:

http://marc.info/?l=linux-acpi&m=119052970904643&w=4
http://marc.info/?l=linux-acpi&m=119073173625910&w=4

Greetings,
Rafael

2007-09-25 16:23:49

by Andrew Morton

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On Tue, 25 Sep 2007 18:45:15 +0400 Alexey Starikovskiy <[email protected]> wrote:

> [fix-ACPI_SLEEP_states.patch text/x-patch (2.0KB)]
> ACPI: suspend: fix ACPI_SLEEP states
>
> From: Alexey Starikovskiy <[email protected]>
>
> Signed-off-by: Alexey Starikovskiy <[email protected]>
> ---
>
> drivers/acpi/sleep/Makefile | 2 +-
> drivers/acpi/sleep/main.c | 4 ++++
> include/acpi/acpi_drivers.h | 4 ----
> 3 files changed, 5 insertions(+), 5 deletions(-)

I get a reject applying this to current mainline. Easy enough to fix it,
but I worry that the fix might be incorrect when applied to some tree other
than that which you were working on.

2007-09-25 16:26:12

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Andrew,

There are 2 patches, this is the second.
Above, Rafael gave link to first. Here it is again:
http://marc.info/?l=linux-kernel&m=119052978117735&w=4

Sorry for confusion,
Alex.

Andrew Morton wrote:
> On Tue, 25 Sep 2007 18:45:15 +0400 Alexey Starikovskiy <[email protected]> wrote:
>
>> [fix-ACPI_SLEEP_states.patch text/x-patch (2.0KB)]
>> ACPI: suspend: fix ACPI_SLEEP states
>>
>> From: Alexey Starikovskiy <[email protected]>
>>
>> Signed-off-by: Alexey Starikovskiy <[email protected]>
>> ---
>>
>> drivers/acpi/sleep/Makefile | 2 +-
>> drivers/acpi/sleep/main.c | 4 ++++
>> include/acpi/acpi_drivers.h | 4 ----
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> I get a reject applying this to current mainline. Easy enough to fix it,
> but I worry that the fix might be incorrect when applied to some tree other
> than that which you were working on.
>
>

2007-09-25 16:30:24

by Torsten Kaiser

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

On 9/25/07, Alexey Starikovskiy <[email protected]> wrote:
> Torsten Kaiser wrote:
> > No, I do not have CONFIG_ACPI_SLEEP set,
> > because I do not have CONFIG_PM_SLEEP set,
> > because I do not want SUSPEND and/or HIBERNATION.
> This is not the reason. SUSPEND is controlled with CONFIG_SUSPEND and
> HIBERNATION is controlled with CONFIG_HIBERNATION.
> But if you want S5 ACPI sleep state you might want to enable ACPI_SLEEP...

What I meant with SUSPEND and/or HIBERNATION was CONFIG_SUSPEND /
CONFIG_HIBERNATION.

And the relations where from Kconfig:
from drivers/acpi/Kconfig:
config ACPI_SLEEP
bool
depends on PM_SLEEP
default y

-> no PM_SLEEP means no ACPI_SLEEP

from kernel/power/Kconfig:
config PM_SLEEP
bool
depends on SUSPEND || HIBERNATION
default y

-> No SUSPEND and/or HIBERNATION means no PM_SLEEP

And if I select SUSPEND and/or HIBERNATION I will not only build this
feature into the kernel, but also HOTPLUG_CPU and I want to avoid
that.

It's exactly as Linus said in his mail: Not everybody wants SUSPEND...

I should have formulated that better in my mail, but that was what I
wanted to say.

Back to debugging this:
http://marc.info/?l=linux-acpi&m=119052970904643&w=4
fails to apply against 2.6.23-rc7-mm1, but moving that function by
hand was not to difficult. ;)
(With only the second patch I got a link error...)

http://marc.info/?l=linux-acpi&m=119073173625910&w=4
applied, and a test showed that my system now powers off again.

Torsten

2007-09-25 16:31:53

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Torsten Kaiser wrote:
> On 9/25/07, Alexey Starikovskiy <[email protected]> wrote:
>> Torsten Kaiser wrote:
>>> No, I do not have CONFIG_ACPI_SLEEP set,
>>> because I do not have CONFIG_PM_SLEEP set,
>>> because I do not want SUSPEND and/or HIBERNATION.
>> This is not the reason. SUSPEND is controlled with CONFIG_SUSPEND and
>> HIBERNATION is controlled with CONFIG_HIBERNATION.
>> But if you want S5 ACPI sleep state you might want to enable ACPI_SLEEP...
>
> What I meant with SUSPEND and/or HIBERNATION was CONFIG_SUSPEND /
> CONFIG_HIBERNATION.
>
> And the relations where from Kconfig:
> from drivers/acpi/Kconfig:
> config ACPI_SLEEP
> bool
> depends on PM_SLEEP
> default y
>
> -> no PM_SLEEP means no ACPI_SLEEP
>
> from kernel/power/Kconfig:
> config PM_SLEEP
> bool
> depends on SUSPEND || HIBERNATION
> default y
>
> -> No SUSPEND and/or HIBERNATION means no PM_SLEEP
>
> And if I select SUSPEND and/or HIBERNATION I will not only build this
> feature into the kernel, but also HOTPLUG_CPU and I want to avoid
> that.
>
> It's exactly as Linus said in his mail: Not everybody wants SUSPEND...
>
> I should have formulated that better in my mail, but that was what I
> wanted to say.
>
>
> Back to debugging this:
> http://marc.info/?l=linux-acpi&m=119052970904643&w=4
> fails to apply against 2.6.23-rc7-mm1, but moving that function by
> hand was not to difficult. ;)
> (With only the second patch I got a link error...)
>
> http://marc.info/?l=linux-acpi&m=119073173625910&w=4
> applied, and a test showed that my system now powers off again.
>
> Torsten
Understood, patches are available, please test.

Regards,
Alex.

2007-09-25 16:34:43

[permalink] [raw]

Subject: Re: ACPI power off regression in 2.6.23-rc8 (NOT in rc7)

Torsten Kaiser wrote:
> Back to debugging this:
> http://marc.info/?l=linux-acpi&m=119052970904643&w=4
> fails to apply against 2.6.23-rc7-mm1, but moving that function by
> hand was not to difficult. ;)
> (With only the second patch I got a link error...)
>
> http://marc.info/?l=linux-acpi&m=119073173625910&w=4
> applied, and a test showed that my system now powers off again.

Good,
thanks, Alex.

2007-09-25 21:44:17

by Frans Pop

[permalink] [raw]

Subject: ACPI suspend/hibernate tests (was: ACPI power off regression in 2.6.23-rc8)

On Tuesday 25 September 2007, Rafael J. Wysocki wrote:
> On Tuesday, 25 September 2007 17:40, Linus Torvalds wrote:
> > Len - why are you guys moving stuff into CONFIG_PM_SLEEP? I know you
> > seem to think that absolutely *everybody* should always support suspend
> > and hibernation, but the fact is, not everybody does. And it's a
> > totally separate thing for normal ACPI CPU runstate support that people
> > have used to manage a *running* CPU (and shutting it down).
>
> This was a mistake and fixes have already been posted:
>
> http://marc.info/?l=linux-acpi&m=119052970904643&w=4
> http://marc.info/?l=linux-acpi&m=119073173625910&w=4

As this whole "separate suspend and hibernate" change seems to have been
trickier than expected, I have done some additional testing using
2.6.23-rc8 plus both patches referred to above.

I compiled kernels (i386) for my laptop (Toshiba Satellite A40) with:
A) PM + SUSPEND + HIBERNATION + ACPI
B) PM + SUSPEND + ACPI
C) PM + HIBERNATION + ACPI
D) PM + ACPI
E) PM + SUSPEND + HIBERNATION but _without_ ACPI
F) without PM

In all cases CONFIG_APM was not set; all compiled without errors.

With all I have have booted, tried to suspend (close lid, which runs sleep
script, which does 'echo mem >/sys/power/state'), tried to hibernate ('echo
disk >/sys/power/state') and powered off (all from KDE sessions).

RESULTS
boot s2ram s2disk off
A) OK OK OK OK
B) OK OK N/A OK
C) OK OK 1) OK OK
D) OK 2) OK 1) N/A OK
E) OK 3) N/A 4) ??? 5) Only halted 6)
F) OK N/A N/A Only halted 6)

Comments:
1) sleep script ran, but laptop stayed on
2) dmesg lists S0,S5 supported
/proc/acpi/{sleep,wakeup} do not exist (is that correct?)
3) /proc/acpi/{sleep,wakeup} do not exist; /sys/power/state only has 'mem'
so SUSPEND seems missing even though compiled in
4) sleep script did not even run (as expected)
5) System did suspend, but power stayed on with on console:
Shutdown: hda \ Power down. \ Shutdown: hda \ hda: lost interrupt (2x)
\ psmouse.c: Failed to deactivate mouse on isa0060/serio1
System powered off manually and resumed correctly after that
6) System halted correctly, but did not power off automatically

The results for E and F are possibly correct as basically the config
(without ACPI) was broken for this laptop. In fact, I was somewhat
surprised how well it behaved without ACPI.

I suspect that whether a system powers off without ACPI or not can depend on
the system?

I have dmesg after boot and some other info available for all kernels if
anybody is interested. If any additional tests are wanted, please ask.

Cheers,
FJP

2007-09-25 22:04:18