[PATCH 0/6] ASoC: samsung: remove cppcheck warnings
No functional changes except for patch 2 and 3 where missing error checks were added for consistency.
Pierre-Louis Bossart (6): ASoC: samsung: i2s: remove unassigned variable ASoC: samsung: s3c24xx_simtec: add missing error check ASoC: samsung: smdk_wm8994: add missing return ASoC: samsung: snow: remove useless test ASoC: samsung: tm2_wm5110: check of_parse return value ASoC: samsung: tm2_wm5510: remove shadowing variable
sound/soc/samsung/i2s.c | 3 +-- sound/soc/samsung/s3c24xx_simtec.c | 5 +++++ sound/soc/samsung/smdk_wm8994.c | 1 + sound/soc/samsung/snow.c | 5 +---- sound/soc/samsung/tm2_wm5110.c | 3 +-- 5 files changed, 9 insertions(+), 8 deletions(-)
cppcheck warning:
sound/soc/samsung/i2s.c:1159:18: style: Variable 'dai' is not assigned a value. [unassignedVariable] struct i2s_dai *dai; ^
This variable is only used for a sizeof(*dai).
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/i2s.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index b043183174b2..c632842d42eb 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -1156,11 +1156,10 @@ static int i2s_alloc_dais(struct samsung_i2s_priv *priv, static const char *stream_names[] = { "Primary Playback", "Secondary Playback" }; struct snd_soc_dai_driver *dai_drv; - struct i2s_dai *dai; int i;
priv->dai = devm_kcalloc(&priv->pdev->dev, num_dais, - sizeof(*dai), GFP_KERNEL); + sizeof(struct i2s_dai), GFP_KERNEL); if (!priv->dai) return -ENOMEM;
On Fri, Feb 19, 2021 at 05:09:13PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/i2s.c:1159:18: style: Variable 'dai' is not assigned a value. [unassignedVariable] struct i2s_dai *dai; ^
This variable is only used for a sizeof(*dai).
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/samsung/i2s.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
cppcheck warning:
sound/soc/samsung/s3c24xx_simtec.c:191:7: style: Variable 'ret' is assigned a value that is never used. [unreadVariable] ret = snd_soc_dai_set_clkdiv(cpu_dai, S3C24XX_DIV_PRESCALER, ^
Looking at the code, it's not clear why the return value is checked in the two other cases but not here, so mirror the behavior and add a check.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/s3c24xx_simtec.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/samsung/s3c24xx_simtec.c b/sound/soc/samsung/s3c24xx_simtec.c index 3cddd11344ac..81a29d12c57d 100644 --- a/sound/soc/samsung/s3c24xx_simtec.c +++ b/sound/soc/samsung/s3c24xx_simtec.c @@ -190,6 +190,11 @@ static int simtec_hw_params(struct snd_pcm_substream *substream,
ret = snd_soc_dai_set_clkdiv(cpu_dai, S3C24XX_DIV_PRESCALER, cdclk_scale); + if (ret) { + pr_err("%s: failed to set clock div\n", + __func__); + return ret; + } }
return 0;
On Fri, Feb 19, 2021 at 05:09:14PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/s3c24xx_simtec.c:191:7: style: Variable 'ret' is assigned a value that is never used. [unreadVariable] ret = snd_soc_dai_set_clkdiv(cpu_dai, S3C24XX_DIV_PRESCALER, ^
Looking at the code, it's not clear why the return value is checked in the two other cases but not here, so mirror the behavior and add a check.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/samsung/s3c24xx_simtec.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
cppcheck warning:
sound/soc/samsung/smdk_wm8994.c:179:6: style: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment] ret = devm_snd_soc_register_card(&pdev->dev, card); ^ sound/soc/samsung/smdk_wm8994.c:166:8: note: ret is assigned ret = -EINVAL; ^ sound/soc/samsung/smdk_wm8994.c:179:6: note: ret is overwritten ret = devm_snd_soc_register_card(&pdev->dev, card); ^
The initial authors bothered to set ret to -EINVAL and throw a dev_err() message, so it looks like there is a missing return to avoid continuing if the property is missing.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/smdk_wm8994.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/samsung/smdk_wm8994.c b/sound/soc/samsung/smdk_wm8994.c index 681b244d5312..39a7a449f554 100644 --- a/sound/soc/samsung/smdk_wm8994.c +++ b/sound/soc/samsung/smdk_wm8994.c @@ -164,6 +164,7 @@ static int smdk_audio_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Property 'samsung,i2s-controller' missing or invalid\n"); ret = -EINVAL; + return ret; }
smdk_dai[0].platforms->name = NULL;
On Fri, Feb 19, 2021 at 05:09:15PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/smdk_wm8994.c:179:6: style: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment] ret = devm_snd_soc_register_card(&pdev->dev, card); ^ sound/soc/samsung/smdk_wm8994.c:166:8: note: ret is assigned ret = -EINVAL; ^ sound/soc/samsung/smdk_wm8994.c:179:6: note: ret is overwritten ret = devm_snd_soc_register_card(&pdev->dev, card); ^
The initial authors bothered to set ret to -EINVAL and throw a dev_err() message, so it looks like there is a missing return to avoid continuing if the property is missing.
Good catch. It's a required property.
Reviewed-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
cppcheck warning:
sound/soc/samsung/snow.c:112:2: style:inconclusive: Found duplicate branches for 'if' and 'else'. [duplicateBranch] if (rtd->num_codecs > 1) ^ sound/soc/samsung/snow.c:114:2: note: Found duplicate branches for 'if' and 'else'. else ^ sound/soc/samsung/snow.c:112:2: note: Found duplicate branches for 'if' and 'else'. if (rtd->num_codecs > 1) ^
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/snow.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/soc/samsung/snow.c b/sound/soc/samsung/snow.c index 989af624dd11..6da674e901ca 100644 --- a/sound/soc/samsung/snow.c +++ b/sound/soc/samsung/snow.c @@ -109,10 +109,7 @@ static int snow_late_probe(struct snd_soc_card *card) rtd = snd_soc_get_pcm_runtime(card, &card->dai_link[0]);
/* In the multi-codec case codec_dais 0 is MAX98095 and 1 is HDMI. */ - if (rtd->num_codecs > 1) - codec_dai = asoc_rtd_to_codec(rtd, 0); - else - codec_dai = asoc_rtd_to_codec(rtd, 0); + codec_dai = asoc_rtd_to_codec(rtd, 0);
/* Set the MCLK rate for the codec */ return snd_soc_dai_set_sysclk(codec_dai, 0,
On Fri, Feb 19, 2021 at 05:09:16PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/snow.c:112:2: style:inconclusive: Found duplicate branches for 'if' and 'else'. [duplicateBranch] if (rtd->num_codecs > 1) ^ sound/soc/samsung/snow.c:114:2: note: Found duplicate branches for 'if' and 'else'. else ^ sound/soc/samsung/snow.c:112:2: note: Found duplicate branches for 'if' and 'else'. if (rtd->num_codecs > 1) ^
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/samsung/snow.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Fixes: 7de6b6bc1a58 ("ASoC: samsung: use asoc_rtd_to_cpu() / asoc_rtd_to_codec() macro for DAI pointer") Reviewed-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
cppcheck warning:
sound/soc/samsung/tm2_wm5110.c:605:6: style: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment] ret = devm_snd_soc_register_component(dev, &tm2_component, ^ sound/soc/samsung/tm2_wm5110.c:554:7: note: ret is assigned ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", ^ sound/soc/samsung/tm2_wm5110.c:605:6: note: ret is overwritten ret = devm_snd_soc_register_component(dev, &tm2_component, ^
it seems wise to first test the return value before checking if the returned argument is not null?
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/tm2_wm5110.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index 9300fef9bf26..da6204248f82 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -553,7 +553,7 @@ static int tm2_probe(struct platform_device *pdev)
ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", cells_name, i, &args); - if (!args.np) { + if (ret || !args.np) { dev_err(dev, "i2s-controller property parse error: %d\n", i); ret = -EINVAL; goto dai_node_put;
On Fri, Feb 19, 2021 at 05:09:17PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/tm2_wm5110.c:605:6: style: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment] ret = devm_snd_soc_register_component(dev, &tm2_component, ^ sound/soc/samsung/tm2_wm5110.c:554:7: note: ret is assigned ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", ^ sound/soc/samsung/tm2_wm5110.c:605:6: note: ret is overwritten ret = devm_snd_soc_register_component(dev, &tm2_component, ^
it seems wise to first test the return value before checking if the returned argument is not null?
Actually this is real bug. The args is a stack variable, so it could have junk (uninitialized) therefore args.np could have a non-NULL and random value even though property was missing. Later could trigger invalid pointer dereference.
Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2 board") Cc: stable@vger.kernel.org
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/samsung/tm2_wm5110.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index 9300fef9bf26..da6204248f82 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -553,7 +553,7 @@ static int tm2_probe(struct platform_device *pdev)
ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", cells_name, i, &args);
if (!args.np) {
if (ret || !args.np) {
Only "if (ret)" because args.np won't be initialized on errors.
Best regards, Krzysztof
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index 9300fef9bf26..da6204248f82 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -553,7 +553,7 @@ static int tm2_probe(struct platform_device *pdev)
ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", cells_name, i, &args);
if (!args.np) {
if (ret || !args.np) {Only "if (ret)" because args.np won't be initialized on errors.
Thanks Krzysztof for the review, I will make that change in a v2. But just to be clear, there's no need to test args.np then?
On Mon, 22 Feb 2021 at 17:45, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index 9300fef9bf26..da6204248f82 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -553,7 +553,7 @@ static int tm2_probe(struct platform_device *pdev)
ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", cells_name, i, &args);
if (!args.np) {
if (ret || !args.np) {Only "if (ret)" because args.np won't be initialized on errors.
Thanks Krzysztof for the review, I will make that change in a v2. But just to be clear, there's no need to test args.np then?
Correct, no need to test args.np.
Best regards, Krzysztof
cppcheck warning:
sound/soc/samsung/tm2_wm5110.c:552:26: style: Local variable 'args' shadows outer variable [shadowVariable] struct of_phandle_args args; ^ sound/soc/samsung/tm2_wm5110.c:504:25: note: Shadowed declaration struct of_phandle_args args; ^ sound/soc/samsung/tm2_wm5110.c:552:26: note: Shadow variable struct of_phandle_args args; ^
it's not clear why there was a need for a local variable at a lower scope, remove it and share the same variable between scopes.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/samsung/tm2_wm5110.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index da6204248f82..b9c17fdd168d 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -549,7 +549,6 @@ static int tm2_probe(struct platform_device *pdev) }
for (i = 0; i < num_codecs; i++) { - struct of_phandle_args args;
ret = of_parse_phandle_with_args(dev->of_node, "i2s-controller", cells_name, i, &args);
On Fri, Feb 19, 2021 at 05:09:18PM -0600, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/tm2_wm5110.c:552:26: style: Local variable 'args' shadows outer variable [shadowVariable] struct of_phandle_args args; ^ sound/soc/samsung/tm2_wm5110.c:504:25: note: Shadowed declaration struct of_phandle_args args; ^ sound/soc/samsung/tm2_wm5110.c:552:26: note: Shadow variable struct of_phandle_args args; ^
it's not clear why there was a need for a local variable at a lower scope, remove it and share the same variable between scopes.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/samsung/tm2_wm5110.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index da6204248f82..b9c17fdd168d 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -549,7 +549,6 @@ static int tm2_probe(struct platform_device *pdev) }
for (i = 0; i < num_codecs; i++) {
struct of_phandle_args args;
I would prefer to have them more local, so: 1. Remove args in tm2_probe() scope. 2. Add args around line 588 in "if (num_codecs > 1) {" block.
Best regards, Krzysztof
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c index da6204248f82..b9c17fdd168d 100644 --- a/sound/soc/samsung/tm2_wm5110.c +++ b/sound/soc/samsung/tm2_wm5110.c @@ -549,7 +549,6 @@ static int tm2_probe(struct platform_device *pdev) }
for (i = 0; i < num_codecs; i++) {
struct of_phandle_args args;I would prefer to have them more local, so:
- Remove args in tm2_probe() scope.
- Add args around line 588 in "if (num_codecs > 1) {" block.
Will change this in a v2.
On 20.02.2021 00:09, Pierre-Louis Bossart wrote:
cppcheck warning:
sound/soc/samsung/tm2_wm5110.c:552:26: style: Local variable 'args' shadows outer variable [shadowVariable] struct of_phandle_args args;
it's not clear why there was a need for a local variable at a lower scope, remove it and share the same variable between scopes.
s/tm2_wm5510/tm2_wm5110
Reviewed-by: Sylwester Nawrocki s.nawrocki@samsung.com
On Fri, 19 Feb 2021 17:09:12 -0600, Pierre-Louis Bossart wrote:
No functional changes except for patch 2 and 3 where missing error checks were added for consistency.
Pierre-Louis Bossart (6): ASoC: samsung: i2s: remove unassigned variable ASoC: samsung: s3c24xx_simtec: add missing error check ASoC: samsung: smdk_wm8994: add missing return ASoC: samsung: snow: remove useless test ASoC: samsung: tm2_wm5110: check of_parse return value ASoC: samsung: tm2_wm5510: remove shadowing variable
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[5/6] ASoC: samsung: tm2_wm5110: check of_parse return value commit: 75fa6833aef349fce1b315eaa96c9611a227014b
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
On Mon, 22 Feb 2021 at 17:08, Mark Brown broonie@kernel.org wrote:
On Fri, 19 Feb 2021 17:09:12 -0600, Pierre-Louis Bossart wrote:
No functional changes except for patch 2 and 3 where missing error checks were added for consistency.
Pierre-Louis Bossart (6): ASoC: samsung: i2s: remove unassigned variable ASoC: samsung: s3c24xx_simtec: add missing error check ASoC: samsung: smdk_wm8994: add missing return ASoC: samsung: snow: remove useless test ASoC: samsung: tm2_wm5110: check of_parse return value ASoC: samsung: tm2_wm5510: remove shadowing variable
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[5/6] ASoC: samsung: tm2_wm5110: check of_parse return value commit: 75fa6833aef349fce1b315eaa96c9611a227014b
Hi Mark,
Hmmm, I had comments about this one so it should not have been applied. The check if (ret || !args.np) is still not good (or confusing) because args is an uninitialized stack value.
Best regards, Krzysztof
On Mon, Feb 22, 2021 at 06:31:15PM +0100, Krzysztof Kozlowski wrote:
Hmmm, I had comments about this one so it should not have been applied. The check if (ret || !args.np) is still not good (or confusing) because args is an uninitialized stack value.
Ah, I saw the "this is actually a fix CC stable" bit, the bit saying there were issues was hidden - it looked like you'd just not deleted context.
On 2/22/21 2:19 PM, Mark Brown wrote:
On Mon, Feb 22, 2021 at 06:31:15PM +0100, Krzysztof Kozlowski wrote:
Hmmm, I had comments about this one so it should not have been applied. The check if (ret || !args.np) is still not good (or confusing) because args is an uninitialized stack value.
Ah, I saw the "this is actually a fix CC stable" bit, the bit saying there were issues was hidden - it looked like you'd just not deleted context.
If you give me about an hour, I can resend a v2 series that includes Krzysztof's tags and provides the correct fix for this patch5.
On Fri, 19 Feb 2021 17:09:12 -0600, Pierre-Louis Bossart wrote:
No functional changes except for patch 2 and 3 where missing error checks were added for consistency.
Pierre-Louis Bossart (6): ASoC: samsung: i2s: remove unassigned variable ASoC: samsung: s3c24xx_simtec: add missing error check ASoC: samsung: smdk_wm8994: add missing return ASoC: samsung: snow: remove useless test ASoC: samsung: tm2_wm5110: check of_parse return value ASoC: samsung: tm2_wm5510: remove shadowing variable
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: samsung: i2s: remove unassigned variable commit: b832fa1ce0826a915a9e1fe533fc86a1cf5ae8cd [2/6] ASoC: samsung: s3c24xx_simtec: add missing error check commit: feb45eb2ecafdfaca5b82f27997e717ae3c70323 [3/6] ASoC: samsung: smdk_wm8994: add missing return commit: 1e4a9fcffd56b73acf4e706465be2df261da83de [4/6] ASoC: samsung: snow: remove useless test commit: 4ff97b8dc7e6a3c5caf733ebad4efaf018829142
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
participants (4)
-
Krzysztof Kozlowski -
Mark Brown -
Pierre-Louis Bossart -
Sylwester Nawrocki