[alsa-devel] [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 at 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 at intel.com>
> > > Signed-off-by: Dharageswari R <dharageswari.r at intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty at intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul at 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

--