| 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: <20230907171010.1447274-5-cristian.ciocaltea@collabora.com>
Date: Thu, 7 Sep 2023 20:10:03 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: James Schulman <james.schulman@...rus.com>,
David Rhodes <david.rhodes@...rus.com>,
Richard Fitzgerald <rf@...nsource.cirrus.com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Stefan Binding <sbinding@...nsource.cirrus.com>,
Charles Keepax <ckeepax@...nsource.cirrus.com>,
Vitaly Rodionov <vitalyr@...nsource.cirrus.com>
Cc: alsa-devel@...a-project.org, patches@...nsource.cirrus.com,
linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: [PATCH v2 04/11] ASoC: cs35l41: Fix broken shared boost activation
Enabling the active/passive shared boosts requires setting SYNC_EN, but
*not* before receiving the PLL Lock signal.
Due to improper error handling, it was not obvious that waiting for the
completion operation times out and, consequently, the shared boost is
never activated.
Further investigations revealed the signal is triggered while
snd_pcm_start() is executed, right after receiving the
SNDRV_PCM_TRIGGER_START command, which happens long after the
SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
snd_pcm_prepare(). That is where cs35l41_global_enable() is called
from.
Increasing the wait duration doesn't help, as it only causes an
unnecessary delay in the invocation of snd_pcm_start(). Moving the wait
and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
callback is not a solution either, since they would be executed in an
IRQ-off atomic context.
Solve the issue by setting the SYNC_EN bit in PWR_CTRL3 register right
after receiving the PLL Lock interrupt.
Additionally, drop the unnecessary writes to PWR_CTRL1 register, part of
the original mdsync_up_seq, which would have toggled GLOBAL_EN with
unwanted consequences on PLL locking behavior.
Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
---
include/sound/cs35l41.h | 4 +--
sound/pci/hda/cs35l41_hda.c | 4 +--
sound/soc/codecs/cs35l41-lib.c | 61 ++++++++++++++++++++--------------
sound/soc/codecs/cs35l41.c | 24 ++++++++-----
sound/soc/codecs/cs35l41.h | 1 -
5 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 1bf757901d02..2fe8c6b0d4cf 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -11,7 +11,6 @@
#define __CS35L41_H
#include <linux/regmap.h>
-#include <linux/completion.h>
#include <linux/firmware/cirrus/cs_dsp.h>
#define CS35L41_FIRSTREG 0x00000000
@@ -902,7 +901,8 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap);
int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
struct cs35l41_hw_cfg *hw_cfg);
bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type);
+int cs35l41_mdsync_up(struct regmap *regmap);
int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
- int enable, struct completion *pll_lock, bool firmware_running);
+ int enable, bool firmware_running);
#endif /* __CS35L41_H */
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f9b77353c266..09a9c135d9b6 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -527,7 +527,7 @@ static void cs35l41_hda_play_done(struct device *dev)
dev_dbg(dev, "Play (Complete)\n");
- cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
+ cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1,
cs35l41->firmware_running);
if (cs35l41->firmware_running) {
regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp,
@@ -546,7 +546,7 @@ static void cs35l41_hda_pause_start(struct device *dev)
dev_dbg(dev, "Pause (Start)\n");
regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
- cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL,
+ cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0,
cs35l41->firmware_running);
}
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index a6c6bb23b957..2ec5fdc875b1 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1192,8 +1192,28 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type)
}
EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
+/*
+ * Enabling the CS35L41_SHD_BOOST_ACTV and CS35L41_SHD_BOOST_PASS shared boosts
+ * does also require a call to cs35l41_mdsync_up(), but not before getting the
+ * PLL Lock signal.
+ *
+ * PLL Lock seems to be triggered soon after snd_pcm_start() is executed and
+ * SNDRV_PCM_TRIGGER_START command is processed, which happens (long) after the
+ * SND_SOC_DAPM_PRE_PMU event handler is invoked as part of snd_pcm_prepare().
+ *
+ * This event handler is where cs35l41_global_enable() is normally called from,
+ * but waiting for PLL Lock here will time out. Increasing the wait duration
+ * will not help, as the only consequence of it would be to add an unnecessary
+ * delay in the invocation of snd_pcm_start().
+ *
+ * Trying to move the wait in the SNDRV_PCM_TRIGGER_START callback is not a
+ * solution either, as the trigger is executed in an IRQ-off atomic context.
+ *
+ * The current approach is to invoke cs35l41_mdsync_up() right after receiving
+ * the PLL Lock interrupt, in the IRQ handler.
+ */
int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
- int enable, struct completion *pll_lock, bool firmware_running)
+ int enable, bool firmware_running)
{
int ret;
unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask;
@@ -1203,11 +1223,6 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
{CS35L41_GPIO_PAD_CONTROL, 0},
{CS35L41_PWR_CTRL1, 0, 3000},
};
- struct reg_sequence cs35l41_mdsync_up_seq[] = {
- {CS35L41_PWR_CTRL3, 0},
- {CS35L41_PWR_CTRL1, 0x00000000, 3000},
- {CS35L41_PWR_CTRL1, 0x00000001, 3000},
- };
pup_pdn_mask = enable ? CS35L41_PUP_DONE_MASK : CS35L41_PDN_DONE_MASK;
@@ -1241,26 +1256,11 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
cs35l41_mdsync_down_seq[0].def = pwr_ctrl3;
cs35l41_mdsync_down_seq[1].def = pad_control;
cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
+
ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
ARRAY_SIZE(cs35l41_mdsync_down_seq));
- if (ret || !enable)
- break;
-
- if (!pll_lock)
- return -EINVAL;
-
- ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
- if (ret == 0) {
- dev_err(dev, "Timed out waiting for pll_lock\n");
- return -ETIMEDOUT;
- }
-
- regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
- pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
- cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
- ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
- ARRAY_SIZE(cs35l41_mdsync_up_seq));
- if (ret)
+ /* Activation to be completed later via cs35l41_mdsync_up() */
+ if (ret || enable)
return ret;
ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
@@ -1269,7 +1269,7 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
if (ret)
dev_err(dev, "Enable(%d) failed: %d\n", enable, ret);
- // Clear PUP/PDN status
+ /* Clear PUP/PDN status */
regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask);
break;
case CS35L41_INT_BOOST:
@@ -1351,6 +1351,17 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
}
EXPORT_SYMBOL_GPL(cs35l41_global_enable);
+/*
+ * To be called after receiving the IRQ Lock interrupt, in order to complete
+ * any shared boost activation initiated by cs35l41_global_enable().
+ */
+int cs35l41_mdsync_up(struct regmap *regmap)
+{
+ return regmap_update_bits(regmap, CS35L41_PWR_CTRL3,
+ CS35L41_SYNC_EN_MASK, CS35L41_SYNC_EN_MASK);
+}
+EXPORT_SYMBOL_GPL(cs35l41_mdsync_up);
+
int cs35l41_gpio_config(struct regmap *regmap, struct cs35l41_hw_cfg *hw_cfg)
{
struct cs35l41_gpio_cfg *gpio1 = &hw_cfg->gpio1;
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index fe5376b3e01b..12327b4c3d56 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -459,7 +459,19 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
if (status[2] & CS35L41_PLL_LOCK) {
regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
- complete(&cs35l41->pll_lock);
+
+ if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV ||
+ cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS) {
+ ret = cs35l41_mdsync_up(cs35l41->regmap);
+ if (ret)
+ dev_err(cs35l41->dev, "MDSYNC-up failed: %d\n", ret);
+ else
+ dev_dbg(cs35l41->dev, "MDSYNC-up done\n");
+
+ dev_dbg(cs35l41->dev, "PUP-done status: %d\n",
+ !!(status[0] & CS35L41_PUP_DONE_MASK));
+ }
+
ret = IRQ_HANDLED;
}
@@ -500,11 +512,11 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
ARRAY_SIZE(cs35l41_pup_patch));
ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
- 1, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+ 1, cs35l41->dsp.cs_dsp.running);
break;
case SND_SOC_DAPM_POST_PMD:
ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
- 0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+ 0, cs35l41->dsp.cs_dsp.running);
regmap_multi_reg_write_bypassed(cs35l41->regmap,
cs35l41_pdn_patch,
@@ -802,10 +814,6 @@ static const struct snd_pcm_hw_constraint_list cs35l41_constraints = {
static int cs35l41_pcm_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
- struct cs35l41_private *cs35l41 = snd_soc_component_get_drvdata(dai->component);
-
- reinit_completion(&cs35l41->pll_lock);
-
if (substream->runtime)
return snd_pcm_hw_constraint_list(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_RATE,
@@ -1273,8 +1281,6 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
0 << CS35L41_INT3_PLL_LOCK_SHIFT);
- init_completion(&cs35l41->pll_lock);
-
ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
IRQF_ONESHOT | IRQF_SHARED | irq_pol,
"cs35l41", cs35l41);
diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h
index 34d967d4372b..c85cbc1dd333 100644
--- a/sound/soc/codecs/cs35l41.h
+++ b/sound/soc/codecs/cs35l41.h
@@ -33,7 +33,6 @@ struct cs35l41_private {
int irq;
/* GPIO for /RST */
struct gpio_desc *reset_gpio;
- struct completion pll_lock;
};
int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg);
--
2.41.0
Powered by blists - more mailing lists