diff mbox

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

Message ID 1495768803-24217-3-git-send-email-vinod.koul@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vinod Koul May 26, 2017, 3:20 a.m. UTC 
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(+)

Comments

Takashi Iwai May 26, 2017, 6:57 a.m. UTC | #1 
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
Vinod Koul May 26, 2017, 7:35 a.m. UTC | #2 
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
Subhransu S. Prusty May 30, 2017, 8:40 a.m. UTC | #3 
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
Takashi Iwai May 30, 2017, 9:02 a.m. UTC | #4 
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
Takashi Iwai May 30, 2017, 9:11 a.m. UTC | #5 
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
diff mbox

Patch

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