[PATCH 0/3] ASoC: Intel: Skylake: Add Support for MIC select module

[PATCH 0/3] ASoC: Intel: Skylake: Add Support for MIC select module

[Vinod Koul]

This series adds support in DSP to add support for MIC select module which
allows user to select 1 to N channels from DMIC channel input.

Further it also adds support in bxt-rt298 machine for such a module.

Kranthikumar, GudishaX (3):
  ASoC: Intel: Skylake: Add mic-select module type
  ASoC: Intel: Skylake: Add enum control for mic selection
  ASoC: Intel: Boards: Add 4-channel DMIC fixup.

 sound/soc/intel/boards/bxt_rt298.c           |   7 +-
 sound/soc/intel/skylake/skl-messages.c       |   2 +
 sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-topology.h       |  20 ++++
 sound/soc/intel/skylake/skl-tplg-interface.h |   2 +
 5 files changed, 169 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 1/3] ASoC: Intel: Skylake: Add mic-select module type

[Vinod Koul]

From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>

mic-select module is a DSP module, which is used to select one or more
input channels.

This patch adds mic-select module type.

Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl-messages.c       | 2 ++
 sound/soc/intel/skylake/skl-tplg-interface.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
index ab1adc0c9cc3..5a465020ebd8 100644
--- a/sound/soc/intel/skylake/skl-messages.c
+++ b/sound/soc/intel/skylake/skl-messages.c
@@ -707,6 +707,7 @@ static u16 skl_get_module_param_size(struct skl_sst *ctx,
 		return param_size;
 
 	case SKL_MODULE_TYPE_BASE_OUTFMT:
+	case SKL_MODULE_TYPE_MIC_SELECT:
 	case SKL_MODULE_TYPE_KPB:
 		return sizeof(struct skl_base_outfmt_cfg);
 
@@ -761,6 +762,7 @@ static int skl_set_module_format(struct skl_sst *ctx,
 		break;
 
 	case SKL_MODULE_TYPE_BASE_OUTFMT:
+	case SKL_MODULE_TYPE_MIC_SELECT:
 	case SKL_MODULE_TYPE_KPB:
 		skl_set_base_outfmt_format(ctx, module_config, *param_data);
 		break;
diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h
index 7a2febf99019..c22517bd2161 100644
--- a/sound/soc/intel/skylake/skl-tplg-interface.h
+++ b/sound/soc/intel/skylake/skl-tplg-interface.h
@@ -82,6 +82,7 @@ enum skl_module_type {
 	SKL_MODULE_TYPE_ALGO,
 	SKL_MODULE_TYPE_BASE_OUTFMT,
 	SKL_MODULE_TYPE_KPB,
+	SKL_MODULE_TYPE_MIC_SELECT,
 };
 
 enum skl_core_affinity {
-- 
1.9.1

Applied "ASoC: Intel: Skylake: Add mic-select module type" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Skylake: Add mic-select module type

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From db6879efb9d1d48ff9c2bd49dde05ecf757d73cf Mon Sep 17 00:00:00 2001
From: Dharageswari R <dharageswari.r@intel.com>
Date: Wed, 31 May 2017 10:30:24 +0530
Subject: [PATCH] ASoC: Intel: Skylake: Add mic-select module type

mic-select module is a DSP module, which is used to select one or more
input channels.

This patch adds mic-select module type.

Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/skylake/skl-messages.c       | 2 ++
 sound/soc/intel/skylake/skl-tplg-interface.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
index ab1adc0c9cc3..5a465020ebd8 100644
--- a/sound/soc/intel/skylake/skl-messages.c
+++ b/sound/soc/intel/skylake/skl-messages.c
@@ -707,6 +707,7 @@ static u16 skl_get_module_param_size(struct skl_sst *ctx,
 		return param_size;
 
 	case SKL_MODULE_TYPE_BASE_OUTFMT:
+	case SKL_MODULE_TYPE_MIC_SELECT:
 	case SKL_MODULE_TYPE_KPB:
 		return sizeof(struct skl_base_outfmt_cfg);
 
@@ -761,6 +762,7 @@ static int skl_set_module_format(struct skl_sst *ctx,
 		break;
 
 	case SKL_MODULE_TYPE_BASE_OUTFMT:
+	case SKL_MODULE_TYPE_MIC_SELECT:
 	case SKL_MODULE_TYPE_KPB:
 		skl_set_base_outfmt_format(ctx, module_config, *param_data);
 		break;
diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h
index 7a2febf99019..c22517bd2161 100644
--- a/sound/soc/intel/skylake/skl-tplg-interface.h
+++ b/sound/soc/intel/skylake/skl-tplg-interface.h
@@ -82,6 +82,7 @@ enum skl_module_type {
 	SKL_MODULE_TYPE_ALGO,
 	SKL_MODULE_TYPE_BASE_OUTFMT,
 	SKL_MODULE_TYPE_KPB,
+	SKL_MODULE_TYPE_MIC_SELECT,
 };
 
 enum skl_core_affinity {
-- 
2.11.0

[PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Vinod Koul]

From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>

User may prefer to select data from particular mics. A mic-select module
in DSP allows this selection.

Create possible enum controls to allow user to select a combination of
mics to capture data from. Based on the user selection, parameters are
generated and passed to mic-select module during init.

Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-topology.h       |  20 ++++
 sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
 3 files changed, 164 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index b687ae455a61..141cfbe40c3a 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -36,6 +36,19 @@
 #define SKL_IN_DIR_BIT_MASK		BIT(0)
 #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
 
+static const int mic_mono_list[] = {
+0, 1, 2, 3,
+};
+static const int mic_stereo_list[][SKL_CH_STEREO] = {
+{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
+};
+static const int mic_trio_list[][SKL_CH_TRIO] = {
+{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
+};
+static const int mic_quatro_list[][SKL_CH_QUATRO] = {
+{0, 1, 2, 3},
+};
+
 void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
 {
 	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
@@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
+	struct skl_module_cfg *mconfig = w->priv;
+	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
+	u32 ch_type = *((u32 *)ec->dobj.private);
+
+	if (mconfig->dmic_ch_type == ch_type)
+		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
+	else
+		ucontrol->value.integer.value[0] = 0;
+
+	return 0;
+}
+
+static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
+	struct skl_mic_sel_config *mic_cfg, struct device *dev)
+{
+	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
+
+	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
+	sp_cfg->set_params = SKL_PARAM_SET;
+	sp_cfg->param_id = 0x00;
+	if (!sp_cfg->caps) {
+		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
+		if (!sp_cfg->caps)
+			return -ENOMEM;
+	}
+
+	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
+	mic_cfg->flags = 0;
+	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
+
+	return 0;
+}
+
+static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
+	struct skl_module_cfg *mconfig = w->priv;
+	struct skl_mic_sel_config mic_cfg = {0};
+	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
+	u32 ch_type = *((u32 *)ec->dobj.private);
+	const int *list;
+	u8 in_ch, out_ch, index;
+
+	mconfig->dmic_ch_type = ch_type;
+	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
+
+	/* enum control index 0 is INVALID, so no channels to be set */
+	if (mconfig->dmic_ch_combo_index == 0)
+		return 0;
+
+	/* No valid channel selection map for index 0, so offset by 1 */
+	index = mconfig->dmic_ch_combo_index - 1;
+
+	switch (ch_type) {
+	case SKL_CH_MONO:
+		list = &mic_mono_list[index];
+		break;
+
+	case SKL_CH_STEREO:
+		list = mic_stereo_list[index];
+		break;
+
+	case SKL_CH_TRIO:
+		list = mic_trio_list[index];
+		break;
+
+	case SKL_CH_QUATRO:
+		list = mic_quatro_list[index];
+		break;
+
+	default:
+		dev_err(w->dapm->dev,
+				"Invalid channel %d for mic_select module\n",
+				ch_type);
+		return -EINVAL;
+
+	}
+
+	/* channel type enum map to number of chanels for that type */
+	for (out_ch = 0; out_ch < ch_type; out_ch++) {
+		in_ch = list[out_ch];
+		mic_cfg.blob[out_ch][in_ch] = SKL_DEFAULT_MIC_SEL_GAIN;
+	}
+
+	return skl_fill_mic_sel_params(mconfig, &mic_cfg, w->dapm->dev);
+}
+
 /*
  * Fill the dma id for host and link. In case of passthrough
  * pipeline, this will both host and link in the same
@@ -1666,6 +1771,11 @@ int skl_tplg_be_update_params(struct snd_soc_dai *dai,
 					skl_tplg_tlv_control_set},
 };
 
+static const struct snd_soc_tplg_kcontrol_ops skl_tplg_kcontrol_ops[] = {
+	{SKL_CONTROL_TYPE_MIC_SELECT, skl_tplg_mic_control_get,
+					skl_tplg_mic_control_set},
+};
+
 static int skl_tplg_fill_pipe_tkn(struct device *dev,
 			struct skl_pipe *pipe, u32 tkn,
 			u32 tkn_val)
@@ -2390,14 +2500,34 @@ static int skl_init_algo_data(struct device *dev, struct soc_bytes_ext *be,
 	return 0;
 }
 
+static int skl_init_enum_data(struct device *dev, struct soc_enum *se,
+				struct snd_soc_tplg_enum_control *ec)
+{
+
+	void *data;
+
+	if (ec->priv.size) {
+		data = devm_kzalloc(dev, sizeof(ec->priv.size), GFP_KERNEL);
+		if (!data)
+			return -ENOMEM;
+		memcpy(data, ec->priv.data, ec->priv.size);
+		se->dobj.private = data;
+	}
+
+	return 0;
+
+}
+
 static int skl_tplg_control_load(struct snd_soc_component *cmpnt,
 				struct snd_kcontrol_new *kctl,
 				struct snd_soc_tplg_ctl_hdr *hdr)
 {
 	struct soc_bytes_ext *sb;
 	struct snd_soc_tplg_bytes_control *tplg_bc;
+	struct snd_soc_tplg_enum_control *tplg_ec;
 	struct hdac_ext_bus *ebus  = snd_soc_component_get_drvdata(cmpnt);
 	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct soc_enum *se;
 
 	switch (hdr->ops.info) {
 	case SND_SOC_TPLG_CTL_BYTES:
@@ -2411,6 +2541,17 @@ static int skl_tplg_control_load(struct snd_soc_component *cmpnt,
 		}
 		break;
 
+	case SND_SOC_TPLG_CTL_ENUM:
+		tplg_ec = container_of(hdr,
+				struct snd_soc_tplg_enum_control, hdr);
+		if (kctl->access & SNDRV_CTL_ELEM_ACCESS_READWRITE) {
+			se = (struct soc_enum *)kctl->private_value;
+			if (tplg_ec->priv.size)
+				return skl_init_enum_data(bus->dev, se,
+						tplg_ec);
+		}
+		break;
+
 	default:
 		dev_warn(bus->dev, "Control load not supported %d:%d:%d\n",
 			hdr->ops.get, hdr->ops.put, hdr->ops.info);
@@ -2639,6 +2780,8 @@ static int skl_manifest_load(struct snd_soc_component *cmpnt,
 	.control_load = skl_tplg_control_load,
 	.bytes_ext_ops = skl_tlv_ops,
 	.bytes_ext_ops_count = ARRAY_SIZE(skl_tlv_ops),
+	.io_ops = skl_tplg_kcontrol_ops,
+	.io_ops_count = ARRAY_SIZE(skl_tplg_kcontrol_ops),
 	.manifest = skl_manifest_load,
 };
 
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index cc64d6bdb4f6..3f51a0a00093 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -39,6 +39,11 @@
 #define MODULE_MAX_IN_PINS	8
 #define MODULE_MAX_OUT_PINS	8
 
+#define SKL_MIC_CH_SUPPORT	4
+#define SKL_MIC_MAX_CH_SUPPORT	8
+#define SKL_DEFAULT_MIC_SEL_GAIN	0x3FF
+#define SKL_MIC_SEL_SWITCH	0x3
+
 enum skl_channel_index {
 	SKL_CHANNEL_LEFT = 0,
 	SKL_CHANNEL_RIGHT = 1,
@@ -309,6 +314,8 @@ struct skl_module_cfg {
 	u8 dev_type;
 	u8 dma_id;
 	u8 time_slot;
+	u8 dmic_ch_combo_index;
+	u32 dmic_ch_type;
 	u32 params_fixup;
 	u32 converter;
 	u32 vbus_id;
@@ -342,6 +349,19 @@ struct skl_module_deferred_bind {
 	struct list_head node;
 };
 
+struct skl_mic_sel_config {
+	u16 mic_switch;
+	u16 flags;
+	u16 blob[SKL_MIC_MAX_CH_SUPPORT][SKL_MIC_MAX_CH_SUPPORT];
+} __packed;
+
+enum skl_channel {
+	SKL_CH_MONO = 1,
+	SKL_CH_STEREO = 2,
+	SKL_CH_TRIO = 3,
+	SKL_CH_QUATRO = 4,
+};
+
 static inline struct skl *get_skl_ctx(struct device *dev)
 {
 	struct hdac_ext_bus *ebus = dev_get_drvdata(dev);
diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h
index c22517bd2161..f8d1749a2e0c 100644
--- a/sound/soc/intel/skylake/skl-tplg-interface.h
+++ b/sound/soc/intel/skylake/skl-tplg-interface.h
@@ -24,6 +24,7 @@
  * SST types start at higher to avoid any overlapping in future
  */
 #define SKL_CONTROL_TYPE_BYTE_TLV	0x100
+#define SKL_CONTROL_TYPE_MIC_SELECT	0x102
 
 #define HDA_SST_CFG_MAX	900 /* size of copier cfg*/
 #define MAX_IN_QUEUE 8
-- 
1.9.1

Re: [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Takashi Iwai]

On Fri, 26 May 2017 05:20:02 +0200,
Vinod Koul wrote:
> 
> From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>
> 
> User may prefer to select data from particular mics. A mic-select module
> in DSP allows this selection.
> 
> Create possible enum controls to allow user to select a combination of
> mics to capture data from. Based on the user selection, parameters are
> generated and passed to mic-select module during init.
> 
> Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
> Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
>  sound/soc/intel/skylake/skl-topology.h       |  20 ++++
>  sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
>  3 files changed, 164 insertions(+)
> 
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index b687ae455a61..141cfbe40c3a 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -36,6 +36,19 @@
>  #define SKL_IN_DIR_BIT_MASK		BIT(0)
>  #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
>  
> +static const int mic_mono_list[] = {
> +0, 1, 2, 3,
> +};
> +static const int mic_stereo_list[][SKL_CH_STEREO] = {
> +{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
> +};
> +static const int mic_trio_list[][SKL_CH_TRIO] = {
> +{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
> +};
> +static const int mic_quatro_list[][SKL_CH_QUATRO] = {
> +{0, 1, 2, 3},
> +};
> +
>  void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
>  {
>  	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
> @@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
>  	return 0;
>  }
>  
> +static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> +	struct skl_module_cfg *mconfig = w->priv;
> +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> +	u32 ch_type = *((u32 *)ec->dobj.private);
> +
> +	if (mconfig->dmic_ch_type == ch_type)
> +		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
> +	else
> +		ucontrol->value.integer.value[0] = 0;
> +
> +	return 0;
> +}
> +
> +static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
> +	struct skl_mic_sel_config *mic_cfg, struct device *dev)
> +{
> +	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
> +
> +	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
> +	sp_cfg->set_params = SKL_PARAM_SET;
> +	sp_cfg->param_id = 0x00;
> +	if (!sp_cfg->caps) {
> +		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
> +		if (!sp_cfg->caps)
> +			return -ENOMEM;
> +	}
> +
> +	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
> +	mic_cfg->flags = 0;
> +	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
> +
> +	return 0;
> +}
> +
> +static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> +	struct skl_module_cfg *mconfig = w->priv;
> +	struct skl_mic_sel_config mic_cfg = {0};
> +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> +	u32 ch_type = *((u32 *)ec->dobj.private);
> +	const int *list;
> +	u8 in_ch, out_ch, index;
> +
> +	mconfig->dmic_ch_type = ch_type;
> +	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
> +
> +	/* enum control index 0 is INVALID, so no channels to be set */
> +	if (mconfig->dmic_ch_combo_index == 0)
> +		return 0;
> +
> +	/* No valid channel selection map for index 0, so offset by 1 */
> +	index = mconfig->dmic_ch_combo_index - 1;
> +
> +	switch (ch_type) {
> +	case SKL_CH_MONO:
> +		list = &mic_mono_list[index];
> +		break;
> +
> +	case SKL_CH_STEREO:
> +		list = mic_stereo_list[index];
> +		break;
> +
> +	case SKL_CH_TRIO:
> +		list = mic_trio_list[index];
> +		break;
> +
> +	case SKL_CH_QUATRO:
> +		list = mic_quatro_list[index];
> +		break;

How do you guarantee that index is in the array range?
It looks easy to make things vulnerable with a firmware.


thanks,

Takashi

Re: [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Vinod Koul]

On Fri, May 26, 2017 at 08:57:44AM +0200, Takashi Iwai wrote:
> On Fri, 26 May 2017 05:20:02 +0200,
> Vinod Koul wrote:
> > 
> > From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>
> > 
> > User may prefer to select data from particular mics. A mic-select module
> > in DSP allows this selection.
> > 
> > Create possible enum controls to allow user to select a combination of
> > mics to capture data from. Based on the user selection, parameters are
> > generated and passed to mic-select module during init.
> > 
> > Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
> > Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
> >  sound/soc/intel/skylake/skl-topology.h       |  20 ++++
> >  sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
> >  3 files changed, 164 insertions(+)
> > 
> > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> > index b687ae455a61..141cfbe40c3a 100644
> > --- a/sound/soc/intel/skylake/skl-topology.c
> > +++ b/sound/soc/intel/skylake/skl-topology.c
> > @@ -36,6 +36,19 @@
> >  #define SKL_IN_DIR_BIT_MASK		BIT(0)
> >  #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
> >  
> > +static const int mic_mono_list[] = {
> > +0, 1, 2, 3,
> > +};
> > +static const int mic_stereo_list[][SKL_CH_STEREO] = {
> > +{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
> > +};
> > +static const int mic_trio_list[][SKL_CH_TRIO] = {
> > +{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
> > +};
> > +static const int mic_quatro_list[][SKL_CH_QUATRO] = {
> > +{0, 1, 2, 3},
> > +};
> > +
> >  void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
> >  {
> >  	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
> > @@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
> >  	return 0;
> >  }
> >  
> > +static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
> > +		struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > +	struct skl_module_cfg *mconfig = w->priv;
> > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > +
> > +	if (mconfig->dmic_ch_type == ch_type)
> > +		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
> > +	else
> > +		ucontrol->value.integer.value[0] = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
> > +	struct skl_mic_sel_config *mic_cfg, struct device *dev)
> > +{
> > +	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
> > +
> > +	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
> > +	sp_cfg->set_params = SKL_PARAM_SET;
> > +	sp_cfg->param_id = 0x00;
> > +	if (!sp_cfg->caps) {
> > +		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
> > +		if (!sp_cfg->caps)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
> > +	mic_cfg->flags = 0;
> > +	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
> > +
> > +	return 0;
> > +}
> > +
> > +static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
> > +			struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > +	struct skl_module_cfg *mconfig = w->priv;
> > +	struct skl_mic_sel_config mic_cfg = {0};
> > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > +	const int *list;
> > +	u8 in_ch, out_ch, index;
> > +
> > +	mconfig->dmic_ch_type = ch_type;
> > +	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
> > +
> > +	/* enum control index 0 is INVALID, so no channels to be set */
> > +	if (mconfig->dmic_ch_combo_index == 0)
> > +		return 0;
> > +
> > +	/* No valid channel selection map for index 0, so offset by 1 */
> > +	index = mconfig->dmic_ch_combo_index - 1;
> > +
> > +	switch (ch_type) {
> > +	case SKL_CH_MONO:
> > +		list = &mic_mono_list[index];
> > +		break;
> > +
> > +	case SKL_CH_STEREO:
> > +		list = mic_stereo_list[index];
> > +		break;
> > +
> > +	case SKL_CH_TRIO:
> > +		list = mic_trio_list[index];
> > +		break;
> > +
> > +	case SKL_CH_QUATRO:
> > +		list = mic_quatro_list[index];
> > +		break;
> 
> How do you guarantee that index is in the array range?
> It looks easy to make things vulnerable with a firmware.

Yes a good catch, we should right check the index here and see if it is
within bounds, will fix it and update

Thanks
-- 
~Vinod

Re: [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Subhransu S. Prusty]

On Fri, May 26, 2017 at 01:05:41PM +0530, Vinod Koul wrote:
> On Fri, May 26, 2017 at 08:57:44AM +0200, Takashi Iwai wrote:
> > On Fri, 26 May 2017 05:20:02 +0200,
> > Vinod Koul wrote:
> > > 
> > > From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>
> > > 
> > > User may prefer to select data from particular mics. A mic-select module
> > > in DSP allows this selection.
> > > 
> > > Create possible enum controls to allow user to select a combination of
> > > mics to capture data from. Based on the user selection, parameters are
> > > generated and passed to mic-select module during init.
> > > 
> > > Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
> > > Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > >  sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
> > >  sound/soc/intel/skylake/skl-topology.h       |  20 ++++
> > >  sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
> > >  3 files changed, 164 insertions(+)
> > > 
> > > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> > > index b687ae455a61..141cfbe40c3a 100644
> > > --- a/sound/soc/intel/skylake/skl-topology.c
> > > +++ b/sound/soc/intel/skylake/skl-topology.c
> > > @@ -36,6 +36,19 @@
> > >  #define SKL_IN_DIR_BIT_MASK		BIT(0)
> > >  #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
> > >  
> > > +static const int mic_mono_list[] = {
> > > +0, 1, 2, 3,
> > > +};
> > > +static const int mic_stereo_list[][SKL_CH_STEREO] = {
> > > +{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
> > > +};
> > > +static const int mic_trio_list[][SKL_CH_TRIO] = {
> > > +{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
> > > +};
> > > +static const int mic_quatro_list[][SKL_CH_QUATRO] = {
> > > +{0, 1, 2, 3},
> > > +};
> > > +
> > >  void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
> > >  {
> > >  	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
> > > @@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
> > >  	return 0;
> > >  }
> > >  
> > > +static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
> > > +		struct snd_ctl_elem_value *ucontrol)
> > > +{
> > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > +	struct skl_module_cfg *mconfig = w->priv;
> > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > +
> > > +	if (mconfig->dmic_ch_type == ch_type)
> > > +		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
> > > +	else
> > > +		ucontrol->value.integer.value[0] = 0;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
> > > +	struct skl_mic_sel_config *mic_cfg, struct device *dev)
> > > +{
> > > +	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
> > > +
> > > +	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
> > > +	sp_cfg->set_params = SKL_PARAM_SET;
> > > +	sp_cfg->param_id = 0x00;
> > > +	if (!sp_cfg->caps) {
> > > +		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
> > > +		if (!sp_cfg->caps)
> > > +			return -ENOMEM;
> > > +	}
> > > +
> > > +	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
> > > +	mic_cfg->flags = 0;
> > > +	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
> > > +			struct snd_ctl_elem_value *ucontrol)
> > > +{
> > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > +	struct skl_module_cfg *mconfig = w->priv;
> > > +	struct skl_mic_sel_config mic_cfg = {0};
> > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > +	const int *list;
> > > +	u8 in_ch, out_ch, index;
> > > +
> > > +	mconfig->dmic_ch_type = ch_type;
> > > +	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
> > > +
> > > +	/* enum control index 0 is INVALID, so no channels to be set */
> > > +	if (mconfig->dmic_ch_combo_index == 0)
> > > +		return 0;
> > > +
> > > +	/* No valid channel selection map for index 0, so offset by 1 */
> > > +	index = mconfig->dmic_ch_combo_index - 1;
> > > +
> > > +	switch (ch_type) {
> > > +	case SKL_CH_MONO:
> > > +		list = &mic_mono_list[index];
> > > +		break;
> > > +
> > > +	case SKL_CH_STEREO:
> > > +		list = mic_stereo_list[index];
> > > +		break;
> > > +
> > > +	case SKL_CH_TRIO:
> > > +		list = mic_trio_list[index];
> > > +		break;
> > > +
> > > +	case SKL_CH_QUATRO:
> > > +		list = mic_quatro_list[index];
> > > +		break;
> > 
> > How do you guarantee that index is in the array range?
> > It looks easy to make things vulnerable with a firmware.
> 
> Yes a good catch, we should right check the index here and see if it is
> within bounds, will fix it and update

Hi Takashi,

Shouldn't the alsa kernel or user space check for the index of enum control
to be within the allowed range? If so, the driver doesn't need to check.

Regards,
Subhransu

> 
> Thanks
> -- 
> ~Vinod

-- 

Re: [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Takashi Iwai]

On Tue, 30 May 2017 10:40:27 +0200,
Subhransu S. Prusty wrote:
> 
> On Fri, May 26, 2017 at 01:05:41PM +0530, Vinod Koul wrote:
> > On Fri, May 26, 2017 at 08:57:44AM +0200, Takashi Iwai wrote:
> > > On Fri, 26 May 2017 05:20:02 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>
> > > > 
> > > > User may prefer to select data from particular mics. A mic-select module
> > > > in DSP allows this selection.
> > > > 
> > > > Create possible enum controls to allow user to select a combination of
> > > > mics to capture data from. Based on the user selection, parameters are
> > > > generated and passed to mic-select module during init.
> > > > 
> > > > Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
> > > > Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
> > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > ---
> > > >  sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
> > > >  sound/soc/intel/skylake/skl-topology.h       |  20 ++++
> > > >  sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
> > > >  3 files changed, 164 insertions(+)
> > > > 
> > > > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> > > > index b687ae455a61..141cfbe40c3a 100644
> > > > --- a/sound/soc/intel/skylake/skl-topology.c
> > > > +++ b/sound/soc/intel/skylake/skl-topology.c
> > > > @@ -36,6 +36,19 @@
> > > >  #define SKL_IN_DIR_BIT_MASK		BIT(0)
> > > >  #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
> > > >  
> > > > +static const int mic_mono_list[] = {
> > > > +0, 1, 2, 3,
> > > > +};
> > > > +static const int mic_stereo_list[][SKL_CH_STEREO] = {
> > > > +{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
> > > > +};
> > > > +static const int mic_trio_list[][SKL_CH_TRIO] = {
> > > > +{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
> > > > +};
> > > > +static const int mic_quatro_list[][SKL_CH_QUATRO] = {
> > > > +{0, 1, 2, 3},
> > > > +};
> > > > +
> > > >  void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
> > > >  {
> > > >  	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
> > > > @@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
> > > > +		struct snd_ctl_elem_value *ucontrol)
> > > > +{
> > > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > > +	struct skl_module_cfg *mconfig = w->priv;
> > > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > > +
> > > > +	if (mconfig->dmic_ch_type == ch_type)
> > > > +		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
> > > > +	else
> > > > +		ucontrol->value.integer.value[0] = 0;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
> > > > +	struct skl_mic_sel_config *mic_cfg, struct device *dev)
> > > > +{
> > > > +	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
> > > > +
> > > > +	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
> > > > +	sp_cfg->set_params = SKL_PARAM_SET;
> > > > +	sp_cfg->param_id = 0x00;
> > > > +	if (!sp_cfg->caps) {
> > > > +		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
> > > > +		if (!sp_cfg->caps)
> > > > +			return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
> > > > +	mic_cfg->flags = 0;
> > > > +	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
> > > > +			struct snd_ctl_elem_value *ucontrol)
> > > > +{
> > > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > > +	struct skl_module_cfg *mconfig = w->priv;
> > > > +	struct skl_mic_sel_config mic_cfg = {0};
> > > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > > +	const int *list;
> > > > +	u8 in_ch, out_ch, index;
> > > > +
> > > > +	mconfig->dmic_ch_type = ch_type;
> > > > +	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
> > > > +
> > > > +	/* enum control index 0 is INVALID, so no channels to be set */
> > > > +	if (mconfig->dmic_ch_combo_index == 0)
> > > > +		return 0;
> > > > +
> > > > +	/* No valid channel selection map for index 0, so offset by 1 */
> > > > +	index = mconfig->dmic_ch_combo_index - 1;
> > > > +
> > > > +	switch (ch_type) {
> > > > +	case SKL_CH_MONO:
> > > > +		list = &mic_mono_list[index];
> > > > +		break;
> > > > +
> > > > +	case SKL_CH_STEREO:
> > > > +		list = mic_stereo_list[index];
> > > > +		break;
> > > > +
> > > > +	case SKL_CH_TRIO:
> > > > +		list = mic_trio_list[index];
> > > > +		break;
> > > > +
> > > > +	case SKL_CH_QUATRO:
> > > > +		list = mic_quatro_list[index];
> > > > +		break;
> > > 
> > > How do you guarantee that index is in the array range?
> > > It looks easy to make things vulnerable with a firmware.
> > 
> > Yes a good catch, we should right check the index here and see if it is
> > within bounds, will fix it and update
> 
> Hi Takashi,
> 
> Shouldn't the alsa kernel or user space check for the index of enum control
> to be within the allowed range? If so, the driver doesn't need to check.

It's not the enum index at all.
Here the index is deduced in a very complex way:

	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
	struct skl_module_cfg *mconfig = w->priv;
	.....
	index = mconfig->dmic_ch_combo_index - 1;

So I feel uneasy unless it's explicitly declared to be really safe.


Takashi

Re: [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection

[Takashi Iwai]

On Tue, 30 May 2017 11:02:01 +0200,
Takashi Iwai wrote:
> 
> On Tue, 30 May 2017 10:40:27 +0200,
> Subhransu S. Prusty wrote:
> > 
> > On Fri, May 26, 2017 at 01:05:41PM +0530, Vinod Koul wrote:
> > > On Fri, May 26, 2017 at 08:57:44AM +0200, Takashi Iwai wrote:
> > > > On Fri, 26 May 2017 05:20:02 +0200,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>
> > > > > 
> > > > > User may prefer to select data from particular mics. A mic-select module
> > > > > in DSP allows this selection.
> > > > > 
> > > > > Create possible enum controls to allow user to select a combination of
> > > > > mics to capture data from. Based on the user selection, parameters are
> > > > > generated and passed to mic-select module during init.
> > > > > 
> > > > > Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
> > > > > Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > ---
> > > > >  sound/soc/intel/skylake/skl-topology.c       | 143 +++++++++++++++++++++++++++
> > > > >  sound/soc/intel/skylake/skl-topology.h       |  20 ++++
> > > > >  sound/soc/intel/skylake/skl-tplg-interface.h |   1 +
> > > > >  3 files changed, 164 insertions(+)
> > > > > 
> > > > > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> > > > > index b687ae455a61..141cfbe40c3a 100644
> > > > > --- a/sound/soc/intel/skylake/skl-topology.c
> > > > > +++ b/sound/soc/intel/skylake/skl-topology.c
> > > > > @@ -36,6 +36,19 @@
> > > > >  #define SKL_IN_DIR_BIT_MASK		BIT(0)
> > > > >  #define SKL_PIN_COUNT_MASK		GENMASK(7, 4)
> > > > >  
> > > > > +static const int mic_mono_list[] = {
> > > > > +0, 1, 2, 3,
> > > > > +};
> > > > > +static const int mic_stereo_list[][SKL_CH_STEREO] = {
> > > > > +{0, 1}, {0, 2}, {0, 3}, {1, 2}, {1, 3}, {2, 3},
> > > > > +};
> > > > > +static const int mic_trio_list[][SKL_CH_TRIO] = {
> > > > > +{0, 1, 2}, {0, 1, 3}, {0, 2, 3}, {1, 2, 3},
> > > > > +};
> > > > > +static const int mic_quatro_list[][SKL_CH_QUATRO] = {
> > > > > +{0, 1, 2, 3},
> > > > > +};
> > > > > +
> > > > >  void skl_tplg_d0i3_get(struct skl *skl, enum d0i3_capability caps)
> > > > >  {
> > > > >  	struct skl_d0i3_data *d0i3 =  &skl->skl_sst->d0i3;
> > > > > @@ -1314,6 +1327,98 @@ static int skl_tplg_tlv_control_set(struct snd_kcontrol *kcontrol,
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static int skl_tplg_mic_control_get(struct snd_kcontrol *kcontrol,
> > > > > +		struct snd_ctl_elem_value *ucontrol)
> > > > > +{
> > > > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > > > +	struct skl_module_cfg *mconfig = w->priv;
> > > > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > > > +
> > > > > +	if (mconfig->dmic_ch_type == ch_type)
> > > > > +		ucontrol->value.integer.value[0] = mconfig->dmic_ch_combo_index;
> > > > > +	else
> > > > > +		ucontrol->value.integer.value[0] = 0;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int skl_fill_mic_sel_params(struct skl_module_cfg *mconfig,
> > > > > +	struct skl_mic_sel_config *mic_cfg, struct device *dev)
> > > > > +{
> > > > > +	struct skl_specific_cfg *sp_cfg = &mconfig->formats_config;
> > > > > +
> > > > > +	sp_cfg->caps_size = sizeof(struct skl_mic_sel_config);
> > > > > +	sp_cfg->set_params = SKL_PARAM_SET;
> > > > > +	sp_cfg->param_id = 0x00;
> > > > > +	if (!sp_cfg->caps) {
> > > > > +		sp_cfg->caps = devm_kzalloc(dev, sp_cfg->caps_size, GFP_KERNEL);
> > > > > +		if (!sp_cfg->caps)
> > > > > +			return -ENOMEM;
> > > > > +	}
> > > > > +
> > > > > +	mic_cfg->mic_switch = SKL_MIC_SEL_SWITCH;
> > > > > +	mic_cfg->flags = 0;
> > > > > +	memcpy(sp_cfg->caps, mic_cfg, sp_cfg->caps_size);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int skl_tplg_mic_control_set(struct snd_kcontrol *kcontrol,
> > > > > +			struct snd_ctl_elem_value *ucontrol)
> > > > > +{
> > > > > +	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> > > > > +	struct skl_module_cfg *mconfig = w->priv;
> > > > > +	struct skl_mic_sel_config mic_cfg = {0};
> > > > > +	struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
> > > > > +	u32 ch_type = *((u32 *)ec->dobj.private);
> > > > > +	const int *list;
> > > > > +	u8 in_ch, out_ch, index;
> > > > > +
> > > > > +	mconfig->dmic_ch_type = ch_type;
> > > > > +	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];
> > > > > +
> > > > > +	/* enum control index 0 is INVALID, so no channels to be set */
> > > > > +	if (mconfig->dmic_ch_combo_index == 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	/* No valid channel selection map for index 0, so offset by 1 */
> > > > > +	index = mconfig->dmic_ch_combo_index - 1;
> > > > > +
> > > > > +	switch (ch_type) {
> > > > > +	case SKL_CH_MONO:
> > > > > +		list = &mic_mono_list[index];
> > > > > +		break;
> > > > > +
> > > > > +	case SKL_CH_STEREO:
> > > > > +		list = mic_stereo_list[index];
> > > > > +		break;
> > > > > +
> > > > > +	case SKL_CH_TRIO:
> > > > > +		list = mic_trio_list[index];
> > > > > +		break;
> > > > > +
> > > > > +	case SKL_CH_QUATRO:
> > > > > +		list = mic_quatro_list[index];
> > > > > +		break;
> > > > 
> > > > How do you guarantee that index is in the array range?
> > > > It looks easy to make things vulnerable with a firmware.
> > > 
> > > Yes a good catch, we should right check the index here and see if it is
> > > within bounds, will fix it and update
> > 
> > Hi Takashi,
> > 
> > Shouldn't the alsa kernel or user space check for the index of enum control
> > to be within the allowed range? If so, the driver doesn't need to check.
> 
> It's not the enum index at all.
> Here the index is deduced in a very complex way:
> 
> 	struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol);
> 	struct skl_module_cfg *mconfig = w->priv;
> 	.....
> 	index = mconfig->dmic_ch_combo_index - 1;
> 
> So I feel uneasy unless it's explicitly declared to be really safe.

Wait, further looking at it, it has the following line:

	mconfig->dmic_ch_type = ch_type;
	mconfig->dmic_ch_combo_index = ucontrol->value.integer.value[0];

OK, so it's a *value*, not the index.

And, the value range isn't checked in the control core side.  Checking
it is the responsibility of the callback.

And, yet another bug in the above: you shouldn't access to
ucontrol->value.integer.xxx for enum ctl.  Instead, access
ucontrol->value.enumerated.xxx.


Takashi

[PATCH 3/3] ASoC: Intel: Boards: Add 4-channel DMIC fixup.

[Vinod Koul]

From: "Kranthikumar, GudishaX" <gudishax.kranthikumar@intel.com>

This patch adds a 4-channel dmic fixup so that DMIC copier will receive
4 channel data and further selection will be done by mic-select module.

Signed-off-by: Kranthikumar, GudishaX <gudishax.kranthikumar@intel.com>
Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/boards/bxt_rt298.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/boards/bxt_rt298.c b/sound/soc/intel/boards/bxt_rt298.c
index 1a68d043c803..36ee7480e9f1 100644
--- a/sound/soc/intel/boards/bxt_rt298.c
+++ b/sound/soc/intel/boards/bxt_rt298.c
@@ -222,16 +222,13 @@ static int broxton_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
 {
 	struct snd_interval *channels = hw_param_interval(params,
 						SNDRV_PCM_HW_PARAM_CHANNELS);
-	if (params_channels(params) == 2)
-		channels->min = channels->max = 2;
-	else
-		channels->min = channels->max = 4;
+	channels->min = channels->max = 4;
 
 	return 0;
 }
 
 static unsigned int channels_dmic[] = {
-	2, 4,
+	1, 2, 3, 4,
 };
 
 static struct snd_pcm_hw_constraint_list constraints_dmic_channels = {
-- 
1.9.1

Applied "ASoC: Intel: Boards: Add 4-channel DMIC fixup." to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: Boards: Add 4-channel DMIC fixup.

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From e8883cb61aa0a91980222e5e9d114100783eb7e2 Mon Sep 17 00:00:00 2001
From: Dharageswari R <dharageswari.r@intel.com>
Date: Wed, 31 May 2017 10:30:26 +0530
Subject: [PATCH] ASoC: Intel: Boards: Add 4-channel DMIC fixup.

This patch adds a 4-channel dmic fixup so that DMIC copier will receive
4 channel data and further selection will be done by mic-select module.

Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/bxt_rt298.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/boards/bxt_rt298.c b/sound/soc/intel/boards/bxt_rt298.c
index 1a68d043c803..36ee7480e9f1 100644
--- a/sound/soc/intel/boards/bxt_rt298.c
+++ b/sound/soc/intel/boards/bxt_rt298.c
@@ -222,16 +222,13 @@ static int broxton_dmic_fixup(struct snd_soc_pcm_runtime *rtd,
 {
 	struct snd_interval *channels = hw_param_interval(params,
 						SNDRV_PCM_HW_PARAM_CHANNELS);
-	if (params_channels(params) == 2)
-		channels->min = channels->max = 2;
-	else
-		channels->min = channels->max = 4;
+	channels->min = channels->max = 4;
 
 	return 0;
 }
 
 static unsigned int channels_dmic[] = {
-	2, 4,
+	1, 2, 3, 4,
 };
 
 static struct snd_pcm_hw_constraint_list constraints_dmic_channels = {
-- 
2.11.0