[PATCH -mm 0/3] swsusp: Add ioctl for swap files support and update documentation

[PATCH -mm 0/3] swsusp: Add ioctl for swap files support and update documentation

[Rafael J. Wysocki]

Hi,

The first two patches update the swsusp userland interface so that swap files
can be used as suspend storage by the userland tools and change the swsusp
documentation accordingly.

The last patch documents the recently merged swsusp testing code.

Please consider for applying.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

[PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Rafael J. Wysocki]

To be able to use swap files as suspend storage from the userland suspend
tools we need an additional ioctl() that will allow us to provide the kernel
with both the swap header's offset and the identification of the resume
partition.

The new ioctl() should be regarded as a replacement for the
SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
obsolete, but has to stay for backwards compatibility of the interface.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/power/power.h |   13 ++++++++++++-
 kernel/power/user.c  |   31 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Index: linux-2.6.18-mm1/kernel/power/power.h
===================================================================
--- linux-2.6.18-mm1.orig/kernel/power/power.h
+++ linux-2.6.18-mm1/kernel/power/power.h
@@ -119,7 +119,18 @@ extern int snapshot_image_loaded(struct 
 #define SNAPSHOT_SET_SWAP_FILE		_IOW(SNAPSHOT_IOC_MAGIC, 10, unsigned int)
 #define SNAPSHOT_S2RAM			_IO(SNAPSHOT_IOC_MAGIC, 11)
 #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
-#define SNAPSHOT_IOC_MAXNR	12
+#define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)
+#define SNAPSHOT_IOC_MAXNR	13
+
+/*
+ * This structure is used to pass the values needed for the identification
+ * of the resume swap area from a user space to the kernel via the
+ * SNAPSHOT_SET_SWAP_AREA ioctl
+ */
+struct resume_swap_area {
+	u_int16_t dev;
+	loff_t offset;
+} __attribute__((packed));
 
 #define PMOPS_PREPARE	1
 #define PMOPS_ENTER	2
Index: linux-2.6.18-mm1/kernel/power/user.c
===================================================================
--- linux-2.6.18-mm1.orig/kernel/power/user.c
+++ linux-2.6.18-mm1/kernel/power/user.c
@@ -343,6 +343,37 @@ OutS3:
 		}
 		break;
 
+	case SNAPSHOT_SET_SWAP_AREA:
+		if (data->bitmap) {
+			error = -EPERM;
+		} else {
+			struct resume_swap_area swap_area;
+			dev_t swdev;
+
+			error = copy_from_user(&swap_area, (void __user *)arg,
+					sizeof(struct resume_swap_area));
+			if (error) {
+				error = -EFAULT;
+				break;
+			}
+
+			/*
+			 * User space encodes device types as two-byte values,
+			 * so we need to recode them
+			 */
+			swdev = old_decode_dev(swap_area.dev);
+			if (swdev) {
+				offset = swap_area.offset;
+				data->swap = swap_type_of(swdev, offset);
+				if (data->swap < 0)
+					error = -ENODEV;
+			} else {
+				data->swap = -1;
+				error = -EINVAL;
+			}
+		}
+		break;
+
 	default:
 		error = -ENOTTY;
 

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Andrew Morton]

On Fri, 29 Sep 2006 00:13:38 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> To be able to use swap files as suspend storage from the userland suspend
> tools we need an additional ioctl() that will allow us to provide the kernel
> with both the swap header's offset and the identification of the resume
> partition.
> 
> The new ioctl() should be regarded as a replacement for the
> SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> obsolete, but has to stay for backwards compatibility of the interface.
> 
> +
> +/*
> + * This structure is used to pass the values needed for the identification
> + * of the resume swap area from a user space to the kernel via the
> + * SNAPSHOT_SET_SWAP_AREA ioctl
> + */
> +struct resume_swap_area {
> +	u_int16_t dev;
> +	loff_t offset;
> +} __attribute__((packed));
>

hmm.  Asking the compiler to pack 16-bit and 64-bit quantities in this
manner is a bit risky.  I guess it'll do the right thing, consistently,
across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.

But from a defensiveness/paranoia POV it'd be better to use a u32 here, I
suspect.  (Will access to that loff_t cause an alignment trap on ia64?  Any
other CPUs?  Dunno).

>  #define PMOPS_PREPARE	1
>  #define PMOPS_ENTER	2
> Index: linux-2.6.18-mm1/kernel/power/user.c
> ===================================================================
> --- linux-2.6.18-mm1.orig/kernel/power/user.c
> +++ linux-2.6.18-mm1/kernel/power/user.c
> @@ -343,6 +343,37 @@ OutS3:
>  		}
>  		break;
>  
> +	case SNAPSHOT_SET_SWAP_AREA:
> +		if (data->bitmap) {
> +			error = -EPERM;
> +		} else {
> +			struct resume_swap_area swap_area;
> +			dev_t swdev;
> +
> +			error = copy_from_user(&swap_area, (void __user *)arg,
> +					sizeof(struct resume_swap_area));
> +			if (error) {
> +				error = -EFAULT;
> +				break;
> +			}
> +
> +			/*
> +			 * User space encodes device types as two-byte values,
> +			 * so we need to recode them
> +			 */

Really?  stat() uses unsigned long and stat64() uses unsigned long long dev_t.



> +			swdev = old_decode_dev(swap_area.dev);
> +			if (swdev) {
> +				offset = swap_area.offset;
> +				data->swap = swap_type_of(swdev, offset);
> +				if (data->swap < 0)
> +					error = -ENODEV;
> +			} else {
> +				data->swap = -1;
> +				error = -EINVAL;
> +			}
> +		}
> +		break;
> +
>  	default:
>  		error = -ENOTTY;

But I wonder if we need to pass the device identified into this ioctl at
all.  What device is the ioctl() against?  ie: what do `filp' and `inode'
point at?  If it's /dev/hda1 then everything we need is right there, is it
not?

ohshit, it's a miscdevice.  I wonder if it would have defined all this
stuff to be operations against the blockdev.  Perhaps not.

Well anyway.  It might be neater to require that userspace open /dev/hda1
and pass in the fd to this ioctl.  But this code will never be neat, so
whatever.

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Rafael J. Wysocki]

On Friday, 29 September 2006 00:42, Andrew Morton wrote:
> On Fri, 29 Sep 2006 00:13:38 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > To be able to use swap files as suspend storage from the userland suspend
> > tools we need an additional ioctl() that will allow us to provide the kernel
> > with both the swap header's offset and the identification of the resume
> > partition.
> > 
> > The new ioctl() should be regarded as a replacement for the
> > SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> > obsolete, but has to stay for backwards compatibility of the interface.
> > 
> > +
> > +/*
> > + * This structure is used to pass the values needed for the identification
> > + * of the resume swap area from a user space to the kernel via the
> > + * SNAPSHOT_SET_SWAP_AREA ioctl
> > + */
> > +struct resume_swap_area {
> > +	u_int16_t dev;
> > +	loff_t offset;
> > +} __attribute__((packed));
> >
> 
> hmm.  Asking the compiler to pack 16-bit and 64-bit quantities in this
> manner is a bit risky.  I guess it'll do the right thing, consistently,
> across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.
> 
> But from a defensiveness/paranoia POV it'd be better to use a u32 here, I
> suspect.  (Will access to that loff_t cause an alignment trap on ia64?  Any
> other CPUs?  Dunno).

I'm not sure too.  Will change it to a 32-bit value.

> 
> >  #define PMOPS_PREPARE	1
> >  #define PMOPS_ENTER	2
> > Index: linux-2.6.18-mm1/kernel/power/user.c
> > ===================================================================
> > --- linux-2.6.18-mm1.orig/kernel/power/user.c
> > +++ linux-2.6.18-mm1/kernel/power/user.c
> > @@ -343,6 +343,37 @@ OutS3:
> >  		}
> >  		break;
> >  
> > +	case SNAPSHOT_SET_SWAP_AREA:
> > +		if (data->bitmap) {
> > +			error = -EPERM;
> > +		} else {
> > +			struct resume_swap_area swap_area;
> > +			dev_t swdev;
> > +
> > +			error = copy_from_user(&swap_area, (void __user *)arg,
> > +					sizeof(struct resume_swap_area));
> > +			if (error) {
> > +				error = -EFAULT;
> > +				break;
> > +			}
> > +
> > +			/*
> > +			 * User space encodes device types as two-byte values,
> > +			 * so we need to recode them
> > +			 */
> 
> Really?  stat() uses unsigned long and stat64() uses unsigned long long dev_t.

I don't know what the remaining 6 bytes are used for.  Anyway we need to use
old_decode_dev() to get this right.

> > +			swdev = old_decode_dev(swap_area.dev);
> > +			if (swdev) {
> > +				offset = swap_area.offset;
> > +				data->swap = swap_type_of(swdev, offset);
> > +				if (data->swap < 0)
> > +					error = -ENODEV;
> > +			} else {
> > +				data->swap = -1;
> > +				error = -EINVAL;
> > +			}
> > +		}
> > +		break;
> > +
> >  	default:
> >  		error = -ENOTTY;
> 
> But I wonder if we need to pass the device identified into this ioctl at
> all.  What device is the ioctl() against?  ie: what do `filp' and `inode'
> point at?  If it's /dev/hda1 then everything we need is right there, is it
> not?
> 
> ohshit, it's a miscdevice.  I wonder if it would have defined all this
> stuff to be operations against the blockdev.  Perhaps not.

Nope.  We need char-like read() and write().

> Well anyway.  It might be neater to require that userspace open /dev/hda1
> and pass in the fd to this ioctl.  But this code will never be neat, so
> whatever.

:-)

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Pavel Machek]

Hi!

> On Fri, 29 Sep 2006 00:13:38 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > To be able to use swap files as suspend storage from the userland suspend
> > tools we need an additional ioctl() that will allow us to provide the kernel
> > with both the swap header's offset and the identification of the resume
> > partition.
> > 
> > The new ioctl() should be regarded as a replacement for the
> > SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> > obsolete, but has to stay for backwards compatibility of the interface.
> > 
> > +
> > +/*
> > + * This structure is used to pass the values needed for the identification
> > + * of the resume swap area from a user space to the kernel via the
> > + * SNAPSHOT_SET_SWAP_AREA ioctl
> > + */
> > +struct resume_swap_area {
> > +	u_int16_t dev;
> > +	loff_t offset;
> > +} __attribute__((packed));

> 
> hmm.  Asking the compiler to pack 16-bit and 64-bit quantities in this
> manner is a bit risky.  I guess it'll do the right thing, consistently,
> across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.
> 
> But from a defensiveness/paranoia POV it'd be better to use a u32 here, I
> suspect.  (Will access to that loff_t cause an alignment trap on ia64?  Any
> other CPUs?  Dunno).

Perhaps just loff_t offset; u32 dev; ? If 64-bit variable is first, we
should avoid most problems, no?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Rafael J. Wysocki]

On Friday, 29 September 2006 01:03, Pavel Machek wrote:
> Hi!
> 
> > On Fri, 29 Sep 2006 00:13:38 +0200
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > To be able to use swap files as suspend storage from the userland suspend
> > > tools we need an additional ioctl() that will allow us to provide the kernel
> > > with both the swap header's offset and the identification of the resume
> > > partition.
> > > 
> > > The new ioctl() should be regarded as a replacement for the
> > > SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> > > obsolete, but has to stay for backwards compatibility of the interface.
> > > 
> > > +
> > > +/*
> > > + * This structure is used to pass the values needed for the identification
> > > + * of the resume swap area from a user space to the kernel via the
> > > + * SNAPSHOT_SET_SWAP_AREA ioctl
> > > + */
> > > +struct resume_swap_area {
> > > +	u_int16_t dev;
> > > +	loff_t offset;
> > > +} __attribute__((packed));
> 
> > 
> > hmm.  Asking the compiler to pack 16-bit and 64-bit quantities in this
> > manner is a bit risky.  I guess it'll do the right thing, consistently,
> > across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.
> > 
> > But from a defensiveness/paranoia POV it'd be better to use a u32 here, I
> > suspect.  (Will access to that loff_t cause an alignment trap on ia64?  Any
> > other CPUs?  Dunno).
> 
> Perhaps just loff_t offset; u32 dev; ? If 64-bit variable is first, we
> should avoid most problems, no?

Done.  Updated patch follows.

Greetings,
Rafael


---
To be able to use swap files as suspend storage from the userland suspend
tools we need an additional ioctl() that will allow us to provide the kernel
with both the swap header's offset and the identification of the resume
partition.

The new ioctl() should be regarded as a replacement for the
SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
obsolete, but has to stay for backwards compatibility of the interface.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/power.h |   13 ++++++++++++-
 kernel/power/user.c  |   31 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Index: linux-2.6.18-mm2/kernel/power/power.h
===================================================================
--- linux-2.6.18-mm2.orig/kernel/power/power.h
+++ linux-2.6.18-mm2/kernel/power/power.h
@@ -119,7 +119,18 @@ extern int snapshot_image_loaded(struct 
 #define SNAPSHOT_SET_SWAP_FILE		_IOW(SNAPSHOT_IOC_MAGIC, 10, unsigned int)
 #define SNAPSHOT_S2RAM			_IO(SNAPSHOT_IOC_MAGIC, 11)
 #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
-#define SNAPSHOT_IOC_MAXNR	12
+#define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)
+#define SNAPSHOT_IOC_MAXNR	13
+
+/*
+ * This structure is used to pass the values needed for the identification
+ * of the resume swap area from a user space to the kernel via the
+ * SNAPSHOT_SET_SWAP_AREA ioctl
+ */
+struct resume_swap_area {
+	loff_t offset;
+	u_int32_t dev;
+} __attribute__((packed));
 
 #define PMOPS_PREPARE	1
 #define PMOPS_ENTER	2
Index: linux-2.6.18-mm2/kernel/power/user.c
===================================================================
--- linux-2.6.18-mm2.orig/kernel/power/user.c
+++ linux-2.6.18-mm2/kernel/power/user.c
@@ -343,6 +343,37 @@ OutS3:
 		}
 		break;
 
+	case SNAPSHOT_SET_SWAP_AREA:
+		if (data->bitmap) {
+			error = -EPERM;
+		} else {
+			struct resume_swap_area swap_area;
+			dev_t swdev;
+
+			error = copy_from_user(&swap_area, (void __user *)arg,
+					sizeof(struct resume_swap_area));
+			if (error) {
+				error = -EFAULT;
+				break;
+			}
+
+			/*
+			 * User space encodes device types as two-byte values,
+			 * so we need to recode them
+			 */
+			swdev = old_decode_dev(swap_area.dev);
+			if (swdev) {
+				offset = swap_area.offset;
+				data->swap = swap_type_of(swdev, offset);
+				if (data->swap < 0)
+					error = -ENODEV;
+			} else {
+				data->swap = -1;
+				error = -EINVAL;
+			}
+		}
+		break;
+
 	default:
 		error = -ENOTTY;
 

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Pavel Machek]

On Fri 2006-09-29 01:35:33, Rafael J. Wysocki wrote:
> On Friday, 29 September 2006 01:03, Pavel Machek wrote:
> > Hi!
> > 
> > > On Fri, 29 Sep 2006 00:13:38 +0200
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > 
> > > > To be able to use swap files as suspend storage from the userland suspend
> > > > tools we need an additional ioctl() that will allow us to provide the kernel
> > > > with both the swap header's offset and the identification of the resume
> > > > partition.
> > > > 
> > > > The new ioctl() should be regarded as a replacement for the
> > > > SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> > > > obsolete, but has to stay for backwards compatibility of the interface.
> > > > 
> > > > +
> > > > +/*
> > > > + * This structure is used to pass the values needed for the identification
> > > > + * of the resume swap area from a user space to the kernel via the
> > > > + * SNAPSHOT_SET_SWAP_AREA ioctl
> > > > + */
> > > > +struct resume_swap_area {
> > > > +	u_int16_t dev;
> > > > +	loff_t offset;
> > > > +} __attribute__((packed));
> > 
> > > 
> > > hmm.  Asking the compiler to pack 16-bit and 64-bit quantities in this
> > > manner is a bit risky.  I guess it'll do the right thing, consistently,
> > > across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.
> > > 
> > > But from a defensiveness/paranoia POV it'd be better to use a u32 here, I
> > > suspect.  (Will access to that loff_t cause an alignment trap on ia64?  Any
> > > other CPUs?  Dunno).
> > 
> > Perhaps just loff_t offset; u32 dev; ? If 64-bit variable is first, we
> > should avoid most problems, no?
> 
> Done.  Updated patch follows.

Looks okay to me.


> Greetings,
> Rafael
> 
> 
> ---
> To be able to use swap files as suspend storage from the userland suspend
> tools we need an additional ioctl() that will allow us to provide the kernel
> with both the swap header's offset and the identification of the resume
> partition.
> 
> The new ioctl() should be regarded as a replacement for the
> SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as
> obsolete, but has to stay for backwards compatibility of the interface.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/power.h |   13 ++++++++++++-
>  kernel/power/user.c  |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.18-mm2/kernel/power/power.h
> ===================================================================
> --- linux-2.6.18-mm2.orig/kernel/power/power.h
> +++ linux-2.6.18-mm2/kernel/power/power.h
> @@ -119,7 +119,18 @@ extern int snapshot_image_loaded(struct 
>  #define SNAPSHOT_SET_SWAP_FILE		_IOW(SNAPSHOT_IOC_MAGIC, 10, unsigned int)
>  #define SNAPSHOT_S2RAM			_IO(SNAPSHOT_IOC_MAGIC, 11)
>  #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
> -#define SNAPSHOT_IOC_MAXNR	12
> +#define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)
> +#define SNAPSHOT_IOC_MAXNR	13
> +
> +/*
> + * This structure is used to pass the values needed for the identification
> + * of the resume swap area from a user space to the kernel via the
> + * SNAPSHOT_SET_SWAP_AREA ioctl
> + */
> +struct resume_swap_area {
> +	loff_t offset;
> +	u_int32_t dev;
> +} __attribute__((packed));
>  
>  #define PMOPS_PREPARE	1
>  #define PMOPS_ENTER	2
> Index: linux-2.6.18-mm2/kernel/power/user.c
> ===================================================================
> --- linux-2.6.18-mm2.orig/kernel/power/user.c
> +++ linux-2.6.18-mm2/kernel/power/user.c
> @@ -343,6 +343,37 @@ OutS3:
>  		}
>  		break;
>  
> +	case SNAPSHOT_SET_SWAP_AREA:
> +		if (data->bitmap) {
> +			error = -EPERM;
> +		} else {
> +			struct resume_swap_area swap_area;
> +			dev_t swdev;
> +
> +			error = copy_from_user(&swap_area, (void __user *)arg,
> +					sizeof(struct resume_swap_area));
> +			if (error) {
> +				error = -EFAULT;
> +				break;
> +			}
> +
> +			/*
> +			 * User space encodes device types as two-byte values,
> +			 * so we need to recode them
> +			 */
> +			swdev = old_decode_dev(swap_area.dev);
> +			if (swdev) {
> +				offset = swap_area.offset;
> +				data->swap = swap_type_of(swdev, offset);
> +				if (data->swap < 0)
> +					error = -ENODEV;
> +			} else {
> +				data->swap = -1;
> +				error = -EINVAL;
> +			}
> +		}
> +		break;
> +
>  	default:
>  		error = -ENOTTY;
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Arnd Bergmann]

Am Friday 29 September 2006 01:35 schrieb Rafael J. Wysocki:
> @@ -119,7 +119,18 @@ extern int snapshot_image_loaded(struct
>  #define SNAPSHOT_SET_SWAP_FILE         _IOW(SNAPSHOT_IOC_MAGIC, 10,
> unsigned int) #define
> SNAPSHOT_S2RAM                 _IO(SNAPSHOT_IOC_MAGIC, 11) #define
> SNAPSHOT_PMOPS                 _IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
> -#define SNAPSHOT_IOC_MAXNR     12
> +#define SNAPSHOT_SET_SWAP_AREA         _IOW(SNAPSHOT_IOC_MAGIC, 13, void *) 
> +#define SNAPSHOT_IOC_MAXNR     13

Your definition looks wrong, '_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)' means
your ioctl passes a pointer to a 'void *'.

You probably mean 

#define SNAPSHOT_SET_SWAP_AREA _IOW(SNAPSHOT_IOC_MAGIC, 13, \
						struct resume_swap_area)

	Arnd <><

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Rafael J. Wysocki]

On Saturday, 30 September 2006 16:15, Arnd Bergmann wrote:
> Am Friday 29 September 2006 01:35 schrieb Rafael J. Wysocki:
> > @@ -119,7 +119,18 @@ extern int snapshot_image_loaded(struct
> >  #define SNAPSHOT_SET_SWAP_FILE         _IOW(SNAPSHOT_IOC_MAGIC, 10,
> > unsigned int) #define
> > SNAPSHOT_S2RAM                 _IO(SNAPSHOT_IOC_MAGIC, 11) #define
> > SNAPSHOT_PMOPS                 _IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
> > -#define SNAPSHOT_IOC_MAXNR     12
> > +#define SNAPSHOT_SET_SWAP_AREA         _IOW(SNAPSHOT_IOC_MAGIC, 13, void *) 
> > +#define SNAPSHOT_IOC_MAXNR     13
> 
> Your definition looks wrong, '_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)' means
> your ioctl passes a pointer to a 'void *'.
> 
> You probably mean 
> 
> #define SNAPSHOT_SET_SWAP_AREA _IOW(SNAPSHOT_IOC_MAGIC, 13, \
> 						struct resume_swap_area)

No.  I mean the ioctl passes a pointer, the size of which is sizeof(void *).

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Arnd Bergmann]

Am Saturday 30 September 2006 21:58 schrieb Rafael J. Wysocki:
> > Your definition looks wrong, '_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)' means
> > your ioctl passes a pointer to a 'void *'.
> >
> > You probably mean
> >
> > #define SNAPSHOT_SET_SWAP_AREA _IOW(SNAPSHOT_IOC_MAGIC, 13, \
> >                                               struct resume_swap_area)
>
> No.  I mean the ioctl passes a pointer, the size of which is sizeof(void *).

That's a very bad thing to do. It means that the ioctl number is different
between 32 and 64 bit and you need to write a conversion handler that
first reads your pointer and then then writes the real data.

Also, it needs to be at least _IOR() in the case you're describing,
because the pointer is read, not written (I hope).

	Arnd <><

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Rafael J. Wysocki]

On Saturday, 30 September 2006 22:37, Arnd Bergmann wrote:
> Am Saturday 30 September 2006 21:58 schrieb Rafael J. Wysocki:
> > > Your definition looks wrong, '_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)' means
> > > your ioctl passes a pointer to a 'void *'.
> > >
> > > You probably mean
> > >
> > > #define SNAPSHOT_SET_SWAP_AREA _IOW(SNAPSHOT_IOC_MAGIC, 13, \
> > >                                               struct resume_swap_area)
> >
> > No.  I mean the ioctl passes a pointer, the size of which is sizeof(void *).
> 
> That's a very bad thing to do. It means that the ioctl number is different
> between 32 and 64 bit and you need to write a conversion handler that
> first reads your pointer and then then writes the real data.

Ouch, I meant exactly what you said above, sorry.

Now that means some other ioctl definitions in kernel/power/power.h are
wrong, but I'm not sure what I should do.

I think I'll just change the new definition and let the others stay as they
are which is done in the appended patch.

Thanks,
Rafael


---
Fix the SNAPSHOT_SET_SWAP_AREA ioctl definition.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
---
 kernel/power/power.h |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Index: linux-2.6.18-mm2/kernel/power/power.h
===================================================================
--- linux-2.6.18-mm2.orig/kernel/power/power.h
+++ linux-2.6.18-mm2/kernel/power/power.h
@@ -106,6 +106,16 @@ extern int snapshot_write_next(struct sn
 extern void snapshot_write_finalize(struct snapshot_handle *handle);
 extern int snapshot_image_loaded(struct snapshot_handle *handle);
 
+/*
+ * This structure is used to pass the values needed for the identification
+ * of the resume swap area from a user space to the kernel via the
+ * SNAPSHOT_SET_SWAP_AREA ioctl
+ */
+struct resume_swap_area {
+	loff_t offset;
+	u_int32_t dev;
+} __attribute__((packed));
+
 #define SNAPSHOT_IOC_MAGIC	'3'
 #define SNAPSHOT_FREEZE			_IO(SNAPSHOT_IOC_MAGIC, 1)
 #define SNAPSHOT_UNFREEZE		_IO(SNAPSHOT_IOC_MAGIC, 2)
@@ -119,19 +129,10 @@ extern int snapshot_image_loaded(struct 
 #define SNAPSHOT_SET_SWAP_FILE		_IOW(SNAPSHOT_IOC_MAGIC, 10, unsigned int)
 #define SNAPSHOT_S2RAM			_IO(SNAPSHOT_IOC_MAGIC, 11)
 #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
-#define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, void *)
+#define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, \
+							struct resume_swap_area)
 #define SNAPSHOT_IOC_MAXNR	13
 
-/*
- * This structure is used to pass the values needed for the identification
- * of the resume swap area from a user space to the kernel via the
- * SNAPSHOT_SET_SWAP_AREA ioctl
- */
-struct resume_swap_area {
-	loff_t offset;
-	u_int32_t dev;
-} __attribute__((packed));
-
 #define PMOPS_PREPARE	1
 #define PMOPS_ENTER	2
 #define PMOPS_FINISH	3

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Arnd Bergmann]

On Sunday 01 October 2006 00:04, Rafael J. Wysocki wrote:
> Now that means some other ioctl definitions in kernel/power/power.h are
> wrong, but I'm not sure what I should do.
> 
> I think I'll just change the new definition and let the others stay as they
> are which is done in the appended patch.
> 

Ok, looks good now.

	Arnd <><

Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support

[Pavel Machek]

Hi!

> > +			swdev = old_decode_dev(swap_area.dev);
> > +			if (swdev) {
> > +				offset = swap_area.offset;
> > +				data->swap = swap_type_of(swdev, offset);
> > +				if (data->swap < 0)
> > +					error = -ENODEV;
> > +			} else {
> > +				data->swap = -1;
> > +				error = -EINVAL;
> > +			}
> > +		}
> > +		break;
> > +
> >  	default:
> >  		error = -ENOTTY;
> 
> But I wonder if we need to pass the device identified into this ioctl at
> all.  What device is the ioctl() against?  ie: what do `filp' and `inode'
> point at?  If it's /dev/hda1 then everything we need is right there, is it
> not?
> 
> ohshit, it's a miscdevice.  I wonder if it would have defined all this
> stuff to be operations against the blockdev.  Perhaps not.

Defining it against blockdev would be ugly... we'll want to suspend to
two devices at the same time, and we'll want to suspend over network etc.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH -mm 2/3] swsusp: Update userland interface documentation

[Rafael J. Wysocki]

The swsusp userland interface has recently changed for a couple of times, but
the changes have not been documented.  Fix this, and document the
SNAPSHOT_SET_SWAP_AREA ioctl().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/swsusp-and-swap-files.txt |   18 +++++--
 Documentation/power/userland-swsusp.txt       |   62 ++++++++++++++++++++------
 2 files changed, 62 insertions(+), 18 deletions(-)

Index: linux-2.6.18-mm2/Documentation/power/userland-swsusp.txt
===================================================================
--- linux-2.6.18-mm2.orig/Documentation/power/userland-swsusp.txt	2006-09-28 22:06:39.000000000 +0200
+++ linux-2.6.18-mm2/Documentation/power/userland-swsusp.txt	2006-09-28 23:02:14.000000000 +0200
@@ -9,9 +9,8 @@ done it already.
 Now, to use the userland interface for software suspend you need special
 utilities that will read/write the system memory snapshot from/to the
 kernel.  Such utilities are available, for example, from
-<http://www.sisk.pl/kernel/utilities/suspend>.  You may want to have
-a look at them if you are going to develop your own suspend/resume
-utilities.
+<http://suspend.sourceforge.net>.  You may want to have a look at them if you
+are going to develop your own suspend/resume utilities.
 
 The interface consists of a character device providing the open(),
 release(), read(), and write() operations as well as several ioctl()
@@ -21,9 +20,9 @@ be read from /sys/class/misc/snapshot/de
 
 The device can be open either for reading or for writing.  If open for
 reading, it is considered to be in the suspend mode.  Otherwise it is
-assumed to be in the resume mode.  The device cannot be open for reading
-and writing.  It is also impossible to have the device open more than once
-at a time.
+assumed to be in the resume mode.  The device cannot be open for simultaneous
+reading and writing.  It is also impossible to have the device open more than
+once at a time.
 
 The ioctl() commands recognized by the device are:
 
@@ -69,9 +68,46 @@ SNAPSHOT_FREE_SWAP_PAGES - free all swap
 SNAPSHOT_SET_SWAP_FILE - set the resume partition (the last ioctl() argument
 	should specify the device's major and minor numbers in the old
 	two-byte format, as returned by the stat() function in the .st_rdev
-	member of the stat structure); it is recommended to always use this
-	call, because the code to set the resume partition could be removed from
-	future kernels
+	member of the stat structure)
+
+SNAPSHOT_SET_SWAP_AREA - set the resume partition and the offset (in <PAGE_SIZE>
+	units) from the beginning of the partition at which the swap header is
+	located (the last ioctl() argument should point to a struct
+	resume_swap_area, as defined in kernel/power/power.h, containing the
+	resume device specification, as for the SNAPSHOT_SET_SWAP_FILE ioctl(),
+	and the offset); for swap partitions the offset is always 0, but it is
+	different to zero for swap files (please see
+	Documentation/swsusp-and-swap-files.txt for details).
+	The SNAPSHOT_SET_SWAP_AREA ioctl() is considered as a replacement for
+	SNAPSHOT_SET_SWAP_FILE which is regarded as obsolete.   It is
+	recommended to always use this call, because the code to set the resume
+	partition may be removed from future kernels
+
+SNAPSHOT_S2RAM - suspend to RAM; using this call causes the kernel to
+	immediately enter the suspend-to-RAM state, so this call must always
+	be preceded by the SNAPSHOT_FREEZE call and it is also necessary
+	to use the SNAPSHOT_UNFREEZE call after the system wakes up.  This call
+	is needed to implement the suspend-to-both mechanism in which the
+	suspend image is first created, as though the system had been suspended
+	to disk, and then the system is suspended to RAM (this makes it possible
+	to resume the system from RAM if there's enough battery power or restore
+	its state on the basis of the saved suspend image otherwise)
+
+SNAPSHOT_PMOPS - enable the usage of the pmops->prepare, pmops->enter and
+	pmops->finish methods (the in-kernel swsusp knows these as the "platform
+	method") which are needed on many machines to (among others) speed up
+	the resume by letting the BIOS skip some steps or to let the system
+	recognise the correct state of the hardware after the resume (in
+	particular on many machines this ensures that unplugged AC
+	adapters get correctly detected and that kacpid does not run wild after
+	the resume).  The last ioctl() argument can take one of the three
+	values, defined in kernel/power/power.h:
+	PMOPS_PREPARE - make the kernel carry out the
+		pm_ops->prepare(PM_SUSPEND_DISK) operation
+	PMOPS_ENTER - make the kernel power off the system by calling
+		pm_ops->enter(PM_SUSPEND_DISK)
+	PMOPS_FINISH - make the kernel carry out the
+		pm_ops->finish(PM_SUSPEND_DISK) operation
 
 The device's read() operation can be used to transfer the snapshot image from
 the kernel.  It has the following limitations:
@@ -92,9 +128,11 @@ still frozen when the device is being cl
 
 Currently it is assumed that the userland utilities reading/writing the
 snapshot image from/to the kernel will use a swap parition, called the resume
-partition, as storage space.  However, this is not really required, as they
-can use, for example, a special (blank) suspend partition or a file on a partition
-that is unmounted before SNAPSHOT_ATOMIC_SNAPSHOT and mounted afterwards.
+partition, or a swap file as storage space (if a swap file is used, the resume
+partition is the partition that holds this file).  However, this is not really
+required, as they can use, for example, a special (blank) suspend partition or
+a file on a partition that is unmounted before SNAPSHOT_ATOMIC_SNAPSHOT and
+mounted afterwards.
 
 These utilities SHOULD NOT make any assumptions regarding the ordering of
 data within the snapshot image, except for the image header that MAY be
Index: linux-2.6.18-mm2/Documentation/power/swsusp-and-swap-files.txt
===================================================================
--- linux-2.6.18-mm2.orig/Documentation/power/swsusp-and-swap-files.txt	2006-09-28 21:53:01.000000000 +0200
+++ linux-2.6.18-mm2/Documentation/power/swsusp-and-swap-files.txt	2006-09-28 23:09:25.000000000 +0200
@@ -38,15 +38,21 @@ resume=<swap_file_partition> resume_offs
 
 where <swap_file_partition> is the partition on which the swap file is located
 and <swap_file_offset> is the offset of the swap header determined by the
-application in 2).  [Of course, this step may be carried out automatically
+application in 2) (of course, this step may be carried out automatically
 by the same application that determies the swap file's header offset using the
-FIBMAP ioctl.]
+FIBMAP ioctl)
+
+OR
+
+Use a userland suspend application that will set the partition and offset
+with the help of the SNAPSHOT_SET_SWAP_AREA ioctl described in
+Documentation/power/userland-swsusp.txt (this is the only method to suspend
+to a swap file allowing the resume to be initiated from an initrd or initramfs
+image).
 
 Now, swsusp will use the swap file in the same way in which it would use a swap
-partition.  [Of course this means that the resume from a swap file cannot be
-initiated from whithin an initrd of initramfs image.]  In particular, the
-swap file has to be active (ie. be present in /proc/swaps) so that it can be
-used for suspending.
+partition.  In particular, the swap file has to be active (ie. be present in
+/proc/swaps) so that it can be used for suspending.
 
 Note that if the swap file used for suspending is deleted and recreated,
 the location of its header need not be the same as before.  Thus every time

[PATCH -mm 3/3] swsusp: Document testing code

[Rafael J. Wysocki]

Update the swsusp documentation to cover the recently introduced testing
code.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/ABI/testing/sysfs-power |   17 ++++++++++++++++-
 Documentation/power/interface.txt     |   13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

Index: linux-2.6.18-mm2/Documentation/ABI/testing/sysfs-power
===================================================================
--- linux-2.6.18-mm2.orig/Documentation/ABI/testing/sysfs-power	2006-09-28 21:53:01.000000000 +0200
+++ linux-2.6.18-mm2/Documentation/ABI/testing/sysfs-power	2006-09-28 23:37:50.000000000 +0200
@@ -21,7 +21,7 @@ Description:
 		these states.
 
 What:		/sys/power/disk
-Date:		August 2006
+Date:		September 2006
 Contact:	Rafael J. Wysocki <rjw@sisk.pl>
 Description:
 		The /sys/power/disk file controls the operating mode of the
@@ -39,6 +39,19 @@ Description:
 		'reboot' - the memory image will be saved by the kernel and
 		the system will be rebooted.
 
+		Additionally, /sys/power/disk can be used to turn on one of the
+		two testing modes of the suspend-to-disk mechanism: 'testproc'
+		or 'test'.  If the suspend-to-disk mechanism is in the
+		'testproc' mode, writing 'disk' to /sys/power/state will cause
+		the kernel to disable nonboot CPUs and freeze tasks, wait for 5
+		seconds, unfreeze tasks and enable nonboot CPUs.  If it is in
+		the 'test' mode, writing 'disk' to /sys/power/state will cause
+		the kernel to disable nonboot CPUs and freeze tasks, shrink
+		memory, suspend devices, wait for 5 seconds, resume devices,
+		unfreeze tasks and enable nonboot CPUs.  Then, we are able to
+		look in the log messages and work out, for example, which code
+		is being slow and which device drivers are misbehaving.
+
 		The suspend-to-disk method may be chosen by writing to this
 		file one of the accepted strings:
 
@@ -46,6 +59,8 @@ Description:
 		'platform'
 		'shutdown'
 		'reboot'
+		'testproc'
+		'test'
 
 		It will only change to 'firmware' or 'platform' if the system
 		supports that.
Index: linux-2.6.18-mm2/Documentation/power/interface.txt
===================================================================
--- linux-2.6.18-mm2.orig/Documentation/power/interface.txt	2006-09-28 21:53:01.000000000 +0200
+++ linux-2.6.18-mm2/Documentation/power/interface.txt	2006-09-28 23:34:39.000000000 +0200
@@ -30,6 +30,17 @@ testing). The system will support either
 that is known a priori. But, the user may choose 'shutdown' or
 'reboot' as alternatives. 
 
+Additionally, /sys/power/disk can be used to turn on one of the two testing
+modes of the suspend-to-disk mechanism: 'testproc' or 'test'.  If the
+suspend-to-disk mechanism is in the 'testproc' mode, writing 'disk' to
+/sys/power/state will cause the kernel to disable nonboot CPUs and freeze
+tasks, wait for 5 seconds, unfreeze tasks and enable nonboot CPUs.  If it is
+in the 'test' mode, writing 'disk' to /sys/power/state will cause the kernel
+to disable nonboot CPUs and freeze tasks, shrink memory, suspend devices, wait
+for 5 seconds, resume devices, unfreeze tasks and enable nonboot CPUs.  Then,
+we are able to look in the log messages and work out, for example, which code
+is being slow and which device drivers are misbehaving.
+
 Reading from this file will display what the mode is currently set
 to. Writing to this file will accept one of
 
@@ -37,6 +48,8 @@ to. Writing to this file will accept one
        'platform'
        'shutdown'
        'reboot'
+       'testproc'
+       'test'
 
 It will only change to 'firmware' or 'platform' if the system supports
 it. 

Re: [PATCH -mm 3/3] swsusp: Document testing code

[Pavel Machek]

On Fri 2006-09-29 00:19:51, Rafael J. Wysocki wrote:
> Update the swsusp documentation to cover the recently introduced testing
> code.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK and thanks.
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html