ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe failure

PATCH Superseded Krzysztof Kozlowski 3 participants 6 messages Jul 26, 2017 – Jul 28, 2017

Subsystems (3)

Sound ASoC Samsung audio (ASoC) drivers

lore

Share & tools

lore

Krzysztof Kozlowski

author

2017-07-26 18:57 UTC #0 lore raw

PATCH 1/2 ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe failure

Commit 87b132bc0315 ("ASoC: samsung: s3c24{xx,12}-i2s: port to use generic dmaengine API") moved ioremap() call from s3c-i2s-v2.c:s3c_i2sv2_probe() to s3c2412-i2s.c:s3c2412_iis_dev_probe() and converted it to devm- resource managed interface.

However the error path in first of them - s3c_i2sv2_probe() - was not updated. If getting a iis clock in s3c_i2sv2_probe() failed, the address space would be unmapped there. This could lead to:
1. double iounmap() later from devm-interface of s3c2412_iis_dev_probe()),
2. accessing the memory by other functions in s3c2412-i2s.c unit.

Anyway, the owner of this mapped region should be s3c2412-i2s.c because it starts the mapping.

Affected are drivers for S3C24xx family although issue was not reproduced.

Fixes 87b132bc0315 ("ASoC: samsung: s3c24{xx,12}-i2s: port to use generic dmaengine API")

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

Not marking as Cc-stable because this is theoretical problem, not reproduced and also not tested.

Please, kindly test on S3C24xx hardware.

---
 sound/soc/samsung/s3c-i2s-v2.c | 1 -
 1 file changed, 1 deletion(-)

sound/soc/samsung/s3c-i2s-v2.c +0 -1

index ca522a95160b..9b28046eea8e 100644
@@ -634,7 +634,6 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,
 	i2s->iis_pclk = clk_get(dev, "iis");
 	if (IS_ERR(i2s->iis_pclk)) {
 		dev_err(dev, "failed to get iis_clock\n");
-		iounmap(i2s->regs);
 		return -ENOENT;
 	}

Signature · 1 line

2.11.0

Krzysztof Kozlowski

author

2017-07-26 18:57 UTC #1 lore raw

PATCH 2/2 ASoC: samsung: Add proper error paths to s3c24xx I2S driver

s3c2412_i2s_probe() might fail so driver has to revert work done by s3c_i2sv2_probe() (clock enabling). Missing doing this would lead to clock enable in-balance.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

Please, kindly test on S3C24xx hardware.

---
 sound/soc/samsung/s3c-i2s-v2.c  |  9 +++++++++
 sound/soc/samsung/s3c-i2s-v2.h  |  7 +++++++
 sound/soc/samsung/s3c2412-i2s.c | 11 +++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

sound/soc/samsung/s3c-i2s-v2.c +9 -0

index 9b28046eea8e..df83f2730a53 100644
@@ -651,6 +651,15 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,
 }
 EXPORT_SYMBOL_GPL(s3c_i2sv2_probe);
 
+void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+		      struct s3c_i2sv2_info *i2s)
+{
+	clk_disable(i2s->iis_pclk);
+	clk_put(i2s->iis_pclk);
+	i2s->iis_pclk = NULL;
+}
+EXPORT_SYMBOL_GPL(s3c_i2sv2_cleanup);
+
 #ifdef CONFIG_PM
 static int s3c2412_i2s_suspend(struct snd_soc_dai *dai)
 {

sound/soc/samsung/s3c-i2s-v2.h +7 -0

index 182d80564e37..3fca20f7a853 100644
@@ -92,6 +92,13 @@ extern int s3c_i2sv2_probe(struct snd_soc_dai *dai,
 			   unsigned long base);
 
 /**
+ * s3c_i2sv2_cleanup - cleanup resources allocated in s3c_i2sv2_probe
+ * @dai: The ASoC DAI structure supplied to the original probe.
+ * @i2s: Our local i2s structure to fill in.
+ */
+extern void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+			      struct s3c_i2sv2_info *i2s);
+/**
  * s3c_i2sv2_register_component - register component and dai with soc core
  * @dev: DAI device
  * @id: DAI ID

sound/soc/samsung/s3c2412-i2s.c +9 -2

index bcd1cbdeac93..005cd6db7d07 100644
@@ -65,7 +65,8 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
 	s3c2412_i2s.iis_cclk = devm_clk_get(dai->dev, "i2sclk");
 	if (IS_ERR(s3c2412_i2s.iis_cclk)) {
 		pr_err("failed to get i2sclk clock\n");
-		return PTR_ERR(s3c2412_i2s.iis_cclk);
+		ret = PTR_ERR(s3c2412_i2s.iis_cclk);
+		goto err;
 	}
 
 	/* Set MPLL as the source for IIS CLK */

@@ -73,7 +74,7 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
 	clk_set_parent(s3c2412_i2s.iis_cclk, clk_get(NULL, "mpll"));
 	ret = clk_prepare_enable(s3c2412_i2s.iis_cclk);
 	if (ret)
-		return ret;
+		goto err;
 
 	s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk;

@@ -82,11 +83,17 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
 			      S3C_GPIO_PULL_NONE);
 
 	return 0;
+
+err:
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);
+
+	return ret;
 }
 
 static int s3c2412_i2s_remove(struct snd_soc_dai *dai)
 {
 	clk_disable_unprepare(s3c2412_i2s.iis_cclk);
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);
 
 	return 0;
 }

Signature · 1 line

2.11.0

Arvind Yadav

2017-07-27 05:11 UTC #2 lore raw

Re: [RFT 2/2] ASoC: samsung: Add proper error paths to s3c24xx I2S driver

Hi,

Quoted 68 lines · Krzysztof Kozlowski

s3c2412_i2s_probe() might fail so driver has to revert work done by s3c_i2sv2_probe() (clock enabling). Missing doing this would lead to clock enable in-balance.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

Please, kindly test on S3C24xx hardware.

---
  sound/soc/samsung/s3c-i2s-v2.c  |  9 +++++++++
  sound/soc/samsung/s3c-i2s-v2.h  |  7 +++++++
  sound/soc/samsung/s3c2412-i2s.c | 11 +++++++++--
  3 files changed, 25 insertions(+), 2 deletions(-)

sound/soc/samsung/s3c-i2s-v2.c +9 -0

index 9b28046eea8e..df83f2730a53 100644
@@ -651,6 +651,15 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,
  }
  EXPORT_SYMBOL_GPL(s3c_i2sv2_probe);
 
+void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+		      struct s3c_i2sv2_info *i2s)
+{
+	clk_disable(i2s->iis_pclk);
+	clk_put(i2s->iis_pclk);
+	i2s->iis_pclk = NULL;
+}
+EXPORT_SYMBOL_GPL(s3c_i2sv2_cleanup);
+
  #ifdef CONFIG_PM
  static int s3c2412_i2s_suspend(struct snd_soc_dai *dai)
  {

sound/soc/samsung/s3c-i2s-v2.h +7 -0

index 182d80564e37..3fca20f7a853 100644
@@ -92,6 +92,13 @@ extern int s3c_i2sv2_probe(struct snd_soc_dai *dai,
  			   unsigned long base);
 
  /**
+ * s3c_i2sv2_cleanup - cleanup resources allocated in s3c_i2sv2_probe
+ * @dai: The ASoC DAI structure supplied to the original probe.
+ * @i2s: Our local i2s structure to fill in.
+ */
+extern void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+			      struct s3c_i2sv2_info *i2s);
+/**
   * s3c_i2sv2_register_component - register component and dai with soc core
   * @dev: DAI device
   * @id: DAI ID

sound/soc/samsung/s3c2412-i2s.c +2 -1

index bcd1cbdeac93..005cd6db7d07 100644
@@ -65,7 +65,8 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  	s3c2412_i2s.iis_cclk = devm_clk_get(dai->dev, "i2sclk");
  	if (IS_ERR(s3c2412_i2s.iis_cclk)) {
  		pr_err("failed to get i2sclk clock\n");
-		return PTR_ERR(s3c2412_i2s.iis_cclk);
+		ret = PTR_ERR(s3c2412_i2s.iis_cclk);
+		goto err;
  	}
 
  	/* Set MPLL as the source for IIS CLK */

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 74

Quoted 1 line

sound/soc/samsung/s3c2412-i2s.c +0 -0

@@ -73,7 +74,7 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  	clk_set_parent(s3c2412_i2s.iis_cclk, clk_get(NULL, "mpll"));

we can use devm_clk_get for "mpll" clock.

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 79

Quoted 6 lines

sound/soc/samsung/s3c2412-i2s.c +1 -1

  	ret = clk_prepare_enable(s3c2412_i2s.iis_cclk);
  	if (ret)
-		return ret;
+		goto err;
 
  	s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk;

Now s3c2412_i2s.iis_cclk and s3c2412_i2s.iis_pclk are holding "iis" clock. Now no one handling "mpll" clock. If we will call s3c2412_i2s_remove. It's means. we are disabling "iis" clock twice. Which is not correct.

Quoted 6 lines

sound/soc/samsung/s3c2412-i2s.c +1 -0

@@ -82,11 +83,17 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  			      S3C_GPIO_PULL_NONE);
 
  	return 0;
+

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 96

Quoted 10 lines

sound/soc/samsung/s3c2412-i2s.c +5 -0

+err:
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);
+
+	return ret;
  }
 
  static int s3c2412_i2s_remove(struct snd_soc_dai *dai)
  {
  	clk_disable_unprepare(s3c2412_i2s.iis_cclk);
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);

here, We are disabling "iis" clock twice. please check my previous comment.

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 98

Quoted 3 lines

sound/soc/samsung/s3c2412-i2s.c +0 -0
  	return 0;
  }

~arvind

Krzysztof Kozlowski

author

2017-07-27 16:51 UTC #3 lore raw

Re: [RFT 2/2] ASoC: samsung: Add proper error paths to s3c24xx I2S driver

Quoted 63 lines · Arvind Yadav

Hi,

Quoted 59 lines · Krzysztof Kozlowski

s3c2412_i2s_probe() might fail so driver has to revert work done by s3c_i2sv2_probe() (clock enabling). Missing doing this would lead to clock enable in-balance.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

Please, kindly test on S3C24xx hardware.

---
  sound/soc/samsung/s3c-i2s-v2.c  |  9 +++++++++
  sound/soc/samsung/s3c-i2s-v2.h  |  7 +++++++
  sound/soc/samsung/s3c2412-i2s.c | 11 +++++++++--
  3 files changed, 25 insertions(+), 2 deletions(-)

sound/soc/samsung/s3c-i2s-v2.c +9 -0

index 9b28046eea8e..df83f2730a53 100644
@@ -651,6 +651,15 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,
  }
  EXPORT_SYMBOL_GPL(s3c_i2sv2_probe);
+void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+		      struct s3c_i2sv2_info *i2s)
+{
+	clk_disable(i2s->iis_pclk);
+	clk_put(i2s->iis_pclk);
+	i2s->iis_pclk = NULL;
+}
+EXPORT_SYMBOL_GPL(s3c_i2sv2_cleanup);
+
  #ifdef CONFIG_PM
  static int s3c2412_i2s_suspend(struct snd_soc_dai *dai)
  {

sound/soc/samsung/s3c-i2s-v2.h +7 -0

index 182d80564e37..3fca20f7a853 100644
@@ -92,6 +92,13 @@ extern int s3c_i2sv2_probe(struct snd_soc_dai *dai,
  			   unsigned long base);
  /**
+ * s3c_i2sv2_cleanup - cleanup resources allocated in s3c_i2sv2_probe
+ * @dai: The ASoC DAI structure supplied to the original probe.
+ * @i2s: Our local i2s structure to fill in.
+ */
+extern void s3c_i2sv2_cleanup(struct snd_soc_dai *dai,
+			      struct s3c_i2sv2_info *i2s);
+/**
   * s3c_i2sv2_register_component - register component and dai with soc core
   * @dev: DAI device
   * @id: DAI ID

sound/soc/samsung/s3c2412-i2s.c +0 -0

index bcd1cbdeac93..005cd6db7d07 100644
@@ -65,7 +65,8 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  	s3c2412_i2s.iis_cclk = devm_clk_get(dai->dev, "i2sclk");
  	if (IS_ERR(s3c2412_i2s.iis_cclk)) {
  		pr_err("failed to get i2sclk clock\n");

Quoted 8 lines

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 74
Quoted 7 lines
sound/soc/samsung/s3c2412-i2s.c +2 -1
-		return PTR_ERR(s3c2412_i2s.iis_cclk);
+		ret = PTR_ERR(s3c2412_i2s.iis_cclk);
+		goto err;
  	}
  	/* Set MPLL as the source for IIS CLK */
@@ -73,7 +74,7 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  	clk_set_parent(s3c2412_i2s.iis_cclk, clk_get(NULL, "mpll"));
we can use devm_clk_get for "mpll" clock.

This should be rather separate change from the fix... unless it is a fix by itself.

Quoted 8 lines

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 79
Quoted 5 lines
sound/soc/samsung/s3c2412-i2s.c +1 -1
  	ret = clk_prepare_enable(s3c2412_i2s.iis_cclk);
  	if (ret)
-		return ret;
+		goto err;
  	s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk;
Now s3c2412_i2s.iis_cclk and s3c2412_i2s.iis_pclk are holding "iis" clock. Now no one handling "mpll" clock. If we will call s3c2412_i2s_remove. It's means. we are disabling "iis" clock twice. Which is not correct.

Right, what a obfuscated code is this... Thanks for pointing this out. Also the s3c_i2sv2_probe() misses clk_prepare().

Best regards,
Krzysztof

Quoted 17 lines

sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 94
Quoted 13 lines
sound/soc/samsung/s3c2412-i2s.c +6 -0
@@ -82,11 +83,17 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai)
  			      S3C_GPIO_PULL_NONE);
  	return 0;
+
+err:
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);
+
+	return ret;
  }
  static int s3c2412_i2s_remove(struct snd_soc_dai *dai)
  {
  	clk_disable_unprepare(s3c2412_i2s.iis_cclk);
+	s3c_i2sv2_cleanup(dai, &s3c2412_i2s);
here, We are disabling "iis" clock twice. please check my previous comment.
sound/soc/samsung/s3c2412-i2s.c static int s3c2412_i2s_probe(struct snd_soc_dai *dai) · line 96
Quoted 2 lines
sound/soc/samsung/s3c2412-i2s.c +0 -0
  	return 0;
  }
~arvind

Mark Brown

maintainer

2017-07-27 16:56 UTC #4 lore raw

Re: [RFT 2/2] ASoC: samsung: Add proper error paths to s3c24xx I2S driver

Quoted 9 lines · Krzysztof Kozlowski

Quoted 5 lines · Arvind Yadav
On Thursday 27 July 2017 12:27 AM, Krzysztof Kozlowski wrote:
Now s3c2412_i2s.iis_cclk and s3c2412_i2s.iis_pclk are holding "iis" clock. Now no one handling "mpll" clock. If we will call s3c2412_i2s_remove. It's means. we are disabling "iis" clock twice. Which is not correct.
Right, what a obfuscated code is this... Thanks for pointing this out. Also the s3c_i2sv2_probe() misses clk_prepare().

Yeah, I think the root cause here is that the code needs a cleanup so people can tell what's going on.

Mark Brown

maintainer

2017-07-28 10:18 UTC #5 lore raw

Applied "ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe failure" to the asoc tree

In reply to Krzysztof Kozlowski

The patch

ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe failure

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

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

Quoted 1 line

From 776bac64bf13e20c27f7ad168811abdf3b72f14d Mon Sep 17 00:00:00 2001

From: Krzysztof Kozlowski <[email protected]>
Date: Thu, 27 Jul 2017 19:13:36 +0200
Subject: [PATCH] ASoC: samsung: Fix possible double iounmap on s3c24xx driver
probe failure

Anyway, the owner of this mapped region should be s3c2412-i2s.c because it starts the mapping.

Affected are drivers for S3C24xx family although issue was not reproduced.

Fixes 87b132bc0315 ("ASoC: samsung: s3c24{xx,12}-i2s: port to use generic dmaengine API")

Signed-off-by Krzysztof Kozlowski <[email protected]>

Acked-by Arvind Yadav<[email protected]>

Signed-off-by Mark Brown <[email protected]>

---
 sound/soc/samsung/s3c-i2s-v2.c | 1 -
 1 file changed, 1 deletion(-)

sound/soc/samsung/s3c-i2s-v2.c +0 -1

index ca522a95160b..9b28046eea8e 100644
@@ -634,7 +634,6 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,
 	i2s->iis_pclk = clk_get(dev, "iis");
 	if (IS_ERR(i2s->iis_pclk)) {
 		dev_err(dev, "failed to get iis_clock\n");
-		iounmap(i2s->regs);
 		return -ENOENT;
 	}

Signature · 1 line

2.13.2