Re: System hang when trying to enter sleep/standby state

Re: System hang when trying to enter sleep/standby state

[Jens Haug]

> > > Think about how this thread started: $Random User sees his system doesn't
> > > suspend when writing '4' to that file, so he tries every other value.
> > 
> > This is not a random user. This is someone who plays around with his
> > system while testing beta kernel drivers. 
> > The random user at this stage doesn't know about acpi at all. The
> > random user in the future (when acpi comes with Suse or RedHat Linux 
> > out of the box) will only use the GUIs to suspend and such. So there's
> > no problem at all, I think.
> 
> The GUIs should not, and will not, support writing directly to that file. 
> In the perfect, abstract future, we will not even have /proc/acpi/sleep, 
> we will have an abstracted for entering sleep states. 
> 
> Note that GUIs already support shutting down safely. 

Then what's your problem? The random user will use these GUIs and 
will not crash his system this way.

> As a whole, we do enforce a minimum amount of policy we do not want to 
> lose users data. And that will happen. 

This happens every day. People use "rm -r" in the wrong directory,
people accidentally pull the plug, people mess around with hdparm...
There are thousands of ways to lose data. You can't change that.
Echoing 5 into /proc/acpi/sleep is one of the *least* likely things.
We're discussing a problem that doesn't really exist.

> Let me repeat, and try and get you to listen: 
> 
> You *will* corrupt your data if you do not flush the disk buffers. 
> You *will* corrupt your data if you do not flush the disk buffers. 
> You *will* corrupt your data if you do not flush the disk buffers. 

So? I think I turned my box off a hundred times without flushing
the disk buffers while testing acpi, swsusp and cpufreq. These
things do happen when you mess around with beta kernel drivers.
One really annoying thing is that my box freezes when I push Fn+F8.
And this really happens by accident, because Fn+F7 turns the LCD
backlight off. 

> You are right, though, if they start writing random values into /proc, it 
> is their fault. But, the random range of numbers to be written to  
> /proc/acpi/sleep is small, and not that random. The documentation suggests 
> that some values work and others don't. It's human nature to exercise 
> curiosity and experiment with other close values. If they do, which they 
> are so likely to, I do not want to see them cause any harm to themselves. 

I do. Because, as you said, the doku suggest that *some don't work*.
So if they experiment out of curiosity, well then it's all their 
fault. Maybe the doku could be more explicit on this (and say that
S5 *does* work, but should normally not be used).

> This particular item is dangerous and will be removed from the kernel.

Is it up to you to decide that? ;-)


Jens

-- 
Jens Haug
IKFF Universität Stuttgart              Tel. 0711/685-6422
Pfaffenwaldring 9                       Fax  0711/685-6356
70550 Stuttgart	                haug-X6ztD3ggwzuBAmxm6OvjtTjhTm2NLCe8@public.gmane.org



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: System hang when trying to enter sleep/standby state

[Patrick Mochel]

> > This particular item is dangerous and will be removed from the kernel.
> 
> Is it up to you to decide that? ;-)

As a matter of fact, yes it is. And it has been removed from Linus's 2.5 
tree as of last night. 

Note that the argument is moot anwyay - there exists a sysrq key 
combination that calls pm_power_off() (which points to acpi_power_off()). 
This gives users the ability to turn their system off immediately, though 
removes it from procfs. 

	-pat



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: Sneaking patches in (was Re: System hang when trying to enter sleep/standby state)

[Patrick Mochel]

> Are we playing patch wars or what?

Not at all. The changes were discussed and sent to Andy, who had me 
forward them to Linus. The issue in question - disallowing the user from 
entering S5 directly from /proc/acpi/sleep was discussed on the list, and 
Andy agreed that it should be removed. 

> I have not seen any patch on the lists, *and*
> you also moved big blocks of code to hide your
> changes. That's pretty nasty behaviour.

That's not true. The patch to make that change is here: 

http://marc.theaimsgroup.com/?l=acpi4linux&m=104508487609853&w=2

And the accusation about moving large blocks of code to cover it up is 
completely unfair. It's not a conspiracy. 

It was also explained, along with every other change in the changelog
entries. I know that you are opposed to using BitKeeper, so I recommend 
that you subscribe to, or read, the bk-commits-head list, which has every 
patch Linus commits, with diffstat output and full changelogs for each. 

You can find info here: 

http://vger.kernel.org/vger-lists.html

and an archive here:

http://marc.theaimsgroup.com/?l=bk-commits-head&r=1&w=2

FYI, there is also a bk-commits-24 for 2.4 commits by Marcelo.

	-pat






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

Re: System hang when trying to enter sleep/standby state

[Christian Zoz]

On Fri, Feb 14, Patrick Mochel wrote:
> 
> > > This particular item is dangerous and will be removed from the kernel.
> > 
> > Is it up to you to decide that? ;-)
> 
> As a matter of fact, yes it is. And it has been removed from Linus's 2.5 
> tree as of last night. 

This is very very poor.

> Note that the argument is moot anwyay - there exists a sysrq key 
> combination that calls pm_power_off() (which points to acpi_power_off()). 
> This gives users the ability to turn their system off immediately, though 
> removes it from procfs. 

Switch it off!!! It's dangerous!

:(

-- 

ciao, christian

  --------------------------------------------------------------------
    Verglichen mit jedem x-beliebigen Redmonder Betriebssystem-Clone
    ist Linux geradezu eine leuchtende Perle der Datensicherheit.
  ------ Frank Rennemann (http://www.linux-knowledge-portal.org) -----


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

Re: Sneaking patches in (was Re: System hang when trying to enter sleep/standby state)

[Pavel Machek]

Hi!

> > I have not seen any patch on the lists, *and*
> > you also moved big blocks of code to hide your
> > changes. That's pretty nasty behaviour.
> 
> That's not true. The patch to make that change is here: 
> 
> http://marc.theaimsgroup.com/?l=acpi4linux&m=104508487609853&w=2
> 
> And the accusation about moving large blocks of code to cover it up is 
> completely unfair. It's not a conspiracy. 

While I agree it is not a conspiracy, the change is still well
hidden. Relevant changesets are:

ChangeSet 1.1003.12.2, 2003/02/13 11:51:15-06:00, mochel-3NddpPZAyC0@public.gmane.org

	acpi: Split i386 support up. 
	
	- Created arch/i386/kernel/acpi/
	- Split file into boot.c and sleep.c.
	- Moved acpi_wakeup.S into there.


ChangeSet 1.1003.2.19, 2003/02/12 19:35:58-06:00, mochel-3NddpPZAyC0@public.gmane.org

	acpi sleep: divorce sleep functionality from power off
functionality.
	
	When ACPI turns the system off on shutdown, it actually enters
S5, a sleep
	state. This functionality is dependent on CONFIG_ACPI_SLEEP,
which is 
	dependent on CONFIG_SOFTWARE_SUSPEND.
	
	This patch breaks the power off functionality into a separate
file, and 
	removes the dependency on the above-mentioned crap. Finally,
power off works
	for me again. 
	
	Thanks to Tobias Ringstrom for the original patch.


ChangeSet 1.1003.2.2, 2003/02/12 15:26:39-06:00, mochel-3NddpPZAyC0@public.gmane.org

	acpi sleep: move sleep support into own subdirectory. 

Even with the changelogs, I can't decide which one of these it
is. Probably 2.19, but as it also moves files, commit list is useless
because it represent moves as delete all lines, add all lines, and any
change is hidden.

@@ -285,16 +271,6 @@
 	printk(")\n");
 
 	acpi_sleep_proc_init();
-
-	/* Install the soft-off (S5) handler. */
-	if (sleep_states[ACPI_STATE_S5]) {
-		pm_power_off = acpi_power_off;
-
-		/* workaround: some systems don't claim S4 support,
but they
-                   do support S5 (power-down). That is all we need,
so
-		   indicate support. */
-		sleep_states[ACPI_STATE_S4] = 1;
-	}
 
 	return_VALUE(0);

Are you sure about this one?

> It was also explained, along with every other change in the changelog
> entries. I know that you are opposed to using BitKeeper, so I recommend 
> that you subscribe to, or read, the bk-commits-head list, which has every 
> patch Linus commits, with diffstat output and full changelogs for
> each. 

Well, in the past we tried to catch bugs *before* they got to the
tree.

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


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

Sneaking patches in (was Re: System hang when trying to enter sleep/standby state)

[Pavel Machek]

Hi!

> > > This particular item is dangerous and will be removed from the kernel.
> > 
> > Is it up to you to decide that? ;-)
> 
> As a matter of fact, yes it is. And it has been removed from Linus's 2.5 
> tree as of last night. 

Are we playing patch wars or what?

I have not seen any patch on the lists, *and*
you also moved big blocks of code to hide your
changes. That's pretty nasty behaviour.

				Pavel



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

Re: Sneaking patches in (was Re: System hang when trying to enter sleep/standby state)

[Patrick Mochel]

On Mon, 17 Feb 2003, Pavel Machek wrote:

> Hi!
> 
> > > I have not seen any patch on the lists, *and*
> > > you also moved big blocks of code to hide your
> > > changes. That's pretty nasty behaviour.
> > 
> > That's not true. The patch to make that change is here: 
> > 
> > http://marc.theaimsgroup.com/?l=acpi4linux&m=104508487609853&w=2
> > 
> > And the accusation about moving large blocks of code to cover it up is 
> > completely unfair. It's not a conspiracy. 
> 
> While I agree it is not a conspiracy, the change is still well
> hidden. Relevant changesets are:

Actually, the relevant changeset is 

ChangeSet-6YnWl+pQIM5bgfau97FWYA@public.gmane.org, 2003-02-12 15:12:58-06:00, mochel@osdl.org
  sleep: fix /proc/acpi/sleep write handling.
  
  - Prevent users from screwing themselves by removing support for entering
    S5 from the proc file. S5 is 'soft-off' and the state the system enters
    when powering down. It needs to be preceded by a proper shutdown sequence
    and should not be triggered manually. 
  - Fix a potential unchecked array reference using the written value as the
    index.

The patch is 


diff -Nru a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
--- a/drivers/acpi/sleep.c      Mon Feb 17 10:18:20 2003
+++ b/drivers/acpi/sleep.c      Mon Feb 17 10:18:20 2003
@@ -318,14 +318,14 @@
        size_t                  count,
        loff_t                  *ppos)
 {
-       acpi_status             status = AE_OK;
+       acpi_status             status = AE_ERROR;
        char                    state_string[12] = {'\0'};
        u32                     state = 0;
 
        ACPI_FUNCTION_TRACE("acpi_system_write_sleep");
 
        if (count > sizeof(state_string) - 1)
-               return_VALUE(-EINVAL);
+               goto Done;
 
        if (copy_from_user(state_string, buffer, count))
                return_VALUE(-EFAULT);
@@ -333,22 +333,25 @@
        state_string[count] = '\0';
                                
        state = simple_strtoul(state_string, NULL, 0);
-                               
+                               
+       if (state < 1 || state > 4)
+               goto Done;
+
        if (!sleep_states[state])
                return_VALUE(-ENODEV);                                          
 
 #ifdef CONFIG_SOFTWARE_SUSPEND                                                 
        if (state == 4) {
                software_suspend();
-               return_VALUE(count);
+               goto Done;      
        }                       
 #endif                         
        status = acpi_suspend(state);
-                               
+ Done:                         
        if (ACPI_FAILURE(status))
-               return_VALUE(-ENODEV);
-       
-       return_VALUE(count);
+               return_VALUE(-EINVAL);
+       else
+               return_VALUE(count);
 }
 
 static int acpi_system_alarm_seq_show(struct seq_file *seq, void *offset)

(Careful, cut-n-pasted from an xterm)


> Even with the changelogs, I can't decide which one of these it
> is. Probably 2.19, but as it also moves files, commit list is useless
> because it represent moves as delete all lines, add all lines, and any
> change is hidden.

As a personal choice, I _never_ make changes when I move files. I've been 
so pissed at Andy for unintentionally obscuring changes this way that I 
swore I would never do it myself. :)

> -
> -	/* Install the soft-off (S5) handler. */
> -	if (sleep_states[ACPI_STATE_S5]) {
> -		pm_power_off = acpi_power_off;
> -
> -		/* workaround: some systems don't claim S4 support,
> but they
> -                   do support S5 (power-down). That is all we need,
> so
> -		   indicate support. */
> -		sleep_states[ACPI_STATE_S4] = 1;
> -	}
>  
>  	return_VALUE(0);
> 
> Are you sure about this one?

Definitely. The setting of pm_power_off was moved to poweroff.c. 

The other part is bogus. S5 is not a sleep state, and shouldn't be treated 
as such. You can still do a swsusp-style suspend and enter S5, in which 
case I wouldn't recommend going through ACPI at all. You simply don't need 
it. You need to save state, etc, then just call pm_power_off(). 

The only thing you lack is a way to trigger swsusp w/o ACPI, right? 


	-pat



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

Re: Sneaking patches in (was Re: System hang when trying to enter sleep/standby state)

[Pavel Machek]

Hi!

> > Even with the changelogs, I can't decide which one of these it
> > is. Probably 2.19, but as it also moves files, commit list is useless
> > because it represent moves as delete all lines, add all lines, and any
> > change is hidden.
> 
> As a personal choice, I _never_ make changes when I move files. I've been 
> so pissed at Andy for unintentionally obscuring changes this way that I 
> swore I would never do it myself. :)

Okay...

> > -
> > -	/* Install the soft-off (S5) handler. */
> > -	if (sleep_states[ACPI_STATE_S5]) {
> > -		pm_power_off = acpi_power_off;
> > -
> > -		/* workaround: some systems don't claim S4 support,
> > but they
> > -                   do support S5 (power-down). That is all we need,
> > so
> > -		   indicate support. */
> > -		sleep_states[ACPI_STATE_S4] = 1;
> > -	}
> >  
> >  	return_VALUE(0);
> > 
> > Are you sure about this one?
> 
> Definitely. The setting of pm_power_off was moved to poweroff.c. 
> 
> The other part is bogus. S5 is not a sleep state, and shouldn't be treated 
> as such. You can still do a swsusp-style suspend and enter S5, in which 
> case I wouldn't recommend going through ACPI at all. You simply don't need 
> it. You need to save state, etc, then just call pm_power_off(). 

Yes but I need S4 to be enabled, i.e. sleep_states[ACPI_STATE_S4] =
1;. OTOH it might be usefull to enable S4 even if S5 is not there, it
is usefull for old machines, too, user just has to turn it off
manually.

> The only thing you lack is a way to trigger swsusp w/o ACPI, right? 

Actually you can trigger swsusp using syscall, but few people do that.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]


-------------------------------------------------------
This SF.net email is sponsored by: SlickEdit Inc. Develop an edge.
The most comprehensive and flexible code editor you can use.
Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial.
www.slickedit.com/sourceforge