* [PATCH] ASoC: rt5514: Revert Hotword Model control
@ 2017-09-15 4:32 Hsin-Yu Chao
2017-09-15 4:59 ` Takashi Sakamoto
0 siblings, 1 reply; 3+ messages in thread
From: Hsin-Yu Chao @ 2017-09-15 4:32 UTC (permalink / raw)
To: broonie, lgirdwood; +Cc: oder_chiou, alsa-devel, Hsin-Yu Chao
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__ */
--
2.12.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: rt5514: Revert Hotword Model control
2017-09-15 4:32 [PATCH] ASoC: rt5514: Revert Hotword Model control Hsin-Yu Chao
@ 2017-09-15 4:59 ` Takashi Sakamoto
2017-09-15 16:36 ` Mark Brown
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Sakamoto @ 2017-09-15 4:59 UTC (permalink / raw)
To: Hsin-Yu Chao, broonie, lgirdwood; +Cc: oder_chiou, alsa-devel
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.html
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: rt5514: Revert Hotword Model control
2017-09-15 4:59 ` Takashi Sakamoto
@ 2017-09-15 16:36 ` Mark Brown
0 siblings, 0 replies; 3+ messages in thread
From: Mark Brown @ 2017-09-15 16:36 UTC (permalink / raw)
To: Takashi Sakamoto; +Cc: oder_chiou, alsa-devel, Hsin-Yu Chao, lgirdwood
[-- Attachment #1.1: Type: text/plain, Size: 595 bytes --]
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.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-15 16:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-15 4:32 [PATCH] ASoC: rt5514: Revert Hotword Model control Hsin-Yu Chao
2017-09-15 4:59 ` Takashi Sakamoto
2017-09-15 16:36 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).