[alsa-devel] Broken child-node lookup in sound/soc/mediatek/mt8173

[Johan Hovold]

Hi,

I'm trying to fix up incorrect usage of of_find_node_by_name() to lookup
child nodes, and found the following code in
sound/soc/mediatek/mt8173/mt8173-rt5650.c:

static int mt8173_rt5650_dev_probe(struct platform_device *pdev)
{
	...

	platform_node = of_parse_phandle(pdev->dev.of_node,
					 "mediatek,platform", 0);
	...

	if (of_find_node_by_name(platform_node, "codec-capture")) {
		np = of_get_child_by_name(pdev->dev.of_node, "codec-capture");
		if (!np) {
			dev_err(&pdev->dev,
				"%s: Can't find codec-capture DT node\n",
				__func__);
			return -EINVAL;
		}
		ret = snd_soc_of_get_dai_name(np, &codec_capture_dai);
		if (ret < 0) {
			dev_err(&pdev->dev,
				"%s codec_capture_dai name fail %d\n",
				__func__, ret);
			return ret;
		}
		mt8173_rt5650_codecs[1].dai_name = codec_capture_dai;
	}

added by commit d349caeb0510 ("ASoC: mediatek: Add second I2S on
mt8173-rt5650 machine driver").

First of all the "codec-capture" node is indeed documented as a child
node of the sound node, so the tree-wide depth-first search from the
platform_node looks entirely bogus.

Note that of_find_node_by_name() also drops a reference to its first
argument, in this case the sound node, which could end up being
prematurely freed. 

And then the reference to any returned codec-capture node (from either
lookup) is never dropped.

And since support for this second codec was added retrospectively and is
documented as optional, that -EINVAL in case the node is missing looks
broken too.

I figured I better just report this one to the author of the patch and
the maintainers to be straightened out.

Thanks,
Johan