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(-)
--
2.21.0
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 <[email protected]>
---
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;
--
2.21.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 <[email protected]>
---
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);
--
2.21.0
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 <[email protected]>
---
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;
}
--
2.21.0
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 <[email protected]>
---
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);
--
2.21.0
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