[PATCH 00/24] ALSA: ctl: refactoring compat layer

[PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Sakamoto]

Hi,

ALSA control core supports system call compatibility layer because some
structures in ALSA control interface includes members of 'long' and
'pointer' types and change own layout according to ABIs. In this point,
it's enough for the layer to have handlers for structures on ABIs with
'ILP32' data model.

A recent commit of 6236d8bb2afc ('ALSA: ctl: Fix ioctls for X32 ABI')
clear that System V ABI for i386 architecture is unique than the other
ABIs with 'ILP32' data model. On this ABI, machine type for storage class
of 'long long' type has 4 bytes alignment. This is different from the
other ABIs.

In current implementation of this layer, the same structure is used for
i386 ABI and the other ABI with 'ILP32' data model except for x32 ABI.
Macro is used to switch between these. But in a view of data model,
it's better to define structure for i386 ABI uniquely against the
other ABIs including x32 for simplicity.

Additionally, this layer includes some points to be improved:
 - cancel allocation in user space from kernel land
 - reduce kernel stack usage

This patchset is my attempts to improve the layer. For this purpose,
this patchset introduces a local framework to describe consistent method
to convert and process data for differences of structure layout.

Takashi Sakamoto (24):
  ALSA: ctl: introduce local framework to handle compat ioctl requests
  ALSA: ctl: add serializer/deserializer of 'elem_list' structure for
    32bit ABI compatibility
  ALSA: ctl: add serializer/deserializer of 'elem_info' structure for
    32bit ABI compatibility
  ALSA: ctl: add serializer/deserializer of 'elem_value' structure for
    modern 32 bit ABIs compatibility
  ALSA: ctl: add serializer/deserializer of 'elem_value' structure for
    i386 ABI compatibility
  ALSA: ctl: change prototype of local function for ELEM_LIST ioctl
  ALSA: ctl: add a helper function to allocate for ELEM_LIST request
  ALSA: ctl: cancel allocation in user space for ELEM_LIST request
  ALSA: ctl: unify calls of D0-wait function for ELEM_INFO request
  ALSA: ctl: unify calls of D0-wait function for ELEM_READ request
  ALSA: ctl: unify calls of D0-wait function for ELEM_WRITE request
  ALSA: ctl: allocation of elem_info data for ELEM_INFO request
  ALSA: ctl: change prototype of local function for ELEM_WRITE ioctl
  ALSA: ctl: change prototype of local function for ELEM_READ ioctl
  ALSA: ctl: allocation of elem_info data for ELEM_ADD/ELEM_REPLACE
    requests
  ALSA: ctl: add replace helper function to allocate own buffer
  ALSA: ctl: move removal code to replace helper function
  ALSA: ctl: replacement for compat ELEM_LIST operation for any 32 bit
    ABI
  ALSA: ctl: replacement for compat ELEM_INFO operation for any 32 bit
    ABI
  ALSA: ctl: replacement for compat ELEM_ADD operation for any 32 bit
    ABI
  ALSA: ctl: replacement for compat ELEM_REPLACE operation for any 32
    bit ABI
  ALSA: ctl: replacement for compat ELEM_READ/ELEM_WRITE operation for
    i386 ABI
  ALSA: ctl: replacement for compat ELEM_READ/ELEM_WRITE operation for
    modern 32 bit ABI
  ALSA: ctl: code cleanup for compat handler

 sound/core/control.c        | 242 ++++++++-----
 sound/core/control_compat.c | 837 +++++++++++++++++++++++++-------------------
 2 files changed, 630 insertions(+), 449 deletions(-)

-- 
2.14.1

[PATCH 01/24] ALSA: ctl: introduce local framework to handle compat ioctl requests

[Takashi Sakamoto]

Some architectures have a execution mode to execute programs for derived
architectures. Linux system supports the execution mode. For this purpose,
system call compatibility layer is available to manage ABI differences of
the architectures; e.g. adoption different data model (ILP32/LP64) and
size/alignment of machine types.

In ALSA control interface, there're some structures which have different
layout depending on the ABIs. To absorb the difference, ALSA control core
has an I/O compatibility layer. Tasks of this layer are summarized as:
1. convert structure layout from program ABI to native ABI
2. process I/O request with structure layout of native ABI
3. convert structure layout from native ABI to program ABI

Current implementation of this layer is not enough structured for the above
processing. This commit adds a consistent way to handle the above tasks.

Actual implementation of the tasks differs depending on the structures.
This commit adds a local framework for handlers tothe tasks. A handler
includes below members:
 - cmd: a command of ioctl(2) on program ABI. The 'size' field is used to
        distinguish compat I/O requests from native I/O requests, and
        allocate data buffer.
 - deserialize: for task 1.
 - func: for task 2.
 - serialize: for task 3.
 - orig_cmd: a command of ioctl(2) on native ABI. The 'size' field is used
             to allocate an alternative buffer.

The handlers are processed in a callback of
'struct file_operations.compat_ioctl'. In this callback, two buffers are
allocated/deallocated for safe conversion of layout between program/native
ABI.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 97 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 74 insertions(+), 23 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index a848836a5de0..dec89717d8e3 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -443,11 +443,25 @@ enum {
 #endif /* CONFIG_X86_X32 */
 };
 
-static inline long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
+static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
+				 unsigned long arg)
 {
+	static const struct {
+		unsigned int cmd;
+		int (*deserialize)(struct snd_ctl_file *ctl_file, void *dst,
+				   void *src);
+		int (*func)(struct snd_ctl_file *ctl_file, void *buf);
+		int (*serialize)(struct snd_ctl_file *ctl_file, void *dst,
+				 void *src);
+		unsigned int orig_cmd;
+	} handlers[] = {
+		{ 0, NULL, NULL, NULL, 0, },
+	};
 	struct snd_ctl_file *ctl;
-	struct snd_kctl_ioctl *p;
 	void __user *argp = compat_ptr(arg);
+	void *buf, *data;
+	unsigned int size;
+	int i;
 	int err;
 
 	ctl = file->private_data;
@@ -455,18 +469,6 @@ static inline long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd, uns
 		return -ENXIO;
 
 	switch (cmd) {
-	case SNDRV_CTL_IOCTL_PVERSION:
-	case SNDRV_CTL_IOCTL_CARD_INFO:
-	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
-	case SNDRV_CTL_IOCTL_POWER:
-	case SNDRV_CTL_IOCTL_POWER_STATE:
-	case SNDRV_CTL_IOCTL_ELEM_LOCK:
-	case SNDRV_CTL_IOCTL_ELEM_UNLOCK:
-	case SNDRV_CTL_IOCTL_ELEM_REMOVE:
-	case SNDRV_CTL_IOCTL_TLV_READ:
-	case SNDRV_CTL_IOCTL_TLV_WRITE:
-	case SNDRV_CTL_IOCTL_TLV_COMMAND:
-		return snd_ctl_ioctl(file, cmd, (unsigned long)argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST32:
 		return snd_ctl_elem_list_compat(ctl->card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO32:
@@ -487,16 +489,65 @@ static inline long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd, uns
 #endif /* CONFIG_X86_X32 */
 	}
 
-	down_read(&snd_ioctl_rwsem);
-	list_for_each_entry(p, &snd_control_compat_ioctls, list) {
-		if (p->fioctl) {
-			err = p->fioctl(ctl->card, ctl, cmd, arg);
-			if (err != -ENOIOCTLCMD) {
-				up_read(&snd_ioctl_rwsem);
-				return err;
+	for (i = 0; i < ARRAY_SIZE(handlers); ++i) {
+		if (handlers[i].cmd == cmd)
+			break;
+	}
+	if (i == ARRAY_SIZE(handlers)) {
+		struct snd_kctl_ioctl *p;
+
+		down_read(&snd_ioctl_rwsem);
+		list_for_each_entry(p, &snd_control_compat_ioctls, list) {
+			if (p->fioctl) {
+				err = p->fioctl(ctl->card, ctl, cmd, arg);
+				if (err != -ENOIOCTLCMD) {
+					up_read(&snd_ioctl_rwsem);
+					return err;
+				}
 			}
 		}
+		up_read(&snd_ioctl_rwsem);
+
+		return snd_ctl_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+	}
+
+	/* Allocate a buffer to convert layout of structure for native ABI. */
+	buf = kzalloc(_IOC_SIZE(handlers[i].orig_cmd), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Allocate an alternative buffer to copy from/to user space. */
+	size = _IOC_SIZE(handlers[i].cmd);
+	data = kzalloc(size, GFP_KERNEL);
+	if (!data) {
+		kfree(buf);
+		return -ENOMEM;
+	}
+
+	if (handlers[i].cmd & IOC_IN) {
+		if (copy_from_user(data, compat_ptr(arg), size)) {
+			err = -EFAULT;
+			goto end;
+		}
 	}
-	up_read(&snd_ioctl_rwsem);
-	return -ENOIOCTLCMD;
+
+	err = handlers[i].deserialize(ctl, buf, data);
+	if (err < 0)
+		goto end;
+
+	err = handlers[i].func(ctl, buf);
+	if (err < 0)
+		goto end;
+
+	err = handlers[i].serialize(ctl, data, buf);
+	if (err >= 0) {
+		if (handlers[i].cmd & IOC_OUT) {
+			if (copy_to_user(compat_ptr(arg), data, size))
+				err = -EFAULT;
+		}
+	}
+end:
+	kfree(data);
+	kfree(buf);
+	return err;
 }
-- 
2.14.1

[PATCH 02/24] ALSA: ctl: add serializer/deserializer of 'elem_list' structure for 32bit ABI compatibility

[Takashi Sakamoto]

A 'snd_ctl_elem_list' structure includes a member of 'pointer' type.
A machine type for the type is known to have different size and alignment
between System V ABIs for architectures which adopts 'ILP32' or 'LP64'
data model.

This commit adds a pair of serializer and deserializer to convert data
between different layout of the structure on the ABIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 82 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index dec89717d8e3..3bd04ef5d221 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -23,6 +23,88 @@
 #include <linux/compat.h>
 #include <linux/slab.h>
 
+/*
+ * In this file, System V ABIs for any 64 bit architecture are assumed:
+ *  - Machine type for storage class of 1 byte has:
+ *    - size: 1 byte
+ *    - alignment: 1 byte
+ *  - Machine type for storage class of 2 bytes has:
+ *    - size: 2 bytes
+ *    - alignment: 2 bytes
+ *  - Machine type for storage class of 4 bytes has:
+ *    - size: 4 bytes
+ *    - alignment: 4 bytes
+ *  - Machine type for storage class of 8 bytes has:
+ *    - size: 8 bytes
+ *    - alignment: 8 bytes
+ *
+ * And System V ABIs for modern 32 bit architecture are assumed to have the same
+ * rule about size and alignment as the above machine types.
+ *
+ * As an exception, System V ABI for i386 architecture is assumed:
+ *  - Machine type for storage class of 8 bytes has:
+ *    - size: 8 bytes
+ *    - alignment: 4 bytes
+ *
+ * Any System V ABIs are assumed to have the same rule for aggregates, unions
+ * and alignment of members with bitfields. Additionally, 'packed' of attribute
+ * is a hint for compiles to remove internal and tail paddings even if bitfields
+ * are used.
+ */
+
+/*
+ * In any System V ABI for 32 bit architecture, the maximum length of members on
+ * this structure is 4 bytes. This member has 4 byte alignment and the size of
+ * this structure is multiples of 4 bytes, equals to 72 bytes. However, total
+ * size of all members is 70 bytes. As a result, 2 bytes are added as padding in
+ * the end.
+ */
+struct snd_ctl_elem_list_32 {
+	u32 offset;
+	u32 space;
+	u32 used;
+	u32 count;
+	u32 pids;		/* pointer on ILP32. */
+	u8 reserved[50];
+	u8 padding[2];
+} __packed;
+
+static int deserialize_from_elem_list_32(struct snd_ctl_file *ctl_file,
+					 void *dst, void *src)
+{
+	struct snd_ctl_elem_list *data = dst;
+	struct snd_ctl_elem_list_32 *data32 = src;
+
+	data->offset = data32->offset;
+	data->space = data32->space;
+	data->used = data32->used;
+	data->count = data32->count;
+
+	data->pids = (struct snd_ctl_elem_id __user *)compat_ptr(data32->pids);
+
+	memcpy(data->reserved, data32->reserved, sizeof(data->reserved));
+
+	return 0;
+}
+
+static int serialize_to_elem_list_32(struct snd_ctl_file *ctl_file, void *dst,
+				     void *src)
+{
+	struct snd_ctl_elem_list_32 *data32 = dst;
+	struct snd_ctl_elem_list *data = src;
+
+	data32->offset = data->offset;
+	data32->space = data->space;
+	data32->used = data->used;
+	data32->count = data->count;
+
+	data32->pids = (u32)ptr_to_compat(data->pids);
+
+	memcpy(data32->reserved, data->reserved, sizeof(data32->reserved));
+
+	return 0;
+}
+
 struct snd_ctl_elem_list32 {
 	u32 offset;
 	u32 space;
-- 
2.14.1

[PATCH 03/24] ALSA: ctl: add serializer/deserializer of 'elem_info' structure for 32bit ABI compatibility

[Takashi Sakamoto]

A 'snd_ctl_elem_info' structure includes a member of 'integer long' type.
A machine type for the type is known to have different size and alignment
between System V ABIs for architectures which adopts 'ILP32' or 'LP64'
data model.

This commit adds a pair of serializer and deserializer to convert data
between different layout of the structure on the ABIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 3bd04ef5d221..7ebae996804a 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -105,6 +105,100 @@ static int serialize_to_elem_list_32(struct snd_ctl_file *ctl_file, void *dst,
 	return 0;
 }
 
+/*
+ * In this structure, '.value' member includes double-word (= 64 bits) member
+ * ('.integer64'). System V ABI for i386 architecture adopts different byte
+ * alignment for this type (4 bytes) than the ones in the other architectures
+ * (8 bytes). Fortunately, the total size of '.id', '.type', '.access', '.count'
+ * and '.owner' is multiples of 8, and there's no issue for offset of the
+ * '.value' member.
+ */
+struct snd_ctl_elem_info_32 {
+	struct snd_ctl_elem_id id;
+	s32 type;
+	u32 access;
+	u32 count;
+	s32 owner;
+	union {
+		struct {
+			s32 min;	/* long on ILP32. */
+			s32 max;	/* long on ILP32. */
+			s32 step;	/* long on ILP32. */
+		} integer;
+		struct {
+			u64 min;
+			u64 max;
+			u64 step;
+		} integer64;
+		struct {
+			u32 items;
+			u32 item;
+			s8 name[64];
+			u64 names_ptr;
+			u32 names_length;
+		} enumerated;
+		u8 reserved[128];
+	} value;
+	u16 dimen[4];
+	u8 reserved[64 - 4 * sizeof(u16)];
+} __packed;
+
+static int deserialize_from_elem_info_32(struct snd_ctl_file *ctl_file,
+					 void *dst, void *src)
+{
+	struct snd_ctl_elem_info *data = dst;
+	struct snd_ctl_elem_info_32 *data32 = src;
+
+	data->id = data32->id;
+	data->type = data32->type;
+	data->access = data32->access;
+	data->count = data32->count;
+	data->owner = data32->owner;
+
+	if (data->type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		data->value.integer.min = (s64)data32->value.integer.min;
+		data->value.integer.max = (s64)data32->value.integer.max;
+		data->value.integer.step = (s64)data32->value.integer.step;
+		/* Drop the rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&data->value, &data32->value, sizeof(data->value));
+	}
+
+	memcpy(&data->dimen, &data32->dimen, sizeof(data->dimen));
+	memcpy(&data->reserved, &data32->reserved, sizeof(data->reserved));
+
+	return 0;
+}
+
+static int serialize_to_elem_info_32(struct snd_ctl_file *ctl_file, void *dst,
+				     void *src)
+{
+	struct snd_ctl_elem_info_32 *data32 = dst;
+	struct snd_ctl_elem_info *data = src;
+
+	data32->id = data->id;
+	data32->type = data->type;
+	data32->access = data->access;
+	data32->count = data->count;
+	data32->owner = data->owner;
+
+	if (data->type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		data32->value.integer.min = (s32)data->value.integer.min;
+		data32->value.integer.max = (s32)data->value.integer.max;
+		data32->value.integer.step = (s32)data->value.integer.step;
+		/* Drop rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&data32->value, &data->value, sizeof(data32->value));
+	}
+
+	memcpy(&data32->dimen, &data->dimen, sizeof(data32->dimen));
+	memcpy(&data32->reserved, &data->reserved, sizeof(data32->reserved));
+
+	return 0;
+}
+
 struct snd_ctl_elem_list32 {
 	u32 offset;
 	u32 space;
-- 
2.14.1

[PATCH 04/24] ALSA: ctl: add serializer/deserializer of 'elem_value' structure for modern 32 bit ABIs compatibility

[Takashi Sakamoto]

A structure of 'snd_ctl_elem_value' includes a member of 'long' type. A
machine type for this type is known to have different size and alignment
between System V ABIs for architectures which adopts 'ILP32' or 'LP64'
data model.

Furthermore, the structure has another issue about offset for union with
a member of 'long long' type. Regardless of the ABIs, this type has 8 bytes
size and 8 bytes alignment. This causes the offset as multiples of 8 bytes.
On the other hand, total in the structure, size of members forth of the
union is not multiples of 8 bytes. This brings 'internal padding' to the
structure.

This commit adds a pair of serializer and deserializer to convert data
between different layout of the structure on the ABIs. To describe
layout of the structure for ILP32 ABI, bitfield is used for the internal
padding, and 'packed' attribute is used as a hint for compilers to
native padding convention.

On machines of Intel architecture, unless support for x32 ABI is enabled,
the serializer/deserializer are useless. For this case, '__maybe_unused'
is added as a hint to compilers for optimization.

Reference: System V application binary interface AMD64 architecture
           processor supplement (with LP64 and ILP32 programming models)
           draft version 0.99.8 (2017, H.J. Lu, Michael Matz, Milind
           Girkar, Jan Hubicka, Andreas Jaeger and Mark Mitchell)
Reference: Procedure call standard for the ARM architecture (2015, ARM
           Ltd.) ARM IHI 0042F
Reference: Procedure call standard for the ARM 64-bit architecture
           (aarch64) (2013, ARM Limited) ARM IHI 0055C_beta
Reference: System V application binary interface PowerPC processor
           supplement, revision A (1995, Steve Zecker and Kari Karhi)
           802-334-10
Reference: 64-bit PowerPC ELF application binary interface supplement, 1.7
           edition (2003, Ian Lance Taylor)
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 109 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 7ebae996804a..5c0e82afe400 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -199,6 +199,115 @@ static int serialize_to_elem_info_32(struct snd_ctl_file *ctl_file, void *dst,
 	return 0;
 }
 
+/*
+ * Unfortunately, unlike the above structure, total size of '.id' and
+ * '.indirect' is not multiples of 8 bytes (it's 68 bytes). Except for System V
+ * ABI for i386 architecture, 'double-word' type has 8 bytes alignment. For such
+ * architectures, 32 bit padding is needed.
+ */
+struct snd_ctl_elem_value_32 {
+	struct snd_ctl_elem_id id;
+	u32 indirect:1;
+	u64 padding1:63;		/* For 8 bytes alignment of '.value'. */
+	union {
+		s32 integer[128];	/* long on ILP32. */
+		u8 data[512];
+		s64 integer64[64];
+	} value;
+	struct {
+		s32 tv_sec;		/* long on ILP32. */
+		s32 tv_nsec;		/* long on ILP32. */
+	} tstamp;
+	u8 reserved[128 - sizeof(s32) - sizeof(s32)];
+} __packed;
+
+static int get_type(struct snd_ctl_file *ctl_file, struct snd_ctl_elem_id *id,
+		    snd_ctl_elem_type_t *type)
+{
+	struct snd_ctl_elem_info *info;
+	int err;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	info->id = *id;
+
+	err = snd_ctl_elem_info(ctl_file, info);
+	if (err >= 0)
+		*type = info->type;
+
+	kfree(info);
+	return err;
+}
+
+static int __maybe_unused deserialize_from_elem_value_32(
+			struct snd_ctl_file *ctl_file, void *dst, void *src)
+{
+	struct snd_ctl_elem_value *data = dst;
+	struct snd_ctl_elem_value_32 *data32 = src;
+	snd_ctl_elem_type_t type;
+	int err;
+
+	err = get_type(ctl_file, &data32->id, &type);
+	if (err < 0)
+		return err;
+
+	data->id = data32->id;
+	data->indirect = data32->indirect;
+
+	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		int i;
+		for (i = 0; i < 128; ++i) {
+			data->value.integer.value[i] =
+						(s64)data32->value.integer[i];
+		}
+		/* Drop rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&data->value, &data32->value, sizeof(data->value));
+	}
+
+	data->tstamp.tv_sec = (s64)data32->tstamp.tv_sec;
+	data->tstamp.tv_nsec = (s64)data32->tstamp.tv_nsec;
+
+	return 0;
+}
+
+static int __maybe_unused serialize_to_elem_value_32(
+			struct snd_ctl_file *ctl_file, void *dst, void *src)
+{
+	struct snd_ctl_elem_value_32 *data32 = dst;
+	struct snd_ctl_elem_value *data = src;
+	snd_ctl_elem_type_t type;
+	int err;
+
+	err = get_type(ctl_file, &data->id, &type);
+	if (err < 0)
+		return err;
+
+	data32->id = data->id;
+	data32->indirect = data->indirect;
+
+	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		int i;
+		for (i = 0; i < 128; ++i) {
+			data32->value.integer[i] =
+					(s32)data->value.integer.value[i];
+		}
+		/* Drop rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&data32->value, &data->value, sizeof(data32->value));
+	}
+
+	data32->tstamp.tv_sec = (s32)data->tstamp.tv_sec;
+	data32->tstamp.tv_nsec = (s32)data->tstamp.tv_nsec;
+
+	return 0;
+}
+
 struct snd_ctl_elem_list32 {
 	u32 offset;
 	u32 space;
-- 
2.14.1

[PATCH 05/24] ALSA: ctl: add serializer/deserializer of 'elem_value' structure for i386 ABI compatibility

[Takashi Sakamoto]

A previous commit adds serializer/deserializer of 'snd_ctl_elem_value'
structure for System V ABIs to modern 32 bit architectures. However, i386
is an exception because it has 4 bytes alignment for double-word machine
type. This brings different 'internal padding' to the structure from the
one on ABIs for modern 32 bit architectures.

This commit adds a pair of serializer and deserializer to convert data
between different layout of the structure on the ABIs. To describe
layout of the structure for i386 ABI on LP64 ABI, bitfield is used for
the internal padding, and 'packed' attribute is used as a hint for
compilers to native padding convention.

The serializer/deserializer are useless on non-Intel architectures. For
this case, '__maybe_unused' is added as a hint to compilers for
optimization.

Reference: System V application binary interface Intel386 architecture
           processor supplenent, draft copy of forth edition (1997,
           The Santa Cruz Operation, Inc. and AT&T)
Reference: System V application binary interface AMD64 architecture
           processor supplement (with LP64 and ILP32 programming models)
           draft version 0.99.8 (2017, H.J. Lu, Michael Matz, Milind
           Girkar, Jan Hubicka, Andreas Jaeger and Mark Mitchell)
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 5c0e82afe400..5dfb31a22470 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -308,6 +308,95 @@ static int __maybe_unused serialize_to_elem_value_32(
 	return 0;
 }
 
+/*
+ * Unlikw any System V ABI for 32 bit architecture, ABI for i386 architecture has
+ * different alignment (4 bytes) for double-word type. Thus offset of '.value'
+ * member is multiples of 4 bytes.
+ */
+struct snd_ctl_elem_value_i386 {
+	struct snd_ctl_elem_id id;
+	u32 indirect:1;
+	u32 padding:31;			/* For 4 bytes alignment of '.value'. */
+	union {
+		s32 integer[128];	/* long on ILP32. */
+		u8 data[512];
+		s64 integer64[64];
+	} value;
+	struct {
+		s32 tv_sec;		/* long on ILP32. */
+		s32 tv_nsec;		/* long on ILP32. */
+	} tstamp;
+	u8 reserved[128 - sizeof(s32) - sizeof(s32)];
+} __packed;
+
+static int __maybe_unused deserialize_from_elem_value_i386(
+			struct snd_ctl_file *ctl_file, void *dst, void *src)
+{
+	struct snd_ctl_elem_value *data = dst;
+	struct snd_ctl_elem_value_i386 *datai386 = src;
+	snd_ctl_elem_type_t type;
+	int err;
+
+	err = get_type(ctl_file, &datai386->id, &type);
+	if (err < 0)
+		return err;
+
+	data->id = datai386->id;
+	data->indirect = datai386->indirect;
+
+	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		int i;
+		for (i = 0; i < 128; ++i) {
+			data->value.integer.value[i] =
+						(s64)datai386->value.integer[i];
+		}
+		/* Drop rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&data->value, &datai386->value, sizeof(data->value));
+	}
+
+	data->tstamp.tv_sec = (s64)datai386->tstamp.tv_sec;
+	data->tstamp.tv_nsec = (s64)datai386->tstamp.tv_nsec;
+
+	return 0;
+}
+
+static int __maybe_unused serialize_to_elem_value_i386(
+			struct snd_ctl_file *ctl_file, void *dst, void *src)
+{
+	struct snd_ctl_elem_value_i386 *datai386 = dst;
+	struct snd_ctl_elem_value *data = src;
+	snd_ctl_elem_type_t type;
+	int err;
+
+	err = get_type(ctl_file, &data->id, &type);
+	if (err < 0)
+		return err;
+
+	datai386->id = data->id;
+	datai386->indirect = data->indirect;
+
+	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
+		int i;
+		for (i = 0; i < 128; ++i) {
+			datai386->value.integer[i] =
+					(s32)data->value.integer.value[i];
+		}
+		/* Drop rest of this field. */
+	} else {
+		/* Copy whole space of this field. */
+		memcpy(&datai386->value, &data->value, sizeof(datai386->value));
+	}
+
+	datai386->tstamp.tv_sec = (s32)data->tstamp.tv_sec;
+	datai386->tstamp.tv_nsec = (s32)data->tstamp.tv_nsec;
+
+	return 0;
+}
+
 struct snd_ctl_elem_list32 {
 	u32 offset;
 	u32 space;
-- 
2.14.1

[PATCH 06/24] ALSA: ctl: change prototype of local function for ELEM_LIST ioctl

[Takashi Sakamoto]

When investigating compatibility layer, several functions for native ABI
are called by the layer. A 'snd_ctl_elem_list()' is such a function,
while it's prototype is not similar to the others. This will bring a
future inconvenience in integration of the layer.

This commit changes its prototype to get a first argument for data of
'struct snd_ctl_file' type.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 12 ++++++------
 sound/core/control_compat.c |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 56b3e2d49c82..8baee922a400 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -744,7 +744,7 @@ static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl,
 	return 0;
 }
 
-static int snd_ctl_elem_list(struct snd_card *card,
+static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file,
 			     struct snd_ctl_elem_list __user *_list)
 {
 	struct snd_ctl_elem_list list;
@@ -758,11 +758,11 @@ static int snd_ctl_elem_list(struct snd_card *card,
 	offset = list.offset;
 	space = list.space;
 
-	down_read(&card->controls_rwsem);
-	list.count = card->controls_count;
+	down_read(&ctl_file->card->controls_rwsem);
+	list.count = ctl_file->card->controls_count;
 	list.used = 0;
 	if (space > 0) {
-		list_for_each_entry(kctl, &card->controls, list) {
+		list_for_each_entry(kctl, &ctl_file->card->controls, list) {
 			if (offset >= kctl->count) {
 				offset -= kctl->count;
 				continue;
@@ -782,7 +782,7 @@ static int snd_ctl_elem_list(struct snd_card *card,
 		}
 	}
  out:
-	up_read(&card->controls_rwsem);
+	up_read(&ctl_file->card->controls_rwsem);
 	if (!err && copy_to_user(_list, &list, sizeof(list)))
 		err = -EFAULT;
 	return err;
@@ -1552,7 +1552,7 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_CARD_INFO:
 		return snd_ctl_card_info(card, ctl, cmd, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST:
-		return snd_ctl_elem_list(card, argp);
+		return snd_ctl_elem_list(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 5dfb31a22470..3594bd41750e 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -406,7 +406,7 @@ struct snd_ctl_elem_list32 {
 	unsigned char reserved[50];
 } /* don't set packed attribute here */;
 
-static int snd_ctl_elem_list_compat(struct snd_card *card,
+static int snd_ctl_elem_list_compat(struct snd_ctl_file *ctl_file,
 				    struct snd_ctl_elem_list32 __user *data32)
 {
 	struct snd_ctl_elem_list __user *data;
@@ -422,7 +422,7 @@ static int snd_ctl_elem_list_compat(struct snd_card *card,
 	if (get_user(ptr, &data32->pids) ||
 	    put_user(compat_ptr(ptr), &data->pids))
 		return -EFAULT;
-	err = snd_ctl_elem_list(card, data);
+	err = snd_ctl_elem_list(ctl_file, data);
 	if (err < 0)
 		return err;
 	/* copy the result */
@@ -844,7 +844,7 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 
 	switch (cmd) {
 	case SNDRV_CTL_IOCTL_ELEM_LIST32:
-		return snd_ctl_elem_list_compat(ctl->card, argp);
+		return snd_ctl_elem_list_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO32:
 		return snd_ctl_elem_info_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ32:
-- 
2.14.1

[PATCH 07/24] ALSA: ctl: add a helper function to allocate for ELEM_LIST request

[Takashi Sakamoto]

When investigating compatibility layer, several functions for native ABI
are called by the layer. A 'snd_ctl_elem_list()' is such a function,
while it cannot receive a second argument for data on kernel space. This
will bring a future inconvenience in integration of the layer.

This commit adds a helper function to allocate memory object on kernel
space for the argument.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 41 ++++++++++++++++++++++++++++-------------
 sound/core/control_compat.c |  2 +-
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 8baee922a400..ad4d27c681c4 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -745,22 +745,19 @@ static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl,
 }
 
 static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file,
-			     struct snd_ctl_elem_list __user *_list)
+			     struct snd_ctl_elem_list *list)
 {
-	struct snd_ctl_elem_list list;
 	struct snd_kcontrol *kctl;
 	struct snd_ctl_elem_id id;
 	unsigned int offset, space, jidx;
 	int err = 0;
 	
-	if (copy_from_user(&list, _list, sizeof(list)))
-		return -EFAULT;
-	offset = list.offset;
-	space = list.space;
+	offset = list->offset;
+	space = list->space;
 
 	down_read(&ctl_file->card->controls_rwsem);
-	list.count = ctl_file->card->controls_count;
-	list.used = 0;
+	list->count = ctl_file->card->controls_count;
+	list->used = 0;
 	if (space > 0) {
 		list_for_each_entry(kctl, &ctl_file->card->controls, list) {
 			if (offset >= kctl->count) {
@@ -769,12 +766,12 @@ static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file,
 			}
 			for (jidx = offset; jidx < kctl->count; jidx++) {
 				snd_ctl_build_ioff(&id, kctl, jidx);
-				if (copy_to_user(list.pids + list.used, &id,
+				if (copy_to_user(list->pids + list->used, &id,
 						 sizeof(id))) {
 					err = -EFAULT;
 					goto out;
 				}
-				list.used++;
+				list->used++;
 				if (!--space)
 					goto out;
 			}
@@ -783,8 +780,26 @@ static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file,
 	}
  out:
 	up_read(&ctl_file->card->controls_rwsem);
-	if (!err && copy_to_user(_list, &list, sizeof(list)))
-		err = -EFAULT;
+	return err;
+}
+
+static int snd_ctl_elem_list_user(struct snd_ctl_file *ctl_file,
+				  void __user *arg)
+{
+	struct snd_ctl_elem_list *list;
+	int err;
+
+	list = memdup_user(arg, sizeof(*list));
+	if (IS_ERR(list))
+		return PTR_ERR(list);
+
+	err = snd_ctl_elem_list(ctl_file, list);
+	if (err >= 0) {
+		if (copy_to_user(arg, list, sizeof(*list)))
+			err = -EFAULT;
+	}
+
+	kfree(list);
 	return err;
 }
 
@@ -1552,7 +1567,7 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_CARD_INFO:
 		return snd_ctl_card_info(card, ctl, cmd, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST:
-		return snd_ctl_elem_list(ctl, argp);
+		return snd_ctl_elem_list_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 3594bd41750e..8a4c58c0cf3b 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -422,7 +422,7 @@ static int snd_ctl_elem_list_compat(struct snd_ctl_file *ctl_file,
 	if (get_user(ptr, &data32->pids) ||
 	    put_user(compat_ptr(ptr), &data->pids))
 		return -EFAULT;
-	err = snd_ctl_elem_list(ctl_file, data);
+	err = snd_ctl_elem_list_user(ctl_file, data);
 	if (err < 0)
 		return err;
 	/* copy the result */
-- 
2.14.1

[PATCH 08/24] ALSA: ctl: cancel allocation in user space for ELEM_LIST request

[Takashi Sakamoto]

In compatibility layer, when handling I/O request of ELEM_LIST, a memory
ofject is allocated in user space, but programs in user land is
unperceiving it. This is not preferable behaviour and it's better to
use an alternative way if available.

This commit adds an allocator on kernel space and copy data on user space
to it, then continue processing.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 8a4c58c0cf3b..f0559ed785e0 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -409,26 +409,30 @@ struct snd_ctl_elem_list32 {
 static int snd_ctl_elem_list_compat(struct snd_ctl_file *ctl_file,
 				    struct snd_ctl_elem_list32 __user *data32)
 {
-	struct snd_ctl_elem_list __user *data;
-	compat_caddr_t ptr;
+	struct snd_ctl_elem_list *list;
+	compat_uptr_t uptr;
 	int err;
 
-	data = compat_alloc_user_space(sizeof(*data));
-
-	/* offset, space, used, count */
-	if (copy_in_user(data, data32, 4 * sizeof(u32)))
-		return -EFAULT;
-	/* pids */
-	if (get_user(ptr, &data32->pids) ||
-	    put_user(compat_ptr(ptr), &data->pids))
+	list = kzalloc(sizeof(*list), GFP_KERNEL);
+	if (!list)
 		return -EFAULT;
-	err = snd_ctl_elem_list_user(ctl_file, data);
+
+	if (copy_from_user(list, data32, 4 * sizeof(u32)) ||
+	    get_user(uptr, &data32->pids)) {
+		err = -EFAULT;
+		goto end;
+	}
+	list->pids = compat_ptr(uptr);
+
+	err = snd_ctl_elem_list(ctl_file, list);
 	if (err < 0)
-		return err;
-	/* copy the result */
-	if (copy_in_user(data32, data, 4 * sizeof(u32)))
-		return -EFAULT;
-	return 0;
+		goto end;
+
+	if (copy_to_user(data32, list, 4 * sizeof(u32)))
+		err = -EFAULT;
+end:
+	kfree(list);
+	return err;
 }
 
 /*
-- 
2.14.1

[PATCH 09/24] ALSA: ctl: unify calls of D0-wait function for ELEM_INFO request

[Takashi Sakamoto]

When handling I/O request for ELEM_INFO, 'snd_power_wait()' function is
used before a call of 'snd_ctl_elem_info()' function. This is perhaps due
to performing I/O operation to actual devices. The same thing is done by
helper functions for native/compat ABI, and they can be unified.

This commit moves these duplicated codes into the callee function.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 7 ++++---
 sound/core/control_compat.c | 3 ---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index ad4d27c681c4..91966b190b8b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -842,6 +842,10 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 	unsigned int index_offset;
 	int result;
 	
+	result = snd_power_wait(ctl->card, SNDRV_CTL_POWER_D0);
+	if (result < 0)
+		return result;
+
 	down_read(&card->controls_rwsem);
 	kctl = snd_ctl_find_id(card, &info->id);
 	if (kctl == NULL) {
@@ -879,9 +883,6 @@ static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl,
 
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
-	result = snd_power_wait(ctl->card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		return result;
 	result = snd_ctl_elem_info(ctl, &info);
 	if (result < 0)
 		return result;
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index f0559ed785e0..e6169b2bc6b3 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -489,9 +489,6 @@ static int snd_ctl_elem_info_compat(struct snd_ctl_file *ctl,
 	if (get_user(data->value.enumerated.item, &data32->value.enumerated.item))
 		goto error;
 
-	err = snd_power_wait(ctl->card, SNDRV_CTL_POWER_D0);
-	if (err < 0)
-		goto error;
 	err = snd_ctl_elem_info(ctl, data);
 	if (err < 0)
 		goto error;
-- 
2.14.1

[PATCH 10/24] ALSA: ctl: unify calls of D0-wait function for ELEM_READ request

[Takashi Sakamoto]

When handling I/O request for ELEM_READ, 'snd_power_wait()' function is
used before a call of 'snd_ctl_elem_read()' function. This is perhaps due
to performing I/O operation to actual devices. The same thing is done by
helper functions for native/compat ABI, and they can be unified.

This commit moves these duplicated codes into the callee function.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 30 +++++++++++++++++++-----------
 sound/core/control_compat.c |  3 ---
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 91966b190b8b..2b5921e9f111 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -897,18 +897,32 @@ static int snd_ctl_elem_read(struct snd_card *card,
 	struct snd_kcontrol *kctl;
 	struct snd_kcontrol_volatile *vd;
 	unsigned int index_offset;
+	int err;
+
+	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
+	if (err < 0)
+		return err;
+
+	down_read(&card->controls_rwsem);
 
 	kctl = snd_ctl_find_id(card, &control->id);
-	if (kctl == NULL)
-		return -ENOENT;
+	if (kctl == NULL) {
+		err = -ENOENT;
+		goto end;
+	}
 
 	index_offset = snd_ctl_get_ioff(kctl, &control->id);
 	vd = &kctl->vd[index_offset];
-	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get == NULL)
-		return -EPERM;
+	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get == NULL) {
+		err = -EPERM;
+		goto end;
+	}
 
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
-	return kctl->get(kctl, control);
+	err = kctl->get(kctl, control);
+end:
+	up_read(&card->controls_rwsem);
+	return err;
 }
 
 static int snd_ctl_elem_read_user(struct snd_card *card,
@@ -921,13 +935,7 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	if (IS_ERR(control))
 		return PTR_ERR(control);
 
-	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		goto error;
-
-	down_read(&card->controls_rwsem);
 	result = snd_ctl_elem_read(card, control);
-	up_read(&card->controls_rwsem);
 	if (result < 0)
 		goto error;
 
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index e6169b2bc6b3..b887cda28d30 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -688,9 +688,6 @@ static int ctl_elem_read_user(struct snd_card *card,
 	if (err < 0)
 		goto error;
 
-	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (err < 0)
-		goto error;
 	err = snd_ctl_elem_read(card, data);
 	if (err < 0)
 		goto error;
-- 
2.14.1

[PATCH 11/24] ALSA: ctl: unify calls of D0-wait function for ELEM_WRITE request

[Takashi Sakamoto]

When handling I/O request for ELEM_WRITE, 'snd_power_wait()' function is
used before a call of 'snd_ctl_elem_write()' function. This is perhaps due
to performing I/O operation to actual devices. The same thing is done by
helper functions for native/compat ABI, and they can be unified.

This commit moves these duplicated codes into the callee function.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 31 +++++++++++++++----------------
 sound/core/control_compat.c |  3 ---
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 2b5921e9f111..8d158a49467d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -954,27 +954,33 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	unsigned int index_offset;
 	int result;
 
-	kctl = snd_ctl_find_id(card, &control->id);
-	if (kctl == NULL)
-		return -ENOENT;
+	result = snd_power_wait(file->card, SNDRV_CTL_POWER_D0);
+	if (result < 0)
+		return result;
+
+	down_write(&file->card->controls_rwsem);
+	kctl = snd_ctl_find_id(file->card, &control->id);
+	if (kctl == NULL) {
+		result = -ENOENT;
+		goto end;
+	}
 
 	index_offset = snd_ctl_get_ioff(kctl, &control->id);
 	vd = &kctl->vd[index_offset];
 	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL ||
 	    (file && vd->owner && vd->owner != file)) {
-		return -EPERM;
+		result = -EPERM;
+		goto end;
 	}
 
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
 	result = kctl->put(kctl, control);
-	if (result < 0)
-		return result;
-
 	if (result > 0) {
 		struct snd_ctl_elem_id id = control->id;
-		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
+		snd_ctl_notify(file->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
 	}
-
+end:
+	up_write(&file->card->controls_rwsem);
 	return 0;
 }
 
@@ -989,14 +995,7 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
 	if (IS_ERR(control))
 		return PTR_ERR(control);
 
-	card = file->card;
-	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		goto error;
-
-	down_write(&card->controls_rwsem);
 	result = snd_ctl_elem_write(card, file, control);
-	up_write(&card->controls_rwsem);
 	if (result < 0)
 		goto error;
 
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index b887cda28d30..b3f0be74ba0f 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -713,9 +713,6 @@ static int ctl_elem_write_user(struct snd_ctl_file *file,
 	if (err < 0)
 		goto error;
 
-	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (err < 0)
-		goto error;
 	err = snd_ctl_elem_write(card, file, data);
 	if (err < 0)
 		goto error;
-- 
2.14.1

[PATCH 12/24] ALSA: ctl: allocation of elem_info data for ELEM_INFO request

[Takashi Sakamoto]

In current implementation, for I/O request of ELEM_INFO, kernel stack
is used to copy 'struct snd_ctl_elem_info' data. However, the size of
this structure is a bit big and usage of kernel stack is not preferable.

This commit allocates a memory object on kernel space, instead.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 8d158a49467d..4cb950e310bf 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -875,20 +875,23 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 	return result;
 }
 
-static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl,
-				  struct snd_ctl_elem_info __user *_info)
+static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl, void __user *arg)
 {
-	struct snd_ctl_elem_info info;
-	int result;
+	struct snd_ctl_elem_info *info;
+	int err;
 
-	if (copy_from_user(&info, _info, sizeof(info)))
-		return -EFAULT;
-	result = snd_ctl_elem_info(ctl, &info);
-	if (result < 0)
-		return result;
-	if (copy_to_user(_info, &info, sizeof(info)))
-		return -EFAULT;
-	return result;
+	info = memdup_user(arg, sizeof(*info));
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	err = snd_ctl_elem_info(ctl, info);
+	if (err >= 0) {
+		if (copy_to_user(arg, info, sizeof(*info)))
+			return -EFAULT;
+	}
+
+	kfree(info);
+	return err;
 }
 
 static int snd_ctl_elem_read(struct snd_card *card,
-- 
2.14.1

[PATCH 13/24] ALSA: ctl: change prototype of local function for ELEM_WRITE ioctl

[Takashi Sakamoto]

When investigating compatibility layer, several functions for native ABI
are called by the layer. A 'snd_ctl_elem_write()' is such a function,
while it's prototype is not similar to the others. This will bring a
future inconvenience in integration of the layer.

This commit changes its prototype to get a first argument for data of
'struct snd_ctl_file' type.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 19 +++++++++----------
 sound/core/control_compat.c |  6 +++---
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 4cb950e310bf..c7b43f3f069b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -949,7 +949,7 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	return result;
 }
 
-static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
+static int snd_ctl_elem_write(struct snd_ctl_file *ctl_file,
 			      struct snd_ctl_elem_value *control)
 {
 	struct snd_kcontrol *kctl;
@@ -957,12 +957,12 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	unsigned int index_offset;
 	int result;
 
-	result = snd_power_wait(file->card, SNDRV_CTL_POWER_D0);
+	result = snd_power_wait(ctl_file->card, SNDRV_CTL_POWER_D0);
 	if (result < 0)
 		return result;
 
-	down_write(&file->card->controls_rwsem);
-	kctl = snd_ctl_find_id(file->card, &control->id);
+	down_write(&ctl_file->card->controls_rwsem);
+	kctl = snd_ctl_find_id(ctl_file->card, &control->id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 		goto end;
@@ -971,7 +971,7 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	index_offset = snd_ctl_get_ioff(kctl, &control->id);
 	vd = &kctl->vd[index_offset];
 	if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL ||
-	    (file && vd->owner && vd->owner != file)) {
+	    (ctl_file && vd->owner && vd->owner != ctl_file)) {
 		result = -EPERM;
 		goto end;
 	}
@@ -980,25 +980,24 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	result = kctl->put(kctl, control);
 	if (result > 0) {
 		struct snd_ctl_elem_id id = control->id;
-		snd_ctl_notify(file->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
+		snd_ctl_notify(ctl_file->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
 	}
 end:
-	up_write(&file->card->controls_rwsem);
+	up_write(&ctl_file->card->controls_rwsem);
 	return 0;
 }
 
-static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
+static int snd_ctl_elem_write_user(struct snd_ctl_file *ctl_file,
 				   struct snd_ctl_elem_value __user *_control)
 {
 	struct snd_ctl_elem_value *control;
-	struct snd_card *card;
 	int result;
 
 	control = memdup_user(_control, sizeof(*control));
 	if (IS_ERR(control))
 		return PTR_ERR(control);
 
-	result = snd_ctl_elem_write(card, file, control);
+	result = snd_ctl_elem_write(ctl_file, control);
 	if (result < 0)
 		goto error;
 
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index b3f0be74ba0f..11e0966f86bb 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -697,11 +697,11 @@ static int ctl_elem_read_user(struct snd_card *card,
 	return err;
 }
 
-static int ctl_elem_write_user(struct snd_ctl_file *file,
+static int ctl_elem_write_user(struct snd_ctl_file *ctl_file,
 			       void __user *userdata, void __user *valuep)
 {
 	struct snd_ctl_elem_value *data;
-	struct snd_card *card = file->card;
+	struct snd_card *card = ctl_file->card;
 	int err, type, count;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -713,7 +713,7 @@ static int ctl_elem_write_user(struct snd_ctl_file *file,
 	if (err < 0)
 		goto error;
 
-	err = snd_ctl_elem_write(card, file, data);
+	err = snd_ctl_elem_write(ctl_file, data);
 	if (err < 0)
 		goto error;
 	err = copy_ctl_value_to_user(userdata, valuep, data, type, count);
-- 
2.14.1

[PATCH 14/24] ALSA: ctl: change prototype of local function for ELEM_READ ioctl

[Takashi Sakamoto]

When investigating compatibility layer, several functions for native ABI
are called by the layer. A 'snd_ctl_elem_read()' is such a function,
while it's prototype is not similar to the others. This will bring a
future inconvenience in integration of the layer.

This commit changes its prototype to get a first argument for data of
'struct snd_ctl_file' type.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 16 ++++++++--------
 sound/core/control_compat.c | 21 ++++++++++-----------
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index c7b43f3f069b..268771ed2939 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -894,7 +894,7 @@ static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl, void __user *arg)
 	return err;
 }
 
-static int snd_ctl_elem_read(struct snd_card *card,
+static int snd_ctl_elem_read(struct snd_ctl_file *ctl_file,
 			     struct snd_ctl_elem_value *control)
 {
 	struct snd_kcontrol *kctl;
@@ -902,13 +902,13 @@ static int snd_ctl_elem_read(struct snd_card *card,
 	unsigned int index_offset;
 	int err;
 
-	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
+	err = snd_power_wait(ctl_file->card, SNDRV_CTL_POWER_D0);
 	if (err < 0)
 		return err;
 
-	down_read(&card->controls_rwsem);
+	down_read(&ctl_file->card->controls_rwsem);
 
-	kctl = snd_ctl_find_id(card, &control->id);
+	kctl = snd_ctl_find_id(ctl_file->card, &control->id);
 	if (kctl == NULL) {
 		err = -ENOENT;
 		goto end;
@@ -924,11 +924,11 @@ static int snd_ctl_elem_read(struct snd_card *card,
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
 	err = kctl->get(kctl, control);
 end:
-	up_read(&card->controls_rwsem);
+	up_read(&ctl_file->card->controls_rwsem);
 	return err;
 }
 
-static int snd_ctl_elem_read_user(struct snd_card *card,
+static int snd_ctl_elem_read_user(struct snd_ctl_file *ctl_file,
 				  struct snd_ctl_elem_value __user *_control)
 {
 	struct snd_ctl_elem_value *control;
@@ -938,7 +938,7 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	if (IS_ERR(control))
 		return PTR_ERR(control);
 
-	result = snd_ctl_elem_read(card, control);
+	result = snd_ctl_elem_read(ctl_file, control);
 	if (result < 0)
 		goto error;
 
@@ -1581,7 +1581,7 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
-		return snd_ctl_elem_read_user(card, argp);
+		return snd_ctl_elem_read_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE:
 		return snd_ctl_elem_write_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LOCK:
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 11e0966f86bb..d6d1a2bf9542 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -673,7 +673,7 @@ static int copy_ctl_value_to_user(void __user *userdata,
 	return 0;
 }
 
-static int ctl_elem_read_user(struct snd_card *card,
+static int ctl_elem_read_user(struct snd_ctl_file *ctl_file,
 			      void __user *userdata, void __user *valuep)
 {
 	struct snd_ctl_elem_value *data;
@@ -683,12 +683,12 @@ static int ctl_elem_read_user(struct snd_card *card,
 	if (data == NULL)
 		return -ENOMEM;
 
-	err = copy_ctl_value_from_user(card, data, userdata, valuep,
+	err = copy_ctl_value_from_user(ctl_file->card, data, userdata, valuep,
 				       &type, &count);
 	if (err < 0)
 		goto error;
 
-	err = snd_ctl_elem_read(card, data);
+	err = snd_ctl_elem_read(ctl_file, data);
 	if (err < 0)
 		goto error;
 	err = copy_ctl_value_to_user(userdata, valuep, data, type, count);
@@ -701,14 +701,13 @@ static int ctl_elem_write_user(struct snd_ctl_file *ctl_file,
 			       void __user *userdata, void __user *valuep)
 {
 	struct snd_ctl_elem_value *data;
-	struct snd_card *card = ctl_file->card;
 	int err, type, count;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (data == NULL)
 		return -ENOMEM;
 
-	err = copy_ctl_value_from_user(card, data, userdata, valuep,
+	err = copy_ctl_value_from_user(ctl_file->card, data, userdata, valuep,
 				       &type, &count);
 	if (err < 0)
 		goto error;
@@ -722,10 +721,10 @@ static int ctl_elem_write_user(struct snd_ctl_file *ctl_file,
 	return err;
 }
 
-static int snd_ctl_elem_read_user_compat(struct snd_card *card,
+static int snd_ctl_elem_read_user_compat(struct snd_ctl_file *ctl_file,
 					 struct snd_ctl_elem_value32 __user *data32)
 {
-	return ctl_elem_read_user(card, data32, &data32->value);
+	return ctl_elem_read_user(ctl_file, data32, &data32->value);
 }
 
 static int snd_ctl_elem_write_user_compat(struct snd_ctl_file *file,
@@ -735,10 +734,10 @@ static int snd_ctl_elem_write_user_compat(struct snd_ctl_file *file,
 }
 
 #ifdef CONFIG_X86_X32
-static int snd_ctl_elem_read_user_x32(struct snd_card *card,
+static int snd_ctl_elem_read_user_x32(struct snd_ctl_file *ctl_file,
 				      struct snd_ctl_elem_value_x32 __user *data32)
 {
-	return ctl_elem_read_user(card, data32, &data32->value);
+	return ctl_elem_read_user(ctl_file, data32, &data32->value);
 }
 
 static int snd_ctl_elem_write_user_x32(struct snd_ctl_file *file,
@@ -843,7 +842,7 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 	case SNDRV_CTL_IOCTL_ELEM_INFO32:
 		return snd_ctl_elem_info_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ32:
-		return snd_ctl_elem_read_user_compat(ctl->card, argp);
+		return snd_ctl_elem_read_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
 		return snd_ctl_elem_write_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_ADD32:
@@ -852,7 +851,7 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return snd_ctl_elem_add_compat(ctl, argp, 1);
 #ifdef CONFIG_X86_X32
 	case SNDRV_CTL_IOCTL_ELEM_READ_X32:
-		return snd_ctl_elem_read_user_x32(ctl->card, argp);
+		return snd_ctl_elem_read_user_x32(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE_X32:
 		return snd_ctl_elem_write_user_x32(ctl, argp);
 #endif /* CONFIG_X86_X32 */
-- 
2.14.1

[PATCH 15/24] ALSA: ctl: allocation of elem_info data for ELEM_ADD/ELEM_REPLACE requests

[Takashi Sakamoto]

In current implementation, for I/O request of ELEM_ADD/ELEM_REPLACE, kernel
stack is used to copy 'struct snd_ctl_elem_info' data. However, the size of
this structure is a bit big and usage of kernel stack is not preferable.

This commit allocates a memory object on kernel space, instead.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 268771ed2939..a321576a6308 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1405,23 +1405,26 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	return 0;
 }
 
-static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
-				 struct snd_ctl_elem_info __user *_info, int replace)
+static int snd_ctl_elem_add_user(struct snd_ctl_file *ctl_file,
+				 void __user *arg, int replace)
 {
-	struct snd_ctl_elem_info info;
+	struct snd_ctl_elem_info *info;
 	int err;
 
-	if (copy_from_user(&info, _info, sizeof(info)))
-		return -EFAULT;
-	err = snd_ctl_elem_add(file, &info, replace);
-	if (err < 0)
-		return err;
-	if (copy_to_user(_info, &info, sizeof(info))) {
-		snd_ctl_remove_user_ctl(file, &info.id);
-		return -EFAULT;
+	info = memdup_user(arg, sizeof(*info));
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	err = snd_ctl_elem_add(ctl_file, info, replace);
+	if (err >= 0) {
+		if (copy_to_user(arg, info, sizeof(*info))) {
+			snd_ctl_remove_user_ctl(ctl_file, &info->id);
+			err = -EFAULT;
+		}
 	}
 
-	return 0;
+	kfree(info);
+	return err;
 }
 
 static int snd_ctl_elem_remove(struct snd_ctl_file *file,
-- 
2.14.1

[PATCH 16/24] ALSA: ctl: add replace helper function to allocate own buffer

[Takashi Sakamoto]

In current implementation, an execution path of ELEM_REPLACE request
joins in an execution path of ELEM_ADD in the last. An advance preparation
of the replacement can be split from processing of the addition. This
separation might be a good granularity of helper functions.

For a preparation to it, this commit adds an unique helper function for
the replacement. In this time, the execution path still joins in the one
of the addition.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index a321576a6308..9e23f84d284d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1406,7 +1406,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 }
 
 static int snd_ctl_elem_add_user(struct snd_ctl_file *ctl_file,
-				 void __user *arg, int replace)
+				 void __user *arg)
 {
 	struct snd_ctl_elem_info *info;
 	int err;
@@ -1415,7 +1415,29 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *ctl_file,
 	if (IS_ERR(info))
 		return PTR_ERR(info);
 
-	err = snd_ctl_elem_add(ctl_file, info, replace);
+	err = snd_ctl_elem_add(ctl_file, info, 0);
+	if (err >= 0) {
+		if (copy_to_user(arg, info, sizeof(*info))) {
+			snd_ctl_remove_user_ctl(ctl_file, &info->id);
+			err = -EFAULT;
+		}
+	}
+
+	kfree(info);
+	return err;
+}
+
+static int snd_ctl_elem_replace_user(struct snd_ctl_file *ctl_file,
+				     void __user *arg)
+{
+	struct snd_ctl_elem_info *info;
+	int err;
+
+	info = memdup_user(arg, sizeof(*info));
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	err = snd_ctl_elem_add(ctl_file, info, 1);
 	if (err >= 0) {
 		if (copy_to_user(arg, info, sizeof(*info))) {
 			snd_ctl_remove_user_ctl(ctl_file, &info->id);
@@ -1592,9 +1614,9 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_ELEM_UNLOCK:
 		return snd_ctl_elem_unlock(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_ADD:
-		return snd_ctl_elem_add_user(ctl, argp, 0);
+		return snd_ctl_elem_add_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_REPLACE:
-		return snd_ctl_elem_add_user(ctl, argp, 1);
+		return snd_ctl_elem_replace_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_REMOVE:
 		return snd_ctl_elem_remove(ctl, argp);
 	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
-- 
2.14.1

[PATCH 17/24] ALSA: ctl: move removal code to replace helper function

[Takashi Sakamoto]

A previous commit adds a helper function for replacement processing. This
commit do actual separation from a helper function for addition.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c        | 34 +++++++++++++++++++++++-----------
 sound/core/control_compat.c |  5 ++++-
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 9e23f84d284d..0aa1f22dcac2 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1256,7 +1256,7 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
 }
 
 static int snd_ctl_elem_add(struct snd_ctl_file *file,
-			    struct snd_ctl_elem_info *info, int replace)
+			    struct snd_ctl_elem_info *info)
 {
 	/* The capacity of struct snd_ctl_elem_value.value.*/
 	static const unsigned int value_sizes[] = {
@@ -1289,14 +1289,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (strnlen(info->id.name, sizeof(info->id.name)) >= sizeof(info->id.name))
 		return -EINVAL;
 
-	/* Delete a control to replace them if needed. */
-	if (replace) {
-		info->id.numid = 0;
-		err = snd_ctl_remove_user_ctl(file, &info->id);
-		if (err)
-			return err;
-	}
-
 	/*
 	 * The number of userspace controls are counted control by control,
 	 * not element by element.
@@ -1415,7 +1407,7 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *ctl_file,
 	if (IS_ERR(info))
 		return PTR_ERR(info);
 
-	err = snd_ctl_elem_add(ctl_file, info, 0);
+	err = snd_ctl_elem_add(ctl_file, info);
 	if (err >= 0) {
 		if (copy_to_user(arg, info, sizeof(*info))) {
 			snd_ctl_remove_user_ctl(ctl_file, &info->id);
@@ -1427,6 +1419,26 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *ctl_file,
 	return err;
 }
 
+static int snd_ctl_elem_replace(struct snd_ctl_file *ctl_file,
+				struct snd_ctl_elem_info *info)
+{
+	int err;
+
+	if (!*info->id.name)
+		return -EINVAL;
+	if (strnlen(info->id.name, sizeof(info->id.name)) >=
+							sizeof(info->id.name))
+		return -EINVAL;
+
+	/* Delete an element set to replace them. */
+	info->id.numid = 0;
+	err = snd_ctl_remove_user_ctl(ctl_file, &info->id);
+	if (err < 0)
+		return err;
+
+	return snd_ctl_elem_add(ctl_file, info);
+}
+
 static int snd_ctl_elem_replace_user(struct snd_ctl_file *ctl_file,
 				     void __user *arg)
 {
@@ -1437,7 +1449,7 @@ static int snd_ctl_elem_replace_user(struct snd_ctl_file *ctl_file,
 	if (IS_ERR(info))
 		return PTR_ERR(info);
 
-	err = snd_ctl_elem_add(ctl_file, info, 1);
+	err = snd_ctl_elem_replace(ctl_file, info);
 	if (err >= 0) {
 		if (copy_to_user(arg, info, sizeof(*info))) {
 			snd_ctl_remove_user_ctl(ctl_file, &info->id);
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index d6d1a2bf9542..1cfacea867b6 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -792,7 +792,10 @@ static int snd_ctl_elem_add_compat(struct snd_ctl_file *file,
 	default:
 		break;
 	}
-	err = snd_ctl_elem_add(file, data, replace);
+	if (!replace)
+		err = snd_ctl_elem_add(file, data);
+	else
+		err = snd_ctl_elem_replace(file, data);
  error:
 	kfree(data);
 	return err;
-- 
2.14.1

[PATCH 18/24] ALSA: ctl: replacement for compat ELEM_LIST operation for any 32 bit ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_LIST operation
with a renewal way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 59 +++++++++++++--------------------------------
 1 file changed, 17 insertions(+), 42 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 1cfacea867b6..427158b39f5a 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -397,44 +397,6 @@ static int __maybe_unused serialize_to_elem_value_i386(
 	return 0;
 }
 
-struct snd_ctl_elem_list32 {
-	u32 offset;
-	u32 space;
-	u32 used;
-	u32 count;
-	u32 pids;
-	unsigned char reserved[50];
-} /* don't set packed attribute here */;
-
-static int snd_ctl_elem_list_compat(struct snd_ctl_file *ctl_file,
-				    struct snd_ctl_elem_list32 __user *data32)
-{
-	struct snd_ctl_elem_list *list;
-	compat_uptr_t uptr;
-	int err;
-
-	list = kzalloc(sizeof(*list), GFP_KERNEL);
-	if (!list)
-		return -EFAULT;
-
-	if (copy_from_user(list, data32, 4 * sizeof(u32)) ||
-	    get_user(uptr, &data32->pids)) {
-		err = -EFAULT;
-		goto end;
-	}
-	list->pids = compat_ptr(uptr);
-
-	err = snd_ctl_elem_list(ctl_file, list);
-	if (err < 0)
-		goto end;
-
-	if (copy_to_user(data32, list, 4 * sizeof(u32)))
-		err = -EFAULT;
-end:
-	kfree(list);
-	return err;
-}
-
 /*
  * control element info
  * it uses union, so the things are not easy..
@@ -801,8 +763,17 @@ static int snd_ctl_elem_add_compat(struct snd_ctl_file *file,
 	return err;
 }  
 
+static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
+					 void *buf)
+{
+	struct snd_ctl_elem_list *list = buf;
+
+	return snd_ctl_elem_list(ctl_file, list);
+}
+
 enum {
-	SNDRV_CTL_IOCTL_ELEM_LIST32 = _IOWR('U', 0x10, struct snd_ctl_elem_list32),
+	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
+				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
 	SNDRV_CTL_IOCTL_ELEM_INFO32 = _IOWR('U', 0x11, struct snd_ctl_elem_info32),
 	SNDRV_CTL_IOCTL_ELEM_READ32 = _IOWR('U', 0x12, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_WRITE32 = _IOWR('U', 0x13, struct snd_ctl_elem_value32),
@@ -826,7 +797,13 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 				 void *src);
 		unsigned int orig_cmd;
 	} handlers[] = {
-		{ 0, NULL, NULL, NULL, 0, },
+		{
+			SNDRV_CTL_IOCTL_ELEM_LIST_32,
+			deserialize_from_elem_list_32,
+			ctl_compat_ioctl_elem_list_32,
+			serialize_to_elem_list_32,
+			SNDRV_CTL_IOCTL_ELEM_LIST,
+		},
 	};
 	struct snd_ctl_file *ctl;
 	void __user *argp = compat_ptr(arg);
@@ -840,8 +817,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return -ENXIO;
 
 	switch (cmd) {
-	case SNDRV_CTL_IOCTL_ELEM_LIST32:
-		return snd_ctl_elem_list_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO32:
 		return snd_ctl_elem_info_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ32:
-- 
2.14.1

[PATCH 19/24] ALSA: ctl: replacement for compat ELEM_INFO operation for any 32 bit ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_INFO operation
with a renewal way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 80 ++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 63 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 427158b39f5a..499cc459917f 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -431,66 +431,6 @@ struct snd_ctl_elem_info32 {
 	unsigned char reserved[64];
 } __attribute__((packed));
 
-static int snd_ctl_elem_info_compat(struct snd_ctl_file *ctl,
-				    struct snd_ctl_elem_info32 __user *data32)
-{
-	struct snd_ctl_elem_info *data;
-	int err;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (! data)
-		return -ENOMEM;
-
-	err = -EFAULT;
-	/* copy id */
-	if (copy_from_user(&data->id, &data32->id, sizeof(data->id)))
-		goto error;
-	/* we need to copy the item index.
-	 * hope this doesn't break anything..
-	 */
-	if (get_user(data->value.enumerated.item, &data32->value.enumerated.item))
-		goto error;
-
-	err = snd_ctl_elem_info(ctl, data);
-	if (err < 0)
-		goto error;
-	/* restore info to 32bit */
-	err = -EFAULT;
-	/* id, type, access, count */
-	if (copy_to_user(&data32->id, &data->id, sizeof(data->id)) ||
-	    copy_to_user(&data32->type, &data->type, 3 * sizeof(u32)))
-		goto error;
-	if (put_user(data->owner, &data32->owner))
-		goto error;
-	switch (data->type) {
-	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		if (put_user(data->value.integer.min, &data32->value.integer.min) ||
-		    put_user(data->value.integer.max, &data32->value.integer.max) ||
-		    put_user(data->value.integer.step, &data32->value.integer.step))
-			goto error;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		if (copy_to_user(&data32->value.integer64,
-				 &data->value.integer64,
-				 sizeof(data->value.integer64)))
-			goto error;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		if (copy_to_user(&data32->value.enumerated,
-				 &data->value.enumerated,
-				 sizeof(data->value.enumerated)))
-			goto error;
-		break;
-	default:
-		break;
-	}
-	err = 0;
- error:
-	kfree(data);
-	return err;
-}
-
 /* read / write */
 struct snd_ctl_elem_value32 {
 	struct snd_ctl_elem_id id;
@@ -771,10 +711,19 @@ static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
 	return snd_ctl_elem_list(ctl_file, list);
 }
 
+static int ctl_compat_ioctl_elem_info_32(struct snd_ctl_file *ctl_file,
+					 void *buf)
+{
+	struct snd_ctl_elem_info *info = buf;
+
+	return snd_ctl_elem_info(ctl_file, info);
+}
+
 enum {
 	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
 				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
-	SNDRV_CTL_IOCTL_ELEM_INFO32 = _IOWR('U', 0x11, struct snd_ctl_elem_info32),
+	SNDRV_CTL_IOCTL_ELEM_INFO_32 =
+				_IOWR('U', 0x11, struct snd_ctl_elem_info_32),
 	SNDRV_CTL_IOCTL_ELEM_READ32 = _IOWR('U', 0x12, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_WRITE32 = _IOWR('U', 0x13, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_ADD32 = _IOWR('U', 0x17, struct snd_ctl_elem_info32),
@@ -804,6 +753,13 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 			serialize_to_elem_list_32,
 			SNDRV_CTL_IOCTL_ELEM_LIST,
 		},
+		{
+			SNDRV_CTL_IOCTL_ELEM_INFO_32,
+			deserialize_from_elem_info_32,
+			ctl_compat_ioctl_elem_info_32,
+			serialize_to_elem_info_32,
+			SNDRV_CTL_IOCTL_ELEM_INFO,
+		},
 	};
 	struct snd_ctl_file *ctl;
 	void __user *argp = compat_ptr(arg);
@@ -817,8 +773,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return -ENXIO;
 
 	switch (cmd) {
-	case SNDRV_CTL_IOCTL_ELEM_INFO32:
-		return snd_ctl_elem_info_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ32:
 		return snd_ctl_elem_read_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
-- 
2.14.1

[PATCH 20/24] ALSA: ctl: replacement for compat ELEM_ADD operation for any 32 bit ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_ADD operation
with a renewal way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 499cc459917f..8e2617c6217e 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -719,6 +719,14 @@ static int ctl_compat_ioctl_elem_info_32(struct snd_ctl_file *ctl_file,
 	return snd_ctl_elem_info(ctl_file, info);
 }
 
+static int ctl_compat_ioctl_elem_add_32(struct snd_ctl_file *ctl_file,
+					void *buf)
+{
+	struct snd_ctl_elem_info *info = buf;
+
+	return snd_ctl_elem_add(ctl_file, info);
+}
+
 enum {
 	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
 				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
@@ -726,7 +734,8 @@ enum {
 				_IOWR('U', 0x11, struct snd_ctl_elem_info_32),
 	SNDRV_CTL_IOCTL_ELEM_READ32 = _IOWR('U', 0x12, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_WRITE32 = _IOWR('U', 0x13, struct snd_ctl_elem_value32),
-	SNDRV_CTL_IOCTL_ELEM_ADD32 = _IOWR('U', 0x17, struct snd_ctl_elem_info32),
+	SNDRV_CTL_IOCTL_ELEM_ADD_32 =
+				_IOWR('U', 0x17, struct snd_ctl_elem_info_32),
 	SNDRV_CTL_IOCTL_ELEM_REPLACE32 = _IOWR('U', 0x18, struct snd_ctl_elem_info32),
 #ifdef CONFIG_X86_X32
 	SNDRV_CTL_IOCTL_ELEM_READ_X32 = _IOWR('U', 0x12, struct snd_ctl_elem_value_x32),
@@ -760,6 +769,13 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 			serialize_to_elem_info_32,
 			SNDRV_CTL_IOCTL_ELEM_INFO,
 		},
+		{
+			SNDRV_CTL_IOCTL_ELEM_ADD_32,
+			deserialize_from_elem_info_32,
+			ctl_compat_ioctl_elem_add_32,
+			serialize_to_elem_info_32,
+			SNDRV_CTL_IOCTL_ELEM_ADD,
+		},
 	};
 	struct snd_ctl_file *ctl;
 	void __user *argp = compat_ptr(arg);
@@ -777,8 +793,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return snd_ctl_elem_read_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
 		return snd_ctl_elem_write_user_compat(ctl, argp);
-	case SNDRV_CTL_IOCTL_ELEM_ADD32:
-		return snd_ctl_elem_add_compat(ctl, argp, 0);
 	case SNDRV_CTL_IOCTL_ELEM_REPLACE32:
 		return snd_ctl_elem_add_compat(ctl, argp, 1);
 #ifdef CONFIG_X86_X32
@@ -846,6 +860,13 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 				err = -EFAULT;
 		}
 	}
+
+	if (err < 0) {
+		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32) {
+			struct snd_ctl_elem_info *info = buf;
+			snd_ctl_remove_user_ctl(ctl, &info->id);
+		}
+	}
 end:
 	kfree(data);
 	kfree(buf);
-- 
2.14.1

[PATCH 21/24] ALSA: ctl: replacement for compat ELEM_REPLACE operation for any 32 bit ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_REPLACE operation
with a renewal way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 111 ++++++++------------------------------------
 1 file changed, 19 insertions(+), 92 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 8e2617c6217e..2982423dccd0 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -397,40 +397,6 @@ static int __maybe_unused serialize_to_elem_value_i386(
 	return 0;
 }
 
-/*
- * control element info
- * it uses union, so the things are not easy..
- */
-
-struct snd_ctl_elem_info32 {
-	struct snd_ctl_elem_id id; // the size of struct is same
-	s32 type;
-	u32 access;
-	u32 count;
-	s32 owner;
-	union {
-		struct {
-			s32 min;
-			s32 max;
-			s32 step;
-		} integer;
-		struct {
-			u64 min;
-			u64 max;
-			u64 step;
-		} integer64;
-		struct {
-			u32 items;
-			u32 item;
-			char name[64];
-			u64 names_ptr;
-			u32 names_length;
-		} enumerated;
-		unsigned char reserved[128];
-	} value;
-	unsigned char reserved[64];
-} __attribute__((packed));
-
 /* read / write */
 struct snd_ctl_elem_value32 {
 	struct snd_ctl_elem_id id;
@@ -649,60 +615,6 @@ static int snd_ctl_elem_write_user_x32(struct snd_ctl_file *file,
 }
 #endif /* CONFIG_X86_X32 */
 
-/* add or replace a user control */
-static int snd_ctl_elem_add_compat(struct snd_ctl_file *file,
-				   struct snd_ctl_elem_info32 __user *data32,
-				   int replace)
-{
-	struct snd_ctl_elem_info *data;
-	int err;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (! data)
-		return -ENOMEM;
-
-	err = -EFAULT;
-	/* id, type, access, count */ \
-	if (copy_from_user(&data->id, &data32->id, sizeof(data->id)) ||
-	    copy_from_user(&data->type, &data32->type, 3 * sizeof(u32)))
-		goto error;
-	if (get_user(data->owner, &data32->owner) ||
-	    get_user(data->type, &data32->type))
-		goto error;
-	switch (data->type) {
-	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		if (get_user(data->value.integer.min, &data32->value.integer.min) ||
-		    get_user(data->value.integer.max, &data32->value.integer.max) ||
-		    get_user(data->value.integer.step, &data32->value.integer.step))
-			goto error;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		if (copy_from_user(&data->value.integer64,
-				   &data32->value.integer64,
-				   sizeof(data->value.integer64)))
-			goto error;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		if (copy_from_user(&data->value.enumerated,
-				   &data32->value.enumerated,
-				   sizeof(data->value.enumerated)))
-			goto error;
-		data->value.enumerated.names_ptr =
-			(uintptr_t)compat_ptr(data->value.enumerated.names_ptr);
-		break;
-	default:
-		break;
-	}
-	if (!replace)
-		err = snd_ctl_elem_add(file, data);
-	else
-		err = snd_ctl_elem_replace(file, data);
- error:
-	kfree(data);
-	return err;
-}  
-
 static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
 					 void *buf)
 {
@@ -727,6 +639,14 @@ static int ctl_compat_ioctl_elem_add_32(struct snd_ctl_file *ctl_file,
 	return snd_ctl_elem_add(ctl_file, info);
 }
 
+static int ctl_compat_ioctl_elem_replace_32(struct snd_ctl_file *ctl_file,
+					    void *buf)
+{
+	struct snd_ctl_elem_info *info = buf;
+
+	return snd_ctl_elem_replace(ctl_file, info);
+}
+
 enum {
 	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
 				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
@@ -736,7 +656,8 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_WRITE32 = _IOWR('U', 0x13, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_ADD_32 =
 				_IOWR('U', 0x17, struct snd_ctl_elem_info_32),
-	SNDRV_CTL_IOCTL_ELEM_REPLACE32 = _IOWR('U', 0x18, struct snd_ctl_elem_info32),
+	SNDRV_CTL_IOCTL_ELEM_REPLACE_32 =
+				_IOWR('U', 0x18, struct snd_ctl_elem_info_32),
 #ifdef CONFIG_X86_X32
 	SNDRV_CTL_IOCTL_ELEM_READ_X32 = _IOWR('U', 0x12, struct snd_ctl_elem_value_x32),
 	SNDRV_CTL_IOCTL_ELEM_WRITE_X32 = _IOWR('U', 0x13, struct snd_ctl_elem_value_x32),
@@ -776,6 +697,13 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 			serialize_to_elem_info_32,
 			SNDRV_CTL_IOCTL_ELEM_ADD,
 		},
+		{
+			SNDRV_CTL_IOCTL_ELEM_REPLACE_32,
+			deserialize_from_elem_info_32,
+			ctl_compat_ioctl_elem_replace_32,
+			serialize_to_elem_info_32,
+			SNDRV_CTL_IOCTL_ELEM_REPLACE,
+		},
 	};
 	struct snd_ctl_file *ctl;
 	void __user *argp = compat_ptr(arg);
@@ -793,8 +721,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return snd_ctl_elem_read_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
 		return snd_ctl_elem_write_user_compat(ctl, argp);
-	case SNDRV_CTL_IOCTL_ELEM_REPLACE32:
-		return snd_ctl_elem_add_compat(ctl, argp, 1);
 #ifdef CONFIG_X86_X32
 	case SNDRV_CTL_IOCTL_ELEM_READ_X32:
 		return snd_ctl_elem_read_user_x32(ctl, argp);
@@ -862,7 +788,8 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 	}
 
 	if (err < 0) {
-		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32) {
+		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32 ||
+		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32) {
 			struct snd_ctl_elem_info *info = buf;
 			snd_ctl_remove_user_ctl(ctl, &info->id);
 		}
-- 
2.14.1

[PATCH 22/24] ALSA: ctl: replacement for compat ELEM_READ/ELEM_WRITE operation for i386 ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_READ/ELEM_WRITE
operation for i386 ABI with a renewal way. Existent implementation for x32
ABI compatibility is removed so that it can be handled by the same way for
the other modern 32 bit ABIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 76 +++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 2982423dccd0..430f178aa4d8 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -404,27 +404,11 @@ struct snd_ctl_elem_value32 {
         union {
 		s32 integer[128];
 		unsigned char data[512];
-#ifndef CONFIG_X86_64
 		s64 integer64[64];
-#endif
         } value;
         unsigned char reserved[128];
 };
 
-#ifdef CONFIG_X86_X32
-/* x32 has a different alignment for 64bit values from ia32 */
-struct snd_ctl_elem_value_x32 {
-	struct snd_ctl_elem_id id;
-	unsigned int indirect;	/* bit-field causes misalignment */
-	union {
-		s32 integer[128];
-		unsigned char data[512];
-		s64 integer64[64];
-	} value;
-	unsigned char reserved[128];
-};
-#endif /* CONFIG_X86_X32 */
-
 /* get the value type and count of the control */
 static int get_ctl_type(struct snd_card *card, struct snd_ctl_elem_id *id,
 			int *countp)
@@ -601,20 +585,6 @@ static int snd_ctl_elem_write_user_compat(struct snd_ctl_file *file,
 	return ctl_elem_write_user(file, data32, &data32->value);
 }
 
-#ifdef CONFIG_X86_X32
-static int snd_ctl_elem_read_user_x32(struct snd_ctl_file *ctl_file,
-				      struct snd_ctl_elem_value_x32 __user *data32)
-{
-	return ctl_elem_read_user(ctl_file, data32, &data32->value);
-}
-
-static int snd_ctl_elem_write_user_x32(struct snd_ctl_file *file,
-				       struct snd_ctl_elem_value_x32 __user *data32)
-{
-	return ctl_elem_write_user(file, data32, &data32->value);
-}
-#endif /* CONFIG_X86_X32 */
-
 static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
 					 void *buf)
 {
@@ -647,6 +617,22 @@ static int ctl_compat_ioctl_elem_replace_32(struct snd_ctl_file *ctl_file,
 	return snd_ctl_elem_replace(ctl_file, info);
 }
 
+static int ctl_compat_ioctl_elem_read_32(struct snd_ctl_file *ctl_file,
+					 void *buf)
+{
+	struct snd_ctl_elem_value *value = buf;
+
+	return snd_ctl_elem_read(ctl_file, value);
+}
+
+static int ctl_compat_ioctl_elem_write_32(struct snd_ctl_file *ctl_file,
+					  void *buf)
+{
+	struct snd_ctl_elem_value *value = buf;
+
+	return snd_ctl_elem_write(ctl_file, value);
+}
+
 enum {
 	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
 				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
@@ -658,10 +644,10 @@ enum {
 				_IOWR('U', 0x17, struct snd_ctl_elem_info_32),
 	SNDRV_CTL_IOCTL_ELEM_REPLACE_32 =
 				_IOWR('U', 0x18, struct snd_ctl_elem_info_32),
-#ifdef CONFIG_X86_X32
-	SNDRV_CTL_IOCTL_ELEM_READ_X32 = _IOWR('U', 0x12, struct snd_ctl_elem_value_x32),
-	SNDRV_CTL_IOCTL_ELEM_WRITE_X32 = _IOWR('U', 0x13, struct snd_ctl_elem_value_x32),
-#endif /* CONFIG_X86_X32 */
+	SNDRV_CTL_IOCTL_ELEM_READ_I386 =
+				_IOWR('U', 0x12, struct snd_ctl_elem_value_i386),
+	SNDRV_CTL_IOCTL_ELEM_WRITE_I386 =
+				_IOWR('U', 0x13, struct snd_ctl_elem_value_i386),
 };
 
 static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
@@ -704,6 +690,22 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 			serialize_to_elem_info_32,
 			SNDRV_CTL_IOCTL_ELEM_REPLACE,
 		},
+#ifdef CONFIG_X86_64
+		{
+			SNDRV_CTL_IOCTL_ELEM_READ_I386,
+			deserialize_from_elem_value_i386,
+			ctl_compat_ioctl_elem_read_32,
+			serialize_to_elem_value_i386,
+			SNDRV_CTL_IOCTL_ELEM_READ,
+		},
+		{
+			SNDRV_CTL_IOCTL_ELEM_WRITE_I386,
+			deserialize_from_elem_value_i386,
+			ctl_compat_ioctl_elem_write_32,
+			serialize_to_elem_value_i386,
+			SNDRV_CTL_IOCTL_ELEM_WRITE,
+		},
+#endif
 	};
 	struct snd_ctl_file *ctl;
 	void __user *argp = compat_ptr(arg);
@@ -721,12 +723,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		return snd_ctl_elem_read_user_compat(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
 		return snd_ctl_elem_write_user_compat(ctl, argp);
-#ifdef CONFIG_X86_X32
-	case SNDRV_CTL_IOCTL_ELEM_READ_X32:
-		return snd_ctl_elem_read_user_x32(ctl, argp);
-	case SNDRV_CTL_IOCTL_ELEM_WRITE_X32:
-		return snd_ctl_elem_write_user_x32(ctl, argp);
-#endif /* CONFIG_X86_X32 */
 	}
 
 	for (i = 0; i < ARRAY_SIZE(handlers); ++i) {
-- 
2.14.1

[PATCH 23/24] ALSA: ctl: replacement for compat ELEM_READ/ELEM_WRITE operation for modern 32 bit ABI

[Takashi Sakamoto]

This commit obsoletes old implementation for compat ELEM_READ/ELEM_WRITE
operation for modern 32 bit ABI with a renewal way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 218 ++++----------------------------------------
 1 file changed, 20 insertions(+), 198 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 430f178aa4d8..bbda8eabf18f 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -397,194 +397,6 @@ static int __maybe_unused serialize_to_elem_value_i386(
 	return 0;
 }
 
-/* read / write */
-struct snd_ctl_elem_value32 {
-	struct snd_ctl_elem_id id;
-	unsigned int indirect;	/* bit-field causes misalignment */
-        union {
-		s32 integer[128];
-		unsigned char data[512];
-		s64 integer64[64];
-        } value;
-        unsigned char reserved[128];
-};
-
-/* get the value type and count of the control */
-static int get_ctl_type(struct snd_card *card, struct snd_ctl_elem_id *id,
-			int *countp)
-{
-	struct snd_kcontrol *kctl;
-	struct snd_ctl_elem_info *info;
-	int err;
-
-	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, id);
-	if (! kctl) {
-		up_read(&card->controls_rwsem);
-		return -ENOENT;
-	}
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (info == NULL) {
-		up_read(&card->controls_rwsem);
-		return -ENOMEM;
-	}
-	info->id = *id;
-	err = kctl->info(kctl, info);
-	up_read(&card->controls_rwsem);
-	if (err >= 0) {
-		err = info->type;
-		*countp = info->count;
-	}
-	kfree(info);
-	return err;
-}
-
-static int get_elem_size(int type, int count)
-{
-	switch (type) {
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		return sizeof(s64) * count;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		return sizeof(int) * count;
-	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		return 512;
-	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		return sizeof(struct snd_aes_iec958);
-	default:
-		return -1;
-	}
-}
-
-static int copy_ctl_value_from_user(struct snd_card *card,
-				    struct snd_ctl_elem_value *data,
-				    void __user *userdata,
-				    void __user *valuep,
-				    int *typep, int *countp)
-{
-	struct snd_ctl_elem_value32 __user *data32 = userdata;
-	int i, type, size;
-	int uninitialized_var(count);
-	unsigned int indirect;
-
-	if (copy_from_user(&data->id, &data32->id, sizeof(data->id)))
-		return -EFAULT;
-	if (get_user(indirect, &data32->indirect))
-		return -EFAULT;
-	if (indirect)
-		return -EINVAL;
-	type = get_ctl_type(card, &data->id, &count);
-	if (type < 0)
-		return type;
-
-	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
-	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
-		for (i = 0; i < count; i++) {
-			s32 __user *intp = valuep;
-			int val;
-			if (get_user(val, &intp[i]))
-				return -EFAULT;
-			data->value.integer.value[i] = val;
-		}
-	} else {
-		size = get_elem_size(type, count);
-		if (size < 0) {
-			dev_err(card->dev, "snd_ioctl32_ctl_elem_value: unknown type %d\n", type);
-			return -EINVAL;
-		}
-		if (copy_from_user(data->value.bytes.data, valuep, size))
-			return -EFAULT;
-	}
-
-	*typep = type;
-	*countp = count;
-	return 0;
-}
-
-/* restore the value to 32bit */
-static int copy_ctl_value_to_user(void __user *userdata,
-				  void __user *valuep,
-				  struct snd_ctl_elem_value *data,
-				  int type, int count)
-{
-	int i, size;
-
-	if (type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
-	    type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
-		for (i = 0; i < count; i++) {
-			s32 __user *intp = valuep;
-			int val;
-			val = data->value.integer.value[i];
-			if (put_user(val, &intp[i]))
-				return -EFAULT;
-		}
-	} else {
-		size = get_elem_size(type, count);
-		if (copy_to_user(valuep, data->value.bytes.data, size))
-			return -EFAULT;
-	}
-	return 0;
-}
-
-static int ctl_elem_read_user(struct snd_ctl_file *ctl_file,
-			      void __user *userdata, void __user *valuep)
-{
-	struct snd_ctl_elem_value *data;
-	int err, type, count;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (data == NULL)
-		return -ENOMEM;
-
-	err = copy_ctl_value_from_user(ctl_file->card, data, userdata, valuep,
-				       &type, &count);
-	if (err < 0)
-		goto error;
-
-	err = snd_ctl_elem_read(ctl_file, data);
-	if (err < 0)
-		goto error;
-	err = copy_ctl_value_to_user(userdata, valuep, data, type, count);
- error:
-	kfree(data);
-	return err;
-}
-
-static int ctl_elem_write_user(struct snd_ctl_file *ctl_file,
-			       void __user *userdata, void __user *valuep)
-{
-	struct snd_ctl_elem_value *data;
-	int err, type, count;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (data == NULL)
-		return -ENOMEM;
-
-	err = copy_ctl_value_from_user(ctl_file->card, data, userdata, valuep,
-				       &type, &count);
-	if (err < 0)
-		goto error;
-
-	err = snd_ctl_elem_write(ctl_file, data);
-	if (err < 0)
-		goto error;
-	err = copy_ctl_value_to_user(userdata, valuep, data, type, count);
- error:
-	kfree(data);
-	return err;
-}
-
-static int snd_ctl_elem_read_user_compat(struct snd_ctl_file *ctl_file,
-					 struct snd_ctl_elem_value32 __user *data32)
-{
-	return ctl_elem_read_user(ctl_file, data32, &data32->value);
-}
-
-static int snd_ctl_elem_write_user_compat(struct snd_ctl_file *file,
-					  struct snd_ctl_elem_value32 __user *data32)
-{
-	return ctl_elem_write_user(file, data32, &data32->value);
-}
-
 static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
 					 void *buf)
 {
@@ -638,12 +450,14 @@ enum {
 				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
 	SNDRV_CTL_IOCTL_ELEM_INFO_32 =
 				_IOWR('U', 0x11, struct snd_ctl_elem_info_32),
-	SNDRV_CTL_IOCTL_ELEM_READ32 = _IOWR('U', 0x12, struct snd_ctl_elem_value32),
-	SNDRV_CTL_IOCTL_ELEM_WRITE32 = _IOWR('U', 0x13, struct snd_ctl_elem_value32),
 	SNDRV_CTL_IOCTL_ELEM_ADD_32 =
 				_IOWR('U', 0x17, struct snd_ctl_elem_info_32),
 	SNDRV_CTL_IOCTL_ELEM_REPLACE_32 =
 				_IOWR('U', 0x18, struct snd_ctl_elem_info_32),
+	SNDRV_CTL_IOCTL_ELEM_READ_32 =
+				_IOWR('U', 0x12, struct snd_ctl_elem_value_32),
+	SNDRV_CTL_IOCTL_ELEM_WRITE_32 =
+				_IOWR('U', 0x13, struct snd_ctl_elem_value_32),
 	SNDRV_CTL_IOCTL_ELEM_READ_I386 =
 				_IOWR('U', 0x12, struct snd_ctl_elem_value_i386),
 	SNDRV_CTL_IOCTL_ELEM_WRITE_I386 =
@@ -690,6 +504,22 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 			serialize_to_elem_info_32,
 			SNDRV_CTL_IOCTL_ELEM_REPLACE,
 		},
+#if !defined(CONFIG_X86_64) || defined(CONFIG_X86_X32)
+		{
+			SNDRV_CTL_IOCTL_ELEM_READ_32,
+			deserialize_from_elem_value_32,
+			ctl_compat_ioctl_elem_read_32,
+			serialize_to_elem_value_32,
+			SNDRV_CTL_IOCTL_ELEM_READ,
+		},
+		{
+			SNDRV_CTL_IOCTL_ELEM_WRITE_32,
+			deserialize_from_elem_value_32,
+			ctl_compat_ioctl_elem_write_32,
+			serialize_to_elem_value_32,
+			SNDRV_CTL_IOCTL_ELEM_WRITE,
+		},
+#endif
 #ifdef CONFIG_X86_64
 		{
 			SNDRV_CTL_IOCTL_ELEM_READ_I386,
@@ -708,7 +538,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 #endif
 	};
 	struct snd_ctl_file *ctl;
-	void __user *argp = compat_ptr(arg);
 	void *buf, *data;
 	unsigned int size;
 	int i;
@@ -718,13 +547,6 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 	if (snd_BUG_ON(!ctl || !ctl->card))
 		return -ENXIO;
 
-	switch (cmd) {
-	case SNDRV_CTL_IOCTL_ELEM_READ32:
-		return snd_ctl_elem_read_user_compat(ctl, argp);
-	case SNDRV_CTL_IOCTL_ELEM_WRITE32:
-		return snd_ctl_elem_write_user_compat(ctl, argp);
-	}
-
 	for (i = 0; i < ARRAY_SIZE(handlers); ++i) {
 		if (handlers[i].cmd == cmd)
 			break;
-- 
2.14.1

[PATCH 24/24] ALSA: ctl: code cleanup for compat handler

[Takashi Sakamoto]

This commit applies some changes for naming consistency. Additionally,
a series of author's work results to replace most of the codes. To
facilitate searching by developers, this commit adds author's signature.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control_compat.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index bbda8eabf18f..ab2ba7994a60 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -2,6 +2,7 @@
  * compat ioctls for control API
  *
  *   Copyright (c) by Takashi Iwai <tiwai@suse.de>
+ *   Copyright (c) 2017 Takashi Sakamoto
  *
  *   This program is free software; you can redistribute it and/or modify
  *   it under the terms of the GNU General Public License as published by
@@ -537,14 +538,14 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		},
 #endif
 	};
-	struct snd_ctl_file *ctl;
+	struct snd_ctl_file *ctl_file;
 	void *buf, *data;
 	unsigned int size;
 	int i;
 	int err;
 
-	ctl = file->private_data;
-	if (snd_BUG_ON(!ctl || !ctl->card))
+	ctl_file = file->private_data;
+	if (snd_BUG_ON(!ctl_file || !ctl_file->card))
 		return -ENXIO;
 
 	for (i = 0; i < ARRAY_SIZE(handlers); ++i) {
@@ -557,7 +558,8 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		down_read(&snd_ioctl_rwsem);
 		list_for_each_entry(p, &snd_control_compat_ioctls, list) {
 			if (p->fioctl) {
-				err = p->fioctl(ctl->card, ctl, cmd, arg);
+				err = p->fioctl(ctl_file->card, ctl_file, cmd,
+						arg);
 				if (err != -ENOIOCTLCMD) {
 					up_read(&snd_ioctl_rwsem);
 					return err;
@@ -589,15 +591,15 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		}
 	}
 
-	err = handlers[i].deserialize(ctl, buf, data);
+	err = handlers[i].deserialize(ctl_file, buf, data);
 	if (err < 0)
 		goto end;
 
-	err = handlers[i].func(ctl, buf);
+	err = handlers[i].func(ctl_file, buf);
 	if (err < 0)
 		goto end;
 
-	err = handlers[i].serialize(ctl, data, buf);
+	err = handlers[i].serialize(ctl_file, data, buf);
 	if (err >= 0) {
 		if (handlers[i].cmd & IOC_OUT) {
 			if (copy_to_user(compat_ptr(arg), data, size))
@@ -609,7 +611,7 @@ static long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd,
 		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32 ||
 		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32) {
 			struct snd_ctl_elem_info *info = buf;
-			snd_ctl_remove_user_ctl(ctl, &info->id);
+			snd_ctl_remove_user_ctl(ctl_file, &info->id);
 		}
 	}
 end:
-- 
2.14.1

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Iwai]

On Sat, 25 Nov 2017 10:19:42 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> ALSA control core supports system call compatibility layer because some
> structures in ALSA control interface includes members of 'long' and
> 'pointer' types and change own layout according to ABIs. In this point,
> it's enough for the layer to have handlers for structures on ABIs with
> 'ILP32' data model.
> 
> A recent commit of 6236d8bb2afc ('ALSA: ctl: Fix ioctls for X32 ABI')
> clear that System V ABI for i386 architecture is unique than the other
> ABIs with 'ILP32' data model. On this ABI, machine type for storage class
> of 'long long' type has 4 bytes alignment. This is different from the
> other ABIs.
> 
> In current implementation of this layer, the same structure is used for
> i386 ABI and the other ABI with 'ILP32' data model except for x32 ABI.
> Macro is used to switch between these. But in a view of data model,
> it's better to define structure for i386 ABI uniquely against the
> other ABIs including x32 for simplicity.
> 
> Additionally, this layer includes some points to be improved:
>  - cancel allocation in user space from kernel land
>  - reduce kernel stack usage
> 
> This patchset is my attempts to improve the layer. For this purpose,
> this patchset introduces a local framework to describe consistent method
> to convert and process data for differences of structure layout.

Basically I like this kind of refactoring, but I hesitate applying at
this time.  First off, the patches make codes bigger in both source
codes and binaries:

 sound/core/control.c        | 242 ++++++++-----
 sound/core/control_compat.c | 837 +++++++++++++++++++++++++-------------------
 2 files changed, 630 insertions(+), 449 deletions(-)
 
(original)
 % size sound.ko 
   text    data     bss     dec     hex filename
  59494    2188    6272   67954   10972 sound/core/snd.ko

(patched)
 % size sound.ko 
   text    data     bss     dec     hex filename
  60186    2200    6272   68658   10c32 sound/core/snd.ko
	  

Another slight concern is that the new code requires one extra kmalloc
at each execution.  The kctl ioctls aren't very hot code path, but
they can be called yet relatively frequently, hence a slowdown isn't
good unless the modification brings significantly (read
"dramatically") improvement of code readability.

Also, did you actually test with all possible architectures, not only
trusting the written spec?  It should be relatively easy to set up the
cross-compilation just only for checking this stuff.  You don't need
to build the whole kernel.


thanks,

Takashi

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Sakamoto]

Hi,

On Nov 28 2017 05:40, Takashi Iwai wrote:
 > Basically I like this kind of refactoring, but I hesitate applying at
 > this time.  First off, the patches make codes bigger in both source
 > codes and binaries:
 >
 >   sound/core/control.c        | 242 ++++++++-----
 >   sound/core/control_compat.c | 837 
+++++++++++++++++++++++++-------------------
 >   2 files changed, 630 insertions(+), 449 deletions(-)
 >
 > (original)
 >   % size sound.ko
 >     text    data     bss     dec     hex filename
 >    59494    2188    6272   67954   10972 sound/core/snd.ko
 >
 > (patched)
 >   % size sound.ko
 >     text    data     bss     dec     hex filename
 >    60186    2200    6272   68658   10c32 sound/core/snd.ko

Yes. This patchset increases binary size. In my build envoronment
(make x86_64_defconfig + disable debug configurations), they're:

(at v4.15-rc1[1])
$ size -A /tmp/snd.ko.master
/tmp/snd.ko.master  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26244      0 <-
.init.text                    470      0
.exit.text                     45      0
.altinstr_replacement          20      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      1784      0 <-
.rodata.str1.1                733      0
.rodata.str1.8                559      0
__ksymtab_strings            1133      0
.modinfo                      423      0
__param                       120      0
.orc_unwind_ip               5220      0
.orc_unwind                  7830      0
.smp_locks                     24      0
.altinstructions               52      0
.data                         480      0
__bug_table                    12      0
.gnu.linkonce.this_module     768      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       49445

(with whole this patchset, at a branch in my repo[2])
$ size -A /tmp/snd.ko.compat
/tmp/snd.ko.compat  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26692      0 <-
.init.text                    470      0
.exit.text                     45      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      2104      0 <-
.rodata.str1.1                733      0
.rodata.str1.8                511      0
__ksymtab_strings            1133      0
.modinfo                      424      0
__param                       120      0
.orc_unwind_ip               5212      0
.orc_unwind                  7818      0
.smp_locks                     24      0
.data                         480      0
__bug_table                    12      0
.gnu.linkonce.this_module     768      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       50074

Both of sections of '.rodata' and '.text' are increased. As a summary:

             v4.15-rc1   compat
.text:      26244       26692
.rodata:    1784        2104
total:      49445       50074

The increase of former comes from 'handlers[]' in
'snd_ctl_ioctl_compat()' and this is unavoided. On the other hand, the
increase of latter comes from a series of function named as
'ctl_compat_ioctl_xxx()'. If 'control.c' got proper arrangements, the
increase can be suppressed somehow.

Actually, this patchset has a next patchset, to apply refactoring the
file[3] (I should have added some comments about it, but they were not
prepared when posting this patchset, sorry...). In the unposted
patchset, the similar idea as this patchset is introduced; i.e. handler,
allocator and deallocator. As a result:

(with the unposted patchset, at a branch in my repo[3])
/tmp/snd.ko.unlocked  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26164      0 <-
.init.text                    470      0
.exit.text                     45      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      2328      0 <-
.rodata.str1.1                733      0
.rodata.str1.8                511      0
__ksymtab_strings            1133      0
.modinfo                      424      0
__param                       120      0
.orc_unwind_ip               5252      0
.orc_unwind                  7878      0
.smp_locks                     24      0
.data                         480      0
__bug_table                    12      0
.gnu.linkonce.this_module     768      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       49870

As a summary:
             v4.15-rc1   compat  unlocked
.text:      26244       26692   26164
.rodata:    1784        2104    2428
total:      49445       50074   49870

The .text section decreases its size, while .rodata increases. Total
size of included sections increases +0.8%. Of course, any debug sections
are excluded from the calculation.

In my opinion, in a point of binary size, the increase is enough
acceptable.

 > Another slight concern is that the new code requires one extra kmalloc
 > at each execution. The kctl ioctls aren't very hot code path, but
 > they can be called yet relatively frequently, hence a slowdown isn't
 > good unless the modification brings significantly (read
 > "dramatically") improvement of code readability.

Hm. I guess that you actually found some advantages of this patchset to
increase code readability and maintainability. The additional step for
allocation on kernel space belongs to optimization domain, however as
you noted code path via ALSA control interface is not designed for
severe use cases such as real-time data transmission. We can be
optimistic for the overhead as long as it's not so large.

 > Also, did you actually test with all possible architectures, not only
 > trusting the written spec?  It should be relatively easy to set up the
 > cross-compilation just only for checking this stuff.  You don't need
 > to build the whole kernel.

I've already done it. To check size of structure and offsets of members
on it, I've generate assembly list, like:

$ cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <stddef.h>
#include <sound/asound.h>
struct snd_ctl_elem_value_32 {
     ...
} __attribute__((packed));
int main(void)
{
     printf("%zd\n", offsetof(struct snd_ctl_elem_value, value));
     return EXIT_SUCCESS;
}
$ x86_64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B 4 printf
     .cfi_def_cfa_register 6
     movl    $72, %esi
     leaq    .LC0(%rip), %rdi
     movl    $0, %eax
     call    printf@PLT
$ aarch64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B1 printf
     mov x1, 72
     bl  printf

Inner GCC, the value of sizeof() and offsetof() is decied in compilation
time, by builtin functions (e.g. '__builtin_offsetof()'). So the list
includes immediate values and easy to read.

But I did the above in user space, so didn't build kernel land stuffs
directly, to save my time.

Actual test is done on x86_64 machine for programs with i386, x32 and
amd64 ABIs. As you know, I used a test program in alsa-lib. (but for
x32, I wrote a small program just to test ELEM_READ/ELEM_WRITE).

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323
[2] https://github.com/takaswie/sound/tree/topic/ctl-compat-refactoring
[3] https://github.com/takaswie/sound/tree/topic/ctl-ioctl-refactoring


Thanks

Takashi Sakamoto

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Iwai]

On Tue, 28 Nov 2017 14:32:51 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 28 2017 05:40, Takashi Iwai wrote:
> > Basically I like this kind of refactoring, but I hesitate applying at
> > this time.  First off, the patches make codes bigger in both source
> > codes and binaries:
> >
> >   sound/core/control.c        | 242 ++++++++-----
> >   sound/core/control_compat.c | 837 
> +++++++++++++++++++++++++-------------------
> >   2 files changed, 630 insertions(+), 449 deletions(-)
> >
> > (original)
> >   % size sound.ko
> >     text    data     bss     dec     hex filename
> >    59494    2188    6272   67954   10972 sound/core/snd.ko
> >
> > (patched)
> >   % size sound.ko
> >     text    data     bss     dec     hex filename
> >    60186    2200    6272   68658   10c32 sound/core/snd.ko
> 
> Yes. This patchset increases binary size. In my build envoronment
> (make x86_64_defconfig + disable debug configurations), they're:
> 
> (at v4.15-rc1[1])
> $ size -A /tmp/snd.ko.master
> /tmp/snd.ko.master  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26244      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> .altinstr_replacement          20      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      1784      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                559      0
> __ksymtab_strings            1133      0
> .modinfo                      423      0
> __param                       120      0
> .orc_unwind_ip               5220      0
> .orc_unwind                  7830      0
> .smp_locks                     24      0
> .altinstructions               52      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       49445
> 
> (with whole this patchset, at a branch in my repo[2])
> $ size -A /tmp/snd.ko.compat
> /tmp/snd.ko.compat  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26692      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      2104      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                511      0
> __ksymtab_strings            1133      0
> .modinfo                      424      0
> __param                       120      0
> .orc_unwind_ip               5212      0
> .orc_unwind                  7818      0
> .smp_locks                     24      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       50074
> 
> Both of sections of '.rodata' and '.text' are increased. As a summary:
> 
>             v4.15-rc1   compat
> .text:      26244       26692
> .rodata:    1784        2104
> total:      49445       50074
> 
> The increase of former comes from 'handlers[]' in
> 'snd_ctl_ioctl_compat()' and this is unavoided. On the other hand, the
> increase of latter comes from a series of function named as
> 'ctl_compat_ioctl_xxx()'. If 'control.c' got proper arrangements, the
> increase can be suppressed somehow.
> 
> Actually, this patchset has a next patchset, to apply refactoring the
> file[3] (I should have added some comments about it, but they were not
> prepared when posting this patchset, sorry...). In the unposted
> patchset, the similar idea as this patchset is introduced; i.e. handler,
> allocator and deallocator. As a result:
> 
> (with the unposted patchset, at a branch in my repo[3])
> /tmp/snd.ko.unlocked  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26164      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      2328      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                511      0
> __ksymtab_strings            1133      0
> .modinfo                      424      0
> __param                       120      0
> .orc_unwind_ip               5252      0
> .orc_unwind                  7878      0
> .smp_locks                     24      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       49870
> 
> As a summary:
>             v4.15-rc1   compat  unlocked
> .text:      26244       26692   26164
> .rodata:    1784        2104    2428
> total:      49445       50074   49870
> 
> The .text section decreases its size, while .rodata increases. Total
> size of included sections increases +0.8%. Of course, any debug sections
> are excluded from the calculation.
> 
> In my opinion, in a point of binary size, the increase is enough
> acceptable.
> 
> > Another slight concern is that the new code requires one extra kmalloc
> > at each execution. The kctl ioctls aren't very hot code path, but
> > they can be called yet relatively frequently, hence a slowdown isn't
> > good unless the modification brings significantly (read
> > "dramatically") improvement of code readability.
> 
> Hm. I guess that you actually found some advantages of this patchset to
> increase code readability and maintainability. The additional step for
> allocation on kernel space belongs to optimization domain, however as
> you noted code path via ALSA control interface is not designed for
> severe use cases such as real-time data transmission. We can be
> optimistic for the overhead as long as it's not so large.

Well, both the code size and the code performance are rather the basic
question.  Usually a code refactoring is done because of performance,
not because of the code itself.  IOW, the refactoring is based on the
implicit rule -- a better code performs better.  If it's not true, the
only exception is about the portability.  But I don't think that your
patchset would improve that dramatically.  It's the point making me
hesitating to apply the patches.

In short, the patch result doesn't look convincing enough.  At least,
I'd like to see an improvement in the resultant binary.


> > Also, did you actually test with all possible architectures, not only
> > trusting the written spec?  It should be relatively easy to set up the
> > cross-compilation just only for checking this stuff.  You don't need
> > to build the whole kernel.
> 
> I've already done it. To check size of structure and offsets of members
> on it, I've generate assembly list, like:
> 
> $ cat test.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <stddef.h>
> #include <sound/asound.h>
> struct snd_ctl_elem_value_32 {
>     ...
> } __attribute__((packed));
> int main(void)
> {
>     printf("%zd\n", offsetof(struct snd_ctl_elem_value, value));
>     return EXIT_SUCCESS;
> }
> $ x86_64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B 4 printf
>     .cfi_def_cfa_register 6
>     movl    $72, %esi
>     leaq    .LC0(%rip), %rdi
>     movl    $0, %eax
>     call    printf@PLT
> $ aarch64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B1 printf
>     mov x1, 72
>     bl  printf

There are lots of other architectures supporting 64/32 compatibility,
e.g. s390, powerpc, sparc, etc.  That's my concern.  At least these
major active architectures have to be checked (maybe MIPS, too).


thanks,

Takashi

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Sakamoto]

Hi,

On Nov 28 2017 22:48, Takashi Iwai wrote:
> Well, both the code size and the code performance are rather the basic
> question.  Usually a code refactoring is done because of performance,
> not because of the code itself.  IOW, the refactoring is based on the
> implicit rule -- a better code performs better.  If it's not true, the
> only exception is about the portability.  But I don't think that your
> patchset would improve that dramatically.  It's the point making me
> hesitating to apply the patches.
> 
> In short, the patch result doesn't look convincing enough.  At least,
> I'd like to see an improvement in the resultant binary.

Hm. For these concerns, I can propose additional improvements to this
patchset.
  * Use kernel stack for buffers to convert layout of structure
   - (pros) This is for your concern about an overhead to use additional
     kernel heap. Additionally, this can reduce one member from handler
     structure because no need to calculate original buffer size.
   - (cons) two buffers for purse consumes the kernel stack by around
     2000 bytes.
  * Change some functions in 'control.c' to get 'void *' as an argument.
   - (pros) these functions can get callback from 'control_compat.c'
     directly. As a result, .text section can be decreased.
   - (cons) these functions lose type information for the argument. This
     also can fake static code analyzer, perhaps.

In the end of this message, I attach a diff for this idea, on the last
patch of this set. When applying, the binary is shrunk.

$ size snd.ko.master
snd.ko.master  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26244      0 <-
.init.text                    470      0
.exit.text                     45      0
.altinstr_replacement          20      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      1784      0 <-
.rodata.str1.1                733      0
.rodata.str1.8                559      0
__ksymtab_strings            1133      0
.modinfo                      423      0
__param                       120      0
.orc_unwind_ip               5224      0
.orc_unwind                  7836      0
.smp_locks                     24      0
.altinstructions               52      0
.data                         480      0
__bug_table                    12      0
.gnu.linkonce.this_module     832      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       49519

$ size snd.ko.compat
sound/core/snd.ko  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26020      0 <-
.init.text                    470      0
.exit.text                     45      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      1976      0 <-
.rodata.str1.1                763      0
.rodata.str1.8                551      0
__ksymtab_strings            1133      0
.modinfo                      424      0
__param                       120      0
.orc_unwind_ip               5004      0
.orc_unwind                  7506      0
.smp_locks                     24      0
.data                         480      0
__bug_table                    24      0
.gnu.linkonce.this_module     832      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       48900

             v4.15-rc1   compat
.text:      26244       26020
.rodata:    1784        1976
total:      49519       48900

If we can accept the negative points, I'll post the revised version.

>>> Also, did you actually test with all possible architectures, not only
>>> trusting the written spec?  It should be relatively easy to set up the
>>> cross-compilation just only for checking this stuff.  You don't need
>>> to build the whole kernel.
>>
>> I've already done it. To check size of structure and offsets of members
>> on it, I've generate assembly list, like:
>>
>> $ cat test.c
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <stddef.h>
>> #include <sound/asound.h>
>> struct snd_ctl_elem_value_32 {
>>      ...
>> } __attribute__((packed));
>> int main(void)
>> {
>>      printf("%zd\n", offsetof(struct snd_ctl_elem_value, value));
>>      return EXIT_SUCCESS;
>> }
>> $ x86_64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B 4 printf
>>      .cfi_def_cfa_register 6
>>      movl    $72, %esi
>>      leaq    .LC0(%rip), %rdi
>>      movl    $0, %eax
>>      call    printf@PLT
>> $ aarch64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B1 printf
>>      mov x1, 72
>>      bl  printf
> 
> There are lots of other architectures supporting 64/32 compatibility,
> e.g. s390, powerpc, sparc, etc.  That's my concern.  At least these
> major active architectures have to be checked (maybe MIPS, too).

If you'd like me to check more architectures, I'll do it. Please send a 
list of architectures to investigate. But in practical points, I'll omit 
s390, sparc and sparc64 because nowadays we have no products based on 
these architectures for consumer use. At least, in data center, sound 
devices are needless.

========== 8< ----------
diff --git a/sound/core/control.c b/sound/core/control.c
index 0aa1f22dcac2..0228d4c3a51d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -744,9 +744,9 @@ static int snd_ctl_card_info(struct snd_card *card, 
struct snd_ctl_file * ctl,
  	return 0;
  }

-static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file,
-			     struct snd_ctl_elem_list *list)
+static int snd_ctl_elem_list(struct snd_ctl_file *ctl_file, void *buf)
  {
+	struct snd_ctl_elem_list *list = buf;
  	struct snd_kcontrol *kctl;
  	struct snd_ctl_elem_id id;
  	unsigned int offset, space, jidx;
@@ -833,9 +833,9 @@ static bool validate_element_member_dimension(struct 
snd_ctl_elem_info *info)
  	return members == info->count;
  }

-static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
-			     struct snd_ctl_elem_info *info)
+static int snd_ctl_elem_info(struct snd_ctl_file *ctl, void *buf)
  {
+	struct snd_ctl_elem_info *info = buf;
  	struct snd_card *card = ctl->card;
  	struct snd_kcontrol *kctl;
  	struct snd_kcontrol_volatile *vd;
@@ -894,9 +894,9 @@ static int snd_ctl_elem_info_user(struct 
snd_ctl_file *ctl, void __user *arg)
  	return err;
  }

-static int snd_ctl_elem_read(struct snd_ctl_file *ctl_file,
-			     struct snd_ctl_elem_value *control)
+static int snd_ctl_elem_read(struct snd_ctl_file *ctl_file, void *buf)
  {
+	struct snd_ctl_elem_value *control = buf;
  	struct snd_kcontrol *kctl;
  	struct snd_kcontrol_volatile *vd;
  	unsigned int index_offset;
@@ -949,9 +949,9 @@ static int snd_ctl_elem_read_user(struct 
snd_ctl_file *ctl_file,
  	return result;
  }

-static int snd_ctl_elem_write(struct snd_ctl_file *ctl_file,
-			      struct snd_ctl_elem_value *control)
+static int snd_ctl_elem_write(struct snd_ctl_file *ctl_file, void *buf)
  {
+	struct snd_ctl_elem_value *control = buf;
  	struct snd_kcontrol *kctl;
  	struct snd_kcontrol_volatile *vd;
  	unsigned int index_offset;
@@ -1255,9 +1255,9 @@ static void snd_ctl_elem_user_free(struct 
snd_kcontrol *kcontrol)
  	kfree(ue);
  }

-static int snd_ctl_elem_add(struct snd_ctl_file *file,
-			    struct snd_ctl_elem_info *info)
+static int snd_ctl_elem_add(struct snd_ctl_file *file, void *buf)
  {
+	struct snd_ctl_elem_info *info = buf;
  	/* The capacity of struct snd_ctl_elem_value.value.*/
  	static const unsigned int value_sizes[] = {
  		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= sizeof(long),
@@ -1419,9 +1419,9 @@ static int snd_ctl_elem_add_user(struct 
snd_ctl_file *ctl_file,
  	return err;
  }

-static int snd_ctl_elem_replace(struct snd_ctl_file *ctl_file,
-				struct snd_ctl_elem_info *info)
+static int snd_ctl_elem_replace(struct snd_ctl_file *ctl_file, void *buf)
  {
+	struct snd_ctl_elem_info *info = buf;
  	int err;

  	if (!*info->id.name)
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index ab2ba7994a60..c4ef8aa25131 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -398,54 +398,6 @@ static int __maybe_unused serialize_to_elem_value_i386(
  	return 0;
  }

-static int ctl_compat_ioctl_elem_list_32(struct snd_ctl_file *ctl_file,
-					 void *buf)
-{
-	struct snd_ctl_elem_list *list = buf;
-
-	return snd_ctl_elem_list(ctl_file, list);
-}
-
-static int ctl_compat_ioctl_elem_info_32(struct snd_ctl_file *ctl_file,
-					 void *buf)
-{
-	struct snd_ctl_elem_info *info = buf;
-
-	return snd_ctl_elem_info(ctl_file, info);
-}
-
-static int ctl_compat_ioctl_elem_add_32(struct snd_ctl_file *ctl_file,
-					void *buf)
-{
-	struct snd_ctl_elem_info *info = buf;
-
-	return snd_ctl_elem_add(ctl_file, info);
-}
-
-static int ctl_compat_ioctl_elem_replace_32(struct snd_ctl_file *ctl_file,
-					    void *buf)
-{
-	struct snd_ctl_elem_info *info = buf;
-
-	return snd_ctl_elem_replace(ctl_file, info);
-}
-
-static int ctl_compat_ioctl_elem_read_32(struct snd_ctl_file *ctl_file,
-					 void *buf)
-{
-	struct snd_ctl_elem_value *value = buf;
-
-	return snd_ctl_elem_read(ctl_file, value);
-}
-
-static int ctl_compat_ioctl_elem_write_32(struct snd_ctl_file *ctl_file,
-					  void *buf)
-{
-	struct snd_ctl_elem_value *value = buf;
-
-	return snd_ctl_elem_write(ctl_file, value);
-}
-
  enum {
  	SNDRV_CTL_IOCTL_ELEM_LIST_32 =
  				_IOWR('U', 0x10, struct snd_ctl_elem_list_32),
@@ -475,71 +427,72 @@ static long snd_ctl_ioctl_compat(struct file 
*file, unsigned int cmd,
  		int (*func)(struct snd_ctl_file *ctl_file, void *buf);
  		int (*serialize)(struct snd_ctl_file *ctl_file, void *dst,
  				 void *src);
-		unsigned int orig_cmd;
  	} handlers[] = {
  		{
  			SNDRV_CTL_IOCTL_ELEM_LIST_32,
  			deserialize_from_elem_list_32,
-			ctl_compat_ioctl_elem_list_32,
+			snd_ctl_elem_list,
  			serialize_to_elem_list_32,
-			SNDRV_CTL_IOCTL_ELEM_LIST,
  		},
  		{
  			SNDRV_CTL_IOCTL_ELEM_INFO_32,
  			deserialize_from_elem_info_32,
-			ctl_compat_ioctl_elem_info_32,
+			snd_ctl_elem_info,
  			serialize_to_elem_info_32,
-			SNDRV_CTL_IOCTL_ELEM_INFO,
  		},
  		{
  			SNDRV_CTL_IOCTL_ELEM_ADD_32,
  			deserialize_from_elem_info_32,
-			ctl_compat_ioctl_elem_add_32,
+			snd_ctl_elem_add,
  			serialize_to_elem_info_32,
-			SNDRV_CTL_IOCTL_ELEM_ADD,
  		},
  		{
  			SNDRV_CTL_IOCTL_ELEM_REPLACE_32,
  			deserialize_from_elem_info_32,
-			ctl_compat_ioctl_elem_replace_32,
+			snd_ctl_elem_replace,
  			serialize_to_elem_info_32,
-			SNDRV_CTL_IOCTL_ELEM_REPLACE,
  		},
  #if !defined(CONFIG_X86_64) || defined(CONFIG_X86_X32)
  		{
  			SNDRV_CTL_IOCTL_ELEM_READ_32,
  			deserialize_from_elem_value_32,
-			ctl_compat_ioctl_elem_read_32,
+			snd_ctl_elem_read,
  			serialize_to_elem_value_32,
-			SNDRV_CTL_IOCTL_ELEM_READ,
  		},
  		{
  			SNDRV_CTL_IOCTL_ELEM_WRITE_32,
  			deserialize_from_elem_value_32,
-			ctl_compat_ioctl_elem_write_32,
+			snd_ctl_elem_write,
  			serialize_to_elem_value_32,
-			SNDRV_CTL_IOCTL_ELEM_WRITE,
  		},
  #endif
  #ifdef CONFIG_X86_64
  		{
  			SNDRV_CTL_IOCTL_ELEM_READ_I386,
  			deserialize_from_elem_value_i386,
-			ctl_compat_ioctl_elem_read_32,
+			snd_ctl_elem_read,
  			serialize_to_elem_value_i386,
-			SNDRV_CTL_IOCTL_ELEM_READ,
  		},
  		{
  			SNDRV_CTL_IOCTL_ELEM_WRITE_I386,
  			deserialize_from_elem_value_i386,
-			ctl_compat_ioctl_elem_write_32,
+			snd_ctl_elem_write,
  			serialize_to_elem_value_i386,
-			SNDRV_CTL_IOCTL_ELEM_WRITE,
  		},
  #endif
  	};
+	union {
+		struct snd_ctl_elem_list_32 elem_list_32;
+		struct snd_ctl_elem_info_32 elem_info_32;
+		struct snd_ctl_elem_value_32 elem_value_32;
+		struct snd_ctl_elem_value_i386 elem_value_i386;
+	} data;
+	union {
+		struct snd_ctl_elem_list elem_list;
+		struct snd_ctl_elem_info elem_info;
+		struct snd_ctl_elem_value elem_value;
+	} buf;
  	struct snd_ctl_file *ctl_file;
-	void *buf, *data;
  	unsigned int size;
  	int i;
  	int err;
@@ -571,51 +524,35 @@ static long snd_ctl_ioctl_compat(struct file 
*file, unsigned int cmd,
  		return snd_ctl_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
  	}

-	/* Allocate a buffer to convert layout of structure for native ABI. */
-	buf = kzalloc(_IOC_SIZE(handlers[i].orig_cmd), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
  	/* Allocate an alternative buffer to copy from/to user space. */
  	size = _IOC_SIZE(handlers[i].cmd);
-	data = kzalloc(size, GFP_KERNEL);
-	if (!data) {
-		kfree(buf);
-		return -ENOMEM;
-	}

  	if (handlers[i].cmd & IOC_IN) {
-		if (copy_from_user(data, compat_ptr(arg), size)) {
-			err = -EFAULT;
-			goto end;
-		}
+		if (copy_from_user(&data, compat_ptr(arg), size))
+			return -EFAULT;
  	}

-	err = handlers[i].deserialize(ctl_file, buf, data);
+	err = handlers[i].deserialize(ctl_file, &buf, &data);
  	if (err < 0)
-		goto end;
+		return err;

-	err = handlers[i].func(ctl_file, buf);
+	err = handlers[i].func(ctl_file, &buf);
  	if (err < 0)
-		goto end;
+		return err;

-	err = handlers[i].serialize(ctl_file, data, buf);
+	err = handlers[i].serialize(ctl_file, &data, &buf);
  	if (err >= 0) {
  		if (handlers[i].cmd & IOC_OUT) {
-			if (copy_to_user(compat_ptr(arg), data, size))
+			if (copy_to_user(compat_ptr(arg), &data, size))
  				err = -EFAULT;
  		}
  	}

  	if (err < 0) {
  		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32 ||
-		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32) {
-			struct snd_ctl_elem_info *info = buf;
-			snd_ctl_remove_user_ctl(ctl_file, &info->id);
-		}
+		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32)
+			snd_ctl_remove_user_ctl(ctl_file, &buf.elem_info.id);
  	}
-end:
-	kfree(data);
-	kfree(buf);
+
  	return err;
  }
========== 8< ----------


Thanks

Takashi Sakamoto

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Iwai]

On Wed, 29 Nov 2017 03:18:57 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 28 2017 22:48, Takashi Iwai wrote:
> > Well, both the code size and the code performance are rather the basic
> > question.  Usually a code refactoring is done because of performance,
> > not because of the code itself.  IOW, the refactoring is based on the
> > implicit rule -- a better code performs better.  If it's not true, the
> > only exception is about the portability.  But I don't think that your
> > patchset would improve that dramatically.  It's the point making me
> > hesitating to apply the patches.
> >
> > In short, the patch result doesn't look convincing enough.  At least,
> > I'd like to see an improvement in the resultant binary.
> 
> Hm. For these concerns, I can propose additional improvements to this
> patchset.
>  * Use kernel stack for buffers to convert layout of structure
>   - (pros) This is for your concern about an overhead to use additional
>     kernel heap. Additionally, this can reduce one member from handler
>     structure because no need to calculate original buffer size.
>   - (cons) two buffers for purse consumes the kernel stack by around
>     2000 bytes.

I'm afraid that this increase isn't acceptable.
The current compat code was originally with stack, too, but had to be
converted later to kmalloc for reducing the stack size usage.  Now the
new code would even double.

Although the requirement of stack usage reduction came from 4k stack
and the condition was relaxed nowadays after giving up the idea, the
stack is still precious and we are not allowed to consume too much.
As a rule of thumb, maybe 100 to 200 bytes would be maximum per
function.

>  * Change some functions in 'control.c' to get 'void *' as an argument.
>   - (pros) these functions can get callback from 'control_compat.c'
>     directly. As a result, .text section can be decreased.
>   - (cons) these functions lose type information for the argument. This
>     also can fake static code analyzer, perhaps.

This isn't fascinating, either, but more acceptable than the former.
(Read: let's think of other ways, but this can be still taken as a
 last resort.)

BTW, if we go in that way, I guess the idea of using the conversion
table can be applied to 64bit native stuff, too.  Basically
memdup_user() and and copy_to_user() calls correspond to deserialize
and serialize.  Using a common helper in both sides might reduce the
code size, and it makes the change to void* more demanded.

> 
> >>> Also, did you actually test with all possible architectures, not only
> >>> trusting the written spec?  It should be relatively easy to set up the
> >>> cross-compilation just only for checking this stuff.  You don't need
> >>> to build the whole kernel.
> >>
> >> I've already done it. To check size of structure and offsets of members
> >> on it, I've generate assembly list, like:
> >>
> >> $ cat test.c
> >> #include <stdio.h>
> >> #include <stdlib.h>
> >> #include <stddef.h>
> >> #include <sound/asound.h>
> >> struct snd_ctl_elem_value_32 {
> >>      ...
> >> } __attribute__((packed));
> >> int main(void)
> >> {
> >>      printf("%zd\n", offsetof(struct snd_ctl_elem_value, value));
> >>      return EXIT_SUCCESS;
> >> }
> >> $ x86_64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B 4 printf
> >>      .cfi_def_cfa_register 6
> >>      movl    $72, %esi
> >>      leaq    .LC0(%rip), %rdi
> >>      movl    $0, %eax
> >>      call    printf@PLT
> >> $ aarch64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B1 printf
> >>      mov x1, 72
> >>      bl  printf
> >
> > There are lots of other architectures supporting 64/32 compatibility,
> > e.g. s390, powerpc, sparc, etc.  That's my concern.  At least these
> > major active architectures have to be checked (maybe MIPS, too).
> 
> If you'd like me to check more architectures, I'll do it. Please send
> a list of architectures to investigate.

You can grep COMPAT definition in Kconfig in arch/*.  mips and tile
seem to have the compat layer, too.


> But in practical points, I'll
> omit s390, sparc and sparc64 because nowadays we have no products
> based on these architectures for consumer use. At least, in data
> center, sound devices are needless.

Don't underestimate it.  There are always crazy users, and the sound
usage isn't always tied with the real hardware but also with a VM or
data processing.  (And also there are still desktop users of SPARC
boxen :)


thanks,

Takashi

Re: [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Sakamoto]

On Nov 29 2017 16:12, Takashi Iwai wrote:
 >> On Nov 28 2017 22:48, Takashi Iwai wrote:
 >>> Well, both the code size and the code performance are rather the basic
 >>> question.  Usually a code refactoring is done because of performance,
 >>> not because of the code itself.  IOW, the refactoring is based on the
 >>> implicit rule -- a better code performs better.  If it's not true, the
 >>> only exception is about the portability.  But I don't think that your
 >>> patchset would improve that dramatically.  It's the point making me
 >>> hesitating to apply the patches.
 >>>
 >>> In short, the patch result doesn't look convincing enough.  At least,
 >>> I'd like to see an improvement in the resultant binary.
 >>
 >> Hm. For these concerns, I can propose additional improvements to this
 >> patchset.
 >>   * Use kernel stack for buffers to convert layout of structure
 >>    - (pros) This is for your concern about an overhead to use additional
 >>      kernel heap. Additionally, this can reduce one member from handler
 >>      structure because no need to calculate original buffer size.
 >>    - (cons) two buffers for purse consumes the kernel stack by around
 >>      2000 bytes.
 >
 > I'm afraid that this increase isn't acceptable.

Yep. It's a reason to allocate on heap, instead of usage of stack, in my
patchset.

 > The current compat code was originally with stack, too, but had to be
 > converted later to kmalloc for reducing the stack size usage.  Now the
 > new code would even double.
 >
 > Although the requirement of stack usage reduction came from 4k stack
 > and the condition was relaxed nowadays after giving up the idea, the
 > stack is still precious and we are not allowed to consume too much.
 > As a rule of thumb, maybe 100 to 200 bytes would be maximum per
 > function.

It's definitely preferable. No objections I have. In this point, it's
better to apply better refactoring to current ALSA control core because
kernel stack is used for some medium structures.

 >>   * Change some functions in 'control.c' to get 'void *' as an argument.
 >>    - (pros) these functions can get callback from 'control_compat.c'
 >>      directly. As a result, .text section can be decreased.
 >>    - (cons) these functions lose type information for the argument. This
 >>      also can fake static code analyzer, perhaps.
 >
 > This isn't fascinating, either, but more acceptable than the former.
 > (Read: let's think of other ways, but this can be still taken as a
 >   last resort.)
 >
 > BTW, if we go in that way, I guess the idea of using the conversion
 > table can be applied to 64bit native stuff, too.  Basically
 > memdup_user() and and copy_to_user() calls correspond to deserialize
 > and serialize.  Using a common helper in both sides might reduce the
 > code size, and it makes the change to void* more demanded.

Hm, OK. There might be a space to re-design my local framework for this
purpose.

 >>> There are lots of other architectures supporting 64/32 compatibility,
 >>> e.g. s390, powerpc, sparc, etc.  That's my concern.  At least these
 >>> major active architectures have to be checked (maybe MIPS, too).
 >>
 >> If you'd like me to check more architectures, I'll do it. Please send
 >> a list of architectures to investigate.
 >
 > You can grep COMPAT definition in Kconfig in arch/*.  mips and tile
 > seem to have the compat layer, too.

$ find arch -name Kconfig | xargs grep -l COMPAT
arch/parisc/Kconfig
arch/arm/Kconfig
arch/mips/Kconfig
arch/arm64/Kconfig
arch/Kconfig
arch/s390/Kconfig
arch/sparc/Kconfig
arch/x86/Kconfig
arch/powerpc/Kconfig
arch/tile/Kconfig

One of my friend who is a debian user uses Tile-Gx machine, but I've
never used it. Far from it, I've never used most of the architectures
actually...

 >> But in practical points, I'll
 >> omit s390, sparc and sparc64 because nowadays we have no products
 >> based on these architectures for consumer use. At least, in data
 >> center, sound devices are needless.
 >
 > Don't underestimate it.  There are always crazy users, and the sound
 > usage isn't always tied with the real hardware but also with a VM or
 > data processing.  (And also there are still desktop users of SPARC
 > boxen :)

(The compat layer is for 64 bit machine. I cannot imagine sparc64
machine for desktop use...)


Thanks

Takashi Sakamoto