From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752596AbdKMLct (ORCPT <rfc822;w@1wt.eu>);
        Mon, 13 Nov 2017 06:32:49 -0500
Received: from mail-lf0-f67.google.com ([209.85.215.67]:46264 "EHLO
        mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752254AbdKMLcs (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 13 Nov 2017 06:32:48 -0500
X-Google-Smtp-Source: AGs4zMZog9RRdgZjsUFw2jyGwoxR7BME6B+9oAITIMDg7zr/MJ1RJ5wmDS8p6L+b3XDkmtkOdqgMXg==
Date: Mon, 13 Nov 2017 12:32:52 +0100
From: Johan Hovold <johan@kernel.org>
To: Mark Brown <broonie@kernel.org>, PC Liao <pc.liao@mediatek.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>, Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Matthias Brugger <matthias.bgg@gmail.com>, alsa-devel@alsa-project.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Broken child-node lookup in sound/soc/mediatek/mt8173
Message-ID: <20171113113252.GT11226@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.7.2 (2016-11-26)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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