[PATCH 0/4] ASoC trivial fixes

[PATCH 0/4] ASoC trivial fixes

[Takashi Iwai]

Hi,

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.


thanks,

Takashi

[PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

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 <tiwai@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

[PATCH 2/4] ASoC: eukrea-tlv320: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

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 <tiwai@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

[PATCH 3/4] ASoC: imx-es8328: Fix of_node_put() call with uninitialized object

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

Signed-off-by: Takashi Iwai <tiwai@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

[PATCH 4/4] ASoC: imx-es8328: Fix missing return code in imx_es8328_probe()

[Takashi Iwai]

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

Signed-off-by: Takashi Iwai <tiwai@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

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 221 bytes --]

On Tue, Oct 07, 2014 at 06:19:51PM +0200, Takashi Iwai wrote:

> -	struct device_node *np;
> +	struct device_node *np = NULL;

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 4/4] ASoC: imx-es8328: Fix missing return code in imx_es8328_probe()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 166 bytes --]

On Tue, Oct 07, 2014 at 06:19:54PM +0200, Takashi Iwai wrote:
> An error code was forgotten to be passed in the error path of
> imx_es8328_probe().

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/4] ASoC trivial fixes

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 305 bytes --]

On Tue, Oct 07, 2014 at 06:19:50PM +0200, Takashi Iwai wrote:

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Tue, 7 Oct 2014 18:09:38 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 06:19:51PM +0200, Takashi Iwai wrote:
> 
> > -	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

Re: [PATCH 0/4] ASoC trivial fixes

[Takashi Iwai]

At Tue, 7 Oct 2014 18:11:34 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 06:19:50PM +0200, Takashi Iwai wrote:
> 
> > 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...

Ah, right, I missed them for these parts.


Takashi

Re: [PATCH 2/4] ASoC: eukrea-tlv320: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 283 bytes --]

On Tue, Oct 07, 2014 at 06:19:52PM +0200, Takashi Iwai wrote:

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 905 bytes --]

On Tue, Oct 07, 2014 at 07:17:08PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > On Tue, Oct 07, 2014 at 06:19:51PM +0200, Takashi Iwai wrote:

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

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Tue, 7 Oct 2014 18:23:37 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 07:17:08PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > On Tue, Oct 07, 2014 at 06:19:51PM +0200, Takashi Iwai wrote:
> 
> > > > -	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.
> 
> > 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.

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


Takashi

Re: [PATCH 2/4] ASoC: eukrea-tlv320: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Tue, 7 Oct 2014 18:18:00 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 06:19:52PM +0200, Takashi Iwai wrote:
> 
> > -	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

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 513 bytes --]

On Tue, Oct 07, 2014 at 07:39:03PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Tue, 7 Oct 2014 19:04:34 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 07:39:03PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > 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;
 }
 

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

On Tue, Oct 07, 2014 at 08:14:00PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v2 2/4] ASoC: eukrea-tlv320: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

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 <tiwai@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

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Tue, 7 Oct 2014 19:54:43 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 08:14:00PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > 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

Re: [PATCH v2 2/4] ASoC: eukrea-tlv320: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 471 bytes --]

On Tue, Oct 07, 2014 at 08:56:29PM +0200, Takashi Iwai wrote:
> 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]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] ASoC: imx-es8328: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 272 bytes --]

On Tue, Oct 07, 2014 at 06:19:53PM +0200, Takashi Iwai wrote:
> 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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 431 bytes --]

On Tue, Oct 07, 2014 at 08:58:38PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > 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!

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: mc13783: Fix of_node_put() call with uninitialized object

[Takashi Iwai]

At Wed, 8 Oct 2014 00:01:50 +0100,
Mark Brown wrote:
> 
> On Tue, Oct 07, 2014 at 08:58:38PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > 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