| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DCNIVW9XSSY3.3H7TSNFDH7TKT@linaro.org>
Date: Mon, 08 Sep 2025 16:26:49 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Srinivas Kandagatla" <srini@...nel.org>
Cc: "Liam Girdwood" <lgirdwood@...il.com>, "Mark Brown"
<broonie@...nel.org>, "Rob Herring" <robh@...nel.org>, "Krzysztof
Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
"Stephen Boyd" <sboyd@...nel.org>, "Lee Jones" <lee@...nel.org>, "Jaroslav
Kysela" <perex@...ex.cz>, "Takashi Iwai" <tiwai@...e.com>,
<linux-arm-msm@...r.kernel.org>, <linux-sound@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Dmitry
Baryshkov" <dmitry.baryshkov@....qualcomm.com>, "Srinivas Kandagatla"
<srinivas.kandagatla@....qualcomm.com>, <christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH v3 2/3] ASoC: codecs: add new pm4125 audio codec driver
Hi Srini,
On Fri Aug 15, 2025 at 4:36 PM BST, Srinivas Kandagatla wrote:
>
[..]
> I manged to test this one, found 2 issues.
>
> 1. incorrect mic bias handling, results in recording stop working.
> 2. memory corruption leading to kernel crash.
>
> More details below..
>
>> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@....qualcomm.com>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@....qualcomm.com>
>> Signed-off-by: Alexey Klimov <alexey.klimov@...aro.org>
>> ---
>> sound/soc/codecs/Kconfig | 18 +
>> sound/soc/codecs/Makefile | 8 +
>> sound/soc/codecs/pm4125-sdw.c | 547 +++++++++++++
>> sound/soc/codecs/pm4125.c | 1793 +++++++++++++++++++++++++++++++++++++++++
>> sound/soc/codecs/pm4125.h | 313 +++++++
>> 5 files changed, 2679 insertions(+)
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125-sdw.c
>> @@ -0,0 +1,547 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright, 2025 Linaro Ltd
>
>
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_rx_ch_info[] = {
>> + WCD_SDW_CH(PM4125_HPH_L, PM4125_HPH_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_HPH_R, PM4125_HPH_PORT, BIT(1)),
>> + WCD_SDW_CH(PM4125_COMP_L, PM4125_COMP_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_COMP_R, PM4125_COMP_PORT, BIT(1)),
> Issue 1: we are adding only 4 channels here however the mixer Switches
> that lookup this table is more than 4.
>> +};
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_tx_ch_info[] = {
>> + WCD_SDW_CH(PM4125_ADC1, PM4125_ADC_1_2_DMIC1L_BCS_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_ADC2, PM4125_ADC_1_2_DMIC1L_BCS_PORT, BIT(1)),
>
> Same issue here,
>> +};
>> +
Okay. Let's fix it as you suggested.
>> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fc320152b9254e4412cbf593cdc456ee159d071f
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125.c
>> @@ -0,0 +1,1793 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright (c) 2025, Linaro Ltd
>> +
>
>> +static int pm4125_rx_clk_enable(struct snd_soc_component *component)
>> +{
>> + struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> + if (atomic_read(&pm4125->rx_clk_cnt))
>> + return 0;
>> +
>> + snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
>> + PM4125_DIG_SWR_RX_CLK_ENABLE);
>> + snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + PM4125_DIG_SWR_ANA_RX_DIV2_CLK_EN_MASK,
>> + PM4125_DIG_SWR_RX_CLK_ENABLE);
>> + usleep_range(5000, 5100);
>> +
>> + pm4125_global_mbias_enable(component);
>
> Please remove handing of mbias calls directly this is racing, Please
> handle it via dapm widgets directly. If not we will endup with switching
> off micbias off while recording is in progress or recording will
> continue assuming that micbias is on, but some path can switch it off.
Please see below, there are problems with playback without pm4125_global_mbias_enable().
>> +
>> + snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> + PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
>> + PM4125_ANA_HPHPA_FSM_DIV_RATIO_68);
>> + snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> + PM4125_ANA_HPHPA_FSM_CLK_DIV_EN_MASK,
>> + PM4125_ANA_HPHPA_FSM_CLK_DIV_ENABLE);
>> + snd_soc_component_update_bits(component, PM4125_ANA_NCP_VCTRL, 0x07, 0x06);
>> + snd_soc_component_write_field(component, PM4125_ANA_NCP_EN,
>> + PM4125_ANA_NCP_ENABLE_MASK,
>> + PM4125_ANA_NCP_ENABLE);
>> + usleep_range(500, 510);
>> +
>> + atomic_inc(&pm4125->rx_clk_cnt);
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_rx_clk_disable(struct snd_soc_component *component)
>> +{
>> + struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> + if (!atomic_read(&pm4125->rx_clk_cnt)) {
>> + dev_err(component->dev, "clk already disabled\n");
>> + return 0;
>> + }
>> +
>> + atomic_dec(&pm4125->rx_clk_cnt);
>> +
>> + snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> + PM4125_ANA_HPHPA_FSM_CLK_DIV_EN_MASK,
>> + PM4125_ANA_HPHPA_FSM_CLK_DIV_DISABLE);
>> + snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> + PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
>> + 0x00);
>> + snd_soc_component_write_field(component, PM4125_ANA_NCP_EN,
>> + PM4125_ANA_NCP_ENABLE_MASK,
>> + PM4125_ANA_NCP_DISABLE);
>> + snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + PM4125_DIG_SWR_ANA_RX_DIV2_CLK_EN_MASK,
>> + PM4125_DIG_SWR_RX_CLK_DISABLE);
>> + snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
>> + PM4125_DIG_SWR_RX_CLK_DISABLE);
>> + pm4125_global_mbias_disable(component);
>> +
>> + return 0;
>> +}
>> +
>> +
>
> %s/^\n\n/\r/
s/^/Ok\n/
>> +static int pm4125_codec_enable_rxclk(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol,
>> + int event)
>> +{
>> + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>>
>> +static const struct snd_kcontrol_new pm4125_snd_controls[] = {
>> + SOC_SINGLE_EXT("HPHL_COMP Switch", SND_SOC_NOPM, 0, 1, 0,
>
> SOC_SINGLE_EXT("HPHL_COMP Switch", PM4125_COMP_L, 0, 1, 0, ?
>
>> + pm4125_get_compander, pm4125_set_compander),
>> + SOC_SINGLE_EXT("HPHR_COMP Switch", SND_SOC_NOPM, 1, 1, 0,
>
> SOC_SINGLE_EXT("HPHR_COMP Switch", PM4125_COMP_R, 1, 1, 0,?
>
>> + pm4125_get_compander, pm4125_set_compander),
>
> This is same issue in one of the WCD codec, which am going to send fixes
> along with my original wcd fixes series.
So this was in other codecs for years, right?
>> +
>> + SOC_SINGLE_TLV("HPHL Volume", PM4125_ANA_HPHPA_L_GAIN, 0, 20, 1,
>> + line_gain),
>> + SOC_SINGLE_TLV("HPHR Volume", PM4125_ANA_HPHPA_R_GAIN, 0, 20, 1,
>> + line_gain),
>> + SOC_SINGLE_TLV("ADC1 Volume", PM4125_ANA_TX_AMIC1, 0, 8, 0,
>> + analog_gain),
>> + SOC_SINGLE_TLV("ADC2 Volume", PM4125_ANA_TX_AMIC2, 0, 8, 0,
>> + analog_gain),
>> +
>> + SOC_SINGLE_EXT("HPHL Switch", PM4125_HPH_L, 0, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>> + SOC_SINGLE_EXT("HPHR Switch", PM4125_HPH_R, 0, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>
> <---
>> + SOC_SINGLE_EXT("LO Switch", PM4125_LO, 0, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),--->
>
>> +
>> + SOC_SINGLE_EXT("ADC1 Switch", PM4125_ADC1, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>> + SOC_SINGLE_EXT("ADC2 Switch", PM4125_ADC2, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
> <-----------------
>> + SOC_SINGLE_EXT("DMIC0 Switch", PM4125_DMIC0, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>> + SOC_SINGLE_EXT("DMIC1 Switch", PM4125_DMIC1, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>> + SOC_SINGLE_EXT("MBHC Switch", PM4125_MBHC, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
>> + SOC_SINGLE_EXT("DMIC2 Switch", PM4125_DMIC2, 1, 1, 0,
>> + pm4125_get_swr_port, pm4125_set_swr_port),
> -------------->
> Please delete these entires as there are no entires for any of this
> channels in pm4125_sdw_rx_ch_info or pm4125_sdw_tx_ch_info.
>
> Side effect of this out of boundary array access is memory corruption as
> we will set port_config values based on port index which could be
> negative in this cases resulting in writing to othe members of
> pm4125_sdw_priv struct.
Ack. I applied your changes that you suggested.
>> +};
>> +
>> +static const struct snd_kcontrol_new adc1_switch[] = {
>> + SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new adc2_switch[] = {
>> + SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new dmic1_switch[] = {
>> + SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new dmic2_switch[] = {
>> + SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new ear_rdac_switch[] = {
>> + SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>
> during my test i had to do below code changes to get things working.
> Please feel free to wrap it into your next version.
>
> ----------------------->cut<----------------------------------
> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
> index fc320152b925..12d4be1f7149 100644
> --- a/sound/soc/codecs/pm4125.c
> +++ b/sound/soc/codecs/pm4125.c
> @@ -264,8 +264,6 @@ static int pm4125_rx_clk_enable(struct
> snd_soc_component *component)
> PM4125_DIG_SWR_RX_CLK_ENABLE);
> usleep_range(5000, 5100);
>
> - pm4125_global_mbias_enable(component);
> -
> snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
> PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
> PM4125_ANA_HPHPA_FSM_DIV_RATIO_68);
> @@ -309,8 +307,6 @@ static int pm4125_rx_clk_disable(struct
> snd_soc_component *component)
> snd_soc_component_write_field(component,
> PM4125_DIG_SWR_CDC_RX_CLK_CTL,
> PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
> PM4125_DIG_SWR_RX_CLK_DISABLE);
> - pm4125_global_mbias_disable(component);
> -
> return 0;
> }
This doesn't work. Playback has two issues: 1) volume is very low and probably
not adjustable and 2) sound during playback dies after a couple of seconds.
Returning these global_mbias() calls restores the good behaviour.
Maybe let's make a widget out of it? In such case I am not sure about
routing meaning that I not sure which paths do require mbias enable.
Best regards,
Alexey
Powered by blists - more mailing lists