[alsa-devel] [PATCH 2/3] ASoC: Intel: Skylake: Add enum control for mic selection
Takashi Iwai
tiwai at suse.de
Tue May 30 11:11:54 CEST 2017
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 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.
>
> 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
More information about the Alsa-devel
mailing list