[PATCH 0/4] ASoC trivial fixes

The commit [a28d167fbbef: ASoC: mc13783: Add missing of_node_put]
fixed the leak of OF node, but it calls of_node_put() unconditionally
at error path where the passed pointer might be uninitialized. It was
caught by a compiler warning:
sound/soc/codecs/mc13783.c:787:13: warning: 'np' may be used uninitialized in this function [-Wuninitialized]

This patch adds the NULL initialization properly for fixing this.

Fixes: a28d167fbbef ('ASoC: mc13783: Add missing of_node_put')
Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/soc/codecs/mc13783.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index 388f90a597fa..f54fdf6fc20d 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -749,7 +749,7 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
{
struct mc13783_priv *priv;
struct mc13xxx_codec_platform_data *pdata = pdev->dev.platform_data;
- struct device_node *np;
+ struct device_node *np = NULL;
int ret;

priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

--
2.1.2

Mark Brown

2014-10-07 17:09:38 UTC

Post by Takashi Iwai
- struct device_node *np;
+ struct device_node *np = NULL;

No, unconditional initialisations like this are worse than the problem
they're trying to fix.

Takashi Iwai

2014-10-07 17:17:08 UTC

At Tue, 7 Oct 2014 18:09:38 +0100,

Post by Takashi Iwai
- struct device_node *np;
+ struct device_node *np = NULL;

No, unconditional initialisations like this are worse than the problem
they're trying to fix.

Which problem they're trying to fix...?

Initializing with NULL for the of_node_put() at error path is a
standard idiom. An alternative fix would be to add "if (!pdata)"
before of_node_put(np). But this isn't really intuitive, either (and
even more error-prone, IMO).

Takashi

Mark Brown

2014-10-07 17:23:37 UTC

Post by Takashi Iwai
- struct device_node *np;
+ struct device_node *np = NULL;

No, unconditional initialisations like this are worse than the problem
they're trying to fix.

Which problem they're trying to fix...?

Shutting up warnings - because they just brute forcing they mean that if
there's anything else we've missed the compiler won't be able to tell us
about it.

Post by Takashi Iwai
Initializing with NULL for the of_node_put() at error path is a
standard idiom. An alternative fix would be to add "if (!pdata)"
before of_node_put(np). But this isn't really intuitive, either (and
even more error-prone, IMO).

Well, in this case I'd just move the of_node_put() into the existing
check for pdata since we don't ever reference np outside of that anyway.

Takashi Iwai

2014-10-07 17:39:03 UTC

At Tue, 7 Oct 2014 18:23:37 +0100,

Post by Takashi Iwai
- struct device_node *np;
+ struct device_node *np = NULL;

No, unconditional initialisations like this are worse than the problem
they're trying to fix.

Which problem they're trying to fix...?

Shutting up warnings - because they just brute forcing they mean that if
there's anything else we've missed the compiler won't be able to tell us
about it.

Well, in this case I'd just move the of_node_put() into the existing
check for pdata since we don't ever reference np outside of that anyway.

Yeah, that's an option, too, but it'd make the code less readable.
So I chose the straightforward way.

Takashi

Mark Brown

2014-10-07 18:04:34 UTC

Post by Mark Brown
Well, in this case I'd just move the of_node_put() into the existing
check for pdata since we don't ever reference np outside of that anyway.

Yeah, that's an option, too, but it'd make the code less readable.
So I chose the straightforward way.

I don't actually see it as a readability concern - to me having the get
and release close to each other and especially having them at the same
level of indentation helps.

Takashi Iwai

2014-10-07 18:14:00 UTC

At Tue, 7 Oct 2014 19:04:34 +0100,

Post by Mark Brown
Well, in this case I'd just move the of_node_put() into the existing
check for pdata since we don't ever reference np outside of that anyway.

Yeah, that's an option, too, but it'd make the code less readable.
So I chose the straightforward way.

I don't actually see it as a readability concern - to me having the get
and release close to each other and especially having them at the same
level of indentation helps.

I do understand the merit, but it looks uglier to my taste.
The success path goes again with if (ret). (Or we'd need two goto's
or deeper if-else blocks.)

Takashi

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index 388f90a597fa..cffbf09ba67c 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -749,7 +749,6 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
{
struct mc13783_priv *priv;
struct mc13xxx_codec_platform_data *pdata = pdev->dev.platform_data;
- struct device_node *np;
int ret;

priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -760,6 +759,8 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
priv->adc_ssi_port = pdata->adc_ssi_port;
priv->dac_ssi_port = pdata->dac_ssi_port;
} else {
+ struct device_node *np;
+
np = of_get_child_by_name(pdev->dev.parent->of_node, "codec");
if (!np)
return -ENOSYS;
@@ -771,6 +772,10 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
ret = of_property_read_u32(np, "dac-port", &priv->dac_ssi_port);
if (ret)
goto out;
+ out:
+ of_node_put(np);
+ if (ret)
+ return ret;
}

dev_set_drvdata(&pdev->dev, priv);
@@ -783,8 +788,6 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
ret = snd_soc_register_codec(&pdev->dev, &soc_codec_dev_mc13783,
mc13783_dai_async, ARRAY_SIZE(mc13783_dai_async));

-out:
- of_node_put(np);
return ret;
}

Mark Brown

2014-10-07 18:54:43 UTC

Post by Mark Brown
I don't actually see it as a readability concern - to me having the get
and release close to each other and especially having them at the same
level of indentation helps.

I do understand the merit, but it looks uglier to my taste.
The success path goes again with if (ret). (Or we'd need two goto's
or deeper if-else blocks.)

That looks ugly, yes - I'd be doing a check for ret before the second
property call or something. Or just put the pdata check in the existing
error path.

Takashi Iwai

2014-10-07 18:58:38 UTC

At Tue, 7 Oct 2014 19:54:43 +0100,

Post by Mark Brown
I don't actually see it as a readability concern - to me having the get
and release close to each other and especially having them at the same
level of indentation helps.

I do understand the merit, but it looks uglier to my taste.
The success path goes again with if (ret). (Or we'd need two goto's
or deeper if-else blocks.)

That looks ugly, yes - I'd be doing a check for ret before the second
property call or something. Or just put the pdata check in the existing
error path.

Well, I don't mind much how it'll be fixed, so I rather toss this fix
to you. Feel free to cook :)

Takashi

Mark Brown

2014-10-07 23:01:50 UTC

Post by Mark Brown
That looks ugly, yes - I'd be doing a check for ret before the second
property call or something. Or just put the pdata check in the existing
error path.

Well, I don't mind much how it'll be fixed, so I rather toss this fix
to you. Feel free to cook :)

Well, I can't even see the warning so as far as I can tell it's all
fixed!

Takashi Iwai

2014-10-08 05:28:52 UTC

At Wed, 8 Oct 2014 00:01:50 +0100,

Post by Mark Brown
That looks ugly, yes - I'd be doing a check for ret before the second
property call or something. Or just put the pdata check in the existing
error path.

Well, I don't mind much how it'll be fixed, so I rather toss this fix
to you. Feel free to cook :)

Well, I can't even see the warning so as far as I can tell it's all
fixed!

Oh, rather trust your eyes, the fault is there obviously.
It's a real bug that can be easily triggered, not in an exceptional
error path.

BTW, putting pdata check in the exit path is more error-prone, as
already mentioned. If anyone puts a new code with "goto out" before
assignment of np, it hits again. So, a NULL initialization would be
safer in the end.

Takashi

Takashi Iwai

2014-10-07 16:19:53 UTC

The of_node_put() calls in imx_es8328_probe() may take uninitialized
pointers when reached though the early error path. This patch adds
the proper NULL initialization for fixing these.

Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/soc/fsl/imx-es8328.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/imx-es8328.c b/sound/soc/fsl/imx-es8328.c
index 653e66d150c8..d6c55c88a069 100644
--- a/sound/soc/fsl/imx-es8328.c
+++ b/sound/soc/fsl/imx-es8328.c
@@ -78,7 +78,7 @@ static const struct snd_soc_dapm_widget imx_es8328_dapm_widgets[] = {
static int imx_es8328_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
- struct device_node *ssi_np, *codec_np;
+ struct device_node *ssi_np = NULL, *codec_np = NULL;
struct platform_device *ssi_pdev;
struct imx_es8328_data *data;
u32 int_port, ext_port;

--
2.1.2

Mark Brown

2014-10-07 22:52:36 UTC

Post by Takashi Iwai
The of_node_put() calls in imx_es8328_probe() may take uninitialized
pointers when reached though the early error path. This patch adds
the proper NULL initialization for fixing these.

Applied, thanks.

Takashi Iwai

2014-10-07 16:19:54 UTC

An error code was forgotten to be passed in the error path of
imx_es8328_probe().

Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/soc/fsl/imx-es8328.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/sound/soc/fsl/imx-es8328.c b/sound/soc/fsl/imx-es8328.c
index d6c55c88a069..f8cf10e16ce9 100644
--- a/sound/soc/fsl/imx-es8328.c
+++ b/sound/soc/fsl/imx-es8328.c
@@ -104,6 +104,7 @@ static int imx_es8328_probe(struct platform_device *pdev)
if (ext_port > MUX_PORT_MAX || ext_port == 0) {
dev_err(dev, "mux-ext-port: hardware only has %d mux ports\n",
MUX_PORT_MAX);
+ ret = -EINVAL;
goto fail;
}

--
2.1.2

Mark Brown

2014-10-07 17:10:41 UTC

Post by Takashi Iwai
An error code was forgotten to be passed in the error path of
imx_es8328_probe().

Applied, thanks.

Takashi Iwai

2014-10-07 16:19:52 UTC

The of_node_put() call in eukrea_tlv320_probe() may take an
uninitialized pointer, as compiler spotted out:
sound/soc/codecs/mc13783.c:787:13: warning: 'np' may be used uninitialized in this function [-Wuninitialized]

This patch adds the proper NULL initialization as a fix.

Fixes: 66f232908de2 ('ASoC: eukrea-tlv320: Add DT support')
Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/soc/fsl/eukrea-tlv320.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/eukrea-tlv320.c b/sound/soc/fsl/eukrea-tlv320.c
index eb093d5b85c4..d53b002595b6 100644
--- a/sound/soc/fsl/eukrea-tlv320.c
+++ b/sound/soc/fsl/eukrea-tlv320.c
@@ -105,7 +105,7 @@ static int eukrea_tlv320_probe(struct platform_device *pdev)
int ret;
int int_port = 0, ext_port;
struct device_node *np = pdev->dev.of_node;
- struct device_node *ssi_np, *codec_np;
+ struct device_node *ssi_np = NULL, *codec_np;

eukrea_tlv320.dev = &pdev->dev;
if (np) {

--
2.1.2

Mark Brown

2014-10-07 17:18:00 UTC

Post by Takashi Iwai
- struct device_node *ssi_np, *codec_np;
+ struct device_node *ssi_np = NULL, *codec_np;

As well as the NULL thing mixing initialized and unintialized things in
one declaration is something that's normally avoided.

Takashi Iwai

2014-10-07 17:39:57 UTC

At Tue, 7 Oct 2014 18:18:00 +0100,

Post by Takashi Iwai
- struct device_node *ssi_np, *codec_np;
+ struct device_node *ssi_np = NULL, *codec_np;

As well as the NULL thing mixing initialized and unintialized things in
one declaration is something that's normally avoided.

I thought of that, too. For simplicity, we can do NULL
initializations for both, of course, if you prefer.

Takashi

Takashi Iwai

2014-10-07 18:56:29 UTC

The of_node_put() call in eukrea_tlv320_probe() may take an
uninitialized pointer, as compiler spotted out:
sound/soc/fsl/eukrea-tlv320.c:221:14: warning: 'ssi_np' may be used uninitialized in this function [-Wuninitialized]

This patch adds the proper NULL initializations as a fix.
(codec_np is also NULL initialized just for consistency.)

Fixes: 66f232908de2 ('ASoC: eukrea-tlv320: Add DT support')
Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/soc/fsl/eukrea-tlv320.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/eukrea-tlv320.c b/sound/soc/fsl/eukrea-tlv320.c
index eb093d5b85c4..d53b002595b6 100644
--- a/sound/soc/fsl/eukrea-tlv320.c
+++ b/sound/soc/fsl/eukrea-tlv320.c
@@ -105,7 +105,7 @@ static int eukrea_tlv320_probe(struct platform_device *pdev)
int ret;
int int_port = 0, ext_port;
struct device_node *np = pdev->dev.of_node;
- struct device_node *ssi_np, *codec_np;
+ struct device_node *ssi_np = NULL, *codec_np = NULL;

eukrea_tlv320.dev = &pdev->dev;
if (np) {

--
2.1.2

Mark Brown

2014-10-07 19:10:44 UTC

Post by Takashi Iwai
The of_node_put() call in eukrea_tlv320_probe() may take an
sound/soc/fsl/eukrea-tlv320.c:221:14: warning: 'ssi_np' may be used uninitialized in this function [-Wuninitialized]

Applied, but please don't send new patches in the middle of e-mail
threads - it can get really confusing trying to work out what the
current version of the series looks like.

Mark Brown

2014-10-07 17:11:34 UTC

Post by Takashi Iwai
this is a series of fixes for the bugs I pointed yesterday.
Since no fix action was seen yet, I quickly put band-aid over them.

So, I now checked who you'd Cced - you didn't CC the Freescale guys so
I'm not surprised you got no reply...

Takashi Iwai

2014-10-07 17:17:36 UTC

At Tue, 7 Oct 2014 18:11:34 +0100,