[PATCH v3 0/2] ASoC: samsung: remove cppcheck warnings

[PATCH v3 0/2] ASoC: samsung: remove cppcheck warnings

[Pierre-Louis Bossart]

v3:
Added Sylwester tag
Rebased and squashed fix with initial patch which was merged at some
point but can't be found in broonie/for-next (not sure what happened?)
Corrected patch subjects to tm2_wm5110
Reverted second patch to initial v1, after agreement between Krzysztof
and Sylwester

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 (2):
  ASoC: samsung: tm2_wm5110: check of of_parse return value
  ASoC: samsung: tm2_wm5110: remove shadowing variable

 sound/soc/samsung/tm2_wm5110.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] ASoC: samsung: tm2_wm5110: 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.

There's no need to check for args.np because args.np won't be
initialized on errors.

Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
Cc: <stable@vger.kernel.org>
Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
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..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 (!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 v3 1/2] ASoC: samsung: tm2_wm5110: 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.

There's no need to check for args.np because args.np won't be
initialized on errors.

Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
Cc: <stable@vger.kernel.org>
Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
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..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 (!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 v3 2/2] ASoC: samsung: tm2_wm5110: remove shadowing 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;
                         ^

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 125e07f65d2b..c1ab6c3afd0e 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);
-- 
2.25.1

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Krzysztof Kozlowski]

On 11/03/2021 01:35, 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.
> 
> There's no need to check for args.np because args.np won't be
> initialized on errors.
> 
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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(-)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Krzysztof Kozlowski]

On 11/03/2021 01:35, 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.
> 
> There's no need to check for args.np because args.np won't be
> initialized on errors.
> 
> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 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(-)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v3 2/2] ASoC: samsung: tm2_wm5110: remove shadowing variable

[Krzysztof Kozlowski]

On 11/03/2021 01:35, 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 125e07f65d2b..c1ab6c3afd0e 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);
> 

The agreement was to keep it as in v2, so to have two local variables
instead of bigger scope.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

On Wed, Mar 10, 2021 at 06:35:15PM -0600, Pierre-Louis Bossart wrote:

> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>

Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
	Has these problem(s):
		- Subject does not match target commit subject
		  Just use
			git log -1 --format='Fixes: %h ("%s")'

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

On Wed, Mar 10, 2021 at 06:35:15PM -0600, Pierre-Louis Bossart wrote:

> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> Cc: <stable@vger.kernel.org>

Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
	Has these problem(s):
		- Subject does not match target commit subject
		  Just use
			git log -1 --format='Fixes: %h ("%s")'

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Pierre-Louis Bossart]

On 3/12/21 8:28 AM, Mark Brown wrote:
> On Wed, Mar 10, 2021 at 06:35:15PM -0600, Pierre-Louis Bossart wrote:
> 
>> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
>> Cc: <stable@vger.kernel.org>
> 
> Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
> 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> 	Has these problem(s):
> 		- Subject does not match target commit subject
> 		  Just use
> 			git log -1 --format='Fixes: %h ("%s")'
> 

Sorry, I don't know what to make of this. I don't see this commit 
11bc3bb24003

Something odd happened, there was an initial merge and it seems to have 
disappeared, it's no longer in the for-next branch?

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Pierre-Louis Bossart]

On 3/12/21 8:28 AM, Mark Brown wrote:
> On Wed, Mar 10, 2021 at 06:35:15PM -0600, Pierre-Louis Bossart wrote:
> 
>> Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
>> Cc: <stable@vger.kernel.org>
> 
> Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
> 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> 	Has these problem(s):
> 		- Subject does not match target commit subject
> 		  Just use
> 			git log -1 --format='Fixes: %h ("%s")'
> 

Sorry, I don't know what to make of this. I don't see this commit 
11bc3bb24003

Something odd happened, there was an initial merge and it seems to have 
disappeared, it's no longer in the for-next branch?

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Fri, Mar 12, 2021 at 10:30:32AM -0600, Pierre-Louis Bossart wrote:
> On 3/12/21 8:28 AM, Mark Brown wrote:

> > Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
> > 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> > 	Has these problem(s):
> > 		- Subject does not match target commit subject
> > 		  Just use
> > 			git log -1 --format='Fixes: %h ("%s")'

> Sorry, I don't know what to make of this. I don't see this commit
> 11bc3bb24003

> Something odd happened, there was an initial merge and it seems to have
> disappeared, it's no longer in the for-next branch?

That commit is your patch being applied, which I've dropped because of
the error reported.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Fri, Mar 12, 2021 at 10:30:32AM -0600, Pierre-Louis Bossart wrote:
> On 3/12/21 8:28 AM, Mark Brown wrote:

> > Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
> > 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
> > 	Has these problem(s):
> > 		- Subject does not match target commit subject
> > 		  Just use
> > 			git log -1 --format='Fixes: %h ("%s")'

> Sorry, I don't know what to make of this. I don't see this commit
> 11bc3bb24003

> Something odd happened, there was an initial merge and it seems to have
> disappeared, it's no longer in the for-next branch?

That commit is your patch being applied, which I've dropped because of
the error reported.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ASoC: samsung: tm2_wm5110: remove shadowing variable

[Pierre-Louis Bossart]

On 3/12/21 4:45 AM, Krzysztof Kozlowski wrote:
> On 11/03/2021 01:35, 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 125e07f65d2b..c1ab6c3afd0e 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);
>>
> 
> The agreement was to keep it as in v2, so to have two local variables
> instead of bigger scope.

Ack, I misunderstood the feedback, will resend.

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Pierre-Louis Bossart]

On 3/12/21 10:31 AM, Mark Brown wrote:
> On Fri, Mar 12, 2021 at 10:30:32AM -0600, Pierre-Louis Bossart wrote:
>> On 3/12/21 8:28 AM, Mark Brown wrote:
> 
>>> Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
>>> 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
>>> 	Has these problem(s):
>>> 		- Subject does not match target commit subject
>>> 		  Just use
>>> 			git log -1 --format='Fixes: %h ("%s")'
> 
>> Sorry, I don't know what to make of this. I don't see this commit
>> 11bc3bb24003
> 
>> Something odd happened, there was an initial merge and it seems to have
>> disappeared, it's no longer in the for-next branch?
> 
> That commit is your patch being applied, which I've dropped because of
> the error reported.

ack, not sure why there's a missing space.

git log -1 --format='Fixes: %h ("%s")' 8d1513cef51a
Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2 
board")

Re: [PATCH v3 1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value

[Pierre-Louis Bossart]

On 3/12/21 10:31 AM, Mark Brown wrote:
> On Fri, Mar 12, 2021 at 10:30:32AM -0600, Pierre-Louis Bossart wrote:
>> On 3/12/21 8:28 AM, Mark Brown wrote:
> 
>>> Commit: 11bc3bb24003 ("ASoC: samsung: tm2_wm5110: check of of_parse return value")
>>> 	Fixes tag: Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2board")
>>> 	Has these problem(s):
>>> 		- Subject does not match target commit subject
>>> 		  Just use
>>> 			git log -1 --format='Fixes: %h ("%s")'
> 
>> Sorry, I don't know what to make of this. I don't see this commit
>> 11bc3bb24003
> 
>> Something odd happened, there was an initial merge and it seems to have
>> disappeared, it's no longer in the for-next branch?
> 
> That commit is your patch being applied, which I've dropped because of
> the error reported.

ack, not sure why there's a missing space.

git log -1 --format='Fixes: %h ("%s")' 8d1513cef51a
Fixes: 8d1513cef51a ("ASoC: samsung: Add support for HDMI audio on TM2 
board")

Re: (subset) [PATCH v3 0/2] ASoC: samsung: remove cppcheck warnings

[Mark Brown]

On Wed, 10 Mar 2021 18:35:14 -0600, Pierre-Louis Bossart wrote:
> v3:
> Added Sylwester tag
> Rebased and squashed fix with initial patch which was merged at some
> point but can't be found in broonie/for-next (not sure what happened?)
> Corrected patch subjects to tm2_wm5110
> Reverted second patch to initial v1, after agreement between Krzysztof
> and Sylwester
> 
> [...]

Applied to

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

Thanks!

[1/2] ASoC: samsung: tm2_wm5110: check of of_parse return value
      commit: d58970da324732686529655c21791cef0ee547c4

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