[PATCH 0/4] ASoC: qcom: fixes for Qualcomm codecs and q6apm
Hi Mark,
Some recent testing found few issues with wcd938x and lpass-tx codec drivers. WCD938x was accessing array out of boundaries resulting in corruption and system crashes along with not handling kcontrol put return values correctly and rx-macro had incorrect sidetone registers offsets. One final fix in q6apm to add a check if graph started before stopping it.
Thanks, srini
Srinivas Kandagatla (4): ASoC: codecs: wcd938x: fix incorrect used of portid ASoC: codecs: lpass-rx-macro: fix sidetone register offsets ASoC: codecs: wcd938x: fix return value of mixer put function ASoC: qdsp6: q6apm-dai: only stop graphs that are started
sound/soc/codecs/lpass-rx-macro.c | 8 ++++---- sound/soc/codecs/wcd938x.c | 31 +++++++++++++++++-------------- sound/soc/qcom/qdsp6/q6apm-dai.c | 7 +++++-- 3 files changed, 26 insertions(+), 20 deletions(-)
Mixer controls have the channel id in mixer->reg, which is not same as port id. port id should be derived from chan_info array. So fix this. Without this, its possible that we could corrupt struct wcd938x_sdw_priv by accessing port_map array out of range with channel id instead of port id.
Fixes: e8ba1e05bdc0 ("ASoC: codecs: wcd938x: add basic controls") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wcd938x.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index eff200a07d9f..5994644c8702 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -1432,14 +1432,10 @@ static int wcd938x_sdw_connect_port(struct wcd938x_sdw_ch_info *ch_info, return 0; }
-static int wcd938x_connect_port(struct wcd938x_sdw_priv *wcd, u8 ch_id, u8 enable) +static int wcd938x_connect_port(struct wcd938x_sdw_priv *wcd, u8 port_num, u8 ch_id, u8 enable) { - u8 port_num; - - port_num = wcd->ch_info[ch_id].port_num; - return wcd938x_sdw_connect_port(&wcd->ch_info[ch_id], - &wcd->port_config[port_num], + &wcd->port_config[port_num - 1], enable); }
@@ -2593,6 +2589,7 @@ static int wcd938x_set_compander(struct snd_kcontrol *kcontrol, struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component); struct wcd938x_sdw_priv *wcd; int value = ucontrol->value.integer.value[0]; + int portidx; struct soc_mixer_control *mc; bool hphr;
@@ -2606,10 +2603,12 @@ static int wcd938x_set_compander(struct snd_kcontrol *kcontrol, else wcd938x->comp1_enable = value;
+ portidx = wcd->ch_info[mc->reg].port_num; + if (value) - wcd938x_connect_port(wcd, mc->reg, true); + wcd938x_connect_port(wcd, portidx, mc->reg, true); else - wcd938x_connect_port(wcd, mc->reg, false); + wcd938x_connect_port(wcd, portidx, mc->reg, false);
return 0; } @@ -2882,9 +2881,11 @@ static int wcd938x_get_swr_port(struct snd_kcontrol *kcontrol, struct wcd938x_sdw_priv *wcd; struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value; int dai_id = mixer->shift; - int portidx = mixer->reg; + int portidx, ch_idx = mixer->reg; +
wcd = wcd938x->sdw_priv[dai_id]; + portidx = wcd->ch_info[ch_idx].port_num;
ucontrol->value.integer.value[0] = wcd->port_enable[portidx];
@@ -2899,12 +2900,14 @@ static int wcd938x_set_swr_port(struct snd_kcontrol *kcontrol, struct wcd938x_sdw_priv *wcd; struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value; - int portidx = mixer->reg; + int ch_idx = mixer->reg; + int portidx; int dai_id = mixer->shift; bool enable;
wcd = wcd938x->sdw_priv[dai_id];
+ portidx = wcd->ch_info[ch_idx].port_num; if (ucontrol->value.integer.value[0]) enable = true; else @@ -2912,7 +2915,7 @@ static int wcd938x_set_swr_port(struct snd_kcontrol *kcontrol,
wcd->port_enable[portidx] = enable;
- wcd938x_connect_port(wcd, portidx, enable); + wcd938x_connect_port(wcd, portidx, ch_idx, enable);
return 0;
For some reason we ended up with incorrect register offfset calcuations for sidetone. regmap clearly throw errors when accessing these incorrect registers as these do not belong to any read/write ranges. so fix them to point to correct register offsets.
Fixes: f3ce6f3c9a99 ("ASoC: codecs: lpass-rx-macro: add iir widgets") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/lpass-rx-macro.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index aec5127260fd..6ffe88345de5 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -2688,8 +2688,8 @@ static uint32_t get_iir_band_coeff(struct snd_soc_component *component, int reg, b2_reg;
/* Address does not automatically update if reading */ - reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B1_CTL + 16 * iir_idx; - b2_reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B2_CTL + 16 * iir_idx; + reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B1_CTL + 0x80 * iir_idx; + b2_reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B2_CTL + 0x80 * iir_idx;
snd_soc_component_write(component, reg, ((band_idx * BAND_MAX + coeff_idx) * @@ -2718,7 +2718,7 @@ static uint32_t get_iir_band_coeff(struct snd_soc_component *component, static void set_iir_band_coeff(struct snd_soc_component *component, int iir_idx, int band_idx, uint32_t value) { - int reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B2_CTL + 16 * iir_idx; + int reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B2_CTL + 0x80 * iir_idx;
snd_soc_component_write(component, reg, (value & 0xFF)); snd_soc_component_write(component, reg, (value >> 8) & 0xFF); @@ -2739,7 +2739,7 @@ static int rx_macro_put_iir_band_audio_mixer( int iir_idx = ctl->iir_idx; int band_idx = ctl->band_idx; u32 coeff[BAND_MAX]; - int reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B1_CTL + 16 * iir_idx; + int reg = CDC_RX_SIDETONE_IIR0_IIR_COEF_B1_CTL + 0x80 * iir_idx;
memcpy(&coeff[0], ucontrol->value.bytes.data, params->max);
wcd938x_ear_pa_put_gain, wcd938x_set_swr_port and wcd938x_set_compander currently returns zero eventhough it changes the value. Fix this, so that change notifications are sent correctly.
Fixes: e8ba1e05bdc01 ("ASoC: codecs: wcd938x: add basic controls") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wcd938x.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index 5994644c8702..36cbc66914f9 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -2559,7 +2559,7 @@ static int wcd938x_ear_pa_put_gain(struct snd_kcontrol *kcontrol, WCD938X_EAR_GAIN_MASK, ucontrol->value.integer.value[0]);
- return 0; + return 1; }
static int wcd938x_get_compander(struct snd_kcontrol *kcontrol, @@ -2610,7 +2610,7 @@ static int wcd938x_set_compander(struct snd_kcontrol *kcontrol, else wcd938x_connect_port(wcd, portidx, mc->reg, false);
- return 0; + return 1; }
static int wcd938x_ldoh_get(struct snd_kcontrol *kcontrol, @@ -2917,7 +2917,7 @@ static int wcd938x_set_swr_port(struct snd_kcontrol *kcontrol,
wcd938x_connect_port(wcd, portidx, ch_idx, enable);
- return 0; + return 1;
}
Its possible that the sound card is just opened and closed without actually playing stream, ex: if the audio file itself is missing.
Even in such cases we do call stop on graphs that are not yet started. DSP can throw errors in such cases, so add a check to see if the graph was started before stopping it.
Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/qcom/qdsp6/q6apm-dai.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c index eb1c3aec479b..19c4a90ec1ea 100644 --- a/sound/soc/qcom/qdsp6/q6apm-dai.c +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c @@ -308,8 +308,11 @@ static int q6apm_dai_close(struct snd_soc_component *component, struct snd_pcm_runtime *runtime = substream->runtime; struct q6apm_dai_rtd *prtd = runtime->private_data;
- q6apm_graph_stop(prtd->graph); - q6apm_unmap_memory_regions(prtd->graph, substream->stream); + if (prtd->state) { /* only stop graph that is started */ + q6apm_graph_stop(prtd->graph); + q6apm_unmap_memory_regions(prtd->graph, substream->stream); + } + q6apm_graph_close(prtd->graph); prtd->graph = NULL; kfree(prtd);
On Wed, Jan 26, 2022 at 11:35:45AM +0000, Srinivas Kandagatla wrote:
Some recent testing found few issues with wcd938x and lpass-tx codec drivers. WCD938x was accessing array out of boundaries resulting in corruption and system crashes along with not handling kcontrol put return values correctly and rx-macro had incorrect sidetone registers offsets. One final fix in q6apm to add a check if graph started before stopping it.
You probably want to run the new mixer kselftest on these devices, including with the patch I posted the other day testing out of bounds writes. It doesn't cover change notifications yet, you might want to set up some automation for that for whenever someone gets round to adding coverage.
Thanks Mark,
On 26/01/2022 13:29, Mark Brown wrote:
On Wed, Jan 26, 2022 at 11:35:45AM +0000, Srinivas Kandagatla wrote:
Some recent testing found few issues with wcd938x and lpass-tx codec drivers. WCD938x was accessing array out of boundaries resulting in corruption and system crashes along with not handling kcontrol put return values correctly and rx-macro had incorrect sidetone registers offsets. One final fix in q6apm to add a check if graph started before stopping it.
You probably want to run the new mixer kselftest on these devices,
Yes, this is really good test indeed already found few issues with existing mixers. I will test this on all the Qualcomm boards and get back with some fixes.
I also made some improvements to the mixer test to not keep writing new values to controls that have returned -EPERM in cases like where put callback is NULL, In my case I have a Impedance value read only control whose value range is MAX_INT.
--srini
including with the patch I posted the other day testing out of bounds writes. It doesn't cover change notifications yet, you might want to set up some automation for that for whenever someone gets round to adding coverage.
On Wed, 26 Jan 2022 11:35:45 +0000, Srinivas Kandagatla wrote:
Some recent testing found few issues with wcd938x and lpass-tx codec drivers. WCD938x was accessing array out of boundaries resulting in corruption and system crashes along with not handling kcontrol put return values correctly and rx-macro had incorrect sidetone registers offsets. One final fix in q6apm to add a check if graph started before stopping it.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus
Thanks!
[1/4] ASoC: codecs: wcd938x: fix incorrect used of portid commit: c5c1546a654f613e291a7c5d6f3660fc1eb6d0c7 [2/4] ASoC: codecs: lpass-rx-macro: fix sidetone register offsets commit: fca041a3ab70a099a6d5519ecb689b6279bd04f3 [3/4] ASoC: codecs: wcd938x: fix return value of mixer put function commit: bd2347fd67d8da0fa76296507cc556da0a233bcb [4/4] ASoC: qdsp6: q6apm-dai: only stop graphs that are started commit: 8f2e5c65ec7534cce6d315fccf2c3aef023f68f0
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Srinivas Kandagatla