| Message ID | 1412698794-25536-2-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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);
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(-)