ASoC: samsung: s3c24xx: Fixes for error paths
Subsystems (3)
v2 cover ASoC: samsung: s3c24xx: Fixes for error paths
Hi,
Changes since v1:
1. Add patch 2/3.
2. Drop assignment of iis_cclk=iis_pclk to have balance with clk disables error and remove paths (pointed by Arvind).
Best regards,
Krzysztof
Krzysztof Kozlowski (3):
ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe
failure
ASoC: samsung: Add missing prepare for iis clock of s3c24xx
ASoC: samsung: Add proper error paths to s3c24xx I2S driver
sound/soc/samsung/s3c-i2s-v2.c | 12 ++++++++++-- sound/soc/samsung/s3c-i2s-v2.h | 7 +++++++ sound/soc/samsung/s3c2412-i2s.c | 13 +++++++++---- 3 files changed, 26 insertions(+), 6 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/3 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.
---
Not marking as Cc-stable because this is theoretical problem, not reproduced and also not tested.
Please, kindly test on S3C24xx hardware.
Changes since v1:
1. None
--- sound/soc/samsung/s3c-i2s-v2.c | 1 - 1 file changed, 1 deletion(-)
sound/soc/samsung/s3c-i2s-v2.c +0 -1
@@ -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
Re: [RFT v2 1/3] ASoC: samsung: Fix possible double iounmap on s3c24xx driver probe failure
Quoted 11 lines · Krzysztof Kozlowski
Commit
87b132bc0315("ASoC: samsung: s3c24{xx,12}-i2s: port to use generic dmaengine API") movedioremap()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 ins3c_i2sv2_probe()failed, the address space would be unmapped there. This could lead to:
1. doubleiounmap()later from devm-interface ofs3c2412_iis_dev_probe()),
2. accessing the memory by other functions ins3c2412-i2s.cunit.
Quoted 7 lines
Anyway, the owner of this mapped region should be
s3c2412-i2s.cbecause 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]>
Quoted 25 lines
---
Not marking as Cc-stable because this is theoretical problem, not reproduced and also not tested.
Please, kindly test on S3C24xx hardware.
Changes since v1:
1. None--- sound/soc/samsung/s3c-i2s-v2.c | 1 - 1 file changed, 1 deletion(-)sound/soc/samsung/s3c-i2s-v2.c +0
-1@@ -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 ; }
PATCH v2 2/3 ASoC: samsung: Add missing prepare for iis clock of s3c24xx
In reply to Krzysztof KozlowskiThe s3c_i2sv2_probe() only enabled iis clock. Missing prepare isn't probably fatal, because for SoC clocks this is usually no-op, but for correctness this clock should be prepared.
---
Changes since v1:
1. New patch
--- sound/soc/samsung/s3c-i2s-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/samsung/s3c-i2s-v2.c +1 -1
@@ -637,7 +637,7 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,return -ENOENT ; } -clk_enable (i2s->iis_pclk); +clk_prepare_enable (i2s->iis_pclk);/* Mark ourselves as in TXRX mode so we can run through our cleanup * process without warnings. */
Signature · 1 line
2.11.0
Re: [RFT v2 2/3] ASoC: samsung: Add missing prepare for iis clock of s3c24xx
Hi,
Quoted 19 lines · Krzysztof Kozlowski
The
s3c_i2sv2_probe()only enabled iis clock. Missing prepare isn't probably fatal, because for SoC clocks this is usually no-op, but for correctness this clock should be prepared.Signed-off-by Krzysztof Kozlowski <[email protected]>---
Changes since v1:
1. New patch--- sound/soc/samsung/s3c-i2s-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)sound/soc/samsung/s3c-i2s-v2.c +0
-0
sound/soc/samsung/s3c-i2s-v2.c int s3c_i2sv2_probe(struct snd_soc_dai *dai, · line 640
Quoted 5 lines
sound/soc/samsung/s3c-i2s-v2.c +1
-1@@ -637,7 +637,7 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,return -ENOENT ; } -clk_enable (i2s->iis_pclk); +clk_prepare_enable (i2s->iis_pclk);
Please, handle are return value of clk_prepare_enble.
sound/soc/samsung/s3c-i2s-v2.c int s3c_i2sv2_probe(struct snd_soc_dai *dai, · line 643
Quoted 3 lines
sound/soc/samsung/s3c-i2s-v2.c +0
-0/* Mark ourselves as in TXRX mode so we can run through our cleanup * process without warnings. */
~arvind
Re: [RFT v2 2/3] ASoC: samsung: Add missing prepare for iis clock of s3c24xx
Quoted 18 lines · Arvind Yadav
Hi,
Quoted 14 lines · Krzysztof Kozlowski
The
s3c_i2sv2_probe()only enabled iis clock. Missing prepare isn't probably fatal, because for SoC clocks this is usually no-op, but for correctness this clock should be prepared.Signed-off-by Krzysztof Kozlowski <[email protected]>---
Changes since v1:
1. New patch--- sound/soc/samsung/s3c-i2s-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines
sound/soc/samsung/s3c-i2s-v2.c int s3c_i2sv2_probe(struct snd_soc_dai *dai, · line 639
Quoted 9 lines
sound/soc/samsung/s3c-i2s-v2.c +1
-1@@ -637,7 +637,7 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,return -ENOENT ; } -clk_enable (i2s->iis_pclk); +clk_prepare_enable (i2s->iis_pclk);Please, handle are return value of
clk_prepare_enble.
Which is a different issue, different bug. We can fix it but it should be not mixed with this fix here.
Best regards,
Krzysztof
Arvind Yadav
Acked-byRe: [RFT v2 2/3] ASoC: samsung: Add missing prepare for iis clock of s3c24xx
Quoted 10 lines · Krzysztof Kozlowski
Quoted 9 lines · Arvind Yadav
Hi,
Quoted 5 lines · Krzysztof Kozlowski
The
s3c_i2sv2_probe()only enabled iis clock. Missing prepare isn't probably fatal, because for SoC clocks this is usually no-op, but for correctness this clock should be prepared.Signed-off-by Krzysztof Kozlowski <[email protected]>
Quoted 25 lines
Quoted 9 lines
Quoted 9 lines
---
Changes since v1:
1. New patch--- sound/soc/samsung/s3c-i2s-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 10 lines
sound/soc/samsung/s3c-i2s-v2.c int s3c_i2sv2_probe(struct snd_soc_dai *dai, · line 639
Quoted 9 lines
sound/soc/samsung/s3c-i2s-v2.c +1
-1@@ -637,7 +637,7 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,return -ENOENT ; } -clk_enable (i2s->iis_pclk); +clk_prepare_enable (i2s->iis_pclk);Please, handle are return value of
clk_prepare_enble.Which is a different issue, different bug. We can fix it but it should be not mixed with this fix here.
Best regards,
Krzysztof
Mark Brown
maintainerApplied "ASoC: samsung: Add missing prepare for iis clock of s3c24xx" to the asoc tree
In reply to Krzysztof KozlowskiThe patch
ASoC: samsung: Add missing prepare for iis clock of s3c24xx
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
81ea6cc73c384e4093519e0c75394aa2a1941a10Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <[email protected]>
Date: Thu, 27 Jul 2017 19:13:37 +0200
Subject: [PATCH] ASoC: samsung: Add missing prepare for iis clock of s3c24xx
The s3c_i2sv2_probe() only enabled iis clock. Missing prepare isn't probably fatal, because for SoC clocks this is usually no-op, but for correctness this clock should be prepared.
--- sound/soc/samsung/s3c-i2s-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/samsung/s3c-i2s-v2.c +1 -1
@@ -637,7 +637,7 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai,return -ENOENT ; } -clk_enable (i2s->iis_pclk); +clk_prepare_enable (i2s->iis_pclk);/* Mark ourselves as in TXRX mode so we can run through our cleanup * process without warnings. */
Signature · 1 line
2.13.2
PATCH v2 3/3 ASoC: samsung: Add proper error paths to s3c24xx I2S driver
In reply to Krzysztof Kozlowskis3c2412_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.
---
Please, kindly test on S3C24xx hardware.
Changes since v1:
1. Drop clk assignment to disable proper clock on remove()
--- sound/soc/samsung/s3c-i2s-v2.c | 9 +++++++++ sound/soc/samsung/s3c-i2s-v2.h | 7 +++++++ sound/soc/samsung/s3c2412-i2s.c | 13 +++++++++---- 3 files changed, 25 insertions(+), 4 deletions(-)
sound/soc/samsung/s3c-i2s-v2.c +9 -0
@@ -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_unprepare (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
@@ -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 -4
@@ -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,20 +74,24 @@ 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; - - s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk; + goto err;/* Configure the I2S pins (GPE0...GPE4) in correct mode */ s3c_gpio_cfgall_range (S3C2410_GPE (0 ),5 ,S3C_GPIO_SFN (2 ),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
Re: [RFT v2 3/3] ASoC: samsung: Add proper error paths to s3c24xx I2S driver
Quoted 5 lines · Krzysztof Kozlowski
s3c2412_i2s_probe()might fail so driver has to revert work done bys3c_i2sv2_probe()(clock enabling). Missing doing this would lead to clock enable in-balance.Signed-off-by Krzysztof Kozlowski <[email protected]>
Quoted 93 lines
---
Please, kindly test on S3C24xx hardware.
Changes since v1:
1. Drop clk assignment to disable proper clock onremove()--- sound/soc/samsung/s3c-i2s-v2.c | 9 +++++++++ sound/soc/samsung/s3c-i2s-v2.h | 7 +++++++ sound/soc/samsung/s3c2412-i2s.c | 13 +++++++++---- 3 files changed, 25 insertions(+), 4 deletions(-)sound/soc/samsung/s3c-i2s-v2.c +9
-0@@ -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_unprepare (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@@ -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
-4@@ -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,20 +74,24 @@ 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; - - s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk; + goto err;/* Configure the I2S pins (GPE0...GPE4) in correct mode */ s3c_gpio_cfgall_range (S3C2410_GPE (0 ),5 ,S3C_GPIO_SFN (2 ),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 ; }
Mark Brown
maintainerApplied "ASoC: samsung: Add proper error paths to s3c24xx I2S driver" to the asoc tree
In reply to Krzysztof KozlowskiThe patch
ASoC: samsung: Add proper error paths to s3c24xx I2S driver
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
6f187f7ef1c15dca9f720b18a7e928718fd1a8b3Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <[email protected]>
Date: Thu, 27 Jul 2017 19:13:38 +0200
Subject: [PATCH] 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.
--- sound/soc/samsung/s3c-i2s-v2.c | 9 +++++++++ sound/soc/samsung/s3c-i2s-v2.h | 7 +++++++ sound/soc/samsung/s3c2412-i2s.c | 13 +++++++++---- 3 files changed, 25 insertions(+), 4 deletions(-)
sound/soc/samsung/s3c-i2s-v2.c +9 -0
@@ -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_unprepare (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
@@ -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 -4
@@ -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,20 +74,24 @@ 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; - - s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk; + goto err;/* Configure the I2S pins (GPE0...GPE4) in correct mode */ s3c_gpio_cfgall_range (S3C2410_GPE (0 ),5 ,S3C_GPIO_SFN (2 ),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.13.2