| Message ID | s5hd2a3lpcn.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; }