compat ioctl32 for /dev/snapshot?

compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Is there any reason why 32-bit uswsusp &Friends does not work
on 64bits kernel?

For one, 32bits s2disk produces the following when trying to
suspend:

  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot

this is coming from:

     error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
     if (error && !offset)
             error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);

but I guess (just guess!) that other SNAPSHOT_* operations will
also fail the same way.

Is there a reason why those are not in compat_ioctl?

Thanks.

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Andi Kleen]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Is there any reason why 32-bit uswsusp &Friends does not work
> on 64bits kernel?
>
> For one, 32bits s2disk produces the following when trying to
> suspend:
>
>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>
> this is coming from:
>
>     error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
>     if (error && !offset)
>             error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
>
> but I guess (just guess!) that other SNAPSHOT_* operations will
> also fail the same way.
>
> Is there a reason why those are not in compat_ioctl?

It's probably just that nobody has written the code yet. In general all
missing compat_ioctls are bugs.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

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

Andi Kleen wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> Is there any reason why 32-bit uswsusp &Friends does not work
>> on 64bits kernel?
>>
>> For one, 32bits s2disk produces the following when trying to
>> suspend:
>>
>>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>>
[]
> It's probably just that nobody has written the code yet. In general all
> missing compat_ioctls are bugs.

Oh well.

Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
The ioctls are:
  o argument-less (most of them are)
  o have single loff_t argument (other ioctls with the same argument are
    marked as COMPAT_IOCTL
  o have single int argument - they's also marked as COMPAT_IOCTL,
  o and one othem, SNAPSHOT_SET_SWAP_AREA, has argument pointing to
    the following structure (include/linux/suspend_ioctls.h):
     struct resume_swap_area {
         loff_t offset;
         u_int32_t dev;
     } __attribute__((packed));
    so I think it also does not need any translation layer.

I can't test it so far, because uswsusp tools are broken in mixed
32/64bit case in other places.  But at least it compiles fine and
does not complain anymore.

I never touched this area before so I may be wrong... but if it's ok...

Signed-Off-By: Michael Tokarev <mjt@tls.msk.ru>

Thanks!

/mjt

[-- Attachment #2: snapshot-compat-ioctl.patch --]
[-- Type: text/x-patch, Size: 1000 bytes --]

Signed-Off-By: Michael Tokarev <mjt@tls.msk.ru>

--- linux-2.6.29/fs/compat_ioctl.c.orig	2009-03-24 02:12:14.000000000 +0300
+++ linux-2.6.29/fs/compat_ioctl.c	2009-05-04 14:46:49.906169841 +0400
@@ -112,2 +112,4 @@
 
+#include <linux/suspend_ioctls.h>
+
 #ifdef CONFIG_SPARC
@@ -2301,2 +2303,17 @@ COMPATIBLE_IOCTL(SOUND_MIXER_SETLEVELS)
 COMPATIBLE_IOCTL(OSS_GETVERSION)
+/* SNAPSHOT */
+COMPATIBLE_IOCTL(SNAPSHOT_FREEZE)
+COMPATIBLE_IOCTL(SNAPSHOT_UNFREEZE)
+COMPATIBLE_IOCTL(SNAPSHOT_ATOMIC_RESTORE)
+COMPATIBLE_IOCTL(SNAPSHOT_FREE)
+COMPATIBLE_IOCTL(SNAPSHOT_FREE_SWAP_PAGES)
+COMPATIBLE_IOCTL(SNAPSHOT_S2RAM)
+COMPATIBLE_IOCTL(SNAPSHOT_SET_SWAP_AREA)	/* struct resume_swap_area */
+COMPATIBLE_IOCTL(SNAPSHOT_GET_IMAGE_SIZE)
+COMPATIBLE_IOCTL(SNAPSHOT_PLATFORM_SUPPORT)
+COMPATIBLE_IOCTL(SNAPSHOT_POWER_OFF)
+COMPATIBLE_IOCTL(SNAPSHOT_CREATE_IMAGE)
+COMPATIBLE_IOCTL(SNAPSHOT_PREF_IMAGE_SIZE)
+COMPATIBLE_IOCTL(SNAPSHOT_AVAIL_SWAP_SIZE)
+COMPATIBLE_IOCTL(SNAPSHOT_ALLOC_SWAP_PAGE)
 /* AUTOFS */

Re: compat ioctl32 for /dev/snapshot?

[Andi Kleen]

On Mon, May 04, 2009 at 02:55:10PM +0400, Michael Tokarev wrote:
> Andi Kleen wrote:
> >Michael Tokarev <mjt@tls.msk.ru> writes:
> >
> >>Is there any reason why 32-bit uswsusp &Friends does not work
> >>on 64bits kernel?
> >>
> >>For one, 32bits s2disk produces the following when trying to
> >>suspend:
> >>
> >> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} 
> >> arg(ff853554) on /dev/snapshot
> >> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} 
> >> arg(00000805) on /dev/snapshot
> >>
> []
> >It's probably just that nobody has written the code yet. In general all
> >missing compat_ioctls are bugs.
> 
> Oh well.
> 
> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from

You should ask Rafael (cc'ed) who maintains that code.
> 
> I can't test it so far, because uswsusp tools are broken in mixed
> 32/64bit case in other places.  But at least it compiles fine and
> does not complain anymore.

It would be good to test before merging.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: compat ioctl32 for /dev/snapshot?

[Rafael J. Wysocki]

On Monday 04 May 2009, Andi Kleen wrote:
> On Mon, May 04, 2009 at 02:55:10PM +0400, Michael Tokarev wrote:
> > Andi Kleen wrote:
> > >Michael Tokarev <mjt@tls.msk.ru> writes:
> > >
> > >>Is there any reason why 32-bit uswsusp &Friends does not work
> > >>on 64bits kernel?
> > >>
> > >>For one, 32bits s2disk produces the following when trying to
> > >>suspend:
> > >>
> > >> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} 
> > >> arg(ff853554) on /dev/snapshot
> > >> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} 
> > >> arg(00000805) on /dev/snapshot
> > >>
> > []
> > >It's probably just that nobody has written the code yet. In general all
> > >missing compat_ioctls are bugs.
> > 
> > Oh well.
> > 
> > Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
> 
> You should ask Rafael (cc'ed) who maintains that code.

Thanks for CCing me. :-)

In fact I don't think the 32-bit user space will work with 64-bit kernels as is
in this particular case, because of the different pointer size.

Having a quick look at the code (I don't remember the details right now)
I think most probably it could be modified to handle this case too, but I'm not
really sure.

Best,
Rafael

Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann]

On Monday 04 May 2009, Rafael J. Wysocki wrote:
> In fact I don't think the 32-bit user space will work with 64-bit kernels as is
> in this particular case, because of the different pointer size.
> 
> Having a quick look at the code (I don't remember the details right now)
> I think most probably it could be modified to handle this case too, but I'm not
> really sure.

I don't see anything in the snapshot code that passes pointers to the
kernel, so why should the pointer size matter?

	Arnd <><

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Arnd Bergmann wrote:
> On Monday 04 May 2009, Rafael J. Wysocki wrote:
>> In fact I don't think the 32-bit user space will work with 64-bit kernels as is
>> in this particular case, because of the different pointer size.
>>
>> Having a quick look at the code (I don't remember the details right now)
>> I think most probably it could be modified to handle this case too, but I'm not
>> really sure.
> 
> I don't see anything in the snapshot code that passes pointers to the
> kernel, so why should the pointer size matter?

It's the userspace part (uswsusp).  In particular it parses swap
on-disk data structures.

But in any case, without compat_ioctl32 in kernel userspace part
can not be fixed.

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Rafael J. Wysocki]

On Tuesday 05 May 2009, Michael Tokarev wrote:
> Arnd Bergmann wrote:
> > On Monday 04 May 2009, Rafael J. Wysocki wrote:
> >> In fact I don't think the 32-bit user space will work with 64-bit kernels as is
> >> in this particular case, because of the different pointer size.
> >>
> >> Having a quick look at the code (I don't remember the details right now)
> >> I think most probably it could be modified to handle this case too, but I'm not
> >> really sure.
> > 
> > I don't see anything in the snapshot code that passes pointers to the
> > kernel, so why should the pointer size matter?
> 
> It's the userspace part (uswsusp).  In particular it parses swap
> on-disk data structures.
> 
> But in any case, without compat_ioctl32 in kernel userspace part
> can not be fixed.

Well, agreed. :-)

Rafael

Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann]

On Monday 04 May 2009, Michael Tokarev wrote:
> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
> include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
> The ioctls are:
>   o argument-less (most of them are)

Some of the ones that do not take a pointer argument actually do
take an integer argument, in particular SNAPSHOT_PREF_IMAGE_SIZE and
SNAPSHOT_PLATFORM_SUPPORT, which you need to list as ULONG_IOCTL
rather than COMPAT_IOCTL if you want to add them to compat_ioctl.c.

>   o have single loff_t argument (other ioctls with the same argument are
>     marked as COMPAT_IOCTL
>   o have single int argument - they's also marked as COMPAT_IOCTL,

right.

>   o and one othem, SNAPSHOT_SET_SWAP_AREA, has argument pointing to
>     the following structure (include/linux/suspend_ioctls.h):
>      struct resume_swap_area {
>          loff_t offset;
>          u_int32_t dev;
>      } __attribute__((packed));
>     so I think it also does not need any translation layer.


this one would be compat_ioctl as well, because the
__attribute__((packed)) turns it into a well-defined size of
12 bytes on any architecture (without the attribute, it would
be 16 bytes on most architectures, but not i386. The recommended
way to do this for new architectures would be explicit padding
rather than packed attribute.).

You are however missing support for deprecated ioctls in your
code: SNAPSHOT_SET_SWAP_FILE and SNAPSHOT_PMOPS are trivial
(COMPATIBLE_IOCTL), but you also need to add support for
these

#define SNAPSHOT_ATOMIC_SNAPSHOT32 _IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
#define SNAPSHOT_SET_IMAGE_SIZE32 _IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
#define SNAPSHOT_AVAIL_SWAP32 _IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
#define SNAPSHOT_GET_SWAP_PAGE32 _IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)

> I can't test it so far, because uswsusp tools are broken in mixed
> 32/64bit case in other places.  But at least it compiles fine and
> does not complain anymore.

I try to reduce the size of compat_ioctl.c. A better implementation
would be to add a ->compat_ioctl() operation to the file_operations
and list the compatible ioctl numbers as well.

Please try this patch instead:

suspend: Add compat_ioctl for snapshot device

All of the ioctl numbers in here are compatible, but for some of
the deprecated numbers, we need to add aliases.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -51,6 +51,10 @@
 #define SNAPSHOT_SET_IMAGE_SIZE		_IOW(SNAPSHOT_IOC_MAGIC, 6, unsigned long)
 #define SNAPSHOT_AVAIL_SWAP		_IOR(SNAPSHOT_IOC_MAGIC, 7, void *)
 #define SNAPSHOT_GET_SWAP_PAGE		_IOR(SNAPSHOT_IOC_MAGIC, 8, void *)
+#define SNAPSHOT_ATOMIC_SNAPSHOT32	_IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
+#define SNAPSHOT_SET_IMAGE_SIZE32	_IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
+#define SNAPSHOT_AVAIL_SWAP32		_IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
+#define SNAPSHOT_GET_SWAP_PAGE32	_IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)
 
 
 #define SNAPSHOT_MINOR	231
@@ -249,6 +253,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_CREATE_IMAGE:
 	case SNAPSHOT_ATOMIC_SNAPSHOT:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_ATOMIC_SNAPSHOT32:
+#endif
 		if (data->mode != O_RDONLY || !data->frozen  || data->ready) {
 			error = -EPERM;
 			break;
@@ -278,6 +285,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_PREF_IMAGE_SIZE:
 	case SNAPSHOT_SET_IMAGE_SIZE:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_SET_IMAGE_SIZE32:
+#endif
 		image_size = arg;
 		break;
 
@@ -293,6 +303,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_AVAIL_SWAP_SIZE:
 	case SNAPSHOT_AVAIL_SWAP:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_AVAIL_SWAP32:
+#endif
 		size = count_swap_pages(data->swap, 1);
 		size <<= PAGE_SHIFT;
 		error = put_user(size, (loff_t __user *)arg);
@@ -300,6 +313,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_ALLOC_SWAP_PAGE:
 	case SNAPSHOT_GET_SWAP_PAGE:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_GET_SWAP_PAGE32:
+#endif
 		if (data->swap < 0 || data->swap >= MAX_SWAPFILES) {
 			error = -ENODEV;
 			break;
@@ -436,6 +452,9 @@ static const struct file_operations snapshot_fops = {
 	.write = snapshot_write,
 	.llseek = no_llseek,
 	.unlocked_ioctl = snapshot_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snapshot_ioctl,
+#endif
 };
 
 static struct miscdevice snapshot_device = {

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Arnd Bergmann wrote:
> On Monday 04 May 2009, Michael Tokarev wrote:
>> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
>> include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
[]

> You are however missing support for deprecated ioctls in your
> code: SNAPSHOT_SET_SWAP_FILE and SNAPSHOT_PMOPS are trivial
> (COMPATIBLE_IOCTL), but you also need to add support for
> these

Well, as comments in that file (kernel/power/user.c) states, those
ioctls are obsolete and will be removed and are only preserved for
compatibility etc.  Since no one complained so far (well, no one
complained about whole /dev/snapshot thing at all ;), maybe there's
no need to define those and the following ones too?

> #define SNAPSHOT_ATOMIC_SNAPSHOT32 _IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
> #define SNAPSHOT_SET_IMAGE_SIZE32 _IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
> #define SNAPSHOT_AVAIL_SWAP32 _IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
> #define SNAPSHOT_GET_SWAP_PAGE32 _IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)
> 
>> I can't test it so far, because uswsusp tools are broken in mixed
>> 32/64bit case in other places.  But at least it compiles fine and
>> does not complain anymore.
> 
> I try to reduce the size of compat_ioctl.c. A better implementation
> would be to add a ->compat_ioctl() operation to the file_operations
> and list the compatible ioctl numbers as well.

Well, your way is definitely a lot cleaner.  But I didn't know about
either of them anyway :)

> Please try this patch instead:

Ok.  Your patch was garbled by your MUA (inserting =3D =20 and breaking
lines), but that's ok.  After de-garbling it now complains about
undefined compat_uptr_t.  For that, I've added

#ifdef CONFIG_COMPAT
#include <linux/compat.h>
#endif

at the end of the file.  Also there was a typo (duplicated above),
compat_ulong should be compat_ulong_t.  After all that it finally
compiles.  As of "works" -- the above my comment still applies,
uswsusp for one is badly broken on 32/64 bits.  I'm trying to
create a simpler test-case, as time permits.

Thanks!

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann]

On Tuesday 05 May 2009, Michael Tokarev wrote:
> Arnd Bergmann wrote:
> > On Monday 04 May 2009, Michael Tokarev wrote:
> >> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
> >> include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
> []
> 
> > You are however missing support for deprecated ioctls in your
> > code: SNAPSHOT_SET_SWAP_FILE and SNAPSHOT_PMOPS are trivial
> > (COMPATIBLE_IOCTL), but you also need to add support for
> > these
> 
> Well, as comments in that file (kernel/power/user.c) states, those
> ioctls are obsolete and will be removed and are only preserved for
> compatibility etc.  Since no one complained so far (well, no one
> complained about whole /dev/snapshot thing at all ;), maybe there's
> no need to define those and the following ones too?
> 
> > #define SNAPSHOT_ATOMIC_SNAPSHOT32 _IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
> > #define SNAPSHOT_SET_IMAGE_SIZE32 _IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
> > #define SNAPSHOT_AVAIL_SWAP32 _IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
> > #define SNAPSHOT_GET_SWAP_PAGE32 _IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)

The point is that they are meant for compatibility with existing binaries,
which is also the reason for having the compat_ioctl in the first place.
The kernel should behave in identical ways for both 32 and 64 bits, so
either we add all of the ioctl numbers in compat mode, or we remove support
for the obsolete calls in native mode as well.

> > Please try this patch instead:
> 
> Ok.  Your patch was garbled by your MUA (inserting =3D =20 and breaking
> lines), but that's ok.

sorry about that, I'll try to find out how that happened.

> After de-garbling it now complains about 
> undefined compat_uptr_t.  For that, I've added
> 
> #ifdef CONFIG_COMPAT
> #include <linux/compat.h>
> #endif

This one does not need to be enclosed in #ifdef CONFIG_COMPAT,
the file can always be included. The reason for the #ifdef inside
of the switch() statement is that you otherwise get warnings
about duplicate case statements on 32 bit kernels.
 
	Arnd <><

Re: compat ioctl32 for /dev/snapshot?

[Rafael J. Wysocki]

On Monday 04 May 2009, Michael Tokarev wrote:
> Is there any reason why 32-bit uswsusp &Friends does not work
> on 64bits kernel?
> 
> For one, 32bits s2disk produces the following when trying to
> suspend:
> 
>   ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>   ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
> 
> this is coming from:
> 
>      error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
>      if (error && !offset)
>              error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
> 
> but I guess (just guess!) that other SNAPSHOT_* operations will
> also fail the same way.
> 
> Is there a reason why those are not in compat_ioctl?

The user land hasn't been designed with that in mind.  You'd most likely have
to modify it too.

Thanks,
Rafael

Re: compat ioctl32 for /dev/snapshot?

[Pavel Machek]

On Mon 2009-05-04 13:29:22, Michael Tokarev wrote:
> Is there any reason why 32-bit uswsusp &Friends does not work
> on 64bits kernel?
>
> For one, 32bits s2disk produces the following when trying to
> suspend:
>
>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>
> this is coming from:
>
>     error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
>     if (error && !offset)
>             error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
>
> but I guess (just guess!) that other SNAPSHOT_* operations will
> also fail the same way.
>
> Is there a reason why those are not in compat_ioctl?

Lazyness...?

Patch would be welcome. On 4GB machine, running 64bit kernel (but
staying with 32bit userland) makes sense...


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

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Pavel Machek wrote:
> On Mon 2009-05-04 13:29:22, Michael Tokarev wrote:
>> Is there any reason why 32-bit uswsusp &Friends does not work
>> on 64bits kernel?
>>
>> For one, 32bits s2disk produces the following when trying to
>> suspend:
>>
>>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>>  ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>>
>> this is coming from:
>>
>>     error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
>>     if (error && !offset)
>>             error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
>>
>> but I guess (just guess!) that other SNAPSHOT_* operations will
>> also fail the same way.
>>
>> Is there a reason why those are not in compat_ioctl?
> 
> Lazyness...?
> 
> Patch would be welcome.

Pavel, you really should take a look at the original thread.

As I mentioned in my first email, I'm not the right person to
do the patch.  But regardless, I spent about a day understanding
this stuff (or trying to, anyway) - 100% useless day - and when
I thought I have a patch someone else spoken up and said this
way (compat_ioctl) is the wrong approach now.  And sent another,
also trivial patch, adding compat calls right to the proper
place in kernel/power.c.  Which (the patch) has been ignored
too.

So "welcome" is a wrong word here, and I'm sorry about that.

 > On 4GB machine, running 64bit kernel (but
> staying with 32bit userland) makes sense...

This is exactly what I'm running here by the way.
But regardless, uswsusp should be fixed too before it will
be useful for that.  And fixing uswsusp is not trivial,
unlike kernel side.  But having in mind amount of useless
jumping needed just for the trivial kernel part, for a
20-minute patch, -- there's not much hope really.  (I
wanted to fix it too, initially, -- not any more).

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann]

On Sunday 12 July 2009, Michael Tokarev wrote:
> As I mentioned in my first email, I'm not the right person to
> do the patch.  But regardless, I spent about a day understanding
> this stuff (or trying to, anyway) - 100% useless day - and when
> I thought I have a patch someone else spoken up and said this
> way (compat_ioctl) is the wrong approach now.  And sent another,
> also trivial patch, adding compat calls right to the proper
> place in kernel/power.c.  Which (the patch) has been ignored
> too.

I never got a reply from you saying whether or not the patch
I sent actually worked. If you or someone else can confirm it,
I'll resend with the fixes you mentioned.

	Arnd <><

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Arnd Bergmann wrote:
> On Sunday 12 July 2009, Michael Tokarev wrote:
>> As I mentioned in my first email, I'm not the right person to
>> do the patch.  But regardless, I spent about a day understanding
>> this stuff (or trying to, anyway) - 100% useless day - and when
>> I thought I have a patch someone else spoken up and said this
>> way (compat_ioctl) is the wrong approach now.  And sent another,
>> also trivial patch, adding compat calls right to the proper
>> place in kernel/power.c.  Which (the patch) has been ignored
>> too.
> 
> I never got a reply from you saying whether or not the patch
> I sent actually worked. If you or someone else can confirm it,
> I'll resend with the fixes you mentioned.

In order to (try to) check if it works or not, another userspace
component has to be fixed to support 32/64 bit mode.  It's uswsusp,
which currently assumes swap space structures are all 32bits.  So
it isn't possible to immediately check if it works or not -- just
ioctl(s) aren't enough.  Complete fix (kernel+user space) requires
both, fixing all remaining (yet unknown) issues in old and new
code on the way.

For now, I think it's best to let Pavel or Rafael to decide what
to do with all this.

Thanks!

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Pavel Machek]

On Mon 2009-07-13 10:51:15, Michael Tokarev wrote:
> Arnd Bergmann wrote:
>> On Sunday 12 July 2009, Michael Tokarev wrote:
>>> As I mentioned in my first email, I'm not the right person to
>>> do the patch.  But regardless, I spent about a day understanding
>>> this stuff (or trying to, anyway) - 100% useless day - and when
>>> I thought I have a patch someone else spoken up and said this
>>> way (compat_ioctl) is the wrong approach now.  And sent another,
>>> also trivial patch, adding compat calls right to the proper
>>> place in kernel/power.c.  Which (the patch) has been ignored
>>> too.
>>
>> I never got a reply from you saying whether or not the patch
>> I sent actually worked. If you or someone else can confirm it,
>> I'll resend with the fixes you mentioned.
>
> In order to (try to) check if it works or not, another userspace
> component has to be fixed to support 32/64 bit mode.  It's uswsusp,
> which currently assumes swap space structures are all 32bits.  So
> it isn't possible to immediately check if it works or not -- just
> ioctl(s) aren't enough.  Complete fix (kernel+user space) requires
> both, fixing all remaining (yet unknown) issues in old and new
> code on the way.

Well, there seems to be single structure in s2disk; that does not seem
that hard to fix.

> For now, I think it's best to let Pavel or Rafael to decide what
> to do with all this.

I don't currently have easy access to 64bit machine, so I guess it is
up to someone else.

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

Re: compat ioctl32 for /dev/snapshot?

[Michael Tokarev]

Pavel Machek wrote:
[]
>> In order to (try to) check if it works or not, another userspace
>> component has to be fixed to support 32/64 bit mode.  It's uswsusp,
>> which currently assumes swap space structures are all 32bits.  So
>> it isn't possible to immediately check if it works or not -- just
>> ioctl(s) aren't enough.  Complete fix (kernel+user space) requires
>> both, fixing all remaining (yet unknown) issues in old and new
>> code on the way.
> 
> Well, there seems to be single structure in s2disk; that does not seem
> that hard to fix.

Which one do you mean?

Also, do you want to make it 32/64 bit clean in userspace too, so that
an image produced by 32-bit s2disk can be read by 64bit resume and
vise versa?  Sure it's good thing to have, to avoid possible issues
with 32bit initramfs on 64bit system for example...

>> For now, I think it's best to let Pavel or Rafael to decide what
>> to do with all this.
> 
> I don't currently have easy access to 64bit machine, so I guess it is
> up to someone else.
[]

Ok.  I applied Arnd's patch again (with two fixes -- adding
#include <linux/compat.h> and s/compat_ulong/compat_ulong_t/ and
tried suspend/resume cycle with unmodified uswsusp-0.8.  Suspend
worked (seemengly - it reported about 4G pages written, on a
machine with 4G memory) but resume failed after reading all the
pages and submitting them to /dev/snapshot.

I've no time right now to debug it further (and again, yet again:
I don't know the code, neither kernel nor userspace part).

So I'd merge this change for now and deal with possible bugs later.

At least there's no failed ioctl()s on /dev/snapshot anymore,
neither at suspend nor at resume time.

/mjt

Re: compat ioctl32 for /dev/snapshot?

[Pavel Machek]

On Tue 2009-07-14 10:57:06, Michael Tokarev wrote:
> Pavel Machek wrote:
> []
>>> In order to (try to) check if it works or not, another userspace
>>> component has to be fixed to support 32/64 bit mode.  It's uswsusp,
>>> which currently assumes swap space structures are all 32bits.  So
>>> it isn't possible to immediately check if it works or not -- just
>>> ioctl(s) aren't enough.  Complete fix (kernel+user space) requires
>>> both, fixing all remaining (yet unknown) issues in old and new
>>> code on the way.
>>
>> Well, there seems to be single structure in s2disk; that does not seem
>> that hard to fix.
>
> Which one do you mean?

struct swsusp_info should be the one...

> Also, do you want to make it 32/64 bit clean in userspace too, so that
> an image produced by 32-bit s2disk can be read by 64bit resume and
> vise versa?  Sure it's good thing to have, to avoid possible issues
> with 32bit initramfs on 64bit system for example...

It would be cool, but...

>>> For now, I think it's best to let Pavel or Rafael to decide what
>>> to do with all this.
>>
>> I don't currently have easy access to 64bit machine, so I guess it is
>> up to someone else.
> []
>
> Ok.  I applied Arnd's patch again (with two fixes -- adding
> #include <linux/compat.h> and s/compat_ulong/compat_ulong_t/ and
> tried suspend/resume cycle with unmodified uswsusp-0.8.  Suspend
> worked (seemengly - it reported about 4G pages written, on a
> machine with 4G memory) but resume failed after reading all the
> pages and submitting them to /dev/snapshot.
>
> I've no time right now to debug it further (and again, yet again:
> I don't know the code, neither kernel nor userspace part).
>
> So I'd merge this change for now and deal with possible bugs later.
>
> At least there's no failed ioctl()s on /dev/snapshot anymore,
> neither at suspend nor at resume time.

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