[PATCH v2 0/6] ASoC: samsung: remove cppcheck warnings

[PATCH v2 0/6] ASoC: samsung: remove cppcheck warnings

[Pierre-Louis Bossart]

No functional changes except for patch 3 and 4 where missing error
checks were added for consistency.

v2:
added Krzysztof Kozlowski's tags
added fix for first patch already merged as suggested by Krzysztof Kozlowski
moved variable to lower scope in patch6

Pierre-Louis Bossart (6):
  ASoC: samsung: tm2_wm5510: fix check of of_parse return value
  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_wm5510: remove shadowed 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     | 5 +++--
 5 files changed, 11 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Pierre-Louis Bossart]

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,
     ^

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.

This patch provides the correct fix, there's no need to check for
args.np because args.np won't be initialized on errors.

Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
Cc: <stable@vger.kernel.org>
Suggested-by: Krzysztof Kozlowski <krzk@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 da6204248f82..125e07f65d2b 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 (ret || !args.np) {
+		if (ret) {
 			dev_err(dev, "i2s-controller property parse error: %d\n", i);
 			ret = -EINVAL;
 			goto dai_node_put;
-- 
2.25.1

[PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Pierre-Louis Bossart]

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,
     ^

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.

This patch provides the correct fix, there's no need to check for
args.np because args.np won't be initialized on errors.

Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
Cc: <stable@vger.kernel.org>
Suggested-by: Krzysztof Kozlowski <krzk@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 da6204248f82..125e07f65d2b 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 (ret || !args.np) {
+		if (ret) {
 			dev_err(dev, "i2s-controller property parse error: %d\n", i);
 			ret = -EINVAL;
 			goto dai_node_put;
-- 
2.25.1

[PATCH v2 2/6] ASoC: samsung: i2s: remove unassigned variable

[Pierre-Louis Bossart]

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).

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
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;
 
-- 
2.25.1

[PATCH v2 3/6] ASoC: samsung: s3c24xx_simtec: add missing error check

[Pierre-Louis Bossart]

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.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
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;
-- 
2.25.1

[PATCH v2 4/6] ASoC: samsung: smdk_wm8994: add missing return

[Pierre-Louis Bossart]

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.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
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;
-- 
2.25.1

[PATCH v2 5/6] ASoC: samsung: snow: remove useless test

[Pierre-Louis Bossart]

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)
 ^

Fixes: 7de6b6bc1a58 ("ASoC: samsung: use asoc_rtd_to_cpu() / asoc_rtd_to_codec() macro for DAI pointer")
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
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,
-- 
2.25.1

[PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Pierre-Louis Bossart]

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;
                         ^
Move the top-level variable to the lower scope where it's needed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/samsung/tm2_wm5110.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
index 125e07f65d2b..84c2c63d5a87 100644
--- a/sound/soc/samsung/tm2_wm5110.c
+++ b/sound/soc/samsung/tm2_wm5110.c
@@ -501,7 +501,6 @@ static int tm2_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct snd_soc_card *card = &tm2_card;
 	struct tm2_machine_priv *priv;
-	struct of_phandle_args args;
 	struct snd_soc_dai_link *dai_link;
 	int num_codecs, ret, i;
 
@@ -585,6 +584,8 @@ static int tm2_probe(struct platform_device *pdev)
 	}
 
 	if (num_codecs > 1) {
+		struct of_phandle_args args;
+
 		/* HDMI DAI link (I2S1) */
 		i = card->num_links - 1;
 
-- 
2.25.1

Re: [PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Sylwester Nawrocki]

On 22.02.2021 22:33, 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,
>      ^
> 
> 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.
> 
> This patch provides the correct fix, there's no need to check for
> args.np because args.np won't be initialized on errors.
> 
> Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thank you for fixing all those issues.

Re: [PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Sylwester Nawrocki]

On 22.02.2021 22:33, 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,
>      ^
> 
> 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.
> 
> This patch provides the correct fix, there's no need to check for
> args.np because args.np won't be initialized on errors.
> 
> Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thank you for fixing all those issues.

Re: [PATCH v2 2/6] ASoC: samsung: i2s: remove unassigned variable

[Sylwester Nawrocki]

On 22.02.2021 22:33, 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).
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH v2 3/6] ASoC: samsung: s3c24xx_simtec: add missing error check

[Sylwester Nawrocki]

On 22.02.2021 22:33, 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.
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH v2 4/6] ASoC: samsung: smdk_wm8994: add missing return

[Sylwester Nawrocki]

On 22.02.2021 22:33, Pierre-Louis Bossart wrote:

> 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.
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.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;

I think it would be better to just make it "return -EINVAL;"

Re: [PATCH v2 5/6] ASoC: samsung: snow: remove useless test

[Sylwester Nawrocki]

On 22.02.2021 22:33, 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)
>  ^
> 
> Fixes: 7de6b6bc1a58 ("ASoC: samsung: use asoc_rtd_to_cpu() / asoc_rtd_to_codec() macro for DAI pointer")
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Sylwester Nawrocki]

On 22.02.2021 22:33, Pierre-Louis Bossart wrote:
> Move the top-level variable to the lower scope where it's needed.

Actually I like your original patch better as there is really no need
for multiple lower scope declarations in that fairly small function.

Re: [PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Pierre-Louis Bossart]

On 2/23/21 5:25 AM, Sylwester Nawrocki wrote:
> On 22.02.2021 22:33, Pierre-Louis Bossart wrote:
>> Move the top-level variable to the lower scope where it's needed.
> 
> Actually I like your original patch better as there is really no need
> for multiple lower scope declarations in that fairly small function.

I have no opinion, just let me know what the consensus is.

Re: [PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Krzysztof Kozlowski]

On Tue, 23 Feb 2021 at 19:20, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 2/23/21 5:25 AM, Sylwester Nawrocki wrote:
> > On 22.02.2021 22:33, Pierre-Louis Bossart wrote:
> >> Move the top-level variable to the lower scope where it's needed.
> >
> > Actually I like your original patch better as there is really no need
> > for multiple lower scope declarations in that fairly small function.
>
> I have no opinion, just let me know what the consensus is.

I proposed to have both variables local scope, to reduce the size of
function-scope variables. Their number tends to grow in probe() a lot,
so when a variable can be localized more, it makes the code easier to
understand. No need to figure out who/where/when uses the variable.
Local scope makes it much easier.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Sylwester Nawrocki]

On 23.02.2021 19:29, Krzysztof Kozlowski wrote:
> On Tue, 23 Feb 2021 at 19:20, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>> On 2/23/21 5:25 AM, Sylwester Nawrocki wrote:
>>> On 22.02.2021 22:33, Pierre-Louis Bossart wrote:
>>>> Move the top-level variable to the lower scope where it's needed.
>>>
>>> Actually I like your original patch better as there is really no need
>>> for multiple lower scope declarations in that fairly small function.
>>
>> I have no opinion, just let me know what the consensus is.
> 
> I proposed to have both variables local scope, to reduce the size of
> function-scope variables. Their number tends to grow in probe() a lot,
> so when a variable can be localized more, it makes the code easier to
> understand. No need to figure out who/where/when uses the variable.
> Local scope makes it much easier.

I don't have strong opinion, let's keep it local then as in current patch.

Re: [PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Krzysztof Kozlowski]

On Mon, Feb 22, 2021 at 03:33:01PM -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,
>      ^
> 
> 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.
> 
> This patch provides the correct fix, there's no need to check for
> args.np because args.np won't be initialized on errors.
> 
> Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/6] ASoC: samsung: tm2_wm5510: fix check of of_parse return value

[Krzysztof Kozlowski]

On Mon, Feb 22, 2021 at 03:33:01PM -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,
>      ^
> 
> 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.
> 
> This patch provides the correct fix, there's no need to check for
> args.np because args.np won't be initialized on errors.
> 
> Fixes: 75fa6833aef3 ("ASoC: samsung: tm2_wm5110: check of_parse return value")
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 6/6] ASoC: samsung: tm2_wm5510: remove shadowed variable

[Krzysztof Kozlowski]

On Mon, Feb 22, 2021 at 03:33:06PM -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;
>                          ^
> Move the top-level variable to the lower scope where it's needed.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/samsung/tm2_wm5110.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof