[RFC][PATCH 0/4] ALSA: control: return payload length of TLV operation

[RFC][PATCH 0/4] ALSA: control: return payload length of TLV operation

[Takashi Sakamoto]

Hi,

ALSA control interface has a feature named TLV. This is originally designed
to transfer information about threshold level. Later, it's used for I/O
operation.

In current implementation of this feature, length of handled data is not
returned to applications. This is not better as a feature related to I/O.

This patchset attempts to improve this feature so that the length is
returned to application in a 'length' field of tlv packet structure
(struct snd_ctl_tlv).

Takashi Sakamoto (4):
  ALSA: control: return payload length for TLV operation
  ALSA: control: delegate checking the length of data payload to each
    drivers
  ALSA: control: add kerneldoc for snd_kcontrol_tlv_rw_t
  ALSA: control: bump up protocol version to 2.0.8

 include/sound/control.h      | 22 ++++++++++++++++++++--
 include/sound/soc.h          |  2 +-
 include/uapi/sound/asound.h  |  2 +-
 sound/core/control.c         | 30 +++++++++++++++++++-----------
 sound/core/pcm_lib.c         | 15 ++++++++-------
 sound/core/vmaster.c         |  2 +-
 sound/hda/hdmi_chmap.c       | 16 +++++++++-------
 sound/pci/hda/hda_codec.c    | 12 ++++++++----
 sound/pci/hda/hda_local.h    |  4 ++--
 sound/pci/hda/patch_ca0132.c |  2 +-
 sound/pci/lola/lola_mixer.c  |  7 +++++--
 sound/soc/soc-ops.c          |  9 +++++----
 sound/usb/mixer.c            |  7 +++++--
 sound/usb/mixer.h            |  2 +-
 sound/usb/stream.c           | 13 ++++++++-----
 15 files changed, 94 insertions(+), 51 deletions(-)

-- 
2.7.4

[PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

TLV feature of control interface is originally introduced at
commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
Control API - more robust TLV implementation"). In this time,
snd_kcontrol_tlv_rw_t is for generating and transferring information about
threshold level for applications.

This feature can transfer arbitrary data in a shape of an array with
members of unsigned int type, therefore it can be used to deliver quite
large arbitrary data from user space to in-kernel drivers via ALSA control
character device. Focusing on this nature, commit 7523a271682f ("ASoC:
core: add a helper for extended byte controls using TLV") introduced
snd_soc_bytes_tlv_callback() just for I/O operations.

In this case, typically, APIs return operated length, while TLV feature
can't. This is inconvenient to applications.

This commit enables control core to return operated length of TLV feature.
This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to
size variable so that each implementation of the prototype can modify the
variable with operated length.

This commit also changes related codes. Below modules are modified:
 - snd
 - hda-core
 - hda-codec
 - hda-codec-ca0132
 - lola
 - usb-audio
 - soc-core

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/control.h      |  2 +-
 include/sound/soc.h          |  2 +-
 sound/core/control.c         | 28 +++++++++++++++++++---------
 sound/core/pcm_lib.c         | 15 ++++++++-------
 sound/core/vmaster.c         |  2 +-
 sound/hda/hdmi_chmap.c       | 16 +++++++++-------
 sound/pci/hda/hda_codec.c    | 12 ++++++++----
 sound/pci/hda/hda_local.h    |  4 ++--
 sound/pci/hda/patch_ca0132.c |  2 +-
 sound/pci/lola/lola_mixer.c  |  7 +++++--
 sound/soc/soc-ops.c          |  9 +++++----
 sound/usb/mixer.c            |  7 +++++--
 sound/usb/mixer.h            |  2 +-
 sound/usb/stream.c           | 13 ++++++++-----
 14 files changed, 74 insertions(+), 47 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index 21d047f..848940c 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -32,7 +32,7 @@ typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl
 typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
 typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
 				    int op_flag, /* SNDRV_CTL_TLV_OP_XXX */
-				    unsigned int size,
+				    unsigned int *size,
 				    unsigned int __user *tlv);
 
 enum {
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6144882..cfd1ca1 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -647,7 +647,7 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_info *ucontrol);
 int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
-	unsigned int size, unsigned int __user *tlv);
+	unsigned int *size, unsigned int __user *tlv);
 int snd_soc_info_xr_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_info *uinfo);
 int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
diff --git a/sound/core/control.c b/sound/core/control.c
index fb096cb..485963e 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1139,7 +1139,7 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol,
 
 static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 				 int op_flag,
-				 unsigned int size,
+				 unsigned int *size,
 				 unsigned int __user *tlv)
 {
 	struct user_element *ue = kcontrol->private_data;
@@ -1147,19 +1147,19 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 	void *new_data;
 
 	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
-		if (size > 1024 * 128)	/* sane value */
+		if (*size > 1024 * 128)	/* sane value */
 			return -EINVAL;
 
-		new_data = memdup_user(tlv, size);
+		new_data = memdup_user(tlv, *size);
 		if (IS_ERR(new_data))
 			return PTR_ERR(new_data);
 		mutex_lock(&ue->card->user_ctl_lock);
-		change = ue->tlv_data_size != size;
+		change = ue->tlv_data_size != *size;
 		if (!change)
-			change = memcmp(ue->tlv_data, new_data, size);
+			change = memcmp(ue->tlv_data, new_data, *size);
 		kfree(ue->tlv_data);
 		ue->tlv_data = new_data;
-		ue->tlv_data_size = size;
+		ue->tlv_data_size = *size;
 		mutex_unlock(&ue->card->user_ctl_lock);
 	} else {
 		int ret = 0;
@@ -1169,12 +1169,13 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 			ret = -ENXIO;
 			goto err_unlock;
 		}
-		if (size < ue->tlv_data_size) {
+		if (*size < ue->tlv_data_size) {
 			ret = -ENOSPC;
 			goto err_unlock;
 		}
 		if (copy_to_user(tlv, ue->tlv_data, ue->tlv_data_size))
 			ret = -EFAULT;
+		*size = ue->tlv_data_size;
 err_unlock:
 		mutex_unlock(&ue->card->user_ctl_lock);
 		if (ret)
@@ -1466,7 +1467,15 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 			err = -EPERM;
 			goto __kctl_end;
 		}
-		err = kctl->tlv.c(kctl, op_flag, tlv.length, _tlv->tlv);
+		len = tlv.length;
+		err = kctl->tlv.c(kctl, op_flag, &len, _tlv->tlv);
+		if (err < 0)
+			goto __kctl_end;
+		/* Return operated bytes. */
+		if (put_user(len, &_tlv->length)) {
+			err = -EFAULT;
+			goto __kctl_end;
+		}
 		if (err > 0) {
 			struct snd_ctl_elem_id id = kctl->id;
 			up_read(&card->controls_rwsem);
@@ -1483,7 +1492,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 			err = -ENOMEM;
 			goto __kctl_end;
 		}
-		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
+		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len) ||
+		    put_user(len, &_tlv->length))
 			err = -EFAULT;
 	}
       __kctl_end:
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index bb12615..dfc84ab 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2516,7 +2516,7 @@ static int pcm_chmap_ctl_get(struct snd_kcontrol *kcontrol,
  * expands the pre-defined channel maps in a form of TLV
  */
 static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			     unsigned int size, unsigned int __user *tlv)
+			     unsigned int *size, unsigned int __user *tlv)
 {
 	struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
 	const struct snd_pcm_chmap_elem *map;
@@ -2525,27 +2525,27 @@ static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 
 	if (snd_BUG_ON(!info->chmap))
 		return -EINVAL;
-	if (size < 8)
+	if (*size < 8)
 		return -ENOMEM;
 	if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv))
 		return -EFAULT;
-	size -= 8;
+	*size -= 8;
 	dst = tlv + 2;
 	for (map = info->chmap; map->channels; map++) {
 		int chs_bytes = map->channels * 4;
 		if (!valid_chmap_channels(info, map->channels))
 			continue;
-		if (size < 8)
+		if (*size < 8)
 			return -ENOMEM;
 		if (put_user(SNDRV_CTL_TLVT_CHMAP_FIXED, dst) ||
 		    put_user(chs_bytes, dst + 1))
 			return -EFAULT;
 		dst += 2;
-		size -= 8;
+		*size -= 8;
 		count += 8;
-		if (size < chs_bytes)
+		if (*size < chs_bytes)
 			return -ENOMEM;
-		size -= chs_bytes;
+		*size -= chs_bytes;
 		count += chs_bytes;
 		for (c = 0; c < map->channels; c++) {
 			if (put_user(map->map[c], dst))
@@ -2555,6 +2555,7 @@ static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 	}
 	if (put_user(count, tlv + 1))
 		return -EFAULT;
+	*size = count + sizeof(unsigned int) * 2;
 	return 0;
 }
 
diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c
index 6c58e6f..03f4681 100644
--- a/sound/core/vmaster.c
+++ b/sound/core/vmaster.c
@@ -220,7 +220,7 @@ static int slave_put(struct snd_kcontrol *kcontrol,
 }
 
 static int slave_tlv_cmd(struct snd_kcontrol *kcontrol,
-			 int op_flag, unsigned int size,
+			 int op_flag, unsigned int *size,
 			 unsigned int __user *tlv)
 {
 	struct link_slave *slave = snd_kcontrol_chip(kcontrol);
diff --git a/sound/hda/hdmi_chmap.c b/sound/hda/hdmi_chmap.c
index 81acc20..3163387 100644
--- a/sound/hda/hdmi_chmap.c
+++ b/sound/hda/hdmi_chmap.c
@@ -661,7 +661,7 @@ static int spk_mask_from_spk_alloc(int spk_alloc)
 }
 
 static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			      unsigned int size, unsigned int __user *tlv)
+			      unsigned int *size, unsigned int __user *tlv)
 {
 	struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
 	struct hdac_chmap *chmap = info->private_data;
@@ -672,11 +672,11 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 	int type;
 	int spk_alloc, spk_mask;
 
-	if (size < 8)
+	if (*size < 8)
 		return -ENOMEM;
 	if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv))
 		return -EFAULT;
-	size -= 8;
+	*size -= 8;
 	dst = tlv + 2;
 
 	spk_alloc = chmap->ops.get_spk_alloc(chmap->hdac, pcm_idx);
@@ -703,7 +703,7 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 							chmap, cap, chs);
 			if (type < 0)
 				return -ENODEV;
-			if (size < 8)
+			if (*size < 8)
 				return -ENOMEM;
 
 			if (put_user(type, dst) ||
@@ -711,13 +711,13 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 				return -EFAULT;
 
 			dst += 2;
-			size -= 8;
+			*size -= 8;
 			count += 8;
 
-			if (size < chs_bytes)
+			if (*size < chs_bytes)
 				return -ENOMEM;
 
-			size -= chs_bytes;
+			*size -= chs_bytes;
 			count += chs_bytes;
 			chmap->ops.cea_alloc_to_tlv_chmap(chmap, cap,
 						tlv_chmap, chs);
@@ -731,6 +731,8 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 	if (put_user(count, tlv + 1))
 		return -EFAULT;
 
+	*size = count + sizeof(unsigned int) * 2;
+
 	return 0;
 }
 
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 9913be8..06d87e7 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1420,7 +1420,7 @@ EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_volume_put);
  * set up via HDA_COMPOSE_AMP_VAL*() or related macros.
  */
 int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			  unsigned int size, unsigned int __user *_tlv)
+			  unsigned int *size, unsigned int __user *_tlv)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
 	hda_nid_t nid = get_amp_nid(kcontrol);
@@ -1429,7 +1429,7 @@ int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 	bool min_mute = get_amp_min_mute(kcontrol);
 	u32 caps, val1, val2;
 
-	if (size < 4 * sizeof(unsigned int))
+	if (*size < 4 * sizeof(unsigned int))
 		return -ENOMEM;
 	caps = query_amp_caps(codec, nid, dir);
 	val2 = (caps & AC_AMPCAP_STEP_SIZE) >> AC_AMPCAP_STEP_SIZE_SHIFT;
@@ -1447,6 +1447,9 @@ int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 		return -EFAULT;
 	if (put_user(val2, _tlv + 3))
 		return -EFAULT;
+
+	*size = sizeof(unsigned int) * 4;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_tlv);
@@ -1737,13 +1740,14 @@ static int get_kctl_0dB_offset(struct hda_codec *codec,
 {
 	int _tlv[4];
 	const int *tlv = NULL;
+	unsigned int size = sizeof(_tlv);
 	int val = -1;
 
 	if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
 		/* FIXME: set_fs() hack for obtaining user-space TLV data */
 		mm_segment_t fs = get_fs();
 		set_fs(get_ds());
-		if (!kctl->tlv.c(kctl, 0, sizeof(_tlv), _tlv))
+		if (!kctl->tlv.c(kctl, 0, &size, _tlv))
 			tlv = _tlv;
 		set_fs(fs);
 	} else if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_READ)
@@ -2207,7 +2211,7 @@ EXPORT_SYMBOL_GPL(snd_hda_mixer_bind_ctls_put);
  * set up via HDA_BIND_VOL() macro.
  */
 int snd_hda_mixer_bind_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			   unsigned int size, unsigned int __user *tlv)
+			   unsigned int *size, unsigned int __user *tlv)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
 	struct hda_bind_ctls *c;
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index d0e066e..35ea160 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -113,7 +113,7 @@ int snd_hda_mixer_amp_volume_get(struct snd_kcontrol *kcontrol,
 int snd_hda_mixer_amp_volume_put(struct snd_kcontrol *kcontrol,
 				 struct snd_ctl_elem_value *ucontrol);
 int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			  unsigned int size, unsigned int __user *tlv);
+			  unsigned int *size, unsigned int __user *tlv);
 int snd_hda_mixer_amp_switch_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo);
 int snd_hda_mixer_amp_switch_get(struct snd_kcontrol *kcontrol,
@@ -218,7 +218,7 @@ int snd_hda_mixer_bind_ctls_get(struct snd_kcontrol *kcontrol,
 int snd_hda_mixer_bind_ctls_put(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol);
 int snd_hda_mixer_bind_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			   unsigned int size, unsigned int __user *tlv);
+			   unsigned int *size, unsigned int __user *tlv);
 
 #define HDA_BIND_VOL(xname, bindrec) \
 	{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 9ceb2bc..bc8f342 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -3868,7 +3868,7 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
 }
 
 static int ca0132_volume_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			     unsigned int size, unsigned int __user *tlv)
+			     unsigned int *size, unsigned int __user *tlv)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
 	struct ca0132_spec *spec = codec->spec;
diff --git a/sound/pci/lola/lola_mixer.c b/sound/pci/lola/lola_mixer.c
index e7fe15d..ee92c31 100644
--- a/sound/pci/lola/lola_mixer.c
+++ b/sound/pci/lola/lola_mixer.c
@@ -553,14 +553,14 @@ static int lola_analog_vol_put(struct snd_kcontrol *kcontrol,
 }
 
 static int lola_analog_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			       unsigned int size, unsigned int __user *tlv)
+			       unsigned int *size, unsigned int __user *tlv)
 {
 	struct lola *chip = snd_kcontrol_chip(kcontrol);
 	int dir = kcontrol->private_value;
 	unsigned int val1, val2;
 	struct lola_pin *pin;
 
-	if (size < 4 * sizeof(unsigned int))
+	if (*size < 4 * sizeof(unsigned int))
 		return -ENOMEM;
 	pin = &chip->pin[dir].pins[0];
 
@@ -577,6 +577,9 @@ static int lola_analog_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 		return -EFAULT;
 	if (put_user(val2, tlv + 3))
 		return -EFAULT;
+
+	*size = 4 * sizeof(unsigned int);
+
 	return 0;
 }
 
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index a513a34..b2f3aa0 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -770,20 +770,21 @@ int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_bytes_info_ext);
 
 int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
-				unsigned int size, unsigned int __user *tlv)
+				unsigned int *size, unsigned int __user *tlv)
 {
 	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
-	unsigned int count = size < params->max ? size : params->max;
 	int ret = -ENXIO;
 
+	*size = min_t(int, *size, params->max);
+
 	switch (op_flag) {
 	case SNDRV_CTL_TLV_OP_READ:
 		if (params->get)
-			ret = params->get(kcontrol, tlv, count);
+			ret = params->get(kcontrol, tlv, *size);
 		break;
 	case SNDRV_CTL_TLV_OP_WRITE:
 		if (params->put)
-			ret = params->put(kcontrol, tlv, count);
+			ret = params->put(kcontrol, tlv, *size);
 		break;
 	}
 	return ret;
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 2f8c388..52a47bc 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel,
  * TLV callback for mixer volume controls
  */
 int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			 unsigned int size, unsigned int __user *_tlv)
+			 unsigned int *size, unsigned int __user *_tlv)
 {
 	struct usb_mixer_elem_info *cval = kcontrol->private_data;
 	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
 
-	if (size < sizeof(scale))
+	if (*size < sizeof(scale))
 		return -ENOMEM;
 	scale[2] = cval->dBmin;
 	scale[3] = cval->dBmax;
 	if (copy_to_user(_tlv, scale, sizeof(scale)))
 		return -EFAULT;
+
+	*size = sizeof(scale);
+
 	return 0;
 }
 
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index 3417ef3..ce4e236 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -84,7 +84,7 @@ void snd_usb_mixer_elem_init_std(struct usb_mixer_elem_list *list,
 				 int unitid);
 
 int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			  unsigned int size, unsigned int __user *_tlv);
+			  unsigned int *size, unsigned int __user *_tlv);
 
 #ifdef CONFIG_PM
 int snd_usb_mixer_suspend(struct usb_mixer_interface *mixer);
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 8e9548bc..373d077 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -136,7 +136,7 @@ static bool have_dup_chmap(struct snd_usb_substream *subs,
 }
 
 static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			     unsigned int size, unsigned int __user *tlv)
+			     unsigned int *size, unsigned int __user *tlv)
 {
 	struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
 	struct snd_usb_substream *subs = info->private_data;
@@ -144,11 +144,11 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 	unsigned int __user *dst;
 	int count = 0;
 
-	if (size < 8)
+	if (*size < 8)
 		return -ENOMEM;
 	if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv))
 		return -EFAULT;
-	size -= 8;
+	*size -= 8;
 	dst = tlv + 2;
 	list_for_each_entry(fp, &subs->fmt_list, list) {
 		int i, ch_bytes;
@@ -159,7 +159,7 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 			continue;
 		/* copy the entry */
 		ch_bytes = fp->chmap->channels * 4;
-		if (size < 8 + ch_bytes)
+		if (*size < 8 + ch_bytes)
 			return -ENOMEM;
 		if (put_user(SNDRV_CTL_TLVT_CHMAP_FIXED, dst) ||
 		    put_user(ch_bytes, dst + 1))
@@ -171,10 +171,13 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 		}
 
 		count += 8 + ch_bytes;
-		size -= 8 + ch_bytes;
+		*size -= 8 + ch_bytes;
 	}
 	if (put_user(count, tlv + 1))
 		return -EFAULT;
+
+	*size = count + sizeof(unsigned int) * 2;
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Tue, 30 Aug 2016 01:44:42 +0200,
Takashi Sakamoto wrote:
> 
> TLV feature of control interface is originally introduced at
> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
> Control API - more robust TLV implementation"). In this time,
> snd_kcontrol_tlv_rw_t is for generating and transferring information about
> threshold level for applications.
> 
> This feature can transfer arbitrary data in a shape of an array with
> members of unsigned int type, therefore it can be used to deliver quite
> large arbitrary data from user space to in-kernel drivers via ALSA control
> character device. Focusing on this nature, commit 7523a271682f ("ASoC:
> core: add a helper for extended byte controls using TLV") introduced
> snd_soc_bytes_tlv_callback() just for I/O operations.
> 
> In this case, typically, APIs return operated length, while TLV feature
> can't. This is inconvenient to applications.

The ASoC TLV (ab)usage still takes / receives the length field of
TLV.  What's missing there?


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 30 2016 14:29, Takashi Iwai wrote:
> On Tue, 30 Aug 2016 01:44:42 +0200,
> Takashi Sakamoto wrote:
>>
>> TLV feature of control interface is originally introduced at
>> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
>> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
>> Control API - more robust TLV implementation"). In this time,
>> snd_kcontrol_tlv_rw_t is for generating and transferring information about
>> threshold level for applications.
>>
>> This feature can transfer arbitrary data in a shape of an array with
>> members of unsigned int type, therefore it can be used to deliver quite
>> large arbitrary data from user space to in-kernel drivers via ALSA control
>> character device. Focusing on this nature, commit 7523a271682f ("ASoC:
>> core: add a helper for extended byte controls using TLV") introduced
>> snd_soc_bytes_tlv_callback() just for I/O operations.
>>
>> In this case, typically, APIs return operated length, while TLV feature
>> can't. This is inconvenient to applications.
>
> The ASoC TLV (ab)usage still takes / receives the length field of
> TLV.  What's missing there?

I don't get exactly what you mean.

The issue in which I'm interested is that applications cannot get to 
know length of actual processed bytes. Nothing others.

When pure threshold level information is transferred, applications can 
get its length of TLV packet payload, because we have a loose protocol 
to store the length of data in second element of the payload. The size 
of 2 elements plus the length equals to the length of TLV packet payload.

When using TLV feature just for I/O, this protocol is not kept, as we 
can see implementation of 'soc_bytes_ext' operation. Thus, the number of 
processed bytes should be returned to applications, by any ways. This 
patchset uses 'length' field in TLV packet header.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Tue, 30 Aug 2016 08:19:46 +0200,
Takashi Sakamoto wrote:
> 
> On Aug 30 2016 14:29, Takashi Iwai wrote:
> > On Tue, 30 Aug 2016 01:44:42 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> TLV feature of control interface is originally introduced at
> >> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
> >> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
> >> Control API - more robust TLV implementation"). In this time,
> >> snd_kcontrol_tlv_rw_t is for generating and transferring information about
> >> threshold level for applications.
> >>
> >> This feature can transfer arbitrary data in a shape of an array with
> >> members of unsigned int type, therefore it can be used to deliver quite
> >> large arbitrary data from user space to in-kernel drivers via ALSA control
> >> character device. Focusing on this nature, commit 7523a271682f ("ASoC:
> >> core: add a helper for extended byte controls using TLV") introduced
> >> snd_soc_bytes_tlv_callback() just for I/O operations.
> >>
> >> In this case, typically, APIs return operated length, while TLV feature
> >> can't. This is inconvenient to applications.
> >
> > The ASoC TLV (ab)usage still takes / receives the length field of
> > TLV.  What's missing there?
> 
> I don't get exactly what you mean.
> 
> The issue in which I'm interested is that applications cannot get to
> know length of actual processed bytes. Nothing others.
> 
> When pure threshold level information is transferred, applications can
> get its length of TLV packet payload, because we have a loose protocol
> to store the length of data in second element of the payload. The size
> of 2 elements plus the length equals to the length of TLV packet
> payload.
> 
> When using TLV feature just for I/O, this protocol is not kept, as we
> can see implementation of 'soc_bytes_ext' operation. Thus, the number
> of processed bytes should be returned to applications, by any
> ways. This patchset uses 'length' field in TLV packet header.

The current way of ASoC ext ctl is intended to be a workaround to pass
a large data via ctl API.  Nothing more than that.  If it's used for
receiving arbitrary size of data from the driver, it's a buggy usage.


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 30 2016 15:59, Takashi Iwai wrote:
> On Tue, 30 Aug 2016 08:19:46 +0200,
> Takashi Sakamoto wrote:
>>
>> On Aug 30 2016 14:29, Takashi Iwai wrote:
>>> On Tue, 30 Aug 2016 01:44:42 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> TLV feature of control interface is originally introduced at
>>>> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
>>>> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
>>>> Control API - more robust TLV implementation"). In this time,
>>>> snd_kcontrol_tlv_rw_t is for generating and transferring information about
>>>> threshold level for applications.
>>>>
>>>> This feature can transfer arbitrary data in a shape of an array with
>>>> members of unsigned int type, therefore it can be used to deliver quite
>>>> large arbitrary data from user space to in-kernel drivers via ALSA control
>>>> character device. Focusing on this nature, commit 7523a271682f ("ASoC:
>>>> core: add a helper for extended byte controls using TLV") introduced
>>>> snd_soc_bytes_tlv_callback() just for I/O operations.
>>>>
>>>> In this case, typically, APIs return operated length, while TLV feature
>>>> can't. This is inconvenient to applications.
>>>
>>> The ASoC TLV (ab)usage still takes / receives the length field of
>>> TLV.  What's missing there?
>>
>> I don't get exactly what you mean.
>>
>> The issue in which I'm interested is that applications cannot get to
>> know length of actual processed bytes. Nothing others.
>>
>> When pure threshold level information is transferred, applications can
>> get its length of TLV packet payload, because we have a loose protocol
>> to store the length of data in second element of the payload. The size
>> of 2 elements plus the length equals to the length of TLV packet
>> payload.
>>
>> When using TLV feature just for I/O, this protocol is not kept, as we
>> can see implementation of 'soc_bytes_ext' operation. Thus, the number
>> of processed bytes should be returned to applications, by any
>> ways. This patchset uses 'length' field in TLV packet header.
>
> The current way of ASoC ext ctl is intended to be a workaround to pass
> a large data via ctl API.  Nothing more than that.  If it's used for
> receiving arbitrary size of data from the driver, it's a buggy usage.

It's not workaround, it's actually used. It's better to consider as one 
of supported ways and pay enough attention so that applications don't 
get disadvantages from it.

The way to use is depending on actual implementation of each driver. 
It's better not to force policy of the usage to driver developers, I think.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Tue, 30 Aug 2016 09:13:54 +0200,
Takashi Sakamoto wrote:
> 
> On Aug 30 2016 15:59, Takashi Iwai wrote:
> > On Tue, 30 Aug 2016 08:19:46 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Aug 30 2016 14:29, Takashi Iwai wrote:
> >>> On Tue, 30 Aug 2016 01:44:42 +0200,
> >>> Takashi Sakamoto wrote:
> >>>>
> >>>> TLV feature of control interface is originally introduced at
> >>>> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for
> >>>> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA]
> >>>> Control API - more robust TLV implementation"). In this time,
> >>>> snd_kcontrol_tlv_rw_t is for generating and transferring information about
> >>>> threshold level for applications.
> >>>>
> >>>> This feature can transfer arbitrary data in a shape of an array with
> >>>> members of unsigned int type, therefore it can be used to deliver quite
> >>>> large arbitrary data from user space to in-kernel drivers via ALSA control
> >>>> character device. Focusing on this nature, commit 7523a271682f ("ASoC:
> >>>> core: add a helper for extended byte controls using TLV") introduced
> >>>> snd_soc_bytes_tlv_callback() just for I/O operations.
> >>>>
> >>>> In this case, typically, APIs return operated length, while TLV feature
> >>>> can't. This is inconvenient to applications.
> >>>
> >>> The ASoC TLV (ab)usage still takes / receives the length field of
> >>> TLV.  What's missing there?
> >>
> >> I don't get exactly what you mean.
> >>
> >> The issue in which I'm interested is that applications cannot get to
> >> know length of actual processed bytes. Nothing others.
> >>
> >> When pure threshold level information is transferred, applications can
> >> get its length of TLV packet payload, because we have a loose protocol
> >> to store the length of data in second element of the payload. The size
> >> of 2 elements plus the length equals to the length of TLV packet
> >> payload.
> >>
> >> When using TLV feature just for I/O, this protocol is not kept, as we
> >> can see implementation of 'soc_bytes_ext' operation. Thus, the number
> >> of processed bytes should be returned to applications, by any
> >> ways. This patchset uses 'length' field in TLV packet header.
> >
> > The current way of ASoC ext ctl is intended to be a workaround to pass
> > a large data via ctl API.  Nothing more than that.  If it's used for
> > receiving arbitrary size of data from the driver, it's a buggy usage.
> 
> It's not workaround, it's actually used.

... more than passing a large data?  If yes, it must be a bug.

> It's better to consider as
> one of supported ways and pay enough attention so that applications
> don't get disadvantages from it.
> 
> The way to use is depending on actual implementation of each
> driver. It's better not to force policy of the usage to driver
> developers, I think.

I don't agree, sorry.  The soc ext ctrl was introduced just for
passing a large set of data.  Nothing more than that.  If we need to
handle the processed data size in return, TLV is no right API to use
at all.  It just needs to use another API.

TLV callback can notify whether it handles all or it fails.  That's
enough for the control elements.  What else do you expect?


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Clemens Ladisch]

Takashi Sakamoto wrote:
> [...] APIs return operated length, while TLV feature
> can't. This is inconvenient to applications.
>
> This commit enables control core to return operated length of TLV feature.
> This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to
> size variable so that each implementation of the prototype can modify the
> variable with operated length.

I'll use this function as an example:

> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel,
>   * TLV callback for mixer volume controls
>   */
>  int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
> -			 unsigned int size, unsigned int __user *_tlv)
> +			 unsigned int *size, unsigned int __user *_tlv)
>  {
>  	struct usb_mixer_elem_info *cval = kcontrol->private_data;
>  	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
>
> -	if (size < sizeof(scale))
> +	if (*size < sizeof(scale))
>  		return -ENOMEM;
>  	scale[2] = cval->dBmin;
>  	scale[3] = cval->dBmax;
>  	if (copy_to_user(_tlv, scale, sizeof(scale)))
>  		return -EFAULT;
> +
> +	*size = sizeof(scale);
> +
>  	return 0;
>  }

The size is already returned in scale[1] (it's initialized by
DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.

All other TLV callbacks also take care to set this field correctly.

If there were any TLV callback that did not set _tlv[1] to the actual
size, it would be buggy, and just needed to be fixed to do so.


Regards,
Clemens

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 30 2016 16:05, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> [...] APIs return operated length, while TLV feature
>> can't. This is inconvenient to applications.
>>
>> This commit enables control core to return operated length of TLV feature.
>> This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to
>> size variable so that each implementation of the prototype can modify the
>> variable with operated length.
>
> I'll use this function as an example:
>
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel,
>>   * TLV callback for mixer volume controls
>>   */
>>  int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
>> -			 unsigned int size, unsigned int __user *_tlv)
>> +			 unsigned int *size, unsigned int __user *_tlv)
>>  {
>>  	struct usb_mixer_elem_info *cval = kcontrol->private_data;
>>  	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
>>
>> -	if (size < sizeof(scale))
>> +	if (*size < sizeof(scale))
>>  		return -ENOMEM;
>>  	scale[2] = cval->dBmin;
>>  	scale[3] = cval->dBmax;
>>  	if (copy_to_user(_tlv, scale, sizeof(scale)))
>>  		return -EFAULT;
>> +
>> +	*size = sizeof(scale);
>> +
>>  	return 0;
>>  }
>
> The size is already returned in scale[1] (it's initialized by
> DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.
>
> All other TLV callbacks also take care to set this field correctly.
>
> If there were any TLV callback that did not set _tlv[1] to the actual
> size, it would be buggy, and just needed to be fixed to do so.

As I described, TLV feature of ALSA control interface is not only used 
to transfer threshold level information, but also arbitrary data for I/O 
by developers in ALSA SoC part. The '_tlv[1]' protocol is not 
necessarily kept by them.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Clemens Ladisch]

Takashi Sakamoto wrote:
> On Aug 30 2016 16:05, Clemens Ladisch wrote:
>> If there were any TLV callback that did not set _tlv[1] to the actual
>> size, it would be buggy, and just needed to be fixed to do so.
>
> TLV feature of ALSA control interface is not only used to transfer
> threshold level information, but also arbitrary data for I/O by
> developers in ALSA SoC part. The '_tlv[1]' protocol is not necessarily
> kept by them.

Which specific ASoC driver does this?


Regards,
Clemens

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 30 2016 17:04, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> On Aug 30 2016 16:05, Clemens Ladisch wrote:
>>> If there were any TLV callback that did not set _tlv[1] to the actual
>>> size, it would be buggy, and just needed to be fixed to do so.
>>
>> TLV feature of ALSA control interface is not only used to transfer
>> threshold level information, but also arbitrary data for I/O by
>> developers in ALSA SoC part. The '_tlv[1]' protocol is not necessarily
>> kept by them.
> 
> Which specific ASoC driver does this?

As of 4.8, soc-wm-adsp does it, with a help of soc-core.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul]

On Tue, Aug 30, 2016 at 04:09:33PM +0900, Takashi Sakamoto wrote:
> On Aug 30 2016 16:05, Clemens Ladisch wrote:
> >Takashi Sakamoto wrote:
> >>[...] APIs return operated length, while TLV feature
> >>can't. This is inconvenient to applications.
> >>
> >>This commit enables control core to return operated length of TLV feature.
> >>This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to
> >>size variable so that each implementation of the prototype can modify the
> >>variable with operated length.
> >
> >I'll use this function as an example:
> >
> >>--- a/sound/usb/mixer.c
> >>+++ b/sound/usb/mixer.c
> >>@@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel,
> >>  * TLV callback for mixer volume controls
> >>  */
> >> int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
> >>-			 unsigned int size, unsigned int __user *_tlv)
> >>+			 unsigned int *size, unsigned int __user *_tlv)
> >> {
> >> 	struct usb_mixer_elem_info *cval = kcontrol->private_data;
> >> 	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
> >>
> >>-	if (size < sizeof(scale))
> >>+	if (*size < sizeof(scale))
> >> 		return -ENOMEM;
> >> 	scale[2] = cval->dBmin;
> >> 	scale[3] = cval->dBmax;
> >> 	if (copy_to_user(_tlv, scale, sizeof(scale)))
> >> 		return -EFAULT;
> >>+
> >>+	*size = sizeof(scale);
> >>+
> >> 	return 0;
> >> }
> >
> >The size is already returned in scale[1] (it's initialized by
> >DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.
> >
> >All other TLV callbacks also take care to set this field correctly.
> >
> >If there were any TLV callback that did not set _tlv[1] to the actual
> >size, it would be buggy, and just needed to be fixed to do so.
> 
> As I described, TLV feature of ALSA control interface is not only
> used to transfer threshold level information, but also arbitrary
> data for I/O by developers in ALSA SoC part. The '_tlv[1]' protocol
> is not necessarily kept by them.

can you explain what you mean by 'to transfer threshold level information'

And on this discussion, IIUC, we should fill tlv[1] with size being returned
right? For the asoc part, I think we should fix snd_soc_bytes_tlv_callback()
to update this.

Thanks
-- 
~Vinod

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 30 2016 23:51, Vinod Koul wrote:
> On Tue, Aug 30, 2016 at 04:09:33PM +0900, Takashi Sakamoto wrote:
>> On Aug 30 2016 16:05, Clemens Ladisch wrote:
>>> Takashi Sakamoto wrote:
>>>> [...] APIs return operated length, while TLV feature
>>>> can't. This is inconvenient to applications.
>>>>
>>>> This commit enables control core to return operated length of TLV feature.
>>>> This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to
>>>> size variable so that each implementation of the prototype can modify the
>>>> variable with operated length.
>>>
>>> I'll use this function as an example:
>>>
>>>> --- a/sound/usb/mixer.c
>>>> +++ b/sound/usb/mixer.c
>>>> @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel,
>>>>  * TLV callback for mixer volume controls
>>>>  */
>>>> int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag,
>>>> -			 unsigned int size, unsigned int __user *_tlv)
>>>> +			 unsigned int *size, unsigned int __user *_tlv)
>>>> {
>>>> 	struct usb_mixer_elem_info *cval = kcontrol->private_data;
>>>> 	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
>>>>
>>>> -	if (size < sizeof(scale))
>>>> +	if (*size < sizeof(scale))
>>>> 		return -ENOMEM;
>>>> 	scale[2] = cval->dBmin;
>>>> 	scale[3] = cval->dBmax;
>>>> 	if (copy_to_user(_tlv, scale, sizeof(scale)))
>>>> 		return -EFAULT;
>>>> +
>>>> +	*size = sizeof(scale);
>>>> +
>>>> 	return 0;
>>>> }
>>>
>>> The size is already returned in scale[1] (it's initialized by
>>> DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.
>>>
>>> All other TLV callbacks also take care to set this field correctly.
>>>
>>> If there were any TLV callback that did not set _tlv[1] to the actual
>>> size, it would be buggy, and just needed to be fixed to do so.
>>
>> As I described, TLV feature of ALSA control interface is not only
>> used to transfer threshold level information, but also arbitrary
>> data for I/O by developers in ALSA SoC part. The '_tlv[1]' protocol
>> is not necessarily kept by them.
> 
> can you explain what you mean by 'to transfer threshold level information'
> 
> And on this discussion, IIUC, we should fill tlv[1] with size being returned
> right? For the asoc part, I think we should fix snd_soc_bytes_tlv_callback()
> to update this.

The layout of TLV packet is:
struct snd_ctl_tlv {
    unsigned int numid;   # numerical ID of a control element
    unsigned int length;  # length of payload
    unsigned int tlv[0];  # payload
};
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945

In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
used to transfer data. For pure threshold level information, we expects
applications and drivers to fill the payload with this protocol:
struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
struct snd_ctl_tlv.tlv[1]:  length of data
struct snd_ctl_tlv.tlv[2..]: data

(You can see SNDRV_CTL_TLVT_XXX in this header.
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
)

On the other hand, ALSA SoC part performs:
struct snd_ctl_tlv.tlv[0..]: arbitrary data

If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.

The issue I address is current implementation cannot correctly handle
this case:
 - applications request a buffer with a certain size
 - drivers processes the request with smaller size
 - application cannot get the size

When implementing I/O operation, in my understanding, this situation
often occurs, depending on driver implementation. Fortunately, current
implementation of WM-ADSP is free from this concern, but it's better for
us not to expect this luck always.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul]

On Wed, Aug 31, 2016 at 07:04:12AM +0900, Takashi Sakamoto wrote:
> >>> The size is already returned in scale[1] (it's initialized by
> >>> DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.
> >>>
> >>> All other TLV callbacks also take care to set this field correctly.
> >>>
> >>> If there were any TLV callback that did not set _tlv[1] to the actual
> >>> size, it would be buggy, and just needed to be fixed to do so.
> >>
> >> As I described, TLV feature of ALSA control interface is not only
> >> used to transfer threshold level information, but also arbitrary
> >> data for I/O by developers in ALSA SoC part. The '_tlv[1]' protocol
> >> is not necessarily kept by them.
> > 
> > can you explain what you mean by 'to transfer threshold level information'
> > 
> > And on this discussion, IIUC, we should fill tlv[1] with size being returned
> > right? For the asoc part, I think we should fix snd_soc_bytes_tlv_callback()
> > to update this.
> 
> The layout of TLV packet is:
> struct snd_ctl_tlv {
>     unsigned int numid;   # numerical ID of a control element
>     unsigned int length;  # length of payload
>     unsigned int tlv[0];  # payload
> };
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945
> 
> In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
> used to transfer data. For pure threshold level information, we expects
> applications and drivers to fill the payload with this protocol:
> struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
> struct snd_ctl_tlv.tlv[1]:  length of data
> struct snd_ctl_tlv.tlv[2..]: data
> 
> (You can see SNDRV_CTL_TLVT_XXX in this header.
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
> )
> 
> On the other hand, ALSA SoC part performs:
> struct snd_ctl_tlv.tlv[0..]: arbitrary data
> 
> If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.
> 
> The issue I address is current implementation cannot correctly handle
> this case:
>  - applications request a buffer with a certain size
>  - drivers processes the request with smaller size
>  - application cannot get the size
> 
> When implementing I/O operation, in my understanding, this situation
> often occurs, depending on driver implementation. Fortunately, current
> implementation of WM-ADSP is free from this concern, but it's better for
> us not to expect this luck always.

Thanks for the detailed explanation,

IIUC, the fix would be to ensure that drivers or ASoC does return the length
value in snd_ctl_tlv.tlv[1] per the API expectations. As I said, in ASoC we
tend to move code to core, so snd_soc_bytes_tlv_callback should be updated

Thanks
-- 
~Vinod

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 31 2016 13:20, Vinod Koul wrote:
>> The layout of TLV packet is:
>> struct snd_ctl_tlv {
>>     unsigned int numid;   # numerical ID of a control element
>>     unsigned int length;  # length of payload
>>     unsigned int tlv[0];  # payload
>> };
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945
>>
>> In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
>> used to transfer data. For pure threshold level information, we expects
>> applications and drivers to fill the payload with this protocol:
>> struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
>> struct snd_ctl_tlv.tlv[1]:  length of data
>> struct snd_ctl_tlv.tlv[2..]: data
>>
>> (You can see SNDRV_CTL_TLVT_XXX in this header.
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
>> )
>>
>> On the other hand, ALSA SoC part performs:
>> struct snd_ctl_tlv.tlv[0..]: arbitrary data
>>
>> If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.
>>
>> The issue I address is current implementation cannot correctly handle
>> this case:
>>  - applications request a buffer with a certain size
>>  - drivers processes the request with smaller size
>>  - application cannot get the size
>>
>> When implementing I/O operation, in my understanding, this situation
>> often occurs, depending on driver implementation. Fortunately, current
>> implementation of WM-ADSP is free from this concern, but it's better for
>> us not to expect this luck always.
> 
> Thanks for the detailed explanation,
> 
> IIUC, the fix would be to ensure that drivers or ASoC does return the length
> value in snd_ctl_tlv.tlv[1] per the API expectations. As I said, in ASoC we
> tend to move code to core, so snd_soc_bytes_tlv_callback should be updated

Instead of it, I proposed to use 'struct snd_ctl_tlv.length' for the
length of TLV packet payload, because the original protocol is too rough
as I/O operations. The issue I suggested for ALSA SoC part is one of
actual examples. We can generate similar issues on user-defined control
element set.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Wed, Aug 31, 2016 at 01:30:02PM +0900, Takashi Sakamoto wrote:
> On Aug 31 2016 13:20, Vinod Koul wrote:
> >> The layout of TLV packet is:
> >> struct snd_ctl_tlv {
> >>     unsigned int numid;   # numerical ID of a control element
> >>     unsigned int length;  # length of payload
> >>     unsigned int tlv[0];  # payload
> >> };
> >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945
> >>
> >> In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
> >> used to transfer data. For pure threshold level information, we expects
> >> applications and drivers to fill the payload with this protocol:
> >> struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
> >> struct snd_ctl_tlv.tlv[1]:  length of data
> >> struct snd_ctl_tlv.tlv[2..]: data
> >>
> >> (You can see SNDRV_CTL_TLVT_XXX in this header.
> >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
> >> )
> >>
> >> On the other hand, ALSA SoC part performs:
> >> struct snd_ctl_tlv.tlv[0..]: arbitrary data
> >>
> >> If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.
> >>
> >> The issue I address is current implementation cannot correctly handle
> >> this case:
> >>  - applications request a buffer with a certain size
> >>  - drivers processes the request with smaller size
> >>  - application cannot get the size

Is this an expected use-case? The TLV controls were implemented
to allow ALSA controls of greater than 512 bytes, I am not sure
the intention was to provide completely generalised binary pipe. In
general the expection for reading a control is that you can
always read the whole control (AFAIK), so it feels like something
returning less than the requested amount of data is buggy.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Wed, 31 Aug 2016 11:05:52 +0200,
Charles Keepax wrote:
> 
> On Wed, Aug 31, 2016 at 01:30:02PM +0900, Takashi Sakamoto wrote:
> > On Aug 31 2016 13:20, Vinod Koul wrote:
> > >> The layout of TLV packet is:
> > >> struct snd_ctl_tlv {
> > >>     unsigned int numid;   # numerical ID of a control element
> > >>     unsigned int length;  # length of payload
> > >>     unsigned int tlv[0];  # payload
> > >> };
> > >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945
> > >>
> > >> In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
> > >> used to transfer data. For pure threshold level information, we expects
> > >> applications and drivers to fill the payload with this protocol:
> > >> struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
> > >> struct snd_ctl_tlv.tlv[1]:  length of data
> > >> struct snd_ctl_tlv.tlv[2..]: data
> > >>
> > >> (You can see SNDRV_CTL_TLVT_XXX in this header.
> > >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
> > >> )
> > >>
> > >> On the other hand, ALSA SoC part performs:
> > >> struct snd_ctl_tlv.tlv[0..]: arbitrary data
> > >>
> > >> If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.
> > >>
> > >> The issue I address is current implementation cannot correctly handle
> > >> this case:
> > >>  - applications request a buffer with a certain size
> > >>  - drivers processes the request with smaller size
> > >>  - application cannot get the size
> 
> Is this an expected use-case? The TLV controls were implemented
> to allow ALSA controls of greater than 512 bytes, I am not sure
> the intention was to provide completely generalised binary pipe. In
> general the expection for reading a control is that you can
> always read the whole control (AFAIK), so it feels like something
> returning less than the requested amount of data is buggy.

Right.  TLV has never been and (will never be) an API to handle a
generic binary stream.  Its usage in control read/write was accepted
because it still fitted with the current TLV design as is.

I'm not against to move some TLV verification or hardening code into
the common place, as long as it improves the code.  But your current
patchset smells fishy, as if it requires surgery over all bodies.
That would require more justification.


thanks,

Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Clemens Ladisch]

Takashi Iwai wrote:
> TLV has never been and (will never be) an API to handle a
> generic binary stream.

It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
defined types.  (But we cannot change soc-wm-adsp to support only proper
TLV data, because this would introduce a regression.)


Anyway, the separate length value could be useful only for drivers (like
soc-wm-adsp) that use a binary stream where TLV data should have been
used.  But the software that writes these coefficients to the WM ADSP
driver is very hardware specific anyway, and it presumably already works
without knowing the returned length value.  So there is no case where
this patchset _actually_ improves the interface.


Regards,
Clemens

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Wed, 31 Aug 2016 13:54:37 +0200,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > TLV has never been and (will never be) an API to handle a
> > generic binary stream.
> 
> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> defined types.  (But we cannot change soc-wm-adsp to support only proper
> TLV data, because this would introduce a regression.)

Well, passing the random hw-specific data is fine, but what I meant is
that a "stream" isn't suitable with TLV.  (And I don't mean it's
impossible, either :)


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Aug 31 2016 21:08, Takashi Iwai wrote:
> On Wed, 31 Aug 2016 13:54:37 +0200,
> Clemens Ladisch wrote:
>>
>> Takashi Iwai wrote:
>>> TLV has never been and (will never be) an API to handle a
>>> generic binary stream.
>>
>> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
>> or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
>> defined types.  (But we cannot change soc-wm-adsp to support only proper
>> TLV data, because this would introduce a regression.)
> 
> Well, passing the random hw-specific data is fine, but what I meant is
> that a "stream" isn't suitable with TLV.  (And I don't mean it's
> impossible, either :)

I'm few interested in whether it's binary stream or not, or
hardware-specific or not. The returned length is the most, in a point of
application developers. I'll show you two cases.

1.Let's see current implementation of 'snd_soc_bytes_tlv_callback()' in
sound/soc/soc-ops.c.

http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772

Here, 'size' argument is struct snd_ctl_tlv.length and 'tlv' argument is
&struct snd_ctl_tlv.tlv[2].

The size of actual I/O is smaller value between the length and hardcoded
'params->max'.

When applications give larger buffer to TLV feature of ALSA control
interface, then the early part of the buffer is filled. On the other
hand, applications cannot get to know the actual length.

2.Let's see core implementation user-defined control element set.

http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c?h=v4.8-rc4#n1140

The 'size' and 'tlv' arguments are the same as the first example.

When write operation, the given buffer and length is stored to a control
element set. When read operation, the given buffer is filled by the
stored data with the stored length. On the other hand, applications
cannot get to know the actual length.

When we're considering just about pure threshold information, there's no
concern about the length, due to struct snd_ctl_tlv.tlv[1]. But now
we've already decided to use this feature to transfer arbitrary length
of data. It's better to consider about what is better for applications.
In this point, I believe that my patchset is not fishy at all.

In the end of this message, I put a sample program for second example.


Regards

Takashi Sakamoto

----- 8< -----

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sound/asound.h>

int main(void)
{
    struct snd_ctl_tlv *packet;
    int fd;
    struct snd_ctl_elem_info info = {0};

    packet = malloc(100);
    if (packet == NULL) {
        printf("malloc(3): %s\n", strerror(ENOMEM));
        goto end_malloc;
    }

    fd = open("/dev/snd/controlC0", O_RDONLY);
    if (fd < 0) {
        printf("open(2): %s\n", strerror(errno));
        goto end_malloc;
    }

    /* Add my control element set. Don't forget to remove them. */
    info.id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
    info.owner = 100;
    strcpy((char *)info.id.name, "sample");
    info.access = SNDRV_CTL_ELEM_ACCESS_READ |
              SNDRV_CTL_ELEM_ACCESS_WRITE |
              SNDRV_CTL_ELEM_ACCESS_TLV_READ |
              SNDRV_CTL_ELEM_ACCESS_TLV_WRITE;
    info.count = 128;
    info.type = SNDRV_CTL_ELEM_TYPE_INTEGER;
    info.value.integer.min = 0;
    info.value.integer.max = 100;
    info.value.integer.step = 1;
    if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) {
        printf("ioctl(ADD): %s\n", strerror(errno));
        goto end_malloc;
    }

    /* In a case to store arbitrary data. */
    packet->numid = info.id.numid;
    packet->length = 6 * sizeof(packet->tlv[0]);
    packet->tlv[0] = 'a';
    packet->tlv[1] = 'b';
    packet->tlv[2] = 'c';
    packet->tlv[3] = 'd';
    packet->tlv[4] = 'e';
    packet->tlv[5] = 'f';
    if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_WRITE, packet) < 0) {
        printf("ioctl(TLV_WRITE): %s\n", strerror(errno));
        goto end_addition;
    }

    /* This simulates the other processes to read it. */
    packet->length = 100 - sizeof(struct snd_ctl_tlv);
    if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) {
        printf("ioctl(TLV_READ): %s\n", strerror(errno));
        goto end_addition;
    }

    printf("struct snd_ctl_tlv.length: %d\n", packet->length);
    printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));
end_addition:
    /* For a combination of PulseAudio and alsa-lib 1.1.1. */
    sleep(2);
    if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info.id) < 0)
        printf("ioctl(REMOVE): %s\n", strerror(errno));
end_malloc:
    free(packet);

    return EXIT_SUCCESS;
}

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Wed, 31 Aug 2016 17:26:14 +0200,
Takashi Sakamoto wrote:
> 
> On Aug 31 2016 21:08, Takashi Iwai wrote:
> > On Wed, 31 Aug 2016 13:54:37 +0200,
> > Clemens Ladisch wrote:
> >>
> >> Takashi Iwai wrote:
> >>> TLV has never been and (will never be) an API to handle a
> >>> generic binary stream.
> >>
> >> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> >> or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> >> defined types.  (But we cannot change soc-wm-adsp to support only proper
> >> TLV data, because this would introduce a regression.)
> > 
> > Well, passing the random hw-specific data is fine, but what I meant is
> > that a "stream" isn't suitable with TLV.  (And I don't mean it's
> > impossible, either :)
> 
> I'm few interested in whether it's binary stream or not, or
> hardware-specific or not. The returned length is the most, in a point of
> application developers. I'll show you two cases.
> 
> 1.Let's see current implementation of 'snd_soc_bytes_tlv_callback()' in
> sound/soc/soc-ops.c.
> 
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772
> 
> Here, 'size' argument is struct snd_ctl_tlv.length and 'tlv' argument is
> &struct snd_ctl_tlv.tlv[2].
> 
> The size of actual I/O is smaller value between the length and hardcoded
> 'params->max'.
> 
> When applications give larger buffer to TLV feature of ALSA control
> interface, then the early part of the buffer is filled. On the other
> hand, applications cannot get to know the actual length.
> 
> 2.Let's see core implementation user-defined control element set.
> 
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c?h=v4.8-rc4#n1140
> 
> The 'size' and 'tlv' arguments are the same as the first example.
> 
> When write operation, the given buffer and length is stored to a control
> element set. When read operation, the given buffer is filled by the
> stored data with the stored length. On the other hand, applications
> cannot get to know the actual length.
> 
> When we're considering just about pure threshold information, there's no
> concern about the length, due to struct snd_ctl_tlv.tlv[1]. But now
> we've already decided to use this feature to transfer arbitrary length
> of data. It's better to consider about what is better for applications.

Which application do you have in mind?  Applications would access via
either alsa-lib or tinyalsa.  And these libraries do already care
about how to access via TLV.  What makes better what, practically
seen?


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

Sorry to be late. I'm a bit busy for my daily work and things related
to my poor life.

On Sep 1 2016 00:40, Takashi Iwai wrote:
> Which application do you have in mind?

At first, I did never suggest that 'TLV feature should handle byte
stream' or 'TLV feature should be extended to somewhere'. I just
addressed that 'Now, TLV feature is not used only for transmission of
pure threshold information between applications/drivers, but it's also
used for I/O operation. Unfortunately, protocol of TLV packet payload is
not kept anymore.'

It's not my intension to discuss about this patchset in a point of 'byte
stream'. It comes from your bias, and the other developers are led to
the wrong direction, sigh. I really hope them to read my comment in this
patchset carefully and compare them to actual code implementations
in driver/core/library and applications[1][2][3].

Again, I have few interests about actual implementation of TLV feature
in driver side and for what purposes applications are developed to
utilize TLV feature. They're free for developers in each area now, and
for future. What we should do is how to assist their activity via the
design of APIs. This is my place in this patchset. It's not my intension
to request extensions of TLV feature.


> Applications would access via either alsa-lib or tinyalsa.  And these
libraries do already care about how to access via TLV.

Without enough care due to implementation in kernel land.

As I already cleared, current TLV feature has a difficulty not to return
the length of actually handled bytes[4]. Correspondingly, APIs in these
libraries have defections, as APIs for I/O operation or transmission of
arbitrary data, because applications cannot get to know the actual
length of handled data.

There's no way for user space to get appropriate length in advance, this
brings contradiction for content of given buffer. For example, in ALSA
SoC part, the length is trimmed according to a parameter of driver
instance, implicitly[5]. As a result, users are beguiled. They requests
a certain length of buffer to be handled via TLV feature. Even if they
receive success, a _part_ of buffer is actually handled. This is not
good for software stacks in a point of abstraction layer of hardware.
There's no transparency independent from hardwares. Application
developers is forced to consider about device driver's codes which are
not open via APIs.

I already wrote patchset for alternative TLV APIs to alsa-lib:
https://github.com/takaswie/alsa-lib/tree/new-tlv-api

In the remote branch, you can see new APIs at commit 5f13de, which
allows applications to receive the length of actually handled data:
https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497


> What makes better what, practically seen?

Already described in this message.

If you're still against to this patchset, I'm OK to cancel this discussion.

But in this case, involuntarily, when my friends are going to use TLV
feature for their ALSA applications (I hope it's unlikely), I'll
recommend them not to use it, because it certainly confuses application
developers and bring them to particular-hardware-specific something with
the lack of transparency. That's a waste of their expensive time.


[1] Apparently, Vinod Koul didn't read the comment before joining in
this discussion. He did the same behavior to my former patchset.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-July/110335.html

I don't prefer prompt responses just to external stimulus, because we're
not wild animals but human being with reasons. I use my private time to
this project and hope to avoid wasting my life for this kind of
unreasonable communication, because good communication is based on
enough understandings in each side.

[2] Charles Keepax still seem to have interests only in hardware to
which he's currently related. He presumably has no good view for API
designs for long term usage.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112378.html

[3] Clemens got near to where I stand. He already gave me his idea to
this patchset, with an alternative idea. I'm really appreciate for his
judgment, even if it's against my idea. His advices have often helped
me, then I respect his way to work for this subsystem.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112373.html

[4] [alsa-devel] [PATCH 1/4] ALSA: control: return payload length for
TLV operation
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html

[5]
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Fri, 02 Sep 2016 13:30:33 +0200,
Takashi Sakamoto wrote:
> 
> Sorry to be late. I'm a bit busy for my daily work and things related
> to my poor life.
> 
> On Sep 1 2016 00:40, Takashi Iwai wrote:
> > Which application do you have in mind?
> 
> At first, I did never suggest that 'TLV feature should handle byte
> stream' or 'TLV feature should be extended to somewhere'. I just
> addressed that 'Now, TLV feature is not used only for transmission of
> pure threshold information between applications/drivers, but it's also
> used for I/O operation. Unfortunately, protocol of TLV packet payload is
> not kept anymore.'
> 
> It's not my intension to discuss about this patchset in a point of 'byte
> stream'. It comes from your bias, and the other developers are led to
> the wrong direction, sigh. I really hope them to read my comment in this
> patchset carefully and compare them to actual code implementations
> in driver/core/library and applications[1][2][3].

Well, without the particular purpose explained, it's hard to
understand why your patchset is required.  That was the failure.
The implementation details come after the design.


> Again, I have few interests about actual implementation of TLV feature
> in driver side and for what purposes applications are developed to
> utilize TLV feature. They're free for developers in each area now, and
> for future. What we should do is how to assist their activity via the
> design of APIs. This is my place in this patchset. It's not my intension
> to request extensions of TLV feature.

But your patchset changes the call patterns largely.  This annoyed me,
and supposedly most people, too.  If the TLV feature isn't needed to
be extended, the necessary change shouldn't be too intrusive, either.


> > Applications would access via either alsa-lib or tinyalsa.  And these
> libraries do already care about how to access via TLV.
> 
> Without enough care due to implementation in kernel land.
> 
> As I already cleared, current TLV feature has a difficulty not to return
> the length of actually handled bytes[4]. Correspondingly, APIs in these
> libraries have defections, as APIs for I/O operation or transmission of
> arbitrary data, because applications cannot get to know the actual
> length of handled data.

It's TLV, so the actual length is always encoded in the block (in
tlv[1]).  What's missing...?


> There's no way for user space to get appropriate length in advance, this
> brings contradiction for content of given buffer. For example, in ALSA
> SoC part, the length is trimmed according to a parameter of driver
> instance, implicitly[5]. As a result, users are beguiled. They requests
> a certain length of buffer to be handled via TLV feature. Even if they
> receive success, a _part_ of buffer is actually handled. This is not
> good for software stacks in a point of abstraction layer of hardware.
> There's no transparency independent from hardwares. Application
> developers is forced to consider about device driver's codes which are
> not open via APIs.
> 
> I already wrote patchset for alternative TLV APIs to alsa-lib:
> https://github.com/takaswie/alsa-lib/tree/new-tlv-api
> 
> In the remote branch, you can see new APIs at commit 5f13de, which
> allows applications to receive the length of actually handled data:
> https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497
> 
> 
> > What makes better what, practically seen?
> 
> Already described in this message.
> 
> If you're still against to this patchset, I'm OK to cancel this discussion.
> 
> But in this case, involuntarily, when my friends are going to use TLV
> feature for their ALSA applications (I hope it's unlikely), I'll
> recommend them not to use it, because it certainly confuses application
> developers and bring them to particular-hardware-specific something with
> the lack of transparency. That's a waste of their expensive time.

The usage of TLV for this type of communication has *NEVER* been
recommended.  That's why I wrote it was an "(ab)usage".  It was
accepted just because the proposed usage fitted with the current TLV
access pattern.  If it were for more generic purpose, it wouldn't have
been accepted from the beginning.

The extended API is merely to pass a large blob over the control API.
That's the only purpose, and wouldn't be more than that.


thanks,

Takashi


> [1] Apparently, Vinod Koul didn't read the comment before joining in
> this discussion. He did the same behavior to my former patchset.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-July/110335.html
> 
> I don't prefer prompt responses just to external stimulus, because we're
> not wild animals but human being with reasons. I use my private time to
> this project and hope to avoid wasting my life for this kind of
> unreasonable communication, because good communication is based on
> enough understandings in each side.
> 
> [2] Charles Keepax still seem to have interests only in hardware to
> which he's currently related. He presumably has no good view for API
> designs for long term usage.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112378.html
> 
> [3] Clemens got near to where I stand. He already gave me his idea to
> this patchset, with an alternative idea. I'm really appreciate for his
> judgment, even if it's against my idea. His advices have often helped
> me, then I respect his way to work for this subsystem.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112373.html
> 
> [4] [alsa-devel] [PATCH 1/4] ALSA: control: return payload length for
> TLV operation
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html
> 
> [5]
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772
> 
> 
> Regards
> 
> Takashi Sakamoto
> 

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

Hi,

On Sep 2 2016 22:09, Takashi Iwai wrote:
> On Fri, 02 Sep 2016 13:30:33 +0200,
> Takashi Sakamoto wrote:
>> On Sep 1 2016 00:40, Takashi Iwai wrote:
>>> Which application do you have in mind?
>>
>> At first, I did never suggest that 'TLV feature should handle byte
>> stream' or 'TLV feature should be extended to somewhere'. I just
>> addressed that 'Now, TLV feature is not used only for transmission of
>> pure threshold information between applications/drivers, but it's also
>> used for I/O operation. Unfortunately, protocol of TLV packet payload is
>> not kept anymore.'
>>
>> It's not my intension to discuss about this patchset in a point of 'byte
>> stream'. It comes from your bias, and the other developers are led to
>> the wrong direction, sigh. I really hope them to read my comment in this
>> patchset carefully and compare them to actual code implementations
>> in driver/core/library and applications[1][2][3].
> 
> Well, without the particular purpose explained, it's hard to
> understand why your patchset is required.  That was the failure.
> The implementation details come after the design.

Read my comment in the first patch, again.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112245.html

I wrote 'This is inconvenient to applications'. It shows that I take
care of ALSA applications in this patch.

>> Again, I have few interests about actual implementation of TLV feature
>> in driver side and for what purposes applications are developed to
>> utilize TLV feature. They're free for developers in each area now, and
>> for future. What we should do is how to assist their activity via the
>> design of APIs. This is my place in this patchset. It's not my intension
>> to request extensions of TLV feature.
> 
> But your patchset changes the call patterns largely.  This annoyed me,
> and supposedly most people, too.  If the TLV feature isn't needed to
> be extended, the necessary change shouldn't be too intrusive, either.

Surprisingly, the annoying planted a bias to you and lost calmness in
your brain, away from the core concept of this patchset.

Well, to me, it's really normal for I/O APIs to return processed bytes
to applications. This is a reason that I explain like that.

Of course, I don't say I can always write comments to fully explains
patch features so that everyone can get them. But I believe that my
comments describes an item:
 - I/O operation recent supported in TLV feature ignores
   the protocol of TLV packet.
 - Then, applications have disadvantages when calling TLV ioctl in a
   point of operated bytes.

That's all of my concerns. Most of you interpret the item as what
convenient to them.

>>> Applications would access via either alsa-lib or tinyalsa.  And these
>> libraries do already care about how to access via TLV.
>>
>> Without enough care due to implementation in kernel land.
>>
>> As I already cleared, current TLV feature has a difficulty not to return
>> the length of actually handled bytes[4]. Correspondingly, APIs in these
>> libraries have defections, as APIs for I/O operation or transmission of
>> arbitrary data, because applications cannot get to know the actual
>> length of handled data.
> 
> It's TLV, so the actual length is always encoded in the block (in
> tlv[1]).  What's missing...?

I've already addressed two cases that 'struct snd_ctl_tlv.tlv[1]'
doesn't have actual length of data:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html

>> There's no way for user space to get appropriate length in advance, this
>> brings contradiction for content of given buffer. For example, in ALSA
>> SoC part, the length is trimmed according to a parameter of driver
>> instance, implicitly[5]. As a result, users are beguiled. They requests
>> a certain length of buffer to be handled via TLV feature. Even if they
>> receive success, a _part_ of buffer is actually handled. This is not
>> good for software stacks in a point of abstraction layer of hardware.
>> There's no transparency independent from hardwares. Application
>> developers is forced to consider about device driver's codes which are
>> not open via APIs.
>>
>> I already wrote patchset for alternative TLV APIs to alsa-lib:
>> https://github.com/takaswie/alsa-lib/tree/new-tlv-api
>>
>> In the remote branch, you can see new APIs at commit 5f13de, which
>> allows applications to receive the length of actually handled data:
>> https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497
>>
>>
>>> What makes better what, practically seen?
>>
>> Already described in this message.
>>
>> If you're still against to this patchset, I'm OK to cancel this discussion.
>>
>> But in this case, involuntarily, when my friends are going to use TLV
>> feature for their ALSA applications (I hope it's unlikely), I'll
>> recommend them not to use it, because it certainly confuses application
>> developers and bring them to particular-hardware-specific something with
>> the lack of transparency. That's a waste of their expensive time.
> 
> The usage of TLV for this type of communication has *NEVER* been
> recommended.  That's why I wrote it was an "(ab)usage".  It was
> accepted just because the proposed usage fitted with the current TLV
> access pattern.  If it were for more generic purpose, it wouldn't have
> been accepted from the beginning.
> 
> The extended API is merely to pass a large blob over the control API.
> That's the only purpose, and wouldn't be more than that.

You missed my intention, again. Please be in user space, then consider
about TLV ioctls and the state of buffers, length. TLV feature supports
read, write and command operations.

I'm sorry but it's time to go to bed. I got tired because it's the last
day of daily work in a week...


Thanks

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Fri, 02 Sep 2016 16:50:39 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 2 2016 22:09, Takashi Iwai wrote:
> > On Fri, 02 Sep 2016 13:30:33 +0200,
> > Takashi Sakamoto wrote:
> >> On Sep 1 2016 00:40, Takashi Iwai wrote:
> >>> Which application do you have in mind?
> >>
> >> At first, I did never suggest that 'TLV feature should handle byte
> >> stream' or 'TLV feature should be extended to somewhere'. I just
> >> addressed that 'Now, TLV feature is not used only for transmission of
> >> pure threshold information between applications/drivers, but it's also
> >> used for I/O operation. Unfortunately, protocol of TLV packet payload is
> >> not kept anymore.'
> >>
> >> It's not my intension to discuss about this patchset in a point of 'byte
> >> stream'. It comes from your bias, and the other developers are led to
> >> the wrong direction, sigh. I really hope them to read my comment in this
> >> patchset carefully and compare them to actual code implementations
> >> in driver/core/library and applications[1][2][3].
> > 
> > Well, without the particular purpose explained, it's hard to
> > understand why your patchset is required.  That was the failure.
> > The implementation details come after the design.
> 
> Read my comment in the first patch, again.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112245.html
> 
> I wrote 'This is inconvenient to applications'. It shows that I take
> care of ALSA applications in this patch.
>
> >> Again, I have few interests about actual implementation of TLV feature
> >> in driver side and for what purposes applications are developed to
> >> utilize TLV feature. They're free for developers in each area now, and
> >> for future. What we should do is how to assist their activity via the
> >> design of APIs. This is my place in this patchset. It's not my intension
> >> to request extensions of TLV feature.
> > 
> > But your patchset changes the call patterns largely.  This annoyed me,
> > and supposedly most people, too.  If the TLV feature isn't needed to
> > be extended, the necessary change shouldn't be too intrusive, either.
> 
> Surprisingly, the annoying planted a bias to you and lost calmness in
> your brain, away from the core concept of this patchset.
> 
> Well, to me, it's really normal for I/O APIs to return processed bytes
> to applications. This is a reason that I explain like that.
> 
> Of course, I don't say I can always write comments to fully explains
> patch features so that everyone can get them. But I believe that my
> comments describes an item:
>  - I/O operation recent supported in TLV feature ignores
>    the protocol of TLV packet.
>  - Then, applications have disadvantages when calling TLV ioctl in a
>    point of operated bytes.
> 
> That's all of my concerns. Most of you interpret the item as what
> convenient to them.

Then raise these things at first before posting a intrusive patchset.
That would have made discussion far easier.


> >>> Applications would access via either alsa-lib or tinyalsa.  And these
> >> libraries do already care about how to access via TLV.
> >>
> >> Without enough care due to implementation in kernel land.
> >>
> >> As I already cleared, current TLV feature has a difficulty not to return
> >> the length of actually handled bytes[4]. Correspondingly, APIs in these
> >> libraries have defections, as APIs for I/O operation or transmission of
> >> arbitrary data, because applications cannot get to know the actual
> >> length of handled data.
> > 
> > It's TLV, so the actual length is always encoded in the block (in
> > tlv[1]).  What's missing...?
> 
> I've already addressed two cases that 'struct snd_ctl_tlv.tlv[1]'
> doesn't have actual length of data:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html

The usage in ASoC is really an oversight.  It shouldn't have been
accepted in that way; at least, the data passed there should have been
encapsulated in TLV.  But it's too late, so we have to live with it,
and leave it as the only exception.  As you may notice, it's an abuse
of TLV callback, and changing the other TLV callers just because of
this abuse is basically nonsense.  This is the principal idea behind
my objection.

That said, it's a restricted API, and its so per design.  It should be
used only for the case where it fits with the limitation (e.g. just
for passing the large COEF blob), and not for any generic I/O.
If we need more features from the I/O, it shouldn't be done per TLV
call but by another way.

About the user-control TLV: it's the responsibility of reader/write
that gives the right content of TLV.  IOW, the content must be TLV,
and the size is passed there.  If not, it's the fault of the write
size.

Currently kernel has no control over it, since the data passed there
is basically irrelevant with the kernel.  The verification can be done
in alsa-lib instead.


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Fri, 02 Sep 2016 17:19:07 +0200,
Takashi Iwai wrote:
> 
> About the user-control TLV: it's the responsibility of reader/write
> that gives the right content of TLV.  IOW, the content must be TLV,
> and the size is passed there.  If not, it's the fault of the write
> size.

s/write size/write side/


Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Fri, Sep 02, 2016 at 08:30:33PM +0900, Takashi Sakamoto wrote:
> Sorry to be late. I'm a bit busy for my daily work and things related
> to my poor life.
> 
> On Sep 1 2016 00:40, Takashi Iwai wrote:
> > Which application do you have in mind?
> 
> At first, I did never suggest that 'TLV feature should handle byte
> stream' or 'TLV feature should be extended to somewhere'. I just
> addressed that 'Now, TLV feature is not used only for transmission of
> pure threshold information between applications/drivers, but it's also
> used for I/O operation. Unfortunately, protocol of TLV packet payload is
> not kept anymore.'
> 
> It's not my intension to discuss about this patchset in a point of 'byte
> stream'. It comes from your bias, and the other developers are led to
> the wrong direction, sigh. I really hope them to read my comment in this
> patchset carefully and compare them to actual code implementations
> in driver/core/library and applications[1][2][3].
> 
> Again, I have few interests about actual implementation of TLV feature
> in driver side and for what purposes applications are developed to
> utilize TLV feature. They're free for developers in each area now, and
> for future. What we should do is how to assist their activity via the
> design of APIs. This is my place in this patchset. It's not my intension
> to request extensions of TLV feature.
> 
> 
> > Applications would access via either alsa-lib or tinyalsa.  And these
> libraries do already care about how to access via TLV.
> 
> Without enough care due to implementation in kernel land.
> 
> As I already cleared, current TLV feature has a difficulty not to return
> the length of actually handled bytes[4]. Correspondingly, APIs in these
> libraries have defections, as APIs for I/O operation or transmission of
> arbitrary data, because applications cannot get to know the actual
> length of handled data.
> 

    /* This simulates the other processes to read it. */
    packet->length = 100 - sizeof(struct snd_ctl_tlv);
    if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) {
        printf("ioctl(TLV_READ): %s\n", strerror(errno));
        goto end_addition;
    }

    printf("struct snd_ctl_tlv.length: %d\n", packet->length);
    printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));

Here you wrote 6 items into the control but the read said it
gave you 100 items and you would have preferred it to have said
6 items? But you have created a 128 item long ALSA control if we
forget about the TLV aspect (which in my view is just a means to
access the control) and assume this was a regular ALSA control
then you would never expect that to give you less data than the
size of the control, so this behaviour would be expected.

As this was originally intended (at least AFAIK) as a mechanism
to provide byte controls >512 bytes, my point is really just that
I would expect this to work like a normal control. There is no
need to pass the length actually processed because this isn't
for arbitrary length data its just a way to read/write a control
which is something that already has a defined size.

> There's no way for user space to get appropriate length in advance, this
> brings contradiction for content of given buffer. For example, in ALSA
> SoC part, the length is trimmed according to a parameter of driver
> instance, implicitly[5]. As a result, users are beguiled. They requests
> a certain length of buffer to be handled via TLV feature. Even if they
> receive success, a _part_ of buffer is actually handled. This is not
> good for software stacks in a point of abstraction layer of hardware.
> There's no transparency independent from hardwares. Application
> developers is forced to consider about device driver's codes which are
> not open via APIs.
> 

That length you refer to is not some secret internal length it
is the length of the control as returned by the info ioctl. I
guess we could make the read/write fail if it was larger than the
control, that might be better behaviour than just silently not
handling all the data.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Sep 3 2016 20:38, Charles Keepax wrote:
> On Fri, Sep 02, 2016 at 08:30:33PM +0900, Takashi Sakamoto wrote:
>> Sorry to be late. I'm a bit busy for my daily work and things related
>> to my poor life.
>>
>> On Sep 1 2016 00:40, Takashi Iwai wrote:
>>> Which application do you have in mind?
>>
>> At first, I did never suggest that 'TLV feature should handle byte
>> stream' or 'TLV feature should be extended to somewhere'. I just
>> addressed that 'Now, TLV feature is not used only for transmission of
>> pure threshold information between applications/drivers, but it's also
>> used for I/O operation. Unfortunately, protocol of TLV packet payload is
>> not kept anymore.'
>>
>> It's not my intension to discuss about this patchset in a point of 'byte
>> stream'. It comes from your bias, and the other developers are led to
>> the wrong direction, sigh. I really hope them to read my comment in this
>> patchset carefully and compare them to actual code implementations
>> in driver/core/library and applications[1][2][3].
>>
>> Again, I have few interests about actual implementation of TLV feature
>> in driver side and for what purposes applications are developed to
>> utilize TLV feature. They're free for developers in each area now, and
>> for future. What we should do is how to assist their activity via the
>> design of APIs. This is my place in this patchset. It's not my intension
>> to request extensions of TLV feature.
>>
>>
>>> Applications would access via either alsa-lib or tinyalsa.  And these
>> libraries do already care about how to access via TLV.
>>
>> Without enough care due to implementation in kernel land.
>>
>> As I already cleared, current TLV feature has a difficulty not to return
>> the length of actually handled bytes[4]. Correspondingly, APIs in these
>> libraries have defections, as APIs for I/O operation or transmission of
>> arbitrary data, because applications cannot get to know the actual
>> length of handled data.
>>
> 
>     /* This simulates the other processes to read it. */
>     packet->length = 100 - sizeof(struct snd_ctl_tlv);
>     if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) {
>         printf("ioctl(TLV_READ): %s\n", strerror(errno));
>         goto end_addition;
>     }
> 
>     printf("struct snd_ctl_tlv.length: %d\n", packet->length);
>     printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));
> 
> Here you wrote 6 items into the control but the read said it
> gave you 100 items and you would have preferred it to have said
> 6 items? But you have created a 128 item long ALSA control if we
> forget about the TLV aspect (which in my view is just a means to
> access the control) and assume this was a regular ALSA control
> then you would never expect that to give you less data than the
> size of the control, so this behaviour would be expected.

You misunderstand the design of ALSA control core. I wrote a part of it,
later.

> As this was originally intended (at least AFAIK) as a mechanism
> to provide byte controls >512 bytes, my point is really just that
> I would expect this to work like a normal control. There is no
> need to pass the length actually processed because this isn't
> for arbitrary length data its just a way to read/write a control
> which is something that already has a defined size.

Just for current implementation, it's appropriate.

But for future, it may be certainly inappropriate.

>> There's no way for user space to get appropriate length in advance, this
>> brings contradiction for content of given buffer. For example, in ALSA
>> SoC part, the length is trimmed according to a parameter of driver
>> instance, implicitly[5]. As a result, users are beguiled. They requests
>> a certain length of buffer to be handled via TLV feature. Even if they
>> receive success, a _part_ of buffer is actually handled. This is not
>> good for software stacks in a point of abstraction layer of hardware.
>> There's no transparency independent from hardwares. Application
>> developers is forced to consider about device driver's codes which are
>> not open via APIs.
>>
> 
> That length you refer to is not some secret internal length it
> is the length of the control as returned by the info ioctl. I
> guess we could make the read/write fail if it was larger than the
> control, that might be better behaviour than just silently not
> handling all the data.

It's an abuse in a point of application interfaces.

'struct snd_ctl_elem_info.count' represents the number of members which
can hold a value, in a element of a element set. It's not for something
about TLV information.

And some core codes and user lands are written according to the design.
Please investigate 'sound/core/control.c' in Linux kernel and
'src/control/*' in alsa-lib and 'alsactl/alsaloop/amixer' in alsa-utils.
Then, we may find to lost something important in kernel land development.

The latest alsa-lib includes a document of the design of ALSA control
core. It may help your understanding.
http://www.alsa-project.org/alsa-doc/alsa-lib/control.html


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Sun, 04 Sep 2016 13:07:41 +0200,
Takashi Sakamoto wrote:
> 
> On Sep 3 2016 20:38, Charles Keepax wrote:
> > On Fri, Sep 02, 2016 at 08:30:33PM +0900, Takashi Sakamoto wrote:
> >> Sorry to be late. I'm a bit busy for my daily work and things related
> >> to my poor life.
> >>
> >> On Sep 1 2016 00:40, Takashi Iwai wrote:
> >>> Which application do you have in mind?
> >>
> >> At first, I did never suggest that 'TLV feature should handle byte
> >> stream' or 'TLV feature should be extended to somewhere'. I just
> >> addressed that 'Now, TLV feature is not used only for transmission of
> >> pure threshold information between applications/drivers, but it's also
> >> used for I/O operation. Unfortunately, protocol of TLV packet payload is
> >> not kept anymore.'
> >>
> >> It's not my intension to discuss about this patchset in a point of 'byte
> >> stream'. It comes from your bias, and the other developers are led to
> >> the wrong direction, sigh. I really hope them to read my comment in this
> >> patchset carefully and compare them to actual code implementations
> >> in driver/core/library and applications[1][2][3].
> >>
> >> Again, I have few interests about actual implementation of TLV feature
> >> in driver side and for what purposes applications are developed to
> >> utilize TLV feature. They're free for developers in each area now, and
> >> for future. What we should do is how to assist their activity via the
> >> design of APIs. This is my place in this patchset. It's not my intension
> >> to request extensions of TLV feature.
> >>
> >>
> >>> Applications would access via either alsa-lib or tinyalsa.  And these
> >> libraries do already care about how to access via TLV.
> >>
> >> Without enough care due to implementation in kernel land.
> >>
> >> As I already cleared, current TLV feature has a difficulty not to return
> >> the length of actually handled bytes[4]. Correspondingly, APIs in these
> >> libraries have defections, as APIs for I/O operation or transmission of
> >> arbitrary data, because applications cannot get to know the actual
> >> length of handled data.
> >>
> > 
> >     /* This simulates the other processes to read it. */
> >     packet->length = 100 - sizeof(struct snd_ctl_tlv);
> >     if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) {
> >         printf("ioctl(TLV_READ): %s\n", strerror(errno));
> >         goto end_addition;
> >     }
> > 
> >     printf("struct snd_ctl_tlv.length: %d\n", packet->length);
> >     printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));
> > 
> > Here you wrote 6 items into the control but the read said it
> > gave you 100 items and you would have preferred it to have said
> > 6 items? But you have created a 128 item long ALSA control if we
> > forget about the TLV aspect (which in my view is just a means to
> > access the control) and assume this was a regular ALSA control
> > then you would never expect that to give you less data than the
> > size of the control, so this behaviour would be expected.
> 
> You misunderstand the design of ALSA control core. I wrote a part of it,
> later.
> 
> > As this was originally intended (at least AFAIK) as a mechanism
> > to provide byte controls >512 bytes, my point is really just that
> > I would expect this to work like a normal control. There is no
> > need to pass the length actually processed because this isn't
> > for arbitrary length data its just a way to read/write a control
> > which is something that already has a defined size.
> 
> Just for current implementation, it's appropriate.
> 
> But for future, it may be certainly inappropriate.
> 
> >> There's no way for user space to get appropriate length in advance, this
> >> brings contradiction for content of given buffer. For example, in ALSA
> >> SoC part, the length is trimmed according to a parameter of driver
> >> instance, implicitly[5]. As a result, users are beguiled. They requests
> >> a certain length of buffer to be handled via TLV feature. Even if they
> >> receive success, a _part_ of buffer is actually handled. This is not
> >> good for software stacks in a point of abstraction layer of hardware.
> >> There's no transparency independent from hardwares. Application
> >> developers is forced to consider about device driver's codes which are
> >> not open via APIs.
> >>
> > 
> > That length you refer to is not some secret internal length it
> > is the length of the control as returned by the info ioctl. I
> > guess we could make the read/write fail if it was larger than the
> > control, that might be better behaviour than just silently not
> > handling all the data.
> 
> It's an abuse in a point of application interfaces.
> 
> 'struct snd_ctl_elem_info.count' represents the number of members which
> can hold a value, in a element of a element set. It's not for something
> about TLV information.
> 
> And some core codes and user lands are written according to the design.
> Please investigate 'sound/core/control.c' in Linux kernel and
> 'src/control/*' in alsa-lib and 'alsactl/alsaloop/amixer' in alsa-utils.
> Then, we may find to lost something important in kernel land development.
> 
> The latest alsa-lib includes a document of the design of ALSA control
> core. It may help your understanding.
> http://www.alsa-project.org/alsa-doc/alsa-lib/control.html

Yeah, there are obviously some issues in the current implementation of
wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
patchset as is from a few reasons, these issues still should be
addressed.

First off, passing the binary blob directly via TLV callback is
incorrect from the ABI perspective.  When Vinod proposed the idea via
TLV access originally, we thought they the data is encoded in TLV
format.  Alas, the resulted code didn't do that and it slipped into
the upstream without consideration. 

Besides that, the second problem is the count value returned via
snd_ctl_elem_info, as mentioned in the above.  It's beyond the
original control API design, and a kind of illegal usage. 
(Well, it's a philosophical argument: what one would expect for an
 element that has neither read nor write access...?)

So, at this point, the main question is whether we keep this access
pattern as is, as a sort of official one, and put some exceptional
rule.  Charles, how is the situation?  Has it been already deployed to
real systems?

If we may still change the wm_adsp behavior, we may "fix" the first
issue by passing the blob properly encoded in TLV format, at least.
OTOH, if we need to keep the current ABI abuse as is, one idea is to
add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
control, and we define more strictly how the code should be
implemented.  Currently we can judge this element as a one that has no
read/write access but with tlv r/w.  But it's way too unclear.


thanks,

Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Sep 5 2016 05:45, Takashi Iwai wrote:
> Yeah, there are obviously some issues in the current implementation of
> wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
> patchset as is from a few reasons, these issues still should be
> addressed.

OK. I welcome to abandon this patchset ;)

> First off, passing the binary blob directly via TLV callback is
> incorrect from the ABI perspective.  When Vinod proposed the idea via
> TLV access originally, we thought they the data is encoded in TLV
> format.  Alas, the resulted code didn't do that and it slipped into
> the upstream without consideration.

+1

> Besides that, the second problem is the count value returned via
> snd_ctl_elem_info, as mentioned in the above.  It's beyond the
> original control API design, and a kind of illegal usage.

+1

> (Well, it's a philosophical argument: what one would expect for an
>  element that has neither read nor write access...?)

It's an element with no sense for applications. A waste of codes in 
kernel land.

> So, at this point, the main question is whether we keep this access
> pattern as is, as a sort of official one, and put some exceptional
> rule.  Charles, how is the situation?  Has it been already deployed to
> real systems?
>
> If we may still change the wm_adsp behavior, we may "fix" the first
> issue by passing the blob properly encoded in TLV format, at least.
> OTOH, if we need to keep the current ABI abuse as is, one idea is to
> add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
> control, and we define more strictly how the code should be
> implemented.  Currently we can judge this element as a one that has no
> read/write access but with tlv r/w.  But it's way too unclear.

The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit 
time to switch my mind for this issue.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> On Sep 5 2016 05:45, Takashi Iwai wrote:
> >Yeah, there are obviously some issues in the current implementation of
> >wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
> >patchset as is from a few reasons, these issues still should be
> >addressed.
> 
> OK. I welcome to abandon this patchset ;)
> 
> >First off, passing the binary blob directly via TLV callback is
> >incorrect from the ABI perspective.  When Vinod proposed the idea via
> >TLV access originally, we thought they the data is encoded in TLV
> >format.  Alas, the resulted code didn't do that and it slipped into
> >the upstream without consideration.
> 
> +1
> 
> >Besides that, the second problem is the count value returned via
> >snd_ctl_elem_info, as mentioned in the above.  It's beyond the
> >original control API design, and a kind of illegal usage.
> 
> +1
> 
> >(Well, it's a philosophical argument: what one would expect for an
> > element that has neither read nor write access...?)
> 
> It's an element with no sense for applications. A waste of codes in kernel
> land.
> 
> >So, at this point, the main question is whether we keep this access
> >pattern as is, as a sort of official one, and put some exceptional
> >rule.  Charles, how is the situation?  Has it been already deployed to
> >real systems?
> >
> >If we may still change the wm_adsp behavior, we may "fix" the first
> >issue by passing the blob properly encoded in TLV format, at least.
> >OTOH, if we need to keep the current ABI abuse as is, one idea is to
> >add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
> >control, and we define more strictly how the code should be
> >implemented.  Currently we can judge this element as a one that has no
> >read/write access but with tlv r/w.  But it's way too unclear.
> 
> The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit
> time to switch my mind for this issue.

Perhaps we should add this as a topic for discussion at the Audio
mini-conference? If the general feeling is that this feature is
badly designed we should certainly be looking at what we can do
to improve it.

I do very much like the idea of the additional access flag as I
said. Wrapping the data in a TLV structure we would have to think
about a little more though as the code has shipped in several
kernel versions at this point.

We are starting to have a few customers use a 4.4 kernel which
does include these controls, all our previous backports had used
a system of partitioning the controls up into multiple 512 byte
controls as these binary TLV controls were not supported. So
there is a little bit of friction to major changes to the ABI,
but I don't think its insurmountable as long as the functionality
remains the same.

But we need to get Intel involved in that discussion too, as they
have used these controls quite widely as well.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul]

On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > On Sep 5 2016 05:45, Takashi Iwai wrote:
> > >Yeah, there are obviously some issues in the current implementation of
> > >wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
> > >patchset as is from a few reasons, these issues still should be
> > >addressed.
> > 
> > OK. I welcome to abandon this patchset ;)
> > 
> > >First off, passing the binary blob directly via TLV callback is
> > >incorrect from the ABI perspective.  When Vinod proposed the idea via
> > >TLV access originally, we thought they the data is encoded in TLV
> > >format.  Alas, the resulted code didn't do that and it slipped into
> > >the upstream without consideration.
> > 
> > +1
> > 
> > >Besides that, the second problem is the count value returned via
> > >snd_ctl_elem_info, as mentioned in the above.  It's beyond the
> > >original control API design, and a kind of illegal usage.
> > 
> > +1
> > 
> > >(Well, it's a philosophical argument: what one would expect for an
> > > element that has neither read nor write access...?)
> > 
> > It's an element with no sense for applications. A waste of codes in kernel
> > land.
> > 
> > >So, at this point, the main question is whether we keep this access
> > >pattern as is, as a sort of official one, and put some exceptional
> > >rule.  Charles, how is the situation?  Has it been already deployed to
> > >real systems?
> > >
> > >If we may still change the wm_adsp behavior, we may "fix" the first
> > >issue by passing the blob properly encoded in TLV format, at least.
> > >OTOH, if we need to keep the current ABI abuse as is, one idea is to
> > >add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
> > >control, and we define more strictly how the code should be
> > >implemented.  Currently we can judge this element as a one that has no
> > >read/write access but with tlv r/w.  But it's way too unclear.
> > 
> > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit
> > time to switch my mind for this issue.
> 
> Perhaps we should add this as a topic for discussion at the Audio
> mini-conference? If the general feeling is that this feature is
> badly designed we should certainly be looking at what we can do
> to improve it.
> 
> I do very much like the idea of the additional access flag as I
> said. Wrapping the data in a TLV structure we would have to think
> about a little more though as the code has shipped in several
> kernel versions at this point.
> 
> We are starting to have a few customers use a 4.4 kernel which
> does include these controls, all our previous backports had used
> a system of partitioning the controls up into multiple 512 byte
> controls as these binary TLV controls were not supported. So
> there is a little bit of friction to major changes to the ABI,
> but I don't think its insurmountable as long as the functionality
> remains the same.
> 
> But we need to get Intel involved in that discussion too, as they
> have used these controls quite widely as well.

Sorry havent been able to follow this yet :(


But yes current Skylake Chromebooks ship with this code so we cant break it.

I am not sure what is the issue with API though. (sorry haven't read the
thread yet). The tlv was designed to allow people send bytes larger than 512
down to kernel. The Type cna be anything (implementation specific, though we
haven't used it yet), length the blob length and then the bytes blob.
We provide a tunnel and pass these to DSP. They maybe module coefficients,
hotwording blobs etc.

Sure, we can this at u-conf :) F2F is always much better ...

Thanks
-- 
~Vinod

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Mon, 12 Sep 2016 17:25:31 +0200,
Vinod Koul wrote:
> 
> On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > > On Sep 5 2016 05:45, Takashi Iwai wrote:
> > > >Yeah, there are obviously some issues in the current implementation of
> > > >wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
> > > >patchset as is from a few reasons, these issues still should be
> > > >addressed.
> > > 
> > > OK. I welcome to abandon this patchset ;)
> > > 
> > > >First off, passing the binary blob directly via TLV callback is
> > > >incorrect from the ABI perspective.  When Vinod proposed the idea via
> > > >TLV access originally, we thought they the data is encoded in TLV
> > > >format.  Alas, the resulted code didn't do that and it slipped into
> > > >the upstream without consideration.
> > > 
> > > +1
> > > 
> > > >Besides that, the second problem is the count value returned via
> > > >snd_ctl_elem_info, as mentioned in the above.  It's beyond the
> > > >original control API design, and a kind of illegal usage.
> > > 
> > > +1
> > > 
> > > >(Well, it's a philosophical argument: what one would expect for an
> > > > element that has neither read nor write access...?)
> > > 
> > > It's an element with no sense for applications. A waste of codes in kernel
> > > land.
> > > 
> > > >So, at this point, the main question is whether we keep this access
> > > >pattern as is, as a sort of official one, and put some exceptional
> > > >rule.  Charles, how is the situation?  Has it been already deployed to
> > > >real systems?
> > > >
> > > >If we may still change the wm_adsp behavior, we may "fix" the first
> > > >issue by passing the blob properly encoded in TLV format, at least.
> > > >OTOH, if we need to keep the current ABI abuse as is, one idea is to
> > > >add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
> > > >control, and we define more strictly how the code should be
> > > >implemented.  Currently we can judge this element as a one that has no
> > > >read/write access but with tlv r/w.  But it's way too unclear.
> > > 
> > > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit
> > > time to switch my mind for this issue.
> > 
> > Perhaps we should add this as a topic for discussion at the Audio
> > mini-conference? If the general feeling is that this feature is
> > badly designed we should certainly be looking at what we can do
> > to improve it.
> > 
> > I do very much like the idea of the additional access flag as I
> > said. Wrapping the data in a TLV structure we would have to think
> > about a little more though as the code has shipped in several
> > kernel versions at this point.
> > 
> > We are starting to have a few customers use a 4.4 kernel which
> > does include these controls, all our previous backports had used
> > a system of partitioning the controls up into multiple 512 byte
> > controls as these binary TLV controls were not supported. So
> > there is a little bit of friction to major changes to the ABI,
> > but I don't think its insurmountable as long as the functionality
> > remains the same.
> > 
> > But we need to get Intel involved in that discussion too, as they
> > have used these controls quite widely as well.
> 
> Sorry havent been able to follow this yet :(
> 
> 
> But yes current Skylake Chromebooks ship with this code so we cant break it.
> 
> I am not sure what is the issue with API though. (sorry haven't read the
> thread yet). The tlv was designed to allow people send bytes larger than 512
> down to kernel. The Type cna be anything (implementation specific, though we
> haven't used it yet), length the blob length and then the bytes blob.

Yes, and this part is missing in wm_adsp driver.  It passes the blob
without TLV encoding, i.e. starting from the offset zero without type
and length encoding.

> We provide a tunnel and pass these to DSP. They maybe module coefficients,
> hotwording blobs etc.

So, does Intel driver pass the blob in TLV format?  Then we have two
different implementations.

Also, still another point is to be decided: is passing an arbitrary
size via info callback for an element without read/write access bits
(but with TLV bit) a right behavior?

> Sure, we can this at u-conf :) F2F is always much better ...

Well, the u-conf is still over a month ahead.  I think we should keep
discussing on ML before u-conf.


thanks,

Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote:
> On Mon, 12 Sep 2016 17:25:31 +0200,
> Vinod Koul wrote:
> > 
> > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > > > On Sep 5 2016 05:45, Takashi Iwai wrote:
> > 
> > Sorry havent been able to follow this yet :(
> > 
> > 
> > But yes current Skylake Chromebooks ship with this code so we cant break it.
> > 
> > I am not sure what is the issue with API though. (sorry haven't read the
> > thread yet). The tlv was designed to allow people send bytes larger than 512
> > down to kernel. The Type cna be anything (implementation specific, though we
> > haven't used it yet), length the blob length and then the bytes blob.
> 
> Yes, and this part is missing in wm_adsp driver.  It passes the blob
> without TLV encoding, i.e. starting from the offset zero without type
> and length encoding.
> 
> > We provide a tunnel and pass these to DSP. They maybe module coefficients,
> > hotwording blobs etc.
> 
> So, does Intel driver pass the blob in TLV format?  Then we have two
> different implementations.
> 

So looking at the Skylake code it does look like certainly the
read opertaion returns a TLV header, its a bit less clear with
the write operation it looks like it does use one but sometimes
it will be passed straight through to the firmware.

So looks more like we really should take the pain of the ABI
change and update wm_adsp to be consistent, two implementations
is not good. Although I do feel we should add/strip the header in
ASoC rather than expecting the driver callbacks to do so, seems
odd to push that requirement into end drivers, it has certainly
taken me by surpise. Also I guess with the current tinyalsa
implementation the end user has to add/strip the headers whereas
really I would have thought tinyalsa should be doing that.

> Also, still another point is to be decided: is passing an arbitrary
> size via info callback for an element without read/write access bits
> (but with TLV bit) a right behavior?
> 

So I guess the question would be if you couldn't read the
controls size from info how would you find out the control size?

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Mon, 12 Sep 2016 18:03:16 +0200,
Charles Keepax wrote:
> 
> On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote:
> > On Mon, 12 Sep 2016 17:25:31 +0200,
> > Vinod Koul wrote:
> > > 
> > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> > > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > > > > On Sep 5 2016 05:45, Takashi Iwai wrote:
> > > 
> > > Sorry havent been able to follow this yet :(
> > > 
> > > 
> > > But yes current Skylake Chromebooks ship with this code so we cant break it.
> > > 
> > > I am not sure what is the issue with API though. (sorry haven't read the
> > > thread yet). The tlv was designed to allow people send bytes larger than 512
> > > down to kernel. The Type cna be anything (implementation specific, though we
> > > haven't used it yet), length the blob length and then the bytes blob.
> > 
> > Yes, and this part is missing in wm_adsp driver.  It passes the blob
> > without TLV encoding, i.e. starting from the offset zero without type
> > and length encoding.
> > 
> > > We provide a tunnel and pass these to DSP. They maybe module coefficients,
> > > hotwording blobs etc.
> > 
> > So, does Intel driver pass the blob in TLV format?  Then we have two
> > different implementations.
> > 
> 
> So looking at the Skylake code it does look like certainly the
> read opertaion returns a TLV header, its a bit less clear with
> the write operation it looks like it does use one but sometimes
> it will be passed straight through to the firmware.
> 
> So looks more like we really should take the pain of the ABI
> change and update wm_adsp to be consistent, two implementations
> is not good. Although I do feel we should add/strip the header in
> ASoC rather than expecting the driver callbacks to do so, seems
> odd to push that requirement into end drivers, it has certainly
> taken me by surpise. Also I guess with the current tinyalsa
> implementation the end user has to add/strip the headers whereas
> really I would have thought tinyalsa should be doing that.

If we can align the behavior, it'd be great.
And, if we can fix the wm_adsp side, we can have the common handler of
TLV inside the ASoC caller side, as one of Sakamoto-san's patches
showed.

> > Also, still another point is to be decided: is passing an arbitrary
> > size via info callback for an element without read/write access bits
> > (but with TLV bit) a right behavior?
> > 
> 
> So I guess the question would be if you couldn't read the
> controls size from info how would you find out the control size?

Well, I'm not against the idea to expose the size in callback.  The
behavior without read/write bits is just undefined, and we need a
clear definition to avoid further confusion.  I guess the introduction
of a new flag would be the start.


thanks,

Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Mon, Sep 12, 2016 at 06:28:56PM +0200, Takashi Iwai wrote:
> On Mon, 12 Sep 2016 18:03:16 +0200,
> Charles Keepax wrote:
> > 
> > On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote:
> > > On Mon, 12 Sep 2016 17:25:31 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote:
> > > > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote:
> > > Also, still another point is to be decided: is passing an arbitrary
> > > size via info callback for an element without read/write access bits
> > > (but with TLV bit) a right behavior?
> > > 
> > 
> > So I guess the question would be if you couldn't read the
> > controls size from info how would you find out the control size?
> 
> Well, I'm not against the idea to expose the size in callback.  The
> behavior without read/write bits is just undefined, and we need a
> clear definition to avoid further confusion.  I guess the introduction
> of a new flag would be the start.

The trouble with that is you have to do a read/write to find out
the size you need to read or write though. So you end up doing
weird stuff like a dummy zero length read or something, which
feels a bit icky.

One other thing to consider is that if we add the new flag it
might be nice to allow normal read/write operations on the
controls as well (only accessing the first 512 bytes of the
control). Certainly for the ADSP control I would have
liked to have done this to preserve better backwards
compatibility with older user-spaces.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote:
> Takashi Iwai wrote:
> > TLV has never been and (will never be) an API to handle a
> > generic binary stream.
> 
> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> defined types.  (But we cannot change soc-wm-adsp to support only proper
> TLV data, because this would introduce a regression.)
> 

It's worth pointing out there are more users than just us, the
Intel Skylake stuff being I guess the most important user.

> 
> Anyway, the separate length value could be useful only for drivers (like
> soc-wm-adsp) that use a binary stream where TLV data should have been
> used.  But the software that writes these coefficients to the WM ADSP
> driver is very hardware specific anyway, and it presumably already works
> without knowing the returned length value.  So there is no case where
> this patchset _actually_ improves the interface.
> 

The software that writes these coefficients to wm_adsp is
not hardware specific at all its just regular amixer and
tinymix. These TLV controls are just like any other ALSA control,
normal ALSA controls don't return the length that was written
because the control is a certain size and the write either
succeeds or fails, the same is true here.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Clemens Ladisch]

Charles Keepax wrote:
> On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote:
>> Anyway, the separate length value could be useful only for drivers (like
>> soc-wm-adsp) that use a binary stream where TLV data should have been
>> used.  But the software that writes these coefficients to the WM ADSP
>> driver is very hardware specific anyway, and it presumably already works
>> without knowing the returned length value.  So there is no case where
>> this patchset _actually_ improves the interface.
>
> The software that writes these coefficients to wm_adsp is
> not hardware specific at all its just regular amixer and
> tinymix.

As far as I can see, neither amixer nor tinymix support writing
or "command"ing TLV data.  Do you mean alsactl or alsaucm?

(And I notice that when alsa-lib's UCM loads TLV data from a file,
it does check that the second word contains the correct size.  Is
this value also correct when reading TLV from these controls?)

> These TLV controls are just like any other ALSA control

You're treating it like one, but actually TLV is not a control type but
metadata attached to a control.


Regards,
Clemens

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Wed, Aug 31, 2016 at 03:24:31PM +0200, Clemens Ladisch wrote:
> Charles Keepax wrote:
> > On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote:
> >> Anyway, the separate length value could be useful only for drivers (like
> >> soc-wm-adsp) that use a binary stream where TLV data should have been
> >> used.  But the software that writes these coefficients to the WM ADSP
> >> driver is very hardware specific anyway, and it presumably already works
> >> without knowing the returned length value.  So there is no case where
> >> this patchset _actually_ improves the interface.
> >
> > The software that writes these coefficients to wm_adsp is
> > not hardware specific at all its just regular amixer and
> > tinymix.
> 
> As far as I can see, neither amixer nor tinymix support writing
> or "command"ing TLV data.  Do you mean alsactl or alsaucm?
> 
> (And I notice that when alsa-lib's UCM loads TLV data from a file,
> it does check that the second word contains the correct size.  Is
> this value also correct when reading TLV from these controls?)
> 

Certainly tinyalsa does:

https://github.com/tinyalsa/tinyalsa/commit/45b2d047b8c2f4d9d1d87244f7f981db8234c906

I thought the Intel guys added support to amixer as well although
I may have been mistaken about that. I will try to find some time
to look at that a little more closely. I guess my main concern
here was the "very hardware specific" the intention is not to
require hardware specific user-space code to use these controls.

> > These TLV controls are just like any other ALSA control
> 
> You're treating it like one, but actually TLV is not a control type but
> metadata attached to a control.
> 

Yes but that metadata is just being used to transfer the actual
data for the control in this case. Personally I would just expect
that control to function the same way as any other binary control
for the user, just it is >512 bytes. Different things happen on
the back end but it would be nice if it felt the same to the
person using it. Which is mostly the case though tinyalsa at the
moment.

Thanks,
Charles

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul]

On Wed, Aug 31, 2016 at 03:18:47PM +0100, Charles Keepax wrote:
> On Wed, Aug 31, 2016 at 03:24:31PM +0200, Clemens Ladisch wrote:
> > Charles Keepax wrote:
> > > On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote:
> > >> Anyway, the separate length value could be useful only for drivers (like
> > >> soc-wm-adsp) that use a binary stream where TLV data should have been
> > >> used.  But the software that writes these coefficients to the WM ADSP
> > >> driver is very hardware specific anyway, and it presumably already works
> > >> without knowing the returned length value.  So there is no case where
> > >> this patchset _actually_ improves the interface.
> > >
> > > The software that writes these coefficients to wm_adsp is
> > > not hardware specific at all its just regular amixer and
> > > tinymix.
> > 
> > As far as I can see, neither amixer nor tinymix support writing
> > or "command"ing TLV data.  Do you mean alsactl or alsaucm?
> > 
> > (And I notice that when alsa-lib's UCM loads TLV data from a file,
> > it does check that the second word contains the correct size.  Is
> > this value also correct when reading TLV from these controls?)
> > 
> 
> Certainly tinyalsa does:
> 
> https://github.com/tinyalsa/tinyalsa/commit/45b2d047b8c2f4d9d1d87244f7f981db8234c906
> 
> I thought the Intel guys added support to amixer as well although
> I may have been mistaken about that. I will try to find some time
> to look at that a little more closely. I guess my main concern
> here was the "very hardware specific" the intention is not to
> require hardware specific user-space code to use these controls.

Note yet, unfortunately :(

I have been trying to do that for amixer. I should be able to complete at
least before our uConf :)

> 
> > > These TLV controls are just like any other ALSA control
> > 
> > You're treating it like one, but actually TLV is not a control type but
> > metadata attached to a control.
> > 
> 
> Yes but that metadata is just being used to transfer the actual
> data for the control in this case. Personally I would just expect
> that control to function the same way as any other binary control
> for the user, just it is >512 bytes. Different things happen on
> the back end but it would be nice if it felt the same to the
> person using it. Which is mostly the case though tinyalsa at the
> moment.
> 
> Thanks,
> Charles

-- 
~Vinod

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

Clemens,

On Aug 31 2016 20:54, Clemens Ladisch wrote:
> Takashi Iwai wrote:
>> TLV has never been and (will never be) an API to handle a
>> generic binary stream.
> 
> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> defined types.  (But we cannot change soc-wm-adsp to support only proper
> TLV data, because this would introduce a regression.)

For our information, I wrote a patch including your idea.


Thanks for your bright comment ;)

Takashi Sakamoto

---- 8< ----

>From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Fri, 2 Sep 2016 00:12:20 +0900
Subject: [PATCH] ALSA: control: coefficient support

---
 include/uapi/sound/asound.h |  5 +++++
 include/uapi/sound/tlv.h    |  1 +
 sound/soc/codecs/wm_adsp.c  |  3 ++-
 sound/soc/soc-ops.c         | 37 ++++++++++++++++++++++++++++++++++---
 sound/soc/soc-topology.c    |  3 ++-
 5 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 609cadb..69c0585 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t;
 #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE		(1<<5)	/* TLV write is possible */
 #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
(SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE)
 #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND	(1<<6)	/* TLV command is
possible */
+/*
+ * Arbitrary data is accepted for coefficients, instead of pure
threshold level
+ * information.
+ */
+#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF		(1<<7)
 #define SNDRV_CTL_ELEM_ACCESS_INACTIVE		(1<<8)	/* control does actually
nothing, but may be updated */
 #define SNDRV_CTL_ELEM_ACCESS_LOCK		(1<<9)	/* write lock */
 #define SNDRV_CTL_ELEM_ACCESS_OWNER		(1<<10)	/* write lock owner */
diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
index ffc4f20..fd1c867 100644
--- a/include/uapi/sound/tlv.h
+++ b/include/uapi/sound/tlv.h
@@ -19,6 +19,7 @@
 #define SNDRV_CTL_TLVT_DB_RANGE 3	/* dB range container */
 #define SNDRV_CTL_TLVT_DB_MINMAX 4	/* dB scale with min/max */
 #define SNDRV_CTL_TLVT_DB_MINMAX_MUTE 5	/* dB scale with min/max with
mute */
+#define SNDRV_CTL_TLVT_COEFF	6	/* Arbitrary data */

 /*
  * channel-mapping TLV items
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 21fbe7d..525d70b 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -930,7 +930,8 @@ static unsigned int wmfw_convert_flags(unsigned int
in, unsigned int len)
 		wr = SNDRV_CTL_ELEM_ACCESS_TLV_WRITE;
 		vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE;

-		out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
+		out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK |
+		      SNDRV_CTL_ELEM_ACCESS_TLV_COEFF;
 	} else {
 		rd = SNDRV_CTL_ELEM_ACCESS_READ;
 		wr = SNDRV_CTL_ELEM_ACCESS_WRITE;
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index a513a34..14cdd59 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -31,6 +31,7 @@
 #include <sound/soc.h>
 #include <sound/soc-dpcm.h>
 #include <sound/initval.h>
+#include <sound/tlv.h>

 /**
  * snd_soc_info_enum_double - enumerated double mixer info callback
@@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol
*kcontrol, int op_flag,
 				unsigned int size, unsigned int __user *tlv)
 {
 	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
-	unsigned int count = size < params->max ? size : params->max;
+	unsigned int count;
+	unsigned int type;
 	int ret = -ENXIO;

+	/*
+	 * The TLV packet can transfer numerical ID for one control element.
+	 * But ALSA control core don't tell it to each implementation of
+	 * TLV callback. Here, instead, use the first volatile data to
+	 * check access information.
+	 */
+	if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF))
+		return -ENXIO;
+
+	/* The data should be constructed according to TLV protocol. */
+	if (copy_from_user(&type, tlv, sizeof(unsigned int)))
+		return -EFAULT;
+
+	/* The type should be an arbitrary data. */
+	if (type != SNDRV_CTL_TLVT_COEFF)
+		return -ENXIO;
+
+	/*
+	 * The second element of TLV packet payload means the length of an
+	 * actual data. But this implementation is going to trim it.
+	 */
+	count = min_t(unsigned int, tlv[1] - sizeof(unsigned int), params->max);
+
 	switch (op_flag) {
 	case SNDRV_CTL_TLV_OP_READ:
 		if (params->get)
-			ret = params->get(kcontrol, tlv, count);
+			ret = params->get(kcontrol, tlv + 2, count);
 		break;
 	case SNDRV_CTL_TLV_OP_WRITE:
 		if (params->put)
-			ret = params->put(kcontrol, tlv, count);
+			ret = params->put(kcontrol, tlv + 2, count);
 		break;
 	}
+
+	if (ret >= 0) {
+		if (copy_to_user(tlv + 1, &count, sizeof(unsigned int)))
+			return -EFAULT;
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_bytes_tlv_callback);
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index ee7f15a..a8f6060 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -507,7 +507,8 @@ static int soc_tplg_kcontrol_bind_io(struct
snd_soc_tplg_ctl_hdr *hdr,
 	if (hdr->ops.info == SND_SOC_TPLG_CTL_BYTES
 		&& k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
 		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
-		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK
+		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF) {
 		struct soc_bytes_ext *sbe;
 		struct snd_soc_tplg_bytes_control *be;

-- 
2.7.4

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Iwai]

On Fri, 02 Sep 2016 13:18:04 +0200,
Takashi Sakamoto wrote:
> 
> Clemens,
> 
> On Aug 31 2016 20:54, Clemens Ladisch wrote:
> > Takashi Iwai wrote:
> >> TLV has never been and (will never be) an API to handle a
> >> generic binary stream.
> > 
> > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> > or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> > defined types.  (But we cannot change soc-wm-adsp to support only proper
> > TLV data, because this would introduce a regression.)
> 
> For our information, I wrote a patch including your idea.
> 
> 
> Thanks for your bright comment ;)
> 
> Takashi Sakamoto
> 
> ---- 8< ----
> 
> >From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Fri, 2 Sep 2016 00:12:20 +0900
> Subject: [PATCH] ALSA: control: coefficient support
> 
> ---
>  include/uapi/sound/asound.h |  5 +++++
>  include/uapi/sound/tlv.h    |  1 +
>  sound/soc/codecs/wm_adsp.c  |  3 ++-
>  sound/soc/soc-ops.c         | 37 ++++++++++++++++++++++++++++++++++---
>  sound/soc/soc-topology.c    |  3 ++-
>  5 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 609cadb..69c0585 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t;
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE		(1<<5)	/* TLV write is possible */
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE)
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND	(1<<6)	/* TLV command is
> possible */
> +/*
> + * Arbitrary data is accepted for coefficients, instead of pure
> threshold level
> + * information.
> + */
> +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF		(1<<7)
>  #define SNDRV_CTL_ELEM_ACCESS_INACTIVE		(1<<8)	/* control does actually
> nothing, but may be updated */
>  #define SNDRV_CTL_ELEM_ACCESS_LOCK		(1<<9)	/* write lock */
>  #define SNDRV_CTL_ELEM_ACCESS_OWNER		(1<<10)	/* write lock owner */

The introduction of a new flag is a good idea (although it's better to
be named not specifically to coef).  Then we can skip such elements to
be accessed from alsactl or amixer.

However...

> @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol
> *kcontrol, int op_flag,
>  				unsigned int size, unsigned int __user *tlv)
>  {
>  	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
> -	unsigned int count = size < params->max ? size : params->max;
> +	unsigned int count;
> +	unsigned int type;
>  	int ret = -ENXIO;
> 
> +	/*
> +	 * The TLV packet can transfer numerical ID for one control element.
> +	 * But ALSA control core don't tell it to each implementation of
> +	 * TLV callback. Here, instead, use the first volatile data to
> +	 * check access information.
> +	 */
> +	if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF))
> +		return -ENXIO;
> +
> +	/* The data should be constructed according to TLV protocol. */
> +	if (copy_from_user(&type, tlv, sizeof(unsigned int)))
> +		return -EFAULT;
> +
> +	/* The type should be an arbitrary data. */
> +	if (type != SNDRV_CTL_TLVT_COEFF)
> +		return -ENXIO;

This breaks the already existing application and is a regression, as
Clemens mentioned.  This should have been forced from the beginning,
but it's too late to put now.  So, it's likely no-go.

But, the addition of the flag is helpful alone.  Could you concentrate
on that and resubmit?


thanks,

Takashi

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Sep 3 2016 01:05, Takashi Iwai wrote:
> But, the addition of the flag is helpful alone.  Could you concentrate
> on that and resubmit?

Before starting it, I'd like to wait for comments from Clemens Ladisch,
Charles Keepax and Vinod Koul, in a view of application interfaces
stable, safe and tolerant for long term usage. I'm unwilling to use my
time more for light-minded work done to bring confusions to hardware-
abstraction layers for user land applications.

But, if possible, I'd request you to re-think about my proposal to
return processed byte of TLV packet payload to user land, again. It
looks to be intrusive, but in my taste, it doesn't bring much impacts to
applications, than adding new flags to kernel/userspace interfaces.

The length of processed bytes from drivers in ALSA SoC part might help
developers to get current value of 'struct soc_bytes_ext.max' of each
control element set. As of kernel 4.8, soc-da7218/nau8825/wm5102 modules
implements the coefficients, and it's already in ALSA topology design. I
believe it better to assist developers for the feature, than judging
it's coarse and abuse for good APIs.


Regards

Takashi Sakamoto

Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Charles Keepax]

On Fri, Sep 02, 2016 at 08:18:04PM +0900, Takashi Sakamoto wrote:
> Clemens,
> 
> On Aug 31 2016 20:54, Clemens Ladisch wrote:
> > Takashi Iwai wrote:
> >> TLV has never been and (will never be) an API to handle a
> >> generic binary stream.
> > 
> > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> > or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> > defined types.  (But we cannot change soc-wm-adsp to support only proper
> > TLV data, because this would introduce a regression.)
> 
> For our information, I wrote a patch including your idea.
> 
> 
> Thanks for your bright comment ;)
> 
> Takashi Sakamoto
> 
> ---- 8< ----
> 
> >From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Fri, 2 Sep 2016 00:12:20 +0900
> Subject: [PATCH] ALSA: control: coefficient support
> 
> ---
>  include/uapi/sound/asound.h |  5 +++++
>  include/uapi/sound/tlv.h    |  1 +
>  sound/soc/codecs/wm_adsp.c  |  3 ++-
>  sound/soc/soc-ops.c         | 37 ++++++++++++++++++++++++++++++++++---
>  sound/soc/soc-topology.c    |  3 ++-
>  5 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 609cadb..69c0585 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t;
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE		(1<<5)	/* TLV write is possible */
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE)
>  #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND	(1<<6)	/* TLV command is
> possible */
> +/*
> + * Arbitrary data is accepted for coefficients, instead of pure
> threshold level
> + * information.
> + */
> +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF		(1<<7)

Yeah the addition of a new flag here would very much get my vote.
The only way at the moment to tell if you are dealing with this
type of control from user-space is to look for
a control of type SNDRV_CTL_ELEM_TYPE_BYTES that doesn't have
SNDRV_CTL_ELEM_ACCESS_READ or SNDRV_CTL_ELEM_ACCESS_WRITE. Which
is far from ideal.

>  #define SNDRV_CTL_ELEM_ACCESS_INACTIVE		(1<<8)	/* control does actually
> nothing, but may be updated */
>  #define SNDRV_CTL_ELEM_ACCESS_LOCK		(1<<9)	/* write lock */
>  #define SNDRV_CTL_ELEM_ACCESS_OWNER		(1<<10)	/* write lock owner */
> diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
> index ffc4f20..fd1c867 100644
> --- a/include/uapi/sound/tlv.h
> +++ b/include/uapi/sound/tlv.h
> @@ -19,6 +19,7 @@
>  #define SNDRV_CTL_TLVT_DB_RANGE 3	/* dB range container */
>  #define SNDRV_CTL_TLVT_DB_MINMAX 4	/* dB scale with min/max */
>  #define SNDRV_CTL_TLVT_DB_MINMAX_MUTE 5	/* dB scale with min/max with
> mute */
> +#define SNDRV_CTL_TLVT_COEFF	6	/* Arbitrary data */
> 
>  /*
>   * channel-mapping TLV items
> diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
> index 21fbe7d..525d70b 100644
> --- a/sound/soc/codecs/wm_adsp.c
> +++ b/sound/soc/codecs/wm_adsp.c
> @@ -930,7 +930,8 @@ static unsigned int wmfw_convert_flags(unsigned int
> in, unsigned int len)
>  		wr = SNDRV_CTL_ELEM_ACCESS_TLV_WRITE;
>  		vol = SNDRV_CTL_ELEM_ACCESS_VOLATILE;
> 
> -		out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
> +		out = SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK |
> +		      SNDRV_CTL_ELEM_ACCESS_TLV_COEFF;
>  	} else {
>  		rd = SNDRV_CTL_ELEM_ACCESS_READ;
>  		wr = SNDRV_CTL_ELEM_ACCESS_WRITE;
> diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
> index a513a34..14cdd59 100644
> --- a/sound/soc/soc-ops.c
> +++ b/sound/soc/soc-ops.c
> @@ -31,6 +31,7 @@
>  #include <sound/soc.h>
>  #include <sound/soc-dpcm.h>
>  #include <sound/initval.h>
> +#include <sound/tlv.h>
> 
>  /**
>   * snd_soc_info_enum_double - enumerated double mixer info callback
> @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol
> *kcontrol, int op_flag,
>  				unsigned int size, unsigned int __user *tlv)
>  {
>  	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
> -	unsigned int count = size < params->max ? size : params->max;
> +	unsigned int count;
> +	unsigned int type;
>  	int ret = -ENXIO;
> 
> +	/*
> +	 * The TLV packet can transfer numerical ID for one control element.
> +	 * But ALSA control core don't tell it to each implementation of
> +	 * TLV callback. Here, instead, use the first volatile data to
> +	 * check access information.
> +	 */
> +	if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF))
> +		return -ENXIO;
> +
> +	/* The data should be constructed according to TLV protocol. */
> +	if (copy_from_user(&type, tlv, sizeof(unsigned int)))
> +		return -EFAULT;
> +
> +	/* The type should be an arbitrary data. */
> +	if (type != SNDRV_CTL_TLVT_COEFF)
> +		return -ENXIO;
> +

I agree it is probably a bit late to add this now.

Thanks,
Charles

[PATCH 2/4] ALSA: control: delegate checking the length of data payload to each drivers

[Takashi Sakamoto]

In ioctl(2) for TLV feature of ALSA control interface, an argument is
a pointer to a data of 'struct snd_ctl_tlv' type in user space.
The first field of this structure is for numerical ID of control element
(numid), the second is for the length of the rest in byte unit (length).
The rest is data payload (tlv).

In current implementation, ALSA control core checks the length field
so that the lest is bigger than sizeof(unsigned int) * 2. This is due to
original design of this feature.

Originally, this feature is designed to allow each driver to handle
threshold level information. The rest of array stores the information.
The information is expected to be this formation:
 - type: e.g. SNDRV_CTL_TLVT_CONTAINER.
 - length: length of the rest
 - the rest: information fields

Against the original design, this feature can also be used to operate I/O.
In this case, the data field in 'struct snd_ctl_tlv' is not necessarily
the formation. Therefore, the check in control core is unreasonable.
Perhaps, there's a case to transfer just 1 byte by this feature.

This commit purges the check and delegate it to each drivers. Fortunately,
each implementation in drivers already check the length of payload.

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

diff --git a/sound/core/control.c b/sound/core/control.c
index 485963e..b6f4ebc 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1438,8 +1438,6 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 
 	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
 		return -EFAULT;
-	if (tlv.length < sizeof(unsigned int) * 2)
-		return -EINVAL;
 	if (!tlv.numid)
 		return -EINVAL;
 	down_read(&card->controls_rwsem);
-- 
2.7.4

Re: [PATCH 2/4] ALSA: control: delegate checking the length of data payload to each drivers

[Vinod Koul]

On Tue, Aug 30, 2016 at 08:44:43AM +0900, Takashi Sakamoto wrote:
> In ioctl(2) for TLV feature of ALSA control interface, an argument is
> a pointer to a data of 'struct snd_ctl_tlv' type in user space.
> The first field of this structure is for numerical ID of control element
> (numid), the second is for the length of the rest in byte unit (length).
> The rest is data payload (tlv).
> 
> In current implementation, ALSA control core checks the length field
> so that the lest is bigger than sizeof(unsigned int) * 2. This is due to
> original design of this feature.
> 
> Originally, this feature is designed to allow each driver to handle
> threshold level information. The rest of array stores the information.
> The information is expected to be this formation:
>  - type: e.g. SNDRV_CTL_TLVT_CONTAINER.
>  - length: length of the rest
>  - the rest: information fields
> 
> Against the original design, this feature can also be used to operate I/O.
> In this case, the data field in 'struct snd_ctl_tlv' is not necessarily
> the formation. Therefore, the check in control core is unreasonable.
> Perhaps, there's a case to transfer just 1 byte by this feature.
> 
> This commit purges the check and delegate it to each drivers. Fortunately,
> each implementation in drivers already check the length of payload.

Shouldn't it be other way around, we want to core to check these type of
things and not duplicate over drivers...

> @@ -1438,8 +1438,6 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
>  
>  	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
>  		return -EFAULT;
> -	if (tlv.length < sizeof(unsigned int) * 2)
> -		return -EINVAL;
>  	if (!tlv.numid)
>  		return -EINVAL;
>  	down_read(&card->controls_rwsem);

-- 
~Vinod

[PATCH 3/4] ALSA: control: add kerneldoc for snd_kcontrol_tlv_rw_t

[Takashi Sakamoto]

Currently, there's no documentation about implementation requirement of
snd_kcontrol_tlv_rw_t. This function prototype includes historical
complexities and probably puzzles driver developers.

This commit adds a kerneldoc for this feature.

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

diff --git a/include/sound/control.h b/include/sound/control.h
index 848940c..6c1c4234 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -30,9 +30,27 @@ struct snd_kcontrol;
 typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
 typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
 typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
+
+/**
+ * snd_kcontrol_tlv_rw_t - a type of callback function to operate information
+ *			   for threshold level or arbitrary I/O, via TLV
+ *			   feature of ALSA control interface.
+ * @kcontrol: A pointer to an instance for control element set.
+ * @op_flag: One of SNDRV_CTL_TLV_OP_READ/WRITE/CMD.
+ * @size: The size of buffer in user space in byte unit. After operated, the
+ *	  size should be changed to size of actually operated data in byte unit,
+ * @tlv: A pointer to the buffer in user space.
+ *
+ * Originally, this function is designed to allow each driver to generate and
+ * transfer data about threshold level. Later, this function is used to operate
+ * arbitrary I/O depending on each driver.
+ *
+ * Return: Zero if successful. Negative value in error. Positive value generates
+ *	   an event with SNDRV_CTL_EVENT_MASK_TLV. This return value is received
+ *	   by applications as a result of ioctl(2).
+ */
 typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
-				    int op_flag, /* SNDRV_CTL_TLV_OP_XXX */
-				    unsigned int *size,
+				    int op_flag, unsigned int *size,
 				    unsigned int __user *tlv);
 
 enum {
-- 
2.7.4

[PATCH 4/4] ALSA: control: bump up protocol version to 2.0.8

[Takashi Sakamoto]

In format commits, TLV feature of control interface can return operated
length. This should be clearly distinguished from former implementation.

This commit bumps up protocol version.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/uapi/sound/asound.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 609cadb..34c6f3e 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -805,7 +805,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 7)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 8)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */
-- 
2.7.4