Skip to contentSkip to conversation

ASoC: ep93xx: Fix unchecked clk_prepare_enable() and add rollback on failure

PATCH In review Jihed Chaibi 2 participants 4 messages Mar 24 – Mar 30

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ASoC: ep93xx: Fix unchecked clk_prepare_enable() and add rollback on failure

ep93xx_i2s_enable() calls clk_prepare_enable() on three clocks in sequence (mclk, sclk, lrclk) without checking the return value of any of them. If an intermediate enable fails, the clocks that were already enabled are never rolled back, leaking them until the next disable cycle — which may never come if the stream never started cleanly.

Change ep93xx_i2s_enable() from void to int. Add error checking after each clk_prepare_enable() call and unwind already-enabled clocks on failure. Propagate the error through ep93xx_i2s_startup() and ep93xx_i2s_resume(), both of which already return int.

---
 sound/soc/cirrus/ep93xx-i2s.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)
sound/soc/cirrus/ep93xx-i2s.c +24 -10
index cca01c03f048..5dba741594fa 100644
@@ -91,16 +91,28 @@ static inline unsigned ep93xx_i2s_read_reg(struct ep93xx_i2s_info *info,
 	return __raw_readl(info->regs + reg);
 }
 
-static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
+static int ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
 {
 	unsigned base_reg;
+	int err;
 
 	if ((ep93xx_i2s_read_reg(info, EP93XX_I2S_TX0EN) & 0x1) == 0 &&
 	    (ep93xx_i2s_read_reg(info, EP93XX_I2S_RX0EN) & 0x1) == 0) {
 		/* Enable clocks */
-		clk_prepare_enable(info->mclk);
-		clk_prepare_enable(info->sclk);
-		clk_prepare_enable(info->lrclk);
+		err = clk_prepare_enable(info->mclk);
+		if (err)
+			return err;
+		err = clk_prepare_enable(info->sclk);
+		if (err) {
+			clk_disable_unprepare(info->mclk);
+			return err;
+		}
+		err = clk_prepare_enable(info->lrclk);
+		if (err) {
+			clk_disable_unprepare(info->sclk);
+			clk_disable_unprepare(info->mclk);
+			return err;
+		}
 
 		/* Enable i2s */
 		ep93xx_i2s_write_reg(info, EP93XX_I2S_GLCTRL, 1);
@@ -119,6 +131,8 @@ static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
 		ep93xx_i2s_write_reg(info, EP93XX_I2S_TXCTRL,
 				     EP93XX_I2S_TXCTRL_TXEMPTY_LVL |
 				     EP93XX_I2S_TXCTRL_TXUFIE);
+
+	return 0;
 }
 
 static void ep93xx_i2s_disable(struct ep93xx_i2s_info *info, int stream)
@@ -195,9 +209,7 @@ static int ep93xx_i2s_startup(struct snd_pcm_substream *substream,
 {
 	struct ep93xx_i2s_info *info = snd_soc_dai_get_drvdata(dai);
 
-	ep93xx_i2s_enable(info, substream->stream);
-
-	return 0;
+	return ep93xx_i2s_enable(info, substream->stream);
 }
 
 static void ep93xx_i2s_shutdown(struct snd_pcm_substream *substream,
@@ -373,14 +385,16 @@ static int ep93xx_i2s_suspend(struct snd_soc_component *component)
 static int ep93xx_i2s_resume(struct snd_soc_component *component)
 {
 	struct ep93xx_i2s_info *info = snd_soc_component_get_drvdata(component);
+	int err;
 
 	if (!snd_soc_component_active(component))
 		return 0;
 
-	ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_PLAYBACK);
-	ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_CAPTURE);
+	err = ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_PLAYBACK);
+	if (err)
+		return err;
 
-	return 0;
+	return ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_CAPTURE);
 }
 #else
 #define ep93xx_i2s_suspend	NULL
Signature · 1 line
2.47.3
#1 lore raw
Quoted 95 lines · Jihed Chaibi

ep93xx_i2s_enable() calls clk_prepare_enable() on three clocks in sequence (mclk, sclk, lrclk) without checking the return value of any of them. If an intermediate enable fails, the clocks that were already enabled are never rolled back, leaking them until the next disable cycle — which may never come if the stream never started cleanly.

Change ep93xx_i2s_enable() from void to int. Add error checking after each clk_prepare_enable() call and unwind already-enabled clocks on failure. Propagate the error through ep93xx_i2s_startup() and ep93xx_i2s_resume(), both of which already return int.

---
 sound/soc/cirrus/ep93xx-i2s.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)
sound/soc/cirrus/ep93xx-i2s.c +24 -11
index cca01c03f048..5dba741594fa 100644
@@ -91,16 +91,28 @@ static inline unsigned ep93xx_i2s_read_reg(struct ep93xx_i2s_info *info,
        return __raw_readl(info->regs + reg);
 }
 
-static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
+static int ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
 {
        unsigned base_reg;
+       int err;
 
        if ((ep93xx_i2s_read_reg(info, EP93XX_I2S_TX0EN) & 0x1) == 0 &&
            (ep93xx_i2s_read_reg(info, EP93XX_I2S_RX0EN) & 0x1) == 0) {
                /* Enable clocks */
-               clk_prepare_enable(info->mclk);
-               clk_prepare_enable(info->sclk);
-               clk_prepare_enable(info->lrclk);
+               err = clk_prepare_enable(info->mclk);
+               if (err)
+                       return err;
+               err = clk_prepare_enable(info->sclk);
+               if (err) {
+                       clk_disable_unprepare(info->mclk);
+                       return err;
+               }
+               err = clk_prepare_enable(info->lrclk);
+               if (err) {
+                       clk_disable_unprepare(info->sclk);
+                       clk_disable_unprepare(info->mclk);
+                       return err;
+               }
 
                /* Enable i2s */
                ep93xx_i2s_write_reg(info, EP93XX_I2S_GLCTRL, 1);
@@ -119,6 +131,8 @@ static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream)
                ep93xx_i2s_write_reg(info, EP93XX_I2S_TXCTRL,
                                     EP93XX_I2S_TXCTRL_TXEMPTY_LVL |
                                     EP93XX_I2S_TXCTRL_TXUFIE);
+
+       return 0;
 }
 
 static void ep93xx_i2s_disable(struct ep93xx_i2s_info *info, int stream)
@@ -195,9 +209,7 @@ static int ep93xx_i2s_startup(struct snd_pcm_substream *substream,
 {
        struct ep93xx_i2s_info *info = snd_soc_dai_get_drvdata(dai);
 
-       ep93xx_i2s_enable(info, substream->stream);
-
-       return 0;
+       return ep93xx_i2s_enable(info, substream->stream);
 }
 
 static void ep93xx_i2s_shutdown(struct snd_pcm_substream *substream,
@@ -373,14 +385,16 @@ static int ep93xx_i2s_suspend(struct snd_soc_component *component)
 static int ep93xx_i2s_resume(struct snd_soc_component *component)
 {
        struct ep93xx_i2s_info *info = snd_soc_component_get_drvdata(component);
+       int err;
 
        if (!snd_soc_component_active(component))
                return 0;
 
-       ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_PLAYBACK);
-       ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_CAPTURE);
+       err = ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_PLAYBACK);
+       if (err)
+               return err;
 
-       return 0;
+       return ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_CAPTURE);
 }
 #else
 #define ep93xx_i2s_suspend     NULL
--
Quoted 2 lines

2.47.3

Adding the missing Fixes tag:

Fixes f4ff6b56bc8a ("ASoC: cirrus: i2s: Prepare clock before using it")
maintainer
#2 lore raw
Quoted 5 lines · Jihed Chaibi
Quoted 3 lines · Jihed Chaibi

ep93xx_i2s_enable() calls clk_prepare_enable() on three clocks in sequence (mclk, sclk, lrclk) without checking the return value of any

Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material.

maintainer
#3 lore raw
In reply to Jihed Chaibi
Quoted 1 line · Jihed Chaibi

ASoC: ep93xx: Fix unchecked clk_prepare_enable() and add rollback on failure

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.0

Thanks!

[1/1] ASoC: ep93xx: Fix unchecked clk_prepare_enable() and add rollback on failure

      https://git.kernel.org/broonie/sound/c/622363757b22

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