Message ID | 87fru14hl7.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from am.mirrors.kernel.org ([147.75.80.249]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-12115-patchwork=linuxtv.org@vger.kernel.org>) id 1sC6ge-0001T4-2i for patchwork@linuxtv.org; Tue, 28 May 2024 23:57:01 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 5A65F1F28972 for <patchwork@linuxtv.org>; Tue, 28 May 2024 23:56:58 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 19D9A1420DD; Tue, 28 May 2024 23:55:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=renesas.com header.i=@renesas.com header.b="dMcwoEME" X-Original-To: linux-media@vger.kernel.org Received: from JPN01-TYC-obe.outbound.protection.outlook.com (mail-tycjpn01on2084.outbound.protection.outlook.com [40.107.114.84]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 34DF71411E3; Tue, 28 May 2024 23:55:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.114.84 ARC-Seal: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716940537; cv=fail; b=qkDnKAL+YEzygYUfdBEuzAu91FSqIbwHiMw0N9XvPpXSSKthuUl3ePOA0xn2tn31NkX0D/dNElVeHOWI4HTEpjZMmK3UsbPzIWSkgluWJ146hZ8PwX/rBbNiRpyy34CwUTbyKF5j7eHJ0J/0u3VQOreICQbOFkUQ1GtqzbL/YGg= ARC-Message-Signature: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716940537; c=relaxed/simple; bh=0HScvF+M5ojSDktqvqOoLC7SJ6NbBO2xvvrb9zqVmaI=; h=Message-ID:From:Subject:To:In-Reply-To:References:Content-Type: Date:MIME-Version; b=N2lGFwi8cHV+qBdo8lSMwrgTovCQlL6cQj5RfP8B99skB4OlaVueaT0L65xNo0NUzk2KhdLzo9MKrb+KMQ0g4DOK9poc9DzIVH1G0zT7s9XoAueOBkb5p+DOd+wHLcTnBCsYvPAXUptKgZ5R/xiDiPlqEzCFHCgbiWR9MFUuzwM= ARC-Authentication-Results: i=2; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=renesas.com; spf=pass smtp.mailfrom=renesas.com; dkim=pass (1024-bit key) header.d=renesas.com header.i=@renesas.com header.b=dMcwoEME; arc=fail smtp.client-ip=40.107.114.84 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=renesas.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=renesas.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KFCzQF2k1boJvqPhNPM/6PnY0XxJGTSMmSUXtcMKKXJMIudmBB2gqAhffFozmP+b/fHClZRtV3SFI5kP7OM/p3EzXYNdZomSbg61j2oj+IKxqQ/657gnTTbpezDZUzkFagHh6EMlkX9qwT6ciNWW/Fmh04YlW8M/1kH/B5ZWaa1mkanKoZthe7p1pgHpjbUXpDj21uGU19O0pkHWPdJHMvwNarPxmHXR+bSr8S/FvUMyoc9Qj3KfmJ4EhNyReaexubtRD6fgYUUJjshvd4fpUzFXJjyNm8joK6e+9NV1dYF93gCFMIhsXUAm4SOVAAqnFHE6xq7f+9a+d9sxZuDcCQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0RBQqFYJ9l6oUjQ+wE7YbR54LyNIP2FkkrriFkT+5Fk=; b=fBirR+70GRfKURj4S0UUHRRk34gMcSRbKlThrsgzssuQbIxS1cenfJh/2eeziYTEl4T0TNj/2iw+gZMLr+vaemtU9kIK5dNjQOvzK47onoWJBg9yNF6Tmm9Lf3C9/0IMhUutrh5D3RrCeY56VBF3ueGPVKh0mKzGtbz0UzYGqgmo34ajPC3ykfwtH6rhYlmSA2OrvAohk0zK2M26rYhCz6qu6oO9SkG8kJnG8OIeGZ1XyRC4p3V3t2sZ5L22yr1Yt2guRyFqsfZpKBOiTxVER7qbGn35eVoOZ29eHOOCZsZKYTV8vFFfiPR5Uep4AdbW28k6BmphecuutWhrtZzYJw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=renesas.com; dmarc=pass action=none header.from=renesas.com; dkim=pass header.d=renesas.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=renesas.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0RBQqFYJ9l6oUjQ+wE7YbR54LyNIP2FkkrriFkT+5Fk=; b=dMcwoEMEqiba/Gdncl6dx2DycToRhPtG20FP2t/CcIZYXZGnqAsXWzP+WISJSQ37ClDnH80jzydcj9WFDQ7Kw4G6cMUaKb2+XaYFl08gpXH9dY4XGHKqhTZMDTpJc2pST5yibs+hPxL4Dhpwy/LXFCVhRYNyom8tBR/QtqpR60A= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=renesas.com; Received: from TYCPR01MB10914.jpnprd01.prod.outlook.com (2603:1096:400:3a9::11) by TYCPR01MB11224.jpnprd01.prod.outlook.com (2603:1096:400:3bf::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7633.17; Tue, 28 May 2024 23:55:32 +0000 Received: from TYCPR01MB10914.jpnprd01.prod.outlook.com ([fe80::c568:1028:2fd1:6e11]) by TYCPR01MB10914.jpnprd01.prod.outlook.com ([fe80::c568:1028:2fd1:6e11%4]) with mapi id 15.20.7611.030; Tue, 28 May 2024 23:55:32 +0000 Message-ID: <87fru14hl7.wl-kuninori.morimoto.gx@renesas.com> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Subject: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node() User-Agent: Wanderlust/2.15.9 Emacs/29.3 Mule/6.0 To: <prabhakar.csengg@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Claudiu Beznea <claudiu.beznea@tuxon.dev>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, Eugen Hristev <eugen.hristev@collabora.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Helge Deller <deller@gmx.de>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Maxime Ripard <mripard@kernel.org>, Michal Simek <michal.simek@amd.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Rob Herring <robh+dt@kernel.org>, Suzuki K Poulose <suzuki.poulose@arm.com>, Thomas Zimmermann <tzimmermann@suse.de>, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>, coresight@lists.linaro.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev In-Reply-To: <87ikyx4hm1.wl-kuninori.morimoto.gx@renesas.com> References: <87ikyx4hm1.wl-kuninori.morimoto.gx@renesas.com> Content-Type: text/plain; charset=US-ASCII Date: Tue, 28 May 2024 23:55:32 +0000 X-ClientProxiedBy: TYAPR01CA0166.jpnprd01.prod.outlook.com (2603:1096:404:7e::34) To TYCPR01MB10914.jpnprd01.prod.outlook.com (2603:1096:400:3a9::11) Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: TYCPR01MB10914:EE_|TYCPR01MB11224:EE_ X-MS-Office365-Filtering-Correlation-Id: ef75b43c-f816-43ed-530b-08dc7f71a937 X-LD-Processed: 53d82571-da19-47e4-9cb4-625a166a4a2a,ExtAddr X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230031|52116005|1800799015|7416005|376005|366007|38350700005|921011; X-Microsoft-Antispam-Message-Info: DhvhBd07dBUaBqXmcv6cLrnEIKFAuAFSLBEtWcw6UMSDLfBzI2pzfRiwPLILnn5JXE1rdl8ZOORxoGyX++loyJMQXNYLKX9Mn4v1pPwMLh1S+uGoW57i2y+UZWSc+0UyAkPuPgYn8SodIVuigM5+jVfjA+LVWd3qm4jr4z3Z1KFDD70bieLpYf88iYjgdDPGtkweQboAIo1I/yqT372lyL5H/ZkqZ5JfRnf+2jTJnAP4H+qH6MOwknZXphze9YYKrCXn9ljeLRUYdF3ei/5olpkR3fKOHYIqa03BVZQuOXPpN5mZK7zpwO9L7AKuBKWcSYIwgpb1pMVNgNmcDZUhEZSVczRw+c1mEHd2PWNVoxqoiYob9fWBhy5BTd+wObiBp18vt/64f7kiYzO520KgnCa+VNUpnQNrlSOFPWTCdWh/YpujSNBa+lfOAHtrjH7T1M/RAKedt8wOe2v1zy3Yn3TuGJh4QQJnQ4VAU0kc/RoJ2sUCwJndANr+x2WCm7QOKxG6vKLDGiJeHPF4d+gbVnVP1SeYI18g5mxiPqpoy0UvxpFMtyAJWrH6A3skUTJ8SnMkuk5SXBqatgriHWXTWs+Kgwc+2oYuq6snYzPrqkawF3mbzVZlIx7gbaVT0GuuquAgRRx5isdT/lexD+h6LEL7Rs8+N5yPkHn0YYGpIukvcmxpmg6JM3LTGSG3yZiUDhana30/qavs2v75HQBrhJZjz9hK7xrqc0bByhkau8OlxYc3j0LlY7yAf2JwGSYzqTI4iTEQQRENEqkdkoLB0AGWI18TE3Oh99lWtSJUGLCNLGTozlJr3JOaxOzkWHRiWakqNWV0rVll/j9NdDjSQ8yKsNl9+Smc+njsj5jSrpU9Gv4MgyjV7Xe7lZ1Oz1kOTp7fZ9sglnyPR5jnEfB+bLz2QdZDuaa+Try9SWksKB5xbx8ni5xfRF8QRHNaWgu0rVi/CEd78ySMMC/1qSvo1wn5QNiB9LiPWVd2OHxLdqvwldgxcAemMcTurXx3K5fvnsE2oCXdnuekcLzVFLuLtovIOswOamJjz7QtKLB8cS18TEKyk/e9QIfovKSuCz8F6DEXqgvsFM+/pf6L7jxuaeHNa05a8A1EoFDPn3JnOv9Zb/TlxewEBZhqhUuguJz1ZZWh/qadHYX2CX7dklbYtgBvEb03fHdASn4whMqU5GDg0+0EBMCy1mrkPNlkb/EDlUF9QlxX6P4Bnnuo4iwFpLITe1F9+KoJZJSy64HboCjZz+ENn2pyivsupGWGPmgN0X/Tzqz/DrwTRJKvaFBVwHcw0zXqxQAHy3SadGgh/XujImvIeIri9ZGqIv8HtFzm5oKA5Y7zCSV29AEhRZg3mXPaJ7eldHCPrV/QdTXOkUw= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:TYCPR01MB10914.jpnprd01.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(52116005)(1800799015)(7416005)(376005)(366007)(38350700005)(921011);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: /TfmRrj/YD4Pg/kFDksrUiNNAHL7dqE9gb24ZncSlI+SB9CEh40aUFuSbUcWGIsnaO7jeKH6eFejttTZykofgGA7oSq3rwHQuCNckwga/o/1WEDsgEc/Xy1xF0W7KAYz+u9aomVTqsTRXECp02AwKU16hhc1aVLkjAGgBp7VY+HU3yGxIlQr2701sbBS75ZLL40mbkup4nYle4lxu/AfXGyDbGC0pXJwhDDwRkdDIS4dpljKWZqAkFEcDX4yT1MsJav/8v/FPZ+Oe4tA4eHdtuJo31OTqgQi/UEUEh0qwog6jvzUxlKNIwDLUUTRNXCfn2vGRwdjCPKisbQHwdnYlRCXXiAE4zEBXYu5g8kTggysCIkeohfNbl/9dvSaYaILZRPtYrygJykJ/VWTRiVBmKQr9HAySAIMSu9Mekcd2bu1vzC+Gfx+Vs5PsYGrmLp5ZhFk9XBPOjJIGhcp4qwF8gXnxAImkJ0bDTkpik+yjX5BCdSFjJfcHSgSRKczXZAt7Kquf+jzkR5/5dEP+w52FD0hIMI9b61mc/zWitPd7RI0rjLczQbTog92aOySRIeJWvaCJx573wgv+F9EgLQu3iVofhfavOd3M/pFsto1jEJpFisr5d3EkLu5JRoeDVM53GGIQW34xWZpbzL+sVk526dBWR0ogiSpRKhYHb3lzJ3j6iQizGI+DKAEVX+BL/4jeLVlfJUkVtWhp6hWW9oHWV/wylR5EtP1KD/ECYguQt4LtzZBZUDUxotA4HW/9lLc8zApOv9ZPCxL9O1B58C/hcD7VlSH7djwLkdNaWyCNsOHM6ShXkDA6vFmc16Y/28xMyd5tLbY9hhM7uWK4AFW5nkRaYB9+gW8ciSYtXSZFcpIhMGw9Wt9sVqdupY3FpBfqgONdJldVMlEHWN5wm78RespZl+HySDUjw9YsbtLOJ0W8xWwA3kcqanY0SIAsVEPzSGPW8oyn4pYqfAciT6tTYUezwibNxKEB2QPQrcwqG1Webzx7mLnYf7pHmLsmM6VBNrWO/psuN903TmdFCyqd/I5cOdzq3cOgsD4BsM9oPEzgViOXat/ieOCKLwVp1z2aUs3AqoXsxCDXq0rIJiZEzKTxIqxL4Q0ZE0JAPlW3650tNQrc7ZNIcHtnuMgf6GNQV7z5XyWDrJQrcoVVQ4JG5oxCpFDuHHyx0r2+z83Exrsf3dWzz43voaQDnNonpUN+cJdSV8KibHU+zJainqHP2PngMlHPOS3+l6mFskNT4np5QAKtE10iejHskg5tqRxK/BtfVk01by/ahft2z7GPCsDfweCmfdRVoFewacbW8MNOMvGg2Q8vAy+k7+QLsJa3yRsgvH95DWxKso7p0LelMAV9GFDsHC+JOYfcPB2OdNaJtyriWWowKxq/qyxBbS9UK8U2L1x/uxUmolT3KxNJ/tx3957WC4bUsECTXWO2amKN9mJYBCo8JwB7JqotlX+uhLU4QTjFq5JchVZxLWSq41HvkMRkszLidhlNp/SQ8mguoIyI/Uwlm70SWCzav/tTauLbvVlYtTZZYiX7VZoVPoVD1MJT4GTho+41f4CKvGelmbzKjypSBFeV2rK0SnYkA+KXSuCgpUWLm2ywPaCWKn1fTyaiHNsbpVe9qoTePM= X-OriginatorOrg: renesas.com X-MS-Exchange-CrossTenant-Network-Message-Id: ef75b43c-f816-43ed-530b-08dc7f71a937 X-MS-Exchange-CrossTenant-AuthSource: TYCPR01MB10914.jpnprd01.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 23:55:32.7738 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 53d82571-da19-47e4-9cb4-625a166a4a2a X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: h/IYjOisvwzLv5n2Vaf318afUUmSLza+JffSQv2LTCyC+tqzxQmff7mbPyI4Wtnl/37zSYmPez7qDtZHfm7PF9hCWIkIhKJckQyK2m8MCoZ6HBCeV+V/UMkpPNihWN2U X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYCPR01MB11224 X-LSpam-Score: -2.6 (--) X-LSpam-Report: No, score=-2.6 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DMARC_PASS=-0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no |
Series |
use for_each_endpoint_of_node()
|
|
Commit Message
Kuninori Morimoto
May 28, 2024, 11:55 p.m. UTC
We already have for_each_endpoint_of_node(), don't use of_graph_get_next_endpoint() directly. Replace it. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- drivers/hwtracing/coresight/coresight-platform.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Hi Morimoto-san, Thank you for the patch. On Tue, May 28, 2024 at 11:55:32PM +0000, Kuninori Morimoto wrote: > We already have for_each_endpoint_of_node(), don't use > of_graph_get_next_endpoint() directly. Replace it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-platform.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c > index 9d550f5697fa8..e9683e613d520 100644 > --- a/drivers/hwtracing/coresight/coresight-platform.c > +++ b/drivers/hwtracing/coresight/coresight-platform.c > @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev, > */ > if (!parent) { > /* > - * Avoid warnings in of_graph_get_next_endpoint() > + * Avoid warnings in for_each_endpoint_of_node() > * if the device doesn't have any graph connections > */ > if (!of_graph_is_present(node)) > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, > } > > /* Iterate through each output port to discover topology */ > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > + for_each_endpoint_of_node(parent, ep) { > /* > * Legacy binding mixes input/output ports under the > * same parent. So, skip the input ports if we are dealing I think there's a bug below. The loop contains ret = of_coresight_parse_endpoint(dev, ep, pdata); if (ret) return ret; which leaks the reference to ep. This is not introduced by this patch, so Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
On 29/05/2024 01:40, Laurent Pinchart wrote: > Hi Morimoto-san, > > Thank you for the patch. > > On Tue, May 28, 2024 at 11:55:32PM +0000, Kuninori Morimoto wrote: >> We already have for_each_endpoint_of_node(), don't use >> of_graph_get_next_endpoint() directly. Replace it. >> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight-platform.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c >> index 9d550f5697fa8..e9683e613d520 100644 >> --- a/drivers/hwtracing/coresight/coresight-platform.c >> +++ b/drivers/hwtracing/coresight/coresight-platform.c >> @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev, >> */ >> if (!parent) { >> /* >> - * Avoid warnings in of_graph_get_next_endpoint() >> + * Avoid warnings in for_each_endpoint_of_node() >> * if the device doesn't have any graph connections >> */ >> if (!of_graph_is_present(node)) >> @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, >> } >> >> /* Iterate through each output port to discover topology */ >> - while ((ep = of_graph_get_next_endpoint(parent, ep))) { >> + for_each_endpoint_of_node(parent, ep) { >> /* >> * Legacy binding mixes input/output ports under the >> * same parent. So, skip the input ports if we are dealing > > I think there's a bug below. The loop contains > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > if (ret) > return ret; > > which leaks the reference to ep. This is not introduced by this patch, > so > Nice catch, I will send a patch. Also: Reviewed-by: James Clark <james.clark@arm.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >
On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, > > } > > > > /* Iterate through each output port to discover topology */ > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > + for_each_endpoint_of_node(parent, ep) { > > /* > > * Legacy binding mixes input/output ports under the > > * same parent. So, skip the input ports if we are dealing > > I think there's a bug below. The loop contains > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > if (ret) > return ret; > > which leaks the reference to ep. This is not introduced by this patch, Someone should create for_each_endpoint_of_node_scoped(). #define for_each_endpoint_of_node_scoped(parent, child) \ for (struct device_node *child __free(device_node) = \ of_graph_get_next_endpoint(parent, NULL); child != NULL; \ child = of_graph_get_next_endpoint(parent, child)) regards, dan carpenter
On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote: > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, > > > } > > > > > > /* Iterate through each output port to discover topology */ > > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > > + for_each_endpoint_of_node(parent, ep) { > > > /* > > > * Legacy binding mixes input/output ports under the > > > * same parent. So, skip the input ports if we are dealing > > > > I think there's a bug below. The loop contains > > > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > > if (ret) > > return ret; > > > > which leaks the reference to ep. This is not introduced by this patch, > > Someone should create for_each_endpoint_of_node_scoped(). > > #define for_each_endpoint_of_node_scoped(parent, child) \ > for (struct device_node *child __free(device_node) = \ > of_graph_get_next_endpoint(parent, NULL); child != NULL; \ > child = of_graph_get_next_endpoint(parent, child)) I was thinking about that too :-) I wondered if we should then bother taking and releasing references, given that references to the children can't be leaked out of the loop. My reasoning was that the parent device_node is guaranteed to be valid throughout the loop, so borrowing references to children instead of creating new ones within the loop should be fine. This assumes that endpoints and ports can't vanish while the parent is there. Thinking further about it, it may not be a safe assumption for the future. As we anyway use functions internally that create new references, we can as well handle them correctly. Using this new macro, the loop body would need to call of_node_get() if it wants to get a reference out of the loop. That's the right thing to do, and I think it would be less error-prone than having to drop references when exiting from the loop as we do today. It would still be nice if we could have an API that allows catching this missing of_node_get() automatically, but I don't see a simple way to do so at the moment.
On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote: > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote: > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, > > > > } > > > > > > > > /* Iterate through each output port to discover topology */ > > > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > > > + for_each_endpoint_of_node(parent, ep) { > > > > /* > > > > * Legacy binding mixes input/output ports under the > > > > * same parent. So, skip the input ports if we are dealing > > > > > > I think there's a bug below. The loop contains > > > > > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > > > if (ret) > > > return ret; > > > > > > which leaks the reference to ep. This is not introduced by this patch, > > > > Someone should create for_each_endpoint_of_node_scoped(). > > > > #define for_each_endpoint_of_node_scoped(parent, child) \ > > for (struct device_node *child __free(device_node) = \ > > of_graph_get_next_endpoint(parent, NULL); child != NULL; \ > > child = of_graph_get_next_endpoint(parent, child)) > > I was thinking about that too :-) I wondered if we should then bother > taking and releasing references, given that references to the children > can't be leaked out of the loop. My reasoning was that the parent > device_node is guaranteed to be valid throughout the loop, so borrowing > references to children instead of creating new ones within the loop > should be fine. This assumes that endpoints and ports can't vanish while > the parent is there. Thinking further about it, it may not be a safe > assumption for the future. As we anyway use functions internally that > create new references, we can as well handle them correctly. > > Using this new macro, the loop body would need to call of_node_get() if > it wants to get a reference out of the loop. The child pointer is declared local to just the loop so you'd need create a different function scoped variable. If it's not local to the loop then we'd end up taking a reference on each iteration and never releasing anything except on error paths. > That's the right thing to > do, and I think it would be less error-prone than having to drop > references when exiting from the loop as we do today. It would still be > nice if we could have an API that allows catching this missing > of_node_get() automatically, but I don't see a simple way to do so at > the moment. That's an interesting point. If we did "function_scope_var = ep;" here then we'd need to take a second reference as you say. With other cleanup stuff like kfree() it's very hard to miss it if we forget to call "no_free_ptr(&ep)" because it's on the success path. It leads to an immediate crash in testing. But here it's just ref counting so possibly we might miss that sort of bug. regards, dan carpenter
On Wed, May 29, 2024 at 06:19:33PM +0300, Dan Carpenter wrote: > On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote: > > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote: > > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote: > > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, > > > > > } > > > > > > > > > > /* Iterate through each output port to discover topology */ > > > > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) { > > > > > + for_each_endpoint_of_node(parent, ep) { > > > > > /* > > > > > * Legacy binding mixes input/output ports under the > > > > > * same parent. So, skip the input ports if we are dealing > > > > > > > > I think there's a bug below. The loop contains > > > > > > > > ret = of_coresight_parse_endpoint(dev, ep, pdata); > > > > if (ret) > > > > return ret; > > > > > > > > which leaks the reference to ep. This is not introduced by this patch, > > > > > > Someone should create for_each_endpoint_of_node_scoped(). > > > > > > #define for_each_endpoint_of_node_scoped(parent, child) \ > > > for (struct device_node *child __free(device_node) = \ > > > of_graph_get_next_endpoint(parent, NULL); child != NULL; \ > > > child = of_graph_get_next_endpoint(parent, child)) > > > > I was thinking about that too :-) I wondered if we should then bother > > taking and releasing references, given that references to the children > > can't be leaked out of the loop. My reasoning was that the parent > > device_node is guaranteed to be valid throughout the loop, so borrowing > > references to children instead of creating new ones within the loop > > should be fine. This assumes that endpoints and ports can't vanish while > > the parent is there. Thinking further about it, it may not be a safe > > assumption for the future. As we anyway use functions internally that > > create new references, we can as well handle them correctly. > > > > Using this new macro, the loop body would need to call of_node_get() if > > it wants to get a reference out of the loop. > > The child pointer is declared local to just the loop so you'd need > create a different function scoped variable. If it's not local to the > loop then we'd end up taking a reference on each iteration and never > releasing anything except on error paths. > > > That's the right thing to > > do, and I think it would be less error-prone than having to drop > > references when exiting from the loop as we do today. It would still be > > nice if we could have an API that allows catching this missing > > of_node_get() automatically, but I don't see a simple way to do so at > > the moment. > > That's an interesting point. > > If we did "function_scope_var = ep;" here then we'd need to take a > second reference as you say. Yes, that's what I meant above, sorry if that wasn't clear. > With other cleanup stuff like kfree() it's > very hard to miss it if we forget to call "no_free_ptr(&ep)" because > it's on the success path. It leads to an immediate crash in testing. > But here it's just ref counting so possibly we might miss that sort of > bug. All this calls for std::shared_ptr<struct device_node> :-D Jokes aside, I think for_each_endpoint_of_node_scoped() would still be safer, as the number of cases where we would have to pass a reference to the outer scope should be quite smaller than the number of cases where we break from for_each_endpoint_of_node() loops today. On the other hand, the consequence of a bug with for_each_endpoint_of_node_scoped() would be a dangling reference, instead of a reference leak with for_each_endpoint_of_node(), so it may be more dangerous the same way that UAF is more dangerous than memory leaks.
Hi Dan > Someone should create for_each_endpoint_of_node_scoped(). > > #define for_each_endpoint_of_node_scoped(parent, child) \ > for (struct device_node *child __free(device_node) = \ > of_graph_get_next_endpoint(parent, NULL); child != NULL; \ > child = of_graph_get_next_endpoint(parent, child)) Thank you for pointing it. I have noticed that _scoped() loop exist, but this patch-set want to focus to use existing for_each_xxx() loop first. Replacing to _scoped() macro is next step. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 9d550f5697fa8..e9683e613d520 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev, */ if (!parent) { /* - * Avoid warnings in of_graph_get_next_endpoint() + * Avoid warnings in for_each_endpoint_of_node() * if the device doesn't have any graph connections */ if (!of_graph_is_present(node)) @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev, } /* Iterate through each output port to discover topology */ - while ((ep = of_graph_get_next_endpoint(parent, ep))) { + for_each_endpoint_of_node(parent, ep) { /* * Legacy binding mixes input/output ports under the * same parent. So, skip the input ports if we are dealing