[alsa-devel] [PATCH] ASoC: rt5514: Revert Hotword Model control
This reverts commit eb33869c7206 ("ASoC: rt5514: Guard Hotword Model bytes loading") and commit d18420b0a0b8 ("ASoC: rt5514: expose Hotword Model control")
It is discouraged to use SND_SOC_BYTES_TLV to load arbitrary bytes from userspace to driver. Removing the 'Hotword Model' control until we have a good way to verify the content of hotword model blobs.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org --- sound/soc/codecs/rt5514.c | 63 ----------------------------------------------- sound/soc/codecs/rt5514.h | 3 --- 2 files changed, 66 deletions(-)
diff --git a/sound/soc/codecs/rt5514.c b/sound/soc/codecs/rt5514.c index 250581082e9d..a2722177470e 100644 --- a/sound/soc/codecs/rt5514.c +++ b/sound/soc/codecs/rt5514.c @@ -335,39 +335,6 @@ static int rt5514_dsp_voice_wake_up_put(struct snd_kcontrol *kcontrol, fw = NULL; }
- if (rt5514->model_buf && rt5514->model_len) { -#if IS_ENABLED(CONFIG_SND_SOC_RT5514_SPI) - int ret; - - ret = rt5514_spi_burst_write(0x4ff80000, - rt5514->model_buf, - ((rt5514->model_len / 8) + 1) * 8); - if (ret) { - dev_err(codec->dev, - "Model load failed %d\n", ret); - return ret; - } -#else - dev_err(codec->dev, - "No SPI driver for loading firmware\n"); -#endif - } else { - request_firmware(&fw, RT5514_FIRMWARE3, - codec->dev); - if (fw) { -#if IS_ENABLED(CONFIG_SND_SOC_RT5514_SPI) - rt5514_spi_burst_write(0x4ff80000, - fw->data, - ((fw->size/8)+1)*8); -#else - dev_err(codec->dev, - "No SPI driver to load fw\n"); -#endif - release_firmware(fw); - fw = NULL; - } - } - /* DSP run */ regmap_write(rt5514->i2c_regmap, 0x18002f00, 0x00055148); @@ -382,34 +349,6 @@ static int rt5514_dsp_voice_wake_up_put(struct snd_kcontrol *kcontrol, return 0; }
-static int rt5514_hotword_model_put(struct snd_kcontrol *kcontrol, - const unsigned int __user *bytes, unsigned int size) -{ - struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); - struct rt5514_priv *rt5514 = snd_soc_component_get_drvdata(component); - struct snd_soc_codec *codec = rt5514->codec; - int ret = 0; - - if (rt5514->model_buf || rt5514->model_len < size) { - if (rt5514->model_buf) - devm_kfree(codec->dev, rt5514->model_buf); - rt5514->model_buf = devm_kmalloc(codec->dev, size, GFP_KERNEL); - if (!rt5514->model_buf) { - ret = -ENOMEM; - goto done; - } - } - - /* Skips the TLV header. */ - bytes += 2; - - if (copy_from_user(rt5514->model_buf, bytes, size)) - ret = -EFAULT; -done: - rt5514->model_len = (ret ? 0 : size); - return ret; -} - static const struct snd_kcontrol_new rt5514_snd_controls[] = { SOC_DOUBLE_TLV("MIC Boost Volume", RT5514_ANA_CTRL_MICBST, RT5514_SEL_BSTL_SFT, RT5514_SEL_BSTR_SFT, 8, 0, bst_tlv), @@ -421,8 +360,6 @@ static const struct snd_kcontrol_new rt5514_snd_controls[] = { adc_vol_tlv), SOC_SINGLE_EXT("DSP Voice Wake Up", SND_SOC_NOPM, 0, 1, 0, rt5514_dsp_voice_wake_up_get, rt5514_dsp_voice_wake_up_put), - SND_SOC_BYTES_TLV("Hotword Model", 0x8504, - NULL, rt5514_hotword_model_put), };
/* ADC Mixer*/ diff --git a/sound/soc/codecs/rt5514.h b/sound/soc/codecs/rt5514.h index 07475a9918d2..02bc212a86d9 100644 --- a/sound/soc/codecs/rt5514.h +++ b/sound/soc/codecs/rt5514.h @@ -236,7 +236,6 @@
#define RT5514_FIRMWARE1 "rt5514_dsp_fw1.bin" #define RT5514_FIRMWARE2 "rt5514_dsp_fw2.bin" -#define RT5514_FIRMWARE3 "rt5514_dsp_fw3.bin"
/* System Clock Source */ enum { @@ -263,8 +262,6 @@ struct rt5514_priv { int pll_in; int pll_out; int dsp_enabled; - u8 *model_buf; - unsigned int model_len; };
#endif /* __RT5514_H__ */
On Sep 15 2017 13:32, Hsin-Yu Chao wrote:
This reverts commit eb33869c7206 ("ASoC: rt5514: Guard Hotword Model bytes loading") and commit d18420b0a0b8 ("ASoC: rt5514: expose Hotword Model control")
It is discouraged to use SND_SOC_BYTES_TLV to load arbitrary bytes from userspace to driver. Removing the 'Hotword Model' control until we have a good way to verify the content of hotword model blobs.
Signed-off-by: Hsin-Yu Chao hychao@chromium.org
sound/soc/codecs/rt5514.c | 63 ----------------------------------------------- sound/soc/codecs/rt5514.h | 3 --- 2 files changed, 66 deletions(-)
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
I reviewed this patch on 8bc23183b6fe ('Merge remote-tracking branches 'asoc/fix/adau17x1', 'asoc/fix/rockchip', 'asoc/fix/rt5514' and 'asoc/fix/samsung' into asoc-linus') in Mark's tree.
For the issue I mentioned[1], please have discussions with Intel developers (especially Vinod Koul) and authors of 'sound/soc/codecs/wm_adsp.c' and find better solution for yours.
I note that ALSA hwdep interface for userspace includes 'SNDRV_HWDEP_IOCTL_DSP_LOAD' ioctl(2) command to load binary blob from userspace.
diff --git a/sound/soc/codecs/rt5514.c b/sound/soc/codecs/rt5514.c index 250581082e9d..a2722177470e 100644 --- a/sound/soc/codecs/rt5514.c +++ b/sound/soc/codecs/rt5514.c @@ -335,39 +335,6 @@ static int rt5514_dsp_voice_wake_up_put(struct snd_kcontrol *kcontrol, fw = NULL; }
if (rt5514->model_buf && rt5514->model_len) {-#if IS_ENABLED(CONFIG_SND_SOC_RT5514_SPI)
int ret;ret = rt5514_spi_burst_write(0x4ff80000,rt5514->model_buf,((rt5514->model_len / 8) + 1) * 8);if (ret) {dev_err(codec->dev,"Model load failed %d\n", ret);return ret;}-#else
dev_err(codec->dev,"No SPI driver for loading firmware\n");-#endif
} else {request_firmware(&fw, RT5514_FIRMWARE3,codec->dev);if (fw) {-#if IS_ENABLED(CONFIG_SND_SOC_RT5514_SPI)
rt5514_spi_burst_write(0x4ff80000,fw->data,((fw->size/8)+1)*8);-#else
dev_err(codec->dev,"No SPI driver to load fw\n");-#endif
release_firmware(fw);fw = NULL;}}/* DSP run */ regmap_write(rt5514->i2c_regmap, 0x18002f00, 0x00055148);@@ -382,34 +349,6 @@ static int rt5514_dsp_voice_wake_up_put(struct snd_kcontrol *kcontrol, return 0; }
-static int rt5514_hotword_model_put(struct snd_kcontrol *kcontrol,
const unsigned int __user *bytes, unsigned int size)-{
- struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
- struct rt5514_priv *rt5514 = snd_soc_component_get_drvdata(component);
- struct snd_soc_codec *codec = rt5514->codec;
- int ret = 0;
- if (rt5514->model_buf || rt5514->model_len < size) {
if (rt5514->model_buf)devm_kfree(codec->dev, rt5514->model_buf);rt5514->model_buf = devm_kmalloc(codec->dev, size, GFP_KERNEL);if (!rt5514->model_buf) {ret = -ENOMEM;goto done;}- }
- /* Skips the TLV header. */
- bytes += 2;
- if (copy_from_user(rt5514->model_buf, bytes, size))
ret = -EFAULT;-done:
- rt5514->model_len = (ret ? 0 : size);
- return ret;
-}
- static const struct snd_kcontrol_new rt5514_snd_controls[] = { SOC_DOUBLE_TLV("MIC Boost Volume", RT5514_ANA_CTRL_MICBST, RT5514_SEL_BSTL_SFT, RT5514_SEL_BSTR_SFT, 8, 0, bst_tlv),
@@ -421,8 +360,6 @@ static const struct snd_kcontrol_new rt5514_snd_controls[] = { adc_vol_tlv), SOC_SINGLE_EXT("DSP Voice Wake Up", SND_SOC_NOPM, 0, 1, 0, rt5514_dsp_voice_wake_up_get, rt5514_dsp_voice_wake_up_put),
SND_SOC_BYTES_TLV("Hotword Model", 0x8504,
NULL, rt5514_hotword_model_put),};
/* ADC Mixer*/
diff --git a/sound/soc/codecs/rt5514.h b/sound/soc/codecs/rt5514.h index 07475a9918d2..02bc212a86d9 100644 --- a/sound/soc/codecs/rt5514.h +++ b/sound/soc/codecs/rt5514.h @@ -236,7 +236,6 @@
#define RT5514_FIRMWARE1 "rt5514_dsp_fw1.bin" #define RT5514_FIRMWARE2 "rt5514_dsp_fw2.bin" -#define RT5514_FIRMWARE3 "rt5514_dsp_fw3.bin"
/* System Clock Source */ enum { @@ -263,8 +262,6 @@ struct rt5514_priv { int pll_in; int pll_out; int dsp_enabled;
u8 *model_buf;
unsigned int model_len; };
#endif /* __RT5514_H__ */
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125474.h...
Regards
Takashi Sakamoto
On Fri, Sep 15, 2017 at 01:59:09PM +0900, Takashi Sakamoto wrote:
I note that ALSA hwdep interface for userspace includes 'SNDRV_HWDEP_IOCTL_DSP_LOAD' ioctl(2) command to load binary blob from userspace.
The problem I see with that is that these things are generally settings that people might want to change dynamically at runtime so you really want to be able to change these things via controls like everything else about the configuration. If they were just fixed and unchanging in a system then request_firmware() would be a good mechanism that's a bit more modern than the ioctl.
participants (3)
-
Hsin-Yu Chao -
Mark Brown -
Takashi Sakamoto