Re: snd-ioctl32 is failing on x86_64 (no crashes, just errors)

[Takashi Iwai <tiwai@suse.de>]

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

At Tue, 11 Jan 2005 01:15:59 +0900,
Georgi Georgiev wrote:
> 
> maillog: 10/01/2005-14:39:35(+0100): Takashi Iwai types
> > At Mon, 10 Jan 2005 21:33:42 +0900, Georgi Georgiev wrote:
> > > Jan 10 21:08:24 lion ioctl32(amixer:243138): Unknown cmd fd(3) cmd(c2c45512){02} arg(ffffc610) on /dev/snd/controlC0
> > > Jan 10 21:08:27 lion ioctl32(amixer:243139): Unknown cmd fd(3) cmd(c2c45512){02} arg(ffffc600) on /dev/snd/controlC0
> > > Jan 10 21:21:06 lion ioctl32(amixer:243280): Unknown cmd fd(3) cmd(c2c45512){02} arg(ffffc670) on /dev/snd/controlC0
> > > Jan 10 21:21:09 lion ioctl32(amixer:243281): Unknown cmd fd(3) cmd(c2c45512){02} arg(ffffc670) on /dev/snd/controlC0
> > > 
> > > though the error message is quite different than the printk above, it
> > > could still be helpful.
> > 
> > ... or, this can be the real problem.  It looks the struct size
> > doesn't match.  Could you try the attached patch?
> 
> Except for the following small typo:
> 
> -}_ _attribute((packed));
> +} __attribute((packed));
> 
> your patch worked just fine. The 32-bit amixer produces meaningful
> output and RealPlayer is able to play sound on my system now.

Thanks for the confirmation.

IIRC, the packed attribute was removed because it had a problem on
SPARC.

Anyway, the patch below simplifies the compat struct definition so
that it will have no alignment problem.  Could you give a try?

If this one works, I'll prefer to commit it than the previous hack.


Takashi

[-- Attachment #2: Type: text/plain, Size: 4330 bytes --]

Index: alsa-kernel/core/ioctl32/ioctl32.c
===================================================================
RCS file: /home/tiwai/cvs/alsa/alsa-kernel/core/ioctl32/ioctl32.c,v
retrieving revision 1.30
diff -u -r1.30 ioctl32.c
--- alsa-kernel/core/ioctl32/ioctl32.c	15 Dec 2004 15:26:24 -0000	1.30
+++ alsa-kernel/core/ioctl32/ioctl32.c	10 Jan 2005 16:59:33 -0000
@@ -219,25 +219,10 @@
 	struct sndrv_ctl_elem_id id;
 	unsigned int indirect;	/* bit-field causes misalignment */
         union {
-		union {
-			s32 value[128];
-			u32 value_ptr;
-		} integer;
-		union {
-			s64 value[64];
-			u32 value_ptr;
-		} integer64;
-		union {
-			u32 item[128];
-			u32 item_ptr;
-		} enumerated;
-		union {
-			unsigned char data[512];
-			u32 data_ptr;
-		} bytes;
-		struct sndrv_aes_iec958 iec958;
+		s32 integer[128];	/* integer and boolean need conversion */
+		unsigned char data[512];	/* others should be compatible */
         } value;
-        unsigned char reserved[128];
+        unsigned char reserved[128];	/* not used */
 };
 
 
@@ -269,7 +254,7 @@
 	struct sndrv_ctl_elem_value *data;
 	struct sndrv_ctl_elem_value32 __user *data32;
 	snd_ctl_file_t *ctl;
-	int err, i;
+	int err, i, indirect;
 	int type;
 
 	/* sanity check */
@@ -281,7 +266,7 @@
 		return -ENOTTY;
 
 	data32 = compat_ptr(arg);
-	data = kmalloc(sizeof(*data), GFP_KERNEL);
+	data = kcalloc(1, sizeof(*data), GFP_KERNEL);
 	if (data == NULL)
 		return -ENOMEM;
 
@@ -289,12 +274,12 @@
 		err = -EFAULT;
 		goto __end;
 	}
-	if (__get_user(data->indirect, &data32->indirect)) {
+	if (__get_user(indirect, &data32->indirect)) {
 		err = -EFAULT;
 		goto __end;
 	}
 	/* FIXME: indirect access is not supported */
-	if (data->indirect) {
+	if (indirect) {
 		err = -EINVAL;
 		goto __end;
 	}
@@ -309,7 +294,7 @@
 	case SNDRV_CTL_ELEM_TYPE_INTEGER:
 		for (i = 0; i < 128; i++) {
 			int val;
-			if (__get_user(val, &data32->value.integer.value[i])) {
+			if (__get_user(val, &data32->value.integer[i])) {
 				err = -EFAULT;
 				goto __end;
 			}
@@ -317,33 +302,12 @@
 		}
 		break;
 	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		if (__copy_from_user(data->value.integer64.value,
-				     data32->value.integer64.value,
-				     sizeof(data->value.integer64.value))) {
-			err = -EFAULT;
-			goto __end;
-		}
-		break;
 	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		if (__copy_from_user(data->value.enumerated.item,
-				     data32->value.enumerated.item,
-				     sizeof(data32->value.enumerated.item))) {
-			err = -EFAULT;
-			goto __end;
-		}
-		break;
 	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		if (__copy_from_user(data->value.bytes.data,
-				     data32->value.bytes.data,
-				     sizeof(data32->value.bytes.data))) {
-			err = -EFAULT;
-			goto __end;
-		}
-		break;
 	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		if (__copy_from_user(&data->value.iec958,
-				     &data32->value.iec958,
-				     sizeof(data32->value.iec958))) {
+		if (__copy_from_user(data->value.data,
+				     data32->value.data,
+				     sizeof(data32->value.data))) {
 			err = -EFAULT;
 			goto __end;
 		}
@@ -367,43 +331,20 @@
 		for (i = 0; i < 128; i++) {
 			int val;
 			val = data->value.integer.value[i];
-			if (__put_user(val, &data32->value.integer.value[i])) {
+			if (__put_user(val, &data32->value.integer[i])) {
 				err = -EFAULT;
 				goto __end;
 			}
 		}
 		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		if (__copy_to_user(data32->value.integer64.value,
-				   data->value.integer64.value,
-				   sizeof(data32->value.integer64.value))) {
-			err = -EFAULT;
-			goto __end;
-		}
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		if (__copy_to_user(data32->value.enumerated.item,
-				   data->value.enumerated.item,
-				   sizeof(data32->value.enumerated.item))) {
-			err = -EFAULT;
-			goto __end;
-		}
-		break;
-	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		if (__copy_to_user(data32->value.bytes.data,
-				   data->value.bytes.data,
-				   sizeof(data32->value.bytes.data))) {
+	default:
+		if (__copy_to_user(data32->value.data,
+				   data->value.data,
+				   sizeof(data32->value.data))) {
 			err = -EFAULT;
 			goto __end;
 		}
 		break;
-	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		if (__copy_to_user(&data32->value.iec958,
-				   &data->value.iec958,
-				   sizeof(data32->value.iec958))) {
-			err = -EFAULT;
-			goto __end;
-		}
 		break;
 	}
 	err = 0;