[PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[P. Christeas]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

Just found out that by 'echo sth_wrong > /sys/power/state' the kernel would 
oops in a fatal way (no clean exit from there).
The oops suggested that the code would enter an invalid fn.

You may apply the included patch to solve the bug. IMHO doing a clean exit is 
much preferrable than having BUG() there.

In the patch I also added some double checking of the fn. You may disable that  
(although sleeping is a one time fn which won't be slowed dow by that). 

Status report:  (HP Omnibook XE3GC)
S1 ('standby') works
S3, S4bios will suspend, but NOT resume.
that's the same as previous kernels I've tested so far.

I still have the ALSA-artsd deadlock I reported w. -test1 (in short, artsd 
resumes from S1 in a 'disk I/O' process state, which deadlocks maestro3)
see: http://marc.theaimsgroup.com/?l=linux-kernel&m=105836807605512&w=2



[-- Attachment #2: sys-power-state.diff --]
[-- Type: text/x-diff, Size: 922 bytes --]

diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c /usr/src/linux/kernel/power/main.c
--- /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23 12:13:17.000000000 +0300
+++ /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300
@@ -304,6 +304,20 @@
 		goto Unlock;
 	}
 
+# if 1 		/* disable once we're sure we got it right */
+	/* double - check  code */
+	if (!s ) {
+		printk (KERN_ERR "Invalid suspend state %d\n",state);
+		error = -EINVAL;
+		goto Unlock ;
+	}
+	if (! s->fn) {
+		printk(KERN_ERR "Invalid fn for suspend state %d\n", state );
+		error = -EINVAL ;
+		goto Unlock;
+	}
+# endif
+
 	pr_debug("PM: Preparing system for suspend.\n");
 	if ((error = suspend_prepare(state)))
 		goto Unlock;
@@ -500,7 +514,7 @@
 		if (s->name && !strcmp(buf,s->name))
 			break;
 	}
-	if (s)
+	if ( (s) && (state < PM_SUSPEND_MAX) )
 		error = enter_state(state);
 	else
 		error = -EINVAL;

[PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[P. Christeas]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

Just found out that by 'echo sth_wrong > /sys/power/state' the kernel would 
oops in a fatal way (no clean exit from there).
The oops suggested that the code would enter an invalid fn.

You may apply the included patch to solve the bug. IMHO doing a clean exit is 
much preferrable than having BUG() there.

In the patch I also added some double checking of the fn. You may disable that  
(although sleeping is a one time fn which won't be slowed dow by that). 

Status report:  (HP Omnibook XE3GC)
S1 ('standby') works
S3, S4bios will suspend, but NOT resume.
that's the same as previous kernels I've tested so far.

I still have the ALSA-artsd deadlock I reported w. -test1 (in short, artsd 
resumes from S1 in a 'disk I/O' process state, which deadlocks maestro3)
see: http://marc.theaimsgroup.com/?l=linux-kernel&m=105836807605512&w=2



[-- Attachment #2: sys-power-state.diff --]
[-- Type: text/x-diff, Size: 922 bytes --]

diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c /usr/src/linux/kernel/power/main.c
--- /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23 12:13:17.000000000 +0300
+++ /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300
@@ -304,6 +304,20 @@
 		goto Unlock;
 	}
 
+# if 1 		/* disable once we're sure we got it right */
+	/* double - check  code */
+	if (!s ) {
+		printk (KERN_ERR "Invalid suspend state %d\n",state);
+		error = -EINVAL;
+		goto Unlock ;
+	}
+	if (! s->fn) {
+		printk(KERN_ERR "Invalid fn for suspend state %d\n", state );
+		error = -EINVAL ;
+		goto Unlock;
+	}
+# endif
+
 	pr_debug("PM: Preparing system for suspend.\n");
 	if ((error = suspend_prepare(state)))
 		goto Unlock;
@@ -500,7 +514,7 @@
 		if (s->name && !strcmp(buf,s->name))
 			break;
 	}
-	if (s)
+	if ( (s) && (state < PM_SUSPEND_MAX) )
 		error = enter_state(state);
 	else
 		error = -EINVAL;

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[Bernd Petrovitsch]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> Just found out that by 'echo sth_wrong > /sys/power/state' the kernel would 
> oops in a fatal way (no clean exit from there).
> The oops suggested that the code would enter an invalid fn.
> 
> You may apply the included patch to solve the bug. IMHO doing a clean exit is 
> much preferrable than having BUG() there.
[...]
> ----
[...]
> diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c /usr/src/linux/kernel/power/main.c
> --- /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23 12:13:17.000000000 +0300
> +++ /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300
> @@ -500,7 +514,7 @@
>  		if (s->name && !strcmp(buf,s->name))
>  			break;
>  	}
> -	if (s)
> +	if ( (s) && (state < PM_SUSPEND_MAX) )
>  		error = enter_state(state);
>  	else
>  		error = -EINVAL;

What do you think about the attached patch to solve the bug and remove a
warning?

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

[-- Attachment #2: power-main.patch --]
[-- Type: text/plain, Size: 393 bytes --]

--- linux-2.6.0-test4/kernel/power/main.c	Sat Aug 23 01:53:13 2003
+++ linux-2.6.0-test4-patched/kernel/power/main.c	Mon Aug 25 21:16:50 2003
@@ -492,7 +492,7 @@
 static ssize_t state_store(struct subsystem * subsys, const char * buf, size_t n)
 {
 	u32 state;
-	struct pm_state * s;
+	struct pm_state * s = NULL;
 	int error;
 
 	for (state = 0; state < PM_SUSPEND_MAX; state++) {

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[Bernd Petrovitsch]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> Just found out that by 'echo sth_wrong > /sys/power/state' the kernel would 
> oops in a fatal way (no clean exit from there).
> The oops suggested that the code would enter an invalid fn.
> 
> You may apply the included patch to solve the bug. IMHO doing a clean exit is 
> much preferrable than having BUG() there.
[...]
> ----
[...]
> diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c /usr/src/linux/kernel/power/main.c
> --- /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23 12:13:17.000000000 +0300
> +++ /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300
> @@ -500,7 +514,7 @@
>  		if (s->name && !strcmp(buf,s->name))
>  			break;
>  	}
> -	if (s)
> +	if ( (s) && (state < PM_SUSPEND_MAX) )
>  		error = enter_state(state);
>  	else
>  		error = -EINVAL;

What do you think about the attached patch to solve the bug and remove a
warning?

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

[-- Attachment #2: power-main.patch --]
[-- Type: text/plain, Size: 393 bytes --]

--- linux-2.6.0-test4/kernel/power/main.c	Sat Aug 23 01:53:13 2003
+++ linux-2.6.0-test4-patched/kernel/power/main.c	Mon Aug 25 21:16:50 2003
@@ -492,7 +492,7 @@
 static ssize_t state_store(struct subsystem * subsys, const char * buf, size_t n)
 {
 	u32 state;
-	struct pm_state * s;
+	struct pm_state * s = NULL;
 	int error;
 
 	for (state = 0; state < PM_SUSPEND_MAX; state++) {

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[P. Christeas]

Bernd Petrovitsch wrote:
> On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> > Just found out that by 'echo sth_wrong > /sys/power/state' the kernel
> > would oops in a fatal way (no clean exit from there).
> > The oops suggested that the code would enter an invalid fn.
> >
> > You may apply the included patch to solve the bug. IMHO doing a clean
> > exit is much preferrable than having BUG() there.
>
> > diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c
> > /usr/src/linux/kernel/power/main.c ---
> > /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23
> > 12:13:17.000000000 +0300 +++
> > /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300 @@
> > -500,7 +514,7 @@
> >  		if (s->name && !strcmp(buf,s->name))
> >  			break;
> >  	}
> > -	if (s)
> > +	if ( (s) && (state < PM_SUSPEND_MAX) )
> >  		error = enter_state(state);
> >  	else
> >  		error = -EINVAL;
>
> What do you think about the attached patch to solve the bug and remove a
> warning?
>
> 	Bernd

Already tried that. If you look more closely, the s will receive the state 
*before* the name *in it* is strcmp()'ed. This means it won't be NULL anyway.
>               if (s->name && !strcmp(buf,s->name))
>                       break;

I also thought of checking "( (s) && (s->name !=NULL) )" ,  but IMHO the 
'state' check is cleaner (no dereference).


--- linux-2.6.0-test4/kernel/power/main.c       Sat Aug 23 01:53:13 2003
+++ linux-2.6.0-test4-patched/kernel/power/main.c       Mon Aug 25 21:16:50 
2003
@@ -492,7 +492,7 @@
 static ssize_t state_store(struct subsystem * subsys, const char * buf, 
size_t n)
 {
        u32 state;
-       struct pm_state * s;
+       struct pm_state * s = NULL;
        int error;
 
        for (state = 0; state < PM_SUSPEND_MAX; state++) {

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[P. Christeas]

Bernd Petrovitsch wrote:
> On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> > Just found out that by 'echo sth_wrong > /sys/power/state' the kernel
> > would oops in a fatal way (no clean exit from there).
> > The oops suggested that the code would enter an invalid fn.
> >
> > You may apply the included patch to solve the bug. IMHO doing a clean
> > exit is much preferrable than having BUG() there.
>
> > diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c
> > /usr/src/linux/kernel/power/main.c ---
> > /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23
> > 12:13:17.000000000 +0300 +++
> > /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300 @@
> > -500,7 +514,7 @@
> >  		if (s->name && !strcmp(buf,s->name))
> >  			break;
> >  	}
> > -	if (s)
> > +	if ( (s) && (state < PM_SUSPEND_MAX) )
> >  		error = enter_state(state);
> >  	else
> >  		error = -EINVAL;
>
> What do you think about the attached patch to solve the bug and remove a
> warning?
>
> 	Bernd

Already tried that. If you look more closely, the s will receive the state 
*before* the name *in it* is strcmp()'ed. This means it won't be NULL anyway.
>               if (s->name && !strcmp(buf,s->name))
>                       break;

I also thought of checking "( (s) && (s->name !=NULL) )" ,  but IMHO the 
'state' check is cleaner (no dereference).


--- linux-2.6.0-test4/kernel/power/main.c       Sat Aug 23 01:53:13 2003
+++ linux-2.6.0-test4-patched/kernel/power/main.c       Mon Aug 25 21:16:50 
2003
@@ -492,7 +492,7 @@
 static ssize_t state_store(struct subsystem * subsys, const char * buf, 
size_t n)
 {
        u32 state;
-       struct pm_state * s;
+       struct pm_state * s = NULL;
        int error;
 
        for (state = 0; state < PM_SUSPEND_MAX; state++) {

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[Bernd Petrovitsch]

On Tue, 2003-08-26 at 00:47, P. Christeas wrote:
> Bernd Petrovitsch wrote:
> > On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> > > Just found out that by 'echo sth_wrong > /sys/power/state' the kernel
> > > would oops in a fatal way (no clean exit from there).
> > > The oops suggested that the code would enter an invalid fn.
> > >
> > > You may apply the included patch to solve the bug. IMHO doing a clean
> > > exit is much preferrable than having BUG() there.
> >
> > > diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c
> > > /usr/src/linux/kernel/power/main.c ---
> > > /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23
> > > 12:13:17.000000000 +0300 +++
> > > /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300 @@
> > > -500,7 +514,7 @@
> > >  		if (s->name && !strcmp(buf,s->name))
> > >  			break;
> > >  	}
> > > -	if (s)
> > > +	if ( (s) && (state < PM_SUSPEND_MAX) )
> > >  		error = enter_state(state);
> > >  	else
> > >  		error = -EINVAL;
> >
> > What do you think about the attached patch to solve the bug and remove a
> > warning?
[...]

> Already tried that. If you look more closely, the s will receive the state 
> *before* the name *in it* is strcmp()'ed. This means it won't be NULL anyway.
> >               if (s->name && !strcmp(buf,s->name))
> >                       break;

*arrgl*, yes. Could only change somthing if the PM_SUSPEND_MAX == 0.
Hmm, the check on `s != NULL' seems also pretty superflous since
s loops over the elements in the array.

> I also thought of checking "( (s) && (s->name !=NULL) )" ,  but IMHO the 
> 'state' check is cleaner (no dereference).

Yup.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



-------------------------------------------------------
This SF.net email is sponsored by: VM Ware
With VMware you can run multiple operating systems on a single machine.
WITHOUT REBOOTING! Mix Linux / Windows / Novell virtual machines
at the same time. Free trial click here:http://www.vmware.com/wl/offer/358/0

Re: [PATCH] 2.6.0-test4: Trivial /sys/power/state patch, sleep status report

[Bernd Petrovitsch]

On Tue, 2003-08-26 at 00:47, P. Christeas wrote:
> Bernd Petrovitsch wrote:
> > On Tue, 2003-08-26 at 00:25, P. Christeas wrote:
> > > Just found out that by 'echo sth_wrong > /sys/power/state' the kernel
> > > would oops in a fatal way (no clean exit from there).
> > > The oops suggested that the code would enter an invalid fn.
> > >
> > > You may apply the included patch to solve the bug. IMHO doing a clean
> > > exit is much preferrable than having BUG() there.
> >
> > > diff -Bbur /diskb/users/panos/linux-off/kernel/power/main.c
> > > /usr/src/linux/kernel/power/main.c ---
> > > /diskb/users/panos/linux-off/kernel/power/main.c	2003-08-23
> > > 12:13:17.000000000 +0300 +++
> > > /usr/src/linux/kernel/power/main.c	2003-08-26 00:59:34.000000000 +0300 @@
> > > -500,7 +514,7 @@
> > >  		if (s->name && !strcmp(buf,s->name))
> > >  			break;
> > >  	}
> > > -	if (s)
> > > +	if ( (s) && (state < PM_SUSPEND_MAX) )
> > >  		error = enter_state(state);
> > >  	else
> > >  		error = -EINVAL;
> >
> > What do you think about the attached patch to solve the bug and remove a
> > warning?
[...]

> Already tried that. If you look more closely, the s will receive the state 
> *before* the name *in it* is strcmp()'ed. This means it won't be NULL anyway.
> >               if (s->name && !strcmp(buf,s->name))
> >                       break;

*arrgl*, yes. Could only change somthing if the PM_SUSPEND_MAX == 0.
Hmm, the check on `s != NULL' seems also pretty superflous since
s loops over the elements in the array.

> I also thought of checking "( (s) && (s->name !=NULL) )" ,  but IMHO the 
> 'state' check is cleaner (no dereference).

Yup.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services