[PATCH 0/8] ALSA: control: refactoring core codes

[PATCH 0/8] ALSA: control: refactoring core codes

[Takashi Sakamoto]

This patchset is for refactoring core control codes. This is also a
preparation for my further work for ALSA control functionality.

Takashi Sakamoto (8):
  ALSA: control: change type from long due to the definition of sizeof
    operator
  ALSA: control: obsolete switch statement with const value table
  ALSA: control: gathering evaluations to access
  ALSA: control: add a comment about locking values after creating
  ALSA: control: rename loop index to i
  ALSA: control: fix over 80 characters lines
  ALSA: control: rename to appropriate macro name
  ALSA: control: arrange snd_ctl_new() as a local function

 sound/core/control.c | 223 ++++++++++++++++++++++++++-------------------------
 1 file changed, 113 insertions(+), 110 deletions(-)

-- 
2.1.0

[PATCH 1/8] ALSA: control: change type from long due to the definition of sizeof operator

[Takashi Sakamoto]

The sizeof() operator returns 'unsigned' value, while it's assigned to 'long'.
And in this case, the value is not so large.

This commit change the type of assigned value to 'unsigned int'. Additonally,
rename local variable assigned to the value.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 35324a8..ea49abc 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1007,7 +1007,7 @@ struct user_element {
 	struct snd_ctl_elem_info info;
 	struct snd_card *card;
 	void *elem_data;		/* element data */
-	unsigned long elem_data_size;	/* size of element data in bytes */
+	unsigned int elem_data_size;	/* size of element data in bytes */
 	void *tlv_data;			/* TLV data */
 	unsigned long tlv_data_size;	/* TLV data size */
 	void *priv_data;		/* private data (like strings for enumerated type) */
@@ -1164,7 +1164,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	struct snd_card *card = file->card;
 	struct snd_kcontrol kctl, *_kctl;
 	unsigned int access;
-	long private_size;
+	unsigned int elem_data_size;
 	struct user_element *ue;
 	int idx, err;
 
@@ -1204,42 +1204,42 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	switch (info->type) {
 	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
 	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		private_size = sizeof(long);
+		elem_data_size = sizeof(long);
 		if (info->count > 128)
 			return -EINVAL;
 		break;
 	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		private_size = sizeof(long long);
+		elem_data_size = sizeof(long long);
 		if (info->count > 64)
 			return -EINVAL;
 		break;
 	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		private_size = sizeof(unsigned int);
+		elem_data_size = sizeof(unsigned int);
 		if (info->count > 128 || info->value.enumerated.items == 0)
 			return -EINVAL;
 		break;
 	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		private_size = sizeof(unsigned char);
+		elem_data_size = sizeof(unsigned char);
 		if (info->count > 512)
 			return -EINVAL;
 		break;
 	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		private_size = sizeof(struct snd_aes_iec958);
+		elem_data_size = sizeof(struct snd_aes_iec958);
 		if (info->count != 1)
 			return -EINVAL;
 		break;
 	default:
 		return -EINVAL;
 	}
-	private_size *= info->count;
-	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
+	elem_data_size *= info->count;
+	ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL);
 	if (ue == NULL)
 		return -ENOMEM;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
 	ue->elem_data = (char *)ue + sizeof(*ue);
-	ue->elem_data_size = private_size;
+	ue->elem_data_size = elem_data_size;
 	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
 		err = snd_ctl_elem_init_enum_names(ue);
 		if (err < 0) {
-- 
2.1.0

[PATCH 2/8] ALSA: control: obsolete switch statement with const value table

[Takashi Sakamoto]

To check parameters from userspace, snd_ctl_elem_add() has switch
statement. This statement checks the number of values in a control and
the size of each value. These two parameters are limited by
struct snd_ctl_elem_value.value and can be replaced with two sets of two
dimension array with constant values, without any calculation.

This commit obsolete the switch statement with the array.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index ea49abc..f248bde 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1161,6 +1161,23 @@ 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)
 {
+	/* The capacity of struct snd_ctl_elem_value.value.*/
+	const unsigned int value_sizes[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= sizeof(unsigned char),
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= sizeof(struct snd_aes_iec958),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
+	};
+	const unsigned int max_value_counts[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= 512,
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= 1,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
+	};
 	struct snd_card *card = file->card;
 	struct snd_kcontrol kctl, *_kctl;
 	unsigned int access;
@@ -1170,6 +1187,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 
 	if (info->count < 1)
 		return -EINVAL;
+
 	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
 		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
 				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
@@ -1201,37 +1219,16 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		kctl.tlv.c = snd_ctl_elem_user_tlv;
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 	}
-	switch (info->type) {
-	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		elem_data_size = sizeof(long);
-		if (info->count > 128)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		elem_data_size = sizeof(long long);
-		if (info->count > 64)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		elem_data_size = sizeof(unsigned int);
-		if (info->count > 128 || info->value.enumerated.items == 0)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		elem_data_size = sizeof(unsigned char);
-		if (info->count > 512)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		elem_data_size = sizeof(struct snd_aes_iec958);
-		if (info->count != 1)
-			return -EINVAL;
-		break;
-	default:
+
+	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
 		return -EINVAL;
-	}
-	elem_data_size *= info->count;
+	if (info->count > max_value_counts[info->type])
+		return -EINVAL;
+	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED &&
+	    info->value.enumerated.items == 0)
+		return -EINVAL;
+	elem_data_size = value_sizes[info->type] * info->count;
 	ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL);
 	if (ue == NULL)
 		return -ENOMEM;
-- 
2.1.0

[PATCH 3/8] ALSA: control: gathering evaluations to access

[Takashi Sakamoto]

The confirmation of access parameter is spread in a function.

This commit gathers these in one block for easy reading.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index f248bde..0baff92 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1188,10 +1188,16 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (info->count < 1)
 		return -EINVAL;
 
-	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				 SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE));
+	access = info->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		  SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		  SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE;
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
+		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
+	access |= SNDRV_CTL_ELEM_ACCESS_USER;
+
 	info->id.numid = 0;
 	memset(&kctl, 0, sizeof(kctl));
 
@@ -1206,7 +1212,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 
 	memcpy(&kctl.id, &info->id, sizeof(info->id));
 	kctl.count = info->owner ? info->owner : 1;
-	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
 		kctl.info = snd_ctl_elem_user_enum_info;
 	else
@@ -1215,10 +1220,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		kctl.get = snd_ctl_elem_user_get;
 	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
 		kctl.put = snd_ctl_elem_user_put;
-	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) {
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
 		kctl.tlv.c = snd_ctl_elem_user_tlv;
-		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
-	}
 
 	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
 	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
-- 
2.1.0

[PATCH 4/8] ALSA: control: add a comment about locking values after creating

[Takashi Sakamoto]

All of created user controls are locked before added to system. This may
be against programmers or users expectation.

This commit adds a comment about this.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 0baff92..19f9f4e 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1255,8 +1255,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		return -ENOMEM;
 	}
 	_kctl->private_data = ue;
+
+	/* Lock values in this user controls. */
 	for (idx = 0; idx < _kctl->count; idx++)
 		_kctl->vd[idx].owner = file;
+
 	err = snd_ctl_add(card, _kctl);
 	if (err < 0)
 		return err;
-- 
2.1.0

[PATCH 5/8] ALSA: control: rename loop index to i

[Takashi Sakamoto]

This is a fashion and subtle issue, but can reduce characters in a file.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 19f9f4e..15af804 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
 	struct snd_card *card;
 	struct snd_ctl_file *ctl;
 	struct snd_kcontrol *control;
-	unsigned int idx;
+	unsigned int i;
 
 	ctl = file->private_data;
 	file->private_data = NULL;
@@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
 	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
 	down_write(&card->controls_rwsem);
 	list_for_each_entry(control, &card->controls, list)
-		for (idx = 0; idx < control->count; idx++)
-			if (control->vd[idx].owner == ctl)
-				control->vd[idx].owner = NULL;
+		for (i = 0; i < control->count; i++)
+			if (control->vd[i].owner == ctl)
+				control->vd[i].owner = NULL;
 	up_write(&card->controls_rwsem);
 	snd_ctl_empty_read_queue(ctl);
 	put_pid(ctl->pid);
@@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 					unsigned int access)
 {
 	struct snd_kcontrol *kctl;
-	unsigned int idx;
+	unsigned int i;
 	
 	if (snd_BUG_ON(!control || !control->count))
 		return NULL;
@@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 		return NULL;
 	}
 	*kctl = *control;
-	for (idx = 0; idx < kctl->count; idx++)
-		kctl->vd[idx].access = access;
+	for (i = 0; i < kctl->count; i++)
+		kctl->vd[i].access = access;
 	return kctl;
 }
 
@@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
 int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 {
 	struct snd_ctl_elem_id id;
-	unsigned int idx;
+	unsigned int i;
 	unsigned int count;
 	int err = -EINVAL;
 
@@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 	id = kcontrol->id;
 	count = kcontrol->count;
 	up_write(&card->controls_rwsem);
-	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
+	for (i = 0; i < count; i++, id.index++, id.numid++)
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
 	return 0;
 
@@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol,
 {
 	struct snd_ctl_elem_id id;
 	unsigned int count;
-	unsigned int idx;
+	unsigned int i;
 	struct snd_kcontrol *old;
 	int ret;
 
@@ -443,7 +443,7 @@ add:
 	id = kcontrol->id;
 	count = kcontrol->count;
 	up_write(&card->controls_rwsem);
-	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
+	for (i = 0; i < count; i++, id.index++, id.numid++)
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
 	return 0;
 
@@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace);
 int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol)
 {
 	struct snd_ctl_elem_id id;
-	unsigned int idx;
+	unsigned int i;
 
 	if (snd_BUG_ON(!card || !kcontrol))
 		return -EINVAL;
 	list_del(&kcontrol->list);
 	card->controls_count -= kcontrol->count;
 	id = kcontrol->id;
-	for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
+	for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++)
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id);
 	snd_ctl_free_one(kcontrol);
 	return 0;
@@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 {
 	struct snd_card *card = file->card;
 	struct snd_kcontrol *kctl;
-	int idx, ret;
+	int i, ret;
 
 	down_write(&card->controls_rwsem);
 	kctl = snd_ctl_find_id(card, id);
@@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 		ret = -EINVAL;
 		goto error;
 	}
-	for (idx = 0; idx < kctl->count; idx++)
-		if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) {
+	for (i = 0; i < kctl->count; i++)
+		if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) {
 			ret = -EBUSY;
 			goto error;
 		}
@@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card,
 	struct snd_ctl_elem_list list;
 	struct snd_kcontrol *kctl;
 	struct snd_ctl_elem_id *dst, *id;
-	unsigned int offset, space, jidx;
+	unsigned int offset, space, i;
 	
 	if (copy_from_user(&list, _list, sizeof(list)))
 		return -EFAULT;
@@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card,
 		id = dst;
 		while (space > 0 && plist != &card->controls) {
 			kctl = snd_kcontrol(plist);
-			for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) {
-				snd_ctl_build_ioff(id, kctl, jidx);
+			for (i = offset; space > 0 && i < kctl->count; i++) {
+				snd_ctl_build_ioff(id, kctl, i);
 				id++;
 				space--;
 				list.used++;
@@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	unsigned int access;
 	unsigned int elem_data_size;
 	struct user_element *ue;
-	int idx, err;
+	int i, err;
 
 	if (info->count < 1)
 		return -EINVAL;
@@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	_kctl->private_data = ue;
 
 	/* Lock values in this user controls. */
-	for (idx = 0; idx < _kctl->count; idx++)
-		_kctl->vd[idx].owner = file;
+	for (i = 0; i < _kctl->count; i++)
+		_kctl->vd[i].owner = file;
 
 	err = snd_ctl_add(card, _kctl);
 	if (err < 0)
-- 
2.1.0

[PATCH 6/8] ALSA: control: fix over 80 characters lines

[Takashi Sakamoto]

This commit arranges lines over 80 characters, just related to this
patchset.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 15af804..c18ac04 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -205,6 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 					unsigned int access)
 {
 	struct snd_kcontrol *kctl;
+	unsigned int size;
 	unsigned int i;
 	
 	if (snd_BUG_ON(!control || !control->count))
@@ -213,7 +214,9 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 	if (control->count > MAX_CONTROL_COUNT)
 		return NULL;
 
-	kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL);
+	size  = sizeof(*kctl);
+	size += sizeof(struct snd_kcontrol_volatile) * control->count;
+	kctl = kzalloc(size, GFP_KERNEL);
 	if (kctl == NULL) {
 		pr_err("ALSA: Cannot allocate control instance\n");
 		return NULL;
@@ -314,11 +317,12 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
 	unsigned int iter = 100000;
 
 	while (snd_ctl_remove_numid_conflict(card, count)) {
-		if (--iter == 0) {
-			/* this situation is very unlikely */
-			dev_err(card->dev, "unable to allocate new control numid\n");
-			return -ENOMEM;
-		}
+		if (--iter != 0)
+			continue;
+
+		/* this situation is very unlikely */
+		dev_err(card->dev, "unable to allocate new control numid\n");
+		return -ENOMEM;
 	}
 	return 0;
 }
@@ -355,12 +359,9 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 	down_write(&card->controls_rwsem);
 	if (snd_ctl_find_id(card, &id)) {
 		up_write(&card->controls_rwsem);
-		dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n",
-					id.iface,
-					id.device,
-					id.subdevice,
-					id.name,
-					id.index);
+		dev_err(card->dev,
+			"control %i:%i:%i:%s:%i is already present\n",
+			id.iface, id.device, id.subdevice, id.name, id.index);
 		err = -EBUSY;
 		goto error;
 	}
@@ -638,14 +639,16 @@ EXPORT_SYMBOL(snd_ctl_rename_id);
  * Return: The pointer of the instance if found, or %NULL if not.
  *
  */
-struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numid)
+struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card,
+					unsigned int numid)
 {
 	struct snd_kcontrol *kctl;
 
 	if (snd_BUG_ON(!card || !numid))
 		return NULL;
 	list_for_each_entry(kctl, &card->controls, list) {
-		if (kctl->id.numid <= numid && kctl->id.numid + kctl->count > numid)
+		if (kctl->id.numid <= numid &&
+		    kctl->id.numid + kctl->count > numid)
 			return kctl;
 	}
 	return NULL;
@@ -1010,7 +1013,8 @@ struct user_element {
 	unsigned int elem_data_size;	/* size of element data in bytes */
 	void *tlv_data;			/* TLV data */
 	unsigned long tlv_data_size;	/* TLV data size */
-	void *priv_data;		/* private data (like strings for enumerated type) */
+	/* private data (like strings for enumerated type) */
+	void *priv_data;
 };
 
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
@@ -1062,7 +1066,7 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 	struct user_element *ue = kcontrol->private_data;
 
 	mutex_lock(&ue->card->user_ctl_lock);
-	change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;
+	change = !!memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size);
 	if (change)
 		memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
 	mutex_unlock(&ue->card->user_ctl_lock);
@@ -1272,7 +1276,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 }
 
 static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
-				 struct snd_ctl_elem_info __user *_info, int replace)
+				 struct snd_ctl_elem_info __user *_info,
+				 int replace)
 {
 	struct snd_ctl_elem_info info;
 	if (copy_from_user(&info, _info, sizeof(info)))
@@ -1373,7 +1378,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 	return err;
 }
 
-static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long snd_ctl_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
 {
 	struct snd_ctl_file *ctl;
 	struct snd_card *card;
@@ -1518,7 +1524,8 @@ static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
  * register the device-specific control-ioctls.
  * called from each device manager like pcm.c, hwdep.c, etc.
  */
-static int _snd_ctl_register_ioctl(snd_kctl_ioctl_func_t fcn, struct list_head *lists)
+static int _snd_ctl_register_ioctl(snd_kctl_ioctl_func_t fcn,
+				   struct list_head *lists)
 {
 	struct snd_kctl_ioctl *pn;
 
@@ -1793,6 +1800,8 @@ EXPORT_SYMBOL(snd_ctl_boolean_stereo_info);
 int snd_ctl_enum_info(struct snd_ctl_elem_info *info, unsigned int channels,
 		      unsigned int items, const char *const names[])
 {
+	const char *name;
+
 	info->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	info->count = channels;
 	info->value.enumerated.items = items;
@@ -1800,11 +1809,12 @@ int snd_ctl_enum_info(struct snd_ctl_elem_info *info, unsigned int channels,
 		return 0;
 	if (info->value.enumerated.item >= items)
 		info->value.enumerated.item = items - 1;
-	WARN(strlen(names[info->value.enumerated.item]) >= sizeof(info->value.enumerated.name),
-	     "ALSA: too long item name '%s'\n",
-	     names[info->value.enumerated.item]);
-	strlcpy(info->value.enumerated.name,
-		names[info->value.enumerated.item],
+
+	name = names[info->value.enumerated.item];
+	WARN(strlen(name) >= sizeof(info->value.enumerated.name),
+	     "ALSA: too long item name '%s'\n", name);
+
+	strlcpy(info->value.enumerated.name, name,
 		sizeof(info->value.enumerated.name));
 	return 0;
 }
-- 
2.1.0

[PATCH 7/8] ALSA: control: rename to appropriate macro name

[Takashi Sakamoto]

MAX_CONTROL_COUNT is used for the maximum number of values included in
a control, therefore should be rename to mean it.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index c18ac04..04534f3 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -30,9 +30,11 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
+/* The maximum number of values which one control has. */
+#define MAX_VALUES_COUNT	1028
+
 /* max number of user-defined controls */
 #define MAX_USER_CONTROLS	32
-#define MAX_CONTROL_COUNT	1028
 
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
@@ -211,7 +213,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 	if (snd_BUG_ON(!control || !control->count))
 		return NULL;
 
-	if (control->count > MAX_CONTROL_COUNT)
+	if (control->count > MAX_VALUES_COUNT)
 		return NULL;
 
 	size  = sizeof(*kctl);
-- 
2.1.0

[PATCH 8/8] ALSA: control: arrange snd_ctl_new() as a local function

[Takashi Sakamoto]

The snd_ctl_new1() was added in 2001. I guess that snd_ctl_new() becames
a local function in this timing.

This commit arranges the function as a local helper function, remove
'snd_' prefix and comments, change comments to refer to the function.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 04534f3..af95783 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -193,18 +193,8 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask,
 }
 EXPORT_SYMBOL(snd_ctl_notify);
 
-/**
- * snd_ctl_new - create a control instance from the template
- * @control: the control template
- * @access: the default control access
- *
- * Allocates a new struct snd_kcontrol instance and copies the given template 
- * to the new instance. It does not copy volatile data (access).
- *
- * Return: The pointer of the new instance, or %NULL on failure.
- */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
-					unsigned int access)
+static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
+				    unsigned int access)
 {
 	struct snd_kcontrol *kctl;
 	unsigned int size;
@@ -273,7 +263,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 	kctl.tlv.p = ncontrol->tlv.p;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
-	return snd_ctl_new(&kctl, access);
+	return ctl_new(&kctl, access);
 }
 EXPORT_SYMBOL(snd_ctl_new1);
 
@@ -281,9 +271,8 @@ EXPORT_SYMBOL(snd_ctl_new1);
  * snd_ctl_free_one - release the control instance
  * @kcontrol: the control instance
  *
- * Releases the control instance created via snd_ctl_new()
- * or snd_ctl_new1().
- * Don't call this after the control was added to the card.
+ * Releases the control instance created via ctl_new() or snd_ctl_new1(). Don't
+ * call this after the control was added to the card.
  */
 void snd_ctl_free_one(struct snd_kcontrol *kcontrol)
 {
@@ -334,9 +323,8 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
  * @card: the card instance
  * @kcontrol: the control instance to add
  *
- * Adds the control instance created via snd_ctl_new() or
- * snd_ctl_new1() to the given card. Assigns also an unique
- * numid used for fast search.
+ * Adds the control instance created via ctl_new() or snd_ctl_new1() to the
+ * given card. Assigns also an unique numid used for fast search.
  *
  * It frees automatically the control which cannot be added.
  *
@@ -1254,7 +1242,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		}
 	}
 	kctl.private_free = snd_ctl_elem_user_free;
-	_kctl = snd_ctl_new(&kctl, access);
+	_kctl = ctl_new(&kctl, access);
 	if (_kctl == NULL) {
 		kfree(ue->priv_data);
 		kfree(ue);
-- 
2.1.0

Re: [PATCH 2/8] ALSA: control: obsolete switch statement with const value table

[Lars-Peter Clausen]

On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
> To check parameters from userspace, snd_ctl_elem_add() has switch
> statement. This statement checks the number of values in a control and
> the size of each value. These two parameters are limited by
> struct snd_ctl_elem_value.value and can be replaced with two sets of two
> dimension array with constant values, without any calculation.
>
> This commit obsolete the switch statement with the array.
>
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>   sound/core/control.c | 57 +++++++++++++++++++++++++---------------------------
>   1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/sound/core/control.c b/sound/core/control.c
> index ea49abc..f248bde 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1161,6 +1161,23 @@ 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)
>   {
> +	/* The capacity of struct snd_ctl_elem_value.value.*/
> +	const unsigned int value_sizes[] = {
> +		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= sizeof(long),
> +		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= sizeof(long),
> +		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),
> +		[SNDRV_CTL_ELEM_TYPE_BYTES]	= sizeof(unsigned char),
> +		[SNDRV_CTL_ELEM_TYPE_IEC958]	= sizeof(struct snd_aes_iec958),
> +		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
> +	};
> +	const unsigned int max_value_counts[] = {
> +		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= 128,
> +		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= 128,
> +		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,
> +		[SNDRV_CTL_ELEM_TYPE_BYTES]	= 512,
> +		[SNDRV_CTL_ELEM_TYPE_IEC958]	= 1,
> +		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
> +	};

The tables should probably be static, no need to put them on the stack.

Re: [PATCH 2/8] ALSA: control: obsolete switch statement with const value table

[Takashi Sakamoto]

Hi Lars,

On Feb 11 2015 19:51, Lars-Peter Clausen wrote:
> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
>> To check parameters from userspace, snd_ctl_elem_add() has switch
>> statement. This statement checks the number of values in a control and
>> the size of each value. These two parameters are limited by
>> struct snd_ctl_elem_value.value and can be replaced with two sets of two
>> dimension array with constant values, without any calculation.
>>
>> This commit obsolete the switch statement with the array.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>   sound/core/control.c | 57
>> +++++++++++++++++++++++++---------------------------
>>   1 file changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index ea49abc..f248bde 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -1161,6 +1161,23 @@ 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)
>>   {
>> +    /* The capacity of struct snd_ctl_elem_value.value.*/
>> +    const unsigned int value_sizes[] = {
>> +        [SNDRV_CTL_ELEM_TYPE_BOOLEAN]    = sizeof(long),
>> +        [SNDRV_CTL_ELEM_TYPE_INTEGER]    = sizeof(long),
>> +        [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),
>> +        [SNDRV_CTL_ELEM_TYPE_BYTES]    = sizeof(unsigned char),
>> +        [SNDRV_CTL_ELEM_TYPE_IEC958]    = sizeof(struct snd_aes_iec958),
>> +        [SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
>> +    };
>> +    const unsigned int max_value_counts[] = {
>> +        [SNDRV_CTL_ELEM_TYPE_BOOLEAN]    = 128,
>> +        [SNDRV_CTL_ELEM_TYPE_INTEGER]    = 128,
>> +        [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,
>> +        [SNDRV_CTL_ELEM_TYPE_BYTES]    = 512,
>> +        [SNDRV_CTL_ELEM_TYPE_IEC958]    = 1,
>> +        [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
>> +    };
> 
> The tables should probably be static, no need to put them on the stack.

Oops, exactly. I've been satisfied just to add const operator, against
my intension... Thanks for your correction.


Takashi Sakamoto

Re: [PATCH 1/8] ALSA: control: change type from long due to the definition of sizeof operator

[Takashi Iwai]

At Wed, 11 Feb 2015 19:37:25 +0900,
Takashi Sakamoto wrote:
> 
> The sizeof() operator returns 'unsigned' value, while it's assigned to 'long'.

The type of sizeof() is size_t, which is unsigned long on Linux.
(Imagine the return value of sizeof() for an array over 4GB.)


Takashi

> And in this case, the value is not so large.
>
> This commit change the type of assigned value to 'unsigned int'. Additonally,
> rename local variable assigned to the value.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 35324a8..ea49abc 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1007,7 +1007,7 @@ struct user_element {
>  	struct snd_ctl_elem_info info;
>  	struct snd_card *card;
>  	void *elem_data;		/* element data */
> -	unsigned long elem_data_size;	/* size of element data in bytes */
> +	unsigned int elem_data_size;	/* size of element data in bytes */
>  	void *tlv_data;			/* TLV data */
>  	unsigned long tlv_data_size;	/* TLV data size */
>  	void *priv_data;		/* private data (like strings for enumerated type) */
> @@ -1164,7 +1164,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	struct snd_card *card = file->card;
>  	struct snd_kcontrol kctl, *_kctl;
>  	unsigned int access;
> -	long private_size;
> +	unsigned int elem_data_size;
>  	struct user_element *ue;
>  	int idx, err;
>  
> @@ -1204,42 +1204,42 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	switch (info->type) {
>  	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
>  	case SNDRV_CTL_ELEM_TYPE_INTEGER:
> -		private_size = sizeof(long);
> +		elem_data_size = sizeof(long);
>  		if (info->count > 128)
>  			return -EINVAL;
>  		break;
>  	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
> -		private_size = sizeof(long long);
> +		elem_data_size = sizeof(long long);
>  		if (info->count > 64)
>  			return -EINVAL;
>  		break;
>  	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
> -		private_size = sizeof(unsigned int);
> +		elem_data_size = sizeof(unsigned int);
>  		if (info->count > 128 || info->value.enumerated.items == 0)
>  			return -EINVAL;
>  		break;
>  	case SNDRV_CTL_ELEM_TYPE_BYTES:
> -		private_size = sizeof(unsigned char);
> +		elem_data_size = sizeof(unsigned char);
>  		if (info->count > 512)
>  			return -EINVAL;
>  		break;
>  	case SNDRV_CTL_ELEM_TYPE_IEC958:
> -		private_size = sizeof(struct snd_aes_iec958);
> +		elem_data_size = sizeof(struct snd_aes_iec958);
>  		if (info->count != 1)
>  			return -EINVAL;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
> -	private_size *= info->count;
> -	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
> +	elem_data_size *= info->count;
> +	ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL);
>  	if (ue == NULL)
>  		return -ENOMEM;
>  	ue->card = card;
>  	ue->info = *info;
>  	ue->info.access = 0;
>  	ue->elem_data = (char *)ue + sizeof(*ue);
> -	ue->elem_data_size = private_size;
> +	ue->elem_data_size = elem_data_size;
>  	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
>  		err = snd_ctl_elem_init_enum_names(ue);
>  		if (err < 0) {
> -- 
> 2.1.0
> 

Re: [PATCH 5/8] ALSA: control: rename loop index to i

[Takashi Iwai]

At Wed, 11 Feb 2015 19:37:29 +0900,
Takashi Sakamoto wrote:
> 
> This is a fashion and subtle issue, but can reduce characters in a file.

Sorry, I see no merit by this action.  The idx is even clearer than
using i for these purposes.  And, if it's i, people expect it being
int, not unsigned.


Takashi

> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 19f9f4e..15af804 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
>  	struct snd_card *card;
>  	struct snd_ctl_file *ctl;
>  	struct snd_kcontrol *control;
> -	unsigned int idx;
> +	unsigned int i;
>  
>  	ctl = file->private_data;
>  	file->private_data = NULL;
> @@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
>  	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
>  	down_write(&card->controls_rwsem);
>  	list_for_each_entry(control, &card->controls, list)
> -		for (idx = 0; idx < control->count; idx++)
> -			if (control->vd[idx].owner == ctl)
> -				control->vd[idx].owner = NULL;
> +		for (i = 0; i < control->count; i++)
> +			if (control->vd[i].owner == ctl)
> +				control->vd[i].owner = NULL;
>  	up_write(&card->controls_rwsem);
>  	snd_ctl_empty_read_queue(ctl);
>  	put_pid(ctl->pid);
> @@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
>  					unsigned int access)
>  {
>  	struct snd_kcontrol *kctl;
> -	unsigned int idx;
> +	unsigned int i;
>  	
>  	if (snd_BUG_ON(!control || !control->count))
>  		return NULL;
> @@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
>  		return NULL;
>  	}
>  	*kctl = *control;
> -	for (idx = 0; idx < kctl->count; idx++)
> -		kctl->vd[idx].access = access;
> +	for (i = 0; i < kctl->count; i++)
> +		kctl->vd[i].access = access;
>  	return kctl;
>  }
>  
> @@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
>  int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>  {
>  	struct snd_ctl_elem_id id;
> -	unsigned int idx;
> +	unsigned int i;
>  	unsigned int count;
>  	int err = -EINVAL;
>  
> @@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>  	id = kcontrol->id;
>  	count = kcontrol->count;
>  	up_write(&card->controls_rwsem);
> -	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
> +	for (i = 0; i < count; i++, id.index++, id.numid++)
>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
>  	return 0;
>  
> @@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol,
>  {
>  	struct snd_ctl_elem_id id;
>  	unsigned int count;
> -	unsigned int idx;
> +	unsigned int i;
>  	struct snd_kcontrol *old;
>  	int ret;
>  
> @@ -443,7 +443,7 @@ add:
>  	id = kcontrol->id;
>  	count = kcontrol->count;
>  	up_write(&card->controls_rwsem);
> -	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
> +	for (i = 0; i < count; i++, id.index++, id.numid++)
>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
>  	return 0;
>  
> @@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace);
>  int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol)
>  {
>  	struct snd_ctl_elem_id id;
> -	unsigned int idx;
> +	unsigned int i;
>  
>  	if (snd_BUG_ON(!card || !kcontrol))
>  		return -EINVAL;
>  	list_del(&kcontrol->list);
>  	card->controls_count -= kcontrol->count;
>  	id = kcontrol->id;
> -	for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
> +	for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++)
>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id);
>  	snd_ctl_free_one(kcontrol);
>  	return 0;
> @@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>  {
>  	struct snd_card *card = file->card;
>  	struct snd_kcontrol *kctl;
> -	int idx, ret;
> +	int i, ret;
>  
>  	down_write(&card->controls_rwsem);
>  	kctl = snd_ctl_find_id(card, id);
> @@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>  		ret = -EINVAL;
>  		goto error;
>  	}
> -	for (idx = 0; idx < kctl->count; idx++)
> -		if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) {
> +	for (i = 0; i < kctl->count; i++)
> +		if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) {
>  			ret = -EBUSY;
>  			goto error;
>  		}
> @@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card,
>  	struct snd_ctl_elem_list list;
>  	struct snd_kcontrol *kctl;
>  	struct snd_ctl_elem_id *dst, *id;
> -	unsigned int offset, space, jidx;
> +	unsigned int offset, space, i;
>  	
>  	if (copy_from_user(&list, _list, sizeof(list)))
>  		return -EFAULT;
> @@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card,
>  		id = dst;
>  		while (space > 0 && plist != &card->controls) {
>  			kctl = snd_kcontrol(plist);
> -			for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) {
> -				snd_ctl_build_ioff(id, kctl, jidx);
> +			for (i = offset; space > 0 && i < kctl->count; i++) {
> +				snd_ctl_build_ioff(id, kctl, i);
>  				id++;
>  				space--;
>  				list.used++;
> @@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	unsigned int access;
>  	unsigned int elem_data_size;
>  	struct user_element *ue;
> -	int idx, err;
> +	int i, err;
>  
>  	if (info->count < 1)
>  		return -EINVAL;
> @@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	_kctl->private_data = ue;
>  
>  	/* Lock values in this user controls. */
> -	for (idx = 0; idx < _kctl->count; idx++)
> -		_kctl->vd[idx].owner = file;
> +	for (i = 0; i < _kctl->count; i++)
> +		_kctl->vd[i].owner = file;
>  
>  	err = snd_ctl_add(card, _kctl);
>  	if (err < 0)
> -- 
> 2.1.0
> 

Re: [PATCH 6/8] ALSA: control: fix over 80 characters lines

[Takashi Iwai]

At Wed, 11 Feb 2015 19:37:30 +0900,
Takashi Sakamoto wrote:
> @@ -1010,7 +1013,8 @@ struct user_element {
>  	unsigned int elem_data_size;	/* size of element data in bytes */
>  	void *tlv_data;			/* TLV data */
>  	unsigned long tlv_data_size;	/* TLV data size */
> -	void *priv_data;		/* private data (like strings for enumerated type) */
> +	/* private data (like strings for enumerated type) */
> +	void *priv_data;

Better to put the comment in the same line like other fields.


Takashi

Re: [PATCH 8/8] ALSA: control: arrange snd_ctl_new() as a local function

[Lars-Peter Clausen]

On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
[...]
> -/**
> - * snd_ctl_new - create a control instance from the template
> - * @control: the control template
> - * @access: the default control access
> - *
> - * Allocates a new struct snd_kcontrol instance and copies the given template
> - * to the new instance. It does not copy volatile data (access).
> - *
> - * Return: The pointer of the new instance, or %NULL on failure.
> - */
> -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
> -					unsigned int access)
> +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,

I'd prefer to keep both the documentation as well as the 'snd_' prefix. 
Maybe just replace the '/**' with '/*' to prevent it from appearing in the 
public API documentation.

- Lars

Re: [PATCH 8/8] ALSA: control: arrange snd_ctl_new() as a local function

[Takashi Iwai]

At Wed, 11 Feb 2015 13:49:29 +0100,
Lars-Peter Clausen wrote:
> 
> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
> [...]
> > -/**
> > - * snd_ctl_new - create a control instance from the template
> > - * @control: the control template
> > - * @access: the default control access
> > - *
> > - * Allocates a new struct snd_kcontrol instance and copies the given template
> > - * to the new instance. It does not copy volatile data (access).
> > - *
> > - * Return: The pointer of the new instance, or %NULL on failure.
> > - */
> > -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
> > -					unsigned int access)
> > +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
> 
> I'd prefer to keep both the documentation as well as the 'snd_' prefix. 
> Maybe just replace the '/**' with '/*' to prevent it from appearing in the 
> public API documentation.

Yes, agreed.  There is no strict rule about snd_ prefix for
non-exported objects.  We prefer not having snd_ prefix for local
stuff in general just for readability.  But in this case, it doesn't
improve so much.


Takashi

Re: [PATCH 0/8] ALSA: control: refactoring core codes

[Takashi Iwai]

At Wed, 11 Feb 2015 19:37:24 +0900,
Takashi Sakamoto wrote:
> 
> This patchset is for refactoring core control codes. This is also a
> preparation for my further work for ALSA control functionality.

As now is already after the 3.20 merge window open, basically I'd take
only fix patches now and postpone the rest for 3.21.  So, please pick
up (and revise) the fix patches that are really worth for 3.20 and
resubmit them.  Submit the rest as another patchset to be 
distinguished from the former.


thanks,

Takashi

> 
> Takashi Sakamoto (8):
>   ALSA: control: change type from long due to the definition of sizeof
>     operator
>   ALSA: control: obsolete switch statement with const value table
>   ALSA: control: gathering evaluations to access
>   ALSA: control: add a comment about locking values after creating
>   ALSA: control: rename loop index to i
>   ALSA: control: fix over 80 characters lines
>   ALSA: control: rename to appropriate macro name
>   ALSA: control: arrange snd_ctl_new() as a local function
> 
>  sound/core/control.c | 223 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 113 insertions(+), 110 deletions(-)
> 
> -- 
> 2.1.0
> 

Re: [PATCH 0/8] ALSA: control: refactoring core codes

[Takashi Sakamoto]

On Feb 11 2015 22:19, Takashi Iwai wrote:
> At Wed, 11 Feb 2015 19:37:24 +0900,
> Takashi Sakamoto wrote:
>>
>> This patchset is for refactoring core control codes. This is also a
>> preparation for my further work for ALSA control functionality.
> 
> As now is already after the 3.20 merge window open, basically I'd take
> only fix patches now and postpone the rest for 3.21.

I'm posting them for 3.21, not for current 3.20. I should add some
comments about this, sorry to trouble you.

> So, please pick up (and revise) the fix patches that are really worth
> for 3.20 and resubmit them.  Submit the rest as another patchset to be 
> distinguished from the former.

Nothing for 3.20. I'll wait for closing the merge window, then post the
revised patches, but hope to continue discussion.


Regards

Takashi Sakamoto

>> Takashi Sakamoto (8):
>>   ALSA: control: change type from long due to the definition of sizeof
>>     operator
>>   ALSA: control: obsolete switch statement with const value table
>>   ALSA: control: gathering evaluations to access
>>   ALSA: control: add a comment about locking values after creating
>>   ALSA: control: rename loop index to i
>>   ALSA: control: fix over 80 characters lines
>>   ALSA: control: rename to appropriate macro name
>>   ALSA: control: arrange snd_ctl_new() as a local function
>>
>>  sound/core/control.c | 223 ++++++++++++++++++++++++++-------------------------
>>  1 file changed, 113 insertions(+), 110 deletions(-)
>>
>> -- 
>> 2.1.0

Re: [PATCH 8/8] ALSA: control: arrange snd_ctl_new() as a local function

[Takashi Sakamoto]

On Feb 11 2015 22:04, Takashi Iwai wrote:
> At Wed, 11 Feb 2015 13:49:29 +0100,
> Lars-Peter Clausen wrote:
>>
>> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
>> [...]
>>> -/**
>>> - * snd_ctl_new - create a control instance from the template
>>> - * @control: the control template
>>> - * @access: the default control access
>>> - *
>>> - * Allocates a new struct snd_kcontrol instance and copies the given template
>>> - * to the new instance. It does not copy volatile data (access).
>>> - *
>>> - * Return: The pointer of the new instance, or %NULL on failure.
>>> - */
>>> -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
>>> -					unsigned int access)
>>> +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
>>
>> I'd prefer to keep both the documentation as well as the 'snd_' prefix. 
>> Maybe just replace the '/**' with '/*' to prevent it from appearing in the 
>> public API documentation.
> 
> Yes, agreed.  There is no strict rule about snd_ prefix for
> non-exported objects.  We prefer not having snd_ prefix for local
> stuff in general just for readability.  But in this case, it doesn't
> improve so much.

Hm. I don't disagree with remaining these comments.

...But I have another issue. In my final patch in another series, I
change this function drastically. Then, should I spend a bit time to
revise them even if this is just a local function?
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/087796.html


Thanks

Takashi Sakamoto

Re: [PATCH 6/8] ALSA: control: fix over 80 characters lines

[Takashi Sakamoto]

On Feb 11 2015 21:01, Takashi Iwai wrote:
> At Wed, 11 Feb 2015 19:37:30 +0900,
> Takashi Sakamoto wrote:
>> @@ -1010,7 +1013,8 @@ struct user_element {
>>  	unsigned int elem_data_size;	/* size of element data in bytes */
>>  	void *tlv_data;			/* TLV data */
>>  	unsigned long tlv_data_size;	/* TLV data size */
>> -	void *priv_data;		/* private data (like strings for enumerated type) */
>> +	/* private data (like strings for enumerated type) */
>> +	void *priv_data;
> 
> Better to put the comment in the same line like other fields.

Which do you mean, we allow this line over 80 characters (keep what it
was), or I should revise this comment within 80 characters considering
the authours idea?


Regards

Takashi Sakamoto

Re: [PATCH 5/8] ALSA: control: rename loop index to i

[Takashi Sakamoto]

On Feb 11 2015 20:55, Takashi Iwai wrote:
> At Wed, 11 Feb 2015 19:37:29 +0900,
> Takashi Sakamoto wrote:
>>
>> This is a fashion and subtle issue, but can reduce characters in a file.
> 
> Sorry, I see no merit by this action.  The idx is even clearer than
> using i for these purposes.  And, if it's i, people expect it being
> int, not unsigned.

OK. I drop this patch.

Thanks

Takashi Sakamoto

>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index 19f9f4e..15af804 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
>>  	struct snd_card *card;
>>  	struct snd_ctl_file *ctl;
>>  	struct snd_kcontrol *control;
>> -	unsigned int idx;
>> +	unsigned int i;
>>  
>>  	ctl = file->private_data;
>>  	file->private_data = NULL;
>> @@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
>>  	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
>>  	down_write(&card->controls_rwsem);
>>  	list_for_each_entry(control, &card->controls, list)
>> -		for (idx = 0; idx < control->count; idx++)
>> -			if (control->vd[idx].owner == ctl)
>> -				control->vd[idx].owner = NULL;
>> +		for (i = 0; i < control->count; i++)
>> +			if (control->vd[i].owner == ctl)
>> +				control->vd[i].owner = NULL;
>>  	up_write(&card->controls_rwsem);
>>  	snd_ctl_empty_read_queue(ctl);
>>  	put_pid(ctl->pid);
>> @@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
>>  					unsigned int access)
>>  {
>>  	struct snd_kcontrol *kctl;
>> -	unsigned int idx;
>> +	unsigned int i;
>>  	
>>  	if (snd_BUG_ON(!control || !control->count))
>>  		return NULL;
>> @@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
>>  		return NULL;
>>  	}
>>  	*kctl = *control;
>> -	for (idx = 0; idx < kctl->count; idx++)
>> -		kctl->vd[idx].access = access;
>> +	for (i = 0; i < kctl->count; i++)
>> +		kctl->vd[i].access = access;
>>  	return kctl;
>>  }
>>  
>> @@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
>>  int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>>  {
>>  	struct snd_ctl_elem_id id;
>> -	unsigned int idx;
>> +	unsigned int i;
>>  	unsigned int count;
>>  	int err = -EINVAL;
>>  
>> @@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>>  	id = kcontrol->id;
>>  	count = kcontrol->count;
>>  	up_write(&card->controls_rwsem);
>> -	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
>> +	for (i = 0; i < count; i++, id.index++, id.numid++)
>>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
>>  	return 0;
>>  
>> @@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol,
>>  {
>>  	struct snd_ctl_elem_id id;
>>  	unsigned int count;
>> -	unsigned int idx;
>> +	unsigned int i;
>>  	struct snd_kcontrol *old;
>>  	int ret;
>>  
>> @@ -443,7 +443,7 @@ add:
>>  	id = kcontrol->id;
>>  	count = kcontrol->count;
>>  	up_write(&card->controls_rwsem);
>> -	for (idx = 0; idx < count; idx++, id.index++, id.numid++)
>> +	for (i = 0; i < count; i++, id.index++, id.numid++)
>>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
>>  	return 0;
>>  
>> @@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace);
>>  int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol)
>>  {
>>  	struct snd_ctl_elem_id id;
>> -	unsigned int idx;
>> +	unsigned int i;
>>  
>>  	if (snd_BUG_ON(!card || !kcontrol))
>>  		return -EINVAL;
>>  	list_del(&kcontrol->list);
>>  	card->controls_count -= kcontrol->count;
>>  	id = kcontrol->id;
>> -	for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
>> +	for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++)
>>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id);
>>  	snd_ctl_free_one(kcontrol);
>>  	return 0;
>> @@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>>  {
>>  	struct snd_card *card = file->card;
>>  	struct snd_kcontrol *kctl;
>> -	int idx, ret;
>> +	int i, ret;
>>  
>>  	down_write(&card->controls_rwsem);
>>  	kctl = snd_ctl_find_id(card, id);
>> @@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>>  		ret = -EINVAL;
>>  		goto error;
>>  	}
>> -	for (idx = 0; idx < kctl->count; idx++)
>> -		if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) {
>> +	for (i = 0; i < kctl->count; i++)
>> +		if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) {
>>  			ret = -EBUSY;
>>  			goto error;
>>  		}
>> @@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card,
>>  	struct snd_ctl_elem_list list;
>>  	struct snd_kcontrol *kctl;
>>  	struct snd_ctl_elem_id *dst, *id;
>> -	unsigned int offset, space, jidx;
>> +	unsigned int offset, space, i;
>>  	
>>  	if (copy_from_user(&list, _list, sizeof(list)))
>>  		return -EFAULT;
>> @@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card,
>>  		id = dst;
>>  		while (space > 0 && plist != &card->controls) {
>>  			kctl = snd_kcontrol(plist);
>> -			for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) {
>> -				snd_ctl_build_ioff(id, kctl, jidx);
>> +			for (i = offset; space > 0 && i < kctl->count; i++) {
>> +				snd_ctl_build_ioff(id, kctl, i);
>>  				id++;
>>  				space--;
>>  				list.used++;
>> @@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  	unsigned int access;
>>  	unsigned int elem_data_size;
>>  	struct user_element *ue;
>> -	int idx, err;
>> +	int i, err;
>>  
>>  	if (info->count < 1)
>>  		return -EINVAL;
>> @@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  	_kctl->private_data = ue;
>>  
>>  	/* Lock values in this user controls. */
>> -	for (idx = 0; idx < _kctl->count; idx++)
>> -		_kctl->vd[idx].owner = file;
>> +	for (i = 0; i < _kctl->count; i++)
>> +		_kctl->vd[i].owner = file;
>>  
>>  	err = snd_ctl_add(card, _kctl);
>>  	if (err < 0)
>> -- 
>> 2.1.0

Re: [PATCH 6/8] ALSA: control: fix over 80 characters lines

[Jaroslav Kysela]

Dne 12.2.2015 v 13:47 Takashi Sakamoto napsal(a):
> On Feb 11 2015 21:01, Takashi Iwai wrote:
>> At Wed, 11 Feb 2015 19:37:30 +0900,
>> Takashi Sakamoto wrote:
>>> @@ -1010,7 +1013,8 @@ struct user_element {
>>>  	unsigned int elem_data_size;	/* size of element data in bytes */
>>>  	void *tlv_data;			/* TLV data */
>>>  	unsigned long tlv_data_size;	/* TLV data size */
>>> -	void *priv_data;		/* private data (like strings for enumerated type) */
>>> +	/* private data (like strings for enumerated type) */
>>> +	void *priv_data;
>>
>> Better to put the comment in the same line like other fields.
> 
> Which do you mean, we allow this line over 80 characters (keep what it
> was), or I should revise this comment within 80 characters considering
> the authours idea?

Keep it as-is. Any change will make the readability more worse.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH 8/8] ALSA: control: arrange snd_ctl_new() as a local function

[Takashi Iwai]

At Thu, 12 Feb 2015 21:45:24 +0900,
Takashi Sakamoto wrote:
> 
> On Feb 11 2015 22:04, Takashi Iwai wrote:
> > At Wed, 11 Feb 2015 13:49:29 +0100,
> > Lars-Peter Clausen wrote:
> >>
> >> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
> >> [...]
> >>> -/**
> >>> - * snd_ctl_new - create a control instance from the template
> >>> - * @control: the control template
> >>> - * @access: the default control access
> >>> - *
> >>> - * Allocates a new struct snd_kcontrol instance and copies the given template
> >>> - * to the new instance. It does not copy volatile data (access).
> >>> - *
> >>> - * Return: The pointer of the new instance, or %NULL on failure.
> >>> - */
> >>> -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
> >>> -					unsigned int access)
> >>> +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
> >>
> >> I'd prefer to keep both the documentation as well as the 'snd_' prefix. 
> >> Maybe just replace the '/**' with '/*' to prevent it from appearing in the 
> >> public API documentation.
> > 
> > Yes, agreed.  There is no strict rule about snd_ prefix for
> > non-exported objects.  We prefer not having snd_ prefix for local
> > stuff in general just for readability.  But in this case, it doesn't
> > improve so much.
> 
> Hm. I don't disagree with remaining these comments.
> 
> ...But I have another issue. In my final patch in another series, I
> change this function drastically. Then, should I spend a bit time to
> revise them even if this is just a local function?

Yes, please.


Takashi

[PATCH 0/2][RFC] ALSA: core: optimizations for creating new controls

[Takashi Sakamoto]

This patchset is for reducing executing time and stack usage at creating
new control instances. Nothing fixes.

Takashi Sakamoto (2):
  ALSA: core: use precomputed table to check userspace control params
  ALSA: core: reduce stack usage related to snd_ctl_new()

 sound/core/control.c | 271 +++++++++++++++++++++++++++++----------------------
 1 file changed, 157 insertions(+), 114 deletions(-)

-- 
2.1.0

[PATCH 1/2] ALSA: core: use precomputed table to check userspace control params

[Takashi Sakamoto]

The parameters can be decided in compile time.

This commit adds precomputed table to reduce calculating time.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 35324a8..3955115 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1161,6 +1161,23 @@ 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)
 {
+	/* The capacity of struct snd_ctl_elem_value.value.*/
+	static const unsigned int value_sizes[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= sizeof(unsigned char),
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= sizeof(struct snd_aes_iec958),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
+	};
+	static const unsigned int max_value_counts[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= 512,
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= 1,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
+	};
 	struct snd_card *card = file->card;
 	struct snd_kcontrol kctl, *_kctl;
 	unsigned int access;
@@ -1168,8 +1185,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	struct user_element *ue;
 	int idx, err;
 
-	if (info->count < 1)
-		return -EINVAL;
 	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
 		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
 				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
@@ -1201,37 +1216,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		kctl.tlv.c = snd_ctl_elem_user_tlv;
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 	}
-	switch (info->type) {
-	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		private_size = sizeof(long);
-		if (info->count > 128)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		private_size = sizeof(long long);
-		if (info->count > 64)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		private_size = sizeof(unsigned int);
-		if (info->count > 128 || info->value.enumerated.items == 0)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		private_size = sizeof(unsigned char);
-		if (info->count > 512)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		private_size = sizeof(struct snd_aes_iec958);
-		if (info->count != 1)
-			return -EINVAL;
-		break;
-	default:
+
+	if ((info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN) ||
+	    (info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64))
 		return -EINVAL;
-	}
-	private_size *= info->count;
+	if ((info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) &&
+	    (info->value.enumerated.items == 0))
+		return -EINVAL;
+	if ((info->count < 1) ||
+	    (info->count > max_value_counts[info->type]))
+		return -EINVAL;
+
+	private_size = value_sizes[info->type] * info->count;
 	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
 	if (ue == NULL)
 		return -ENOMEM;
-- 
2.1.0

[PATCH 2/2] ALSA: core: reduce stack usage related to snd_ctl_new()

[Takashi Sakamoto]

The callers of snd_ctl_new() need to have 'struct snd_kcontrol' data,
and pass the data as template. Then, the function allocates the structure
data again and copy from the template. This is a waste of resources.
Especially, the callers use large stack for the template.

This commit removes a need of template for the function, thus, changes
the prototype of snd_ctl_new(). Furthermore, this commit changes
the code of callers, snd_ctl_new1() and snd_ctl_elem_add() for better
shape.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 3955115..1b3880a 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -192,36 +192,43 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask,
 EXPORT_SYMBOL(snd_ctl_notify);
 
 /**
- * snd_ctl_new - create a control instance from the template
- * @control: the control template
- * @access: the default control access
+ * snd_ctl_new - create a new control instance with some elements
+ * @kctl: the pointer to store new control instance
+ * @count: the number of elements in this control
+ * @access: the default access flags for elements in this control
+ * @file: given when locking these elements
  *
- * Allocates a new struct snd_kcontrol instance and copies the given template 
- * to the new instance. It does not copy volatile data (access).
+ * Allocates a memory object for a new control instance. The instance has
+ * elements as many as the given number (@count). Each element has given
+ * access permissions (@access). Each element is locked when @file is given.
  *
- * Return: The pointer of the new instance, or %NULL on failure.
+ * Return: 0 on success, error code on failure
  */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
-					unsigned int access)
+static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count,
+		       unsigned int access, struct snd_ctl_file *file)
 {
-	struct snd_kcontrol *kctl;
+	unsigned int size;
 	unsigned int idx;
 	
-	if (snd_BUG_ON(!control || !control->count))
-		return NULL;
+	if ((count == 0) || (count > MAX_CONTROL_COUNT))
+		return -EINVAL;
 
-	if (control->count > MAX_CONTROL_COUNT)
-		return NULL;
+	size  = sizeof(struct snd_kcontrol);
+	size += sizeof(struct snd_kcontrol_volatile) * count;
 
-	kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL);
-	if (kctl == NULL) {
+	*kctl = kzalloc(size, GFP_KERNEL);
+	if (*kctl == NULL) {
 		pr_err("ALSA: Cannot allocate control instance\n");
-		return NULL;
+		return -ENOMEM;
 	}
-	*kctl = *control;
-	for (idx = 0; idx < kctl->count; idx++)
-		kctl->vd[idx].access = access;
-	return kctl;
+
+	for (idx = 0; idx < count; idx++) {
+		(*kctl)->vd[idx].access = access;
+		(*kctl)->vd[idx].owner = file;
+	}
+	(*kctl)->count = count;
+
+	return 0;
 }
 
 /**
@@ -238,37 +245,53 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 				  void *private_data)
 {
-	struct snd_kcontrol kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
+	int err;
 	
 	if (snd_BUG_ON(!ncontrol || !ncontrol->info))
 		return NULL;
-	memset(&kctl, 0, sizeof(kctl));
-	kctl.id.iface = ncontrol->iface;
-	kctl.id.device = ncontrol->device;
-	kctl.id.subdevice = ncontrol->subdevice;
+
+	count = ncontrol->count;
+	if (count == 0)
+		count = 1;
+
+	access = ncontrol->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_VOLATILE |
+		   SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK);
+
+	err = snd_ctl_new(&kctl, count, access, NULL);
+	if (err < 0)
+		return NULL;
+
+	/* The 'numid' member is decided when calling snd_ctl_add(). */
+	kctl->id.iface = ncontrol->iface;
+	kctl->id.device = ncontrol->device;
+	kctl->id.subdevice = ncontrol->subdevice;
 	if (ncontrol->name) {
-		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
-		if (strcmp(ncontrol->name, kctl.id.name) != 0)
+		strlcpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name));
+		if (strcmp(ncontrol->name, kctl->id.name) != 0)
 			pr_warn("ALSA: Control name '%s' truncated to '%s'\n",
-				ncontrol->name, kctl.id.name);
+				ncontrol->name, kctl->id.name);
 	}
-	kctl.id.index = ncontrol->index;
-	kctl.count = ncontrol->count ? ncontrol->count : 1;
-	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		 (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_VOLATILE|
-				      SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK));
-	kctl.info = ncontrol->info;
-	kctl.get = ncontrol->get;
-	kctl.put = ncontrol->put;
-	kctl.tlv.p = ncontrol->tlv.p;
-	kctl.private_value = ncontrol->private_value;
-	kctl.private_data = private_data;
-	return snd_ctl_new(&kctl, access);
+	kctl->id.index = ncontrol->index;
+
+	kctl->info = ncontrol->info;
+	kctl->get = ncontrol->get;
+	kctl->put = ncontrol->put;
+	kctl->tlv.p = ncontrol->tlv.p;
+
+	kctl->private_value = ncontrol->private_value;
+	kctl->private_data = private_data;
+
+	return kctl;
 }
 EXPORT_SYMBOL(snd_ctl_new1);
 
@@ -1179,44 +1202,48 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
 	};
 	struct snd_card *card = file->card;
-	struct snd_kcontrol kctl, *_kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
 	long private_size;
 	struct user_element *ue;
-	int idx, err;
-
-	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				 SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE));
-	info->id.numid = 0;
-	memset(&kctl, 0, sizeof(kctl));
+	int err;
 
+	/* 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;
 	}
 
-	if (card->user_ctl_count >= MAX_USER_CONTROLS)
+	/*
+	 * The number of userspace controls are counted control by control,
+	 * not element by element.
+	 */
+	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
 		return -ENOMEM;
 
-	memcpy(&kctl.id, &info->id, sizeof(info->id));
-	kctl.count = info->owner ? info->owner : 1;
-	access |= SNDRV_CTL_ELEM_ACCESS_USER;
-	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
-		kctl.info = snd_ctl_elem_user_enum_info;
-	else
-		kctl.info = snd_ctl_elem_user_info;
-	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
-		kctl.get = snd_ctl_elem_user_get;
-	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
-		kctl.put = snd_ctl_elem_user_put;
-	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) {
-		kctl.tlv.c = snd_ctl_elem_user_tlv;
+	/* Check the number of elements for this userspace control. */
+	count = info->owner;
+	if (count == 0)
+		count = 1;
+
+	/* Arrange access permissions if needed. */
+	access = info->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE);
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
-	}
+	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 
+	/*
+	 * Check information and calculate the size of data specific to
+	 * this userspace control.
+	 */
 	if ((info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN) ||
 	    (info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64))
 		return -EINVAL;
@@ -1226,11 +1253,27 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if ((info->count < 1) ||
 	    (info->count > max_value_counts[info->type]))
 		return -EINVAL;
-
 	private_size = value_sizes[info->type] * info->count;
-	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
-	if (ue == NULL)
+
+	/*
+	 * Keep memory object for this userspace control. After passing this
+	 * code block, the instance should be freed by snd_ctl_free_one().
+	 *
+	 * Note that these elements in this control are locked.
+	 */
+	err = snd_ctl_new(&kctl, count, access, file);
+	if (err < 0)
+		return err;
+	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
+				     GFP_KERNEL);
+	if (kctl->private_data == NULL) {
+		kfree(kctl);
 		return -ENOMEM;
+	}
+	kctl->private_free = snd_ctl_elem_user_free;
+
+	/* Set private data for this userspace control. */
+	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
@@ -1239,21 +1282,25 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
 		err = snd_ctl_elem_init_enum_names(ue);
 		if (err < 0) {
-			kfree(ue);
+			snd_ctl_free_one(kctl);
 			return err;
 		}
 	}
-	kctl.private_free = snd_ctl_elem_user_free;
-	_kctl = snd_ctl_new(&kctl, access);
-	if (_kctl == NULL) {
-		kfree(ue->priv_data);
-		kfree(ue);
-		return -ENOMEM;
-	}
-	_kctl->private_data = ue;
-	for (idx = 0; idx < _kctl->count; idx++)
-		_kctl->vd[idx].owner = file;
-	err = snd_ctl_add(card, _kctl);
+
+	/* Set callback functions. */
+	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
+		kctl->info = snd_ctl_elem_user_enum_info;
+	else
+		kctl->info = snd_ctl_elem_user_info;
+	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
+		kctl->get = snd_ctl_elem_user_get;
+	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
+		kctl->put = snd_ctl_elem_user_put;
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
+		kctl->tlv.c = snd_ctl_elem_user_tlv;
+
+	/* This function manage to free the instance on failure. */
+	err = snd_ctl_add(card, kctl);
 	if (err < 0)
 		return err;
 
-- 
2.1.0

Re: [PATCH 0/2][RFC] ALSA: core: optimizations for creating new controls

[Takashi Iwai]

At Fri,  6 Mar 2015 00:43:24 +0900,
Takashi Sakamoto wrote:
> 
> This patchset is for reducing executing time and stack usage at creating
> new control instances. Nothing fixes.
> 
> Takashi Sakamoto (2):
>   ALSA: core: use precomputed table to check userspace control params
>   ALSA: core: reduce stack usage related to snd_ctl_new()

Both look good, but just minor nitpicking: could you remove
superfluous parentheses in cases like below?

	if ((a > 0) && (b < 0)) ...

Most people are no lispers.


thanks,

Takashi

Re: [PATCH 0/2][RFC] ALSA: core: optimizations for creating new controls

[Takashi Sakamoto]

On Mar 6 2015 00:58, Takashi Iwai wrote:
> At Fri,  6 Mar 2015 00:43:24 +0900,
> Takashi Sakamoto wrote:
>>
>> This patchset is for reducing executing time and stack usage at creating
>> new control instances. Nothing fixes.
>>
>> Takashi Sakamoto (2):
>>   ALSA: core: use precomputed table to check userspace control params
>>   ALSA: core: reduce stack usage related to snd_ctl_new()
> 
> Both look good, but just minor nitpicking: could you remove
> superfluous parentheses in cases like below?
> 
> 	if ((a > 0) && (b < 0)) ...
> 
> Most people are no lispers.

Hm. I may be an alien with several eyes and an additional hand in my
nose, when programming conservatively ;)

OK. In next patchset, I'll remove such parentheses, as much as possible.


Thanks

Takashi Sakamoto

[PATCH 0/2 v2] ALSA: core: optimizations for creating new controls

[Takashi Sakamoto]

This patchset is for reducing calculating time and stack usage at creating
new control instances, fixes nothing.

Differences from my previous patches:
 - Remove superfluous parentheses.

Takashi Sakamoto (2):
  ALSA: core: use precomputed table to check userspace control params
  ALSA: core: reduce stack usage related to snd_ctl_new()

 sound/core/control.c | 271 +++++++++++++++++++++++++++++----------------------
 1 file changed, 157 insertions(+), 114 deletions(-)

-- 
2.1.0

[PATCH 1/2] ALSA: core: use precomputed table to check userspace control params

[Takashi Sakamoto]

The parameters can be decided in compile time.

This commit adds precomputed table to reduce calculating time.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 35324a8..0b85cbc 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1161,6 +1161,23 @@ 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)
 {
+	/* The capacity of struct snd_ctl_elem_value.value.*/
+	static const unsigned int value_sizes[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= sizeof(long),
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= sizeof(unsigned char),
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= sizeof(struct snd_aes_iec958),
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
+	};
+	static const unsigned int max_value_counts[] = {
+		[SNDRV_CTL_ELEM_TYPE_BOOLEAN]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER]	= 128,
+		[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,
+		[SNDRV_CTL_ELEM_TYPE_BYTES]	= 512,
+		[SNDRV_CTL_ELEM_TYPE_IEC958]	= 1,
+		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
+	};
 	struct snd_card *card = file->card;
 	struct snd_kcontrol kctl, *_kctl;
 	unsigned int access;
@@ -1168,8 +1185,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	struct user_element *ue;
 	int idx, err;
 
-	if (info->count < 1)
-		return -EINVAL;
 	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
 		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
 				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
@@ -1201,37 +1216,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		kctl.tlv.c = snd_ctl_elem_user_tlv;
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 	}
-	switch (info->type) {
-	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
-	case SNDRV_CTL_ELEM_TYPE_INTEGER:
-		private_size = sizeof(long);
-		if (info->count > 128)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
-		private_size = sizeof(long long);
-		if (info->count > 64)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
-		private_size = sizeof(unsigned int);
-		if (info->count > 128 || info->value.enumerated.items == 0)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_BYTES:
-		private_size = sizeof(unsigned char);
-		if (info->count > 512)
-			return -EINVAL;
-		break;
-	case SNDRV_CTL_ELEM_TYPE_IEC958:
-		private_size = sizeof(struct snd_aes_iec958);
-		if (info->count != 1)
-			return -EINVAL;
-		break;
-	default:
+
+	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
 		return -EINVAL;
-	}
-	private_size *= info->count;
+	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED &&
+	    info->value.enumerated.items == 0)
+		return -EINVAL;
+	if (info->count < 1 ||
+	    info->count > max_value_counts[info->type])
+		return -EINVAL;
+
+	private_size = value_sizes[info->type] * info->count;
 	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
 	if (ue == NULL)
 		return -ENOMEM;
-- 
2.1.0

[PATCH 2/2] ALSA: core: reduce stack usage related to snd_ctl_new()

[Takashi Sakamoto]

The callers of snd_ctl_new() need to have 'struct snd_kcontrol' data,
and pass the data as template. Then, the function allocates the structure
data again and copy from the template. This is a waste of resources.
Especially, the callers use large stack for the template.

This commit removes a need of template for the function, thus, changes
the prototype of snd_ctl_new(). Furthermore, this commit changes
the code of callers, snd_ctl_new1() and snd_ctl_elem_add() for better
shape.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 0b85cbc..e2d0ece 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -192,36 +192,43 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask,
 EXPORT_SYMBOL(snd_ctl_notify);
 
 /**
- * snd_ctl_new - create a control instance from the template
- * @control: the control template
- * @access: the default control access
+ * snd_ctl_new - create a new control instance with some elements
+ * @kctl: the pointer to store new control instance
+ * @count: the number of elements in this control
+ * @access: the default access flags for elements in this control
+ * @file: given when locking these elements
  *
- * Allocates a new struct snd_kcontrol instance and copies the given template 
- * to the new instance. It does not copy volatile data (access).
+ * Allocates a memory object for a new control instance. The instance has
+ * elements as many as the given number (@count). Each element has given
+ * access permissions (@access). Each element is locked when @file is given.
  *
- * Return: The pointer of the new instance, or %NULL on failure.
+ * Return: 0 on success, error code on failure
  */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
-					unsigned int access)
+static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count,
+		       unsigned int access, struct snd_ctl_file *file)
 {
-	struct snd_kcontrol *kctl;
+	unsigned int size;
 	unsigned int idx;
 	
-	if (snd_BUG_ON(!control || !control->count))
-		return NULL;
+	if ((count == 0) || (count > MAX_CONTROL_COUNT))
+		return -EINVAL;
 
-	if (control->count > MAX_CONTROL_COUNT)
-		return NULL;
+	size  = sizeof(struct snd_kcontrol);
+	size += sizeof(struct snd_kcontrol_volatile) * count;
 
-	kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL);
-	if (kctl == NULL) {
+	*kctl = kzalloc(size, GFP_KERNEL);
+	if (*kctl == NULL) {
 		pr_err("ALSA: Cannot allocate control instance\n");
-		return NULL;
+		return -ENOMEM;
 	}
-	*kctl = *control;
-	for (idx = 0; idx < kctl->count; idx++)
-		kctl->vd[idx].access = access;
-	return kctl;
+
+	for (idx = 0; idx < count; idx++) {
+		(*kctl)->vd[idx].access = access;
+		(*kctl)->vd[idx].owner = file;
+	}
+	(*kctl)->count = count;
+
+	return 0;
 }
 
 /**
@@ -238,37 +245,53 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
 struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 				  void *private_data)
 {
-	struct snd_kcontrol kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
+	int err;
 	
 	if (snd_BUG_ON(!ncontrol || !ncontrol->info))
 		return NULL;
-	memset(&kctl, 0, sizeof(kctl));
-	kctl.id.iface = ncontrol->iface;
-	kctl.id.device = ncontrol->device;
-	kctl.id.subdevice = ncontrol->subdevice;
+
+	count = ncontrol->count;
+	if (count == 0)
+		count = 1;
+
+	access = ncontrol->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_VOLATILE |
+		   SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK);
+
+	err = snd_ctl_new(&kctl, count, access, NULL);
+	if (err < 0)
+		return NULL;
+
+	/* The 'numid' member is decided when calling snd_ctl_add(). */
+	kctl->id.iface = ncontrol->iface;
+	kctl->id.device = ncontrol->device;
+	kctl->id.subdevice = ncontrol->subdevice;
 	if (ncontrol->name) {
-		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
-		if (strcmp(ncontrol->name, kctl.id.name) != 0)
+		strlcpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name));
+		if (strcmp(ncontrol->name, kctl->id.name) != 0)
 			pr_warn("ALSA: Control name '%s' truncated to '%s'\n",
-				ncontrol->name, kctl.id.name);
+				ncontrol->name, kctl->id.name);
 	}
-	kctl.id.index = ncontrol->index;
-	kctl.count = ncontrol->count ? ncontrol->count : 1;
-	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		 (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_VOLATILE|
-				      SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK));
-	kctl.info = ncontrol->info;
-	kctl.get = ncontrol->get;
-	kctl.put = ncontrol->put;
-	kctl.tlv.p = ncontrol->tlv.p;
-	kctl.private_value = ncontrol->private_value;
-	kctl.private_data = private_data;
-	return snd_ctl_new(&kctl, access);
+	kctl->id.index = ncontrol->index;
+
+	kctl->info = ncontrol->info;
+	kctl->get = ncontrol->get;
+	kctl->put = ncontrol->put;
+	kctl->tlv.p = ncontrol->tlv.p;
+
+	kctl->private_value = ncontrol->private_value;
+	kctl->private_data = private_data;
+
+	return kctl;
 }
 EXPORT_SYMBOL(snd_ctl_new1);
 
@@ -1179,44 +1202,48 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
 	};
 	struct snd_card *card = file->card;
-	struct snd_kcontrol kctl, *_kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
 	long private_size;
 	struct user_element *ue;
-	int idx, err;
-
-	access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		(info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				 SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				 SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE));
-	info->id.numid = 0;
-	memset(&kctl, 0, sizeof(kctl));
+	int err;
 
+	/* 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;
 	}
 
-	if (card->user_ctl_count >= MAX_USER_CONTROLS)
+	/*
+	 * The number of userspace controls are counted control by control,
+	 * not element by element.
+	 */
+	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
 		return -ENOMEM;
 
-	memcpy(&kctl.id, &info->id, sizeof(info->id));
-	kctl.count = info->owner ? info->owner : 1;
-	access |= SNDRV_CTL_ELEM_ACCESS_USER;
-	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
-		kctl.info = snd_ctl_elem_user_enum_info;
-	else
-		kctl.info = snd_ctl_elem_user_info;
-	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
-		kctl.get = snd_ctl_elem_user_get;
-	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
-		kctl.put = snd_ctl_elem_user_put;
-	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) {
-		kctl.tlv.c = snd_ctl_elem_user_tlv;
+	/* Check the number of elements for this userspace control. */
+	count = info->owner;
+	if (count == 0)
+		count = 1;
+
+	/* Arrange access permissions if needed. */
+	access = info->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		   SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		   SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE);
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
-	}
+	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 
+	/*
+	 * Check information and calculate the size of data specific to
+	 * this userspace control.
+	 */
 	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
 	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
 		return -EINVAL;
@@ -1226,11 +1253,27 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (info->count < 1 ||
 	    info->count > max_value_counts[info->type])
 		return -EINVAL;
-
 	private_size = value_sizes[info->type] * info->count;
-	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
-	if (ue == NULL)
+
+	/*
+	 * Keep memory object for this userspace control. After passing this
+	 * code block, the instance should be freed by snd_ctl_free_one().
+	 *
+	 * Note that these elements in this control are locked.
+	 */
+	err = snd_ctl_new(&kctl, count, access, file);
+	if (err < 0)
+		return err;
+	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,
+				     GFP_KERNEL);
+	if (kctl->private_data == NULL) {
+		kfree(kctl);
 		return -ENOMEM;
+	}
+	kctl->private_free = snd_ctl_elem_user_free;
+
+	/* Set private data for this userspace control. */
+	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
@@ -1239,21 +1282,25 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
 		err = snd_ctl_elem_init_enum_names(ue);
 		if (err < 0) {
-			kfree(ue);
+			snd_ctl_free_one(kctl);
 			return err;
 		}
 	}
-	kctl.private_free = snd_ctl_elem_user_free;
-	_kctl = snd_ctl_new(&kctl, access);
-	if (_kctl == NULL) {
-		kfree(ue->priv_data);
-		kfree(ue);
-		return -ENOMEM;
-	}
-	_kctl->private_data = ue;
-	for (idx = 0; idx < _kctl->count; idx++)
-		_kctl->vd[idx].owner = file;
-	err = snd_ctl_add(card, _kctl);
+
+	/* Set callback functions. */
+	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
+		kctl->info = snd_ctl_elem_user_enum_info;
+	else
+		kctl->info = snd_ctl_elem_user_info;
+	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
+		kctl->get = snd_ctl_elem_user_get;
+	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
+		kctl->put = snd_ctl_elem_user_put;
+	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
+		kctl->tlv.c = snd_ctl_elem_user_tlv;
+
+	/* This function manage to free the instance on failure. */
+	err = snd_ctl_add(card, kctl);
 	if (err < 0)
 		return err;
 
-- 
2.1.0

Re: [PATCH 0/2 v2] ALSA: core: optimizations for creating new controls

[Takashi Iwai]

At Tue, 10 Mar 2015 22:13:29 +0900,
Takashi Sakamoto wrote:
> 
> This patchset is for reducing calculating time and stack usage at creating
> new control instances, fixes nothing.
> 
> Differences from my previous patches:
>  - Remove superfluous parentheses.
> 
> Takashi Sakamoto (2):
>   ALSA: core: use precomputed table to check userspace control params
>   ALSA: core: reduce stack usage related to snd_ctl_new()

Thanks, applied both patches now.


Takashi

> 
>  sound/core/control.c | 271 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 157 insertions(+), 114 deletions(-)
> 
> -- 
> 2.1.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>