[alsa-devel] [PATCH 0/4] ASoC trivial fixes
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
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);
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.
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
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.
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
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.
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; }
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.
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
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!
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
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) {
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.
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
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) {
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.
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;
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.
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; }
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...
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
participants (2)
-
Mark Brown -
Takashi Iwai