Message ID | 20240228062138.1275542-1-peng.fan@oss.nxp.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-84558-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp3162257dyb; Tue, 27 Feb 2024 22:13:52 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCVxg2S1wSBCtlPHrmIYs7M7MaUE48MaPY9opcZwFZNk1uFJ16JsMGMRf9vKTQIsc05QSAnDK5SfbQQ1j5Ijj5eYONDelQ== X-Google-Smtp-Source: AGHT+IG52LueMExhmzvcPCjd9/CwGeV3QbI2EpyZ9CA6tSh0ujVLF3uetyRIT3NeNEVhnct+Pu9C X-Received: by 2002:a17:906:69b:b0:a42:f3a6:9f7f with SMTP id u27-20020a170906069b00b00a42f3a69f7fmr6929880ejb.13.1709100832095; Tue, 27 Feb 2024 22:13:52 -0800 (PST) Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id o7-20020a17090611c700b00a430230b844si1468153eja.751.2024.02.27.22.13.51 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Feb 2024 22:13:52 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-84558-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@NXP1.onmicrosoft.com header.s=selector2-NXP1-onmicrosoft-com header.b="l6kVPfT/"; arc=fail (signature failed); spf=pass (google.com: domain of linux-kernel+bounces-84558-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-84558-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nxp.com 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 8CFAE1F238BC for <ouuuleilei@gmail.com>; Wed, 28 Feb 2024 06:13:51 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 4349E224DD; Wed, 28 Feb 2024 06:13:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=NXP1.onmicrosoft.com header.i=@NXP1.onmicrosoft.com header.b="l6kVPfT/" Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-vi1eur04on2047.outbound.protection.outlook.com [40.107.8.47]) (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 3A0432231F; Wed, 28 Feb 2024 06:13:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.8.47 ARC-Seal: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709100813; cv=fail; b=p/vbTdj9E8zEtQ/bFTIC3PWpDFlgoY1fB2Q1eLHEkjCcUIiP2f3gdYubJe+zE/krjKQhVYz+Zwkze2CmMKZoap1e9cV2kNJgyz3DBM6UzmiJRyZWuq7KuXyFGrQpxvg+aFYuriI7IHJI9dhpb5QtKZQHHxFPrEuUX0t+b37FgLE= ARC-Message-Signature: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709100813; c=relaxed/simple; bh=G0SeBhj9J/uGRnoGOA8shOErvODLDgSZvinFJ9PvNyY=; h=From:To:Cc:Subject:Date:Message-Id:Content-Type:MIME-Version; b=WcBwzharaV7sA0EG2VhAWWChWreyOl0ppZbof9Wjg4f1A0vTmINyDGDu8VPa7zJ0uhxIqks60xkuMzyjjaJnoOZaxybE9xQPqgoBnPkks6+pvKIMijgXofArbtYw6/ry2a0ZfsCDDut9+Jkc3n/jqInpRmCK0P7DX/Bent1Gols= ARC-Authentication-Results: i=2; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=oss.nxp.com; spf=pass smtp.mailfrom=oss.nxp.com; dkim=pass (1024-bit key) header.d=NXP1.onmicrosoft.com header.i=@NXP1.onmicrosoft.com header.b=l6kVPfT/; arc=fail smtp.client-ip=40.107.8.47 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=oss.nxp.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=oss.nxp.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RHiyTOTiozr7O7Ivao8SFcomL6LAjWMV5hHGpQCYjfPGXHdtEJnnXlHHG4ZpLqnj5JUXZgrxF5LyZ9XCUaEqQ1FtTao1MFOJvbETwdj0NlSDqz4io1K42/bY6VEoE2HMCs2TAvX/6nO5siWHHHejEy3QNaVFSqb+sMeagFNDe3T5oG3Yi5FIQZsWgovUGIj9N1Dh9Qz4L4qc6go484RTGD4M7JJpaIWkECvspjQW7sYEKqiitXGdHurfnQD1Wr7vI2fIcB9t9MbJdT2DKxWCRV2LFK4bgCpEPQxUYIwAUTelTNv8QP1hFbyxQjNgqkcST7wcPuNiPF9mnXLmDtJnzg== 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=SYlCFgTuvtNHMibcgf3vcP7TLphCQm3TcjRhEMJbZRI=; b=JzLkCq6bCWnF+y48WotmmRR/GMimp6wiqGlnEBl8p4vr/YCHJgrr/ASSdwJqQPueYeZteJQmPR7AOVVL7sriCxCYDa6fDiDO3hu+dlxPFU6J5Dr3uADT9c5pxgeINDx9WHUzT7JB3/wcps+QfAg8GYrNhZzVfBe2C43pZXPBZDyOAH51HrqsBlf7ngR55Mggs7vpsGGzY1Yw3npTVZpBjH02ThZ29/PkyLAJqkb19pDeiTSWGKAe2QdFcNUhBmIvVjdGOvvlDfuaxKw1A0ZtqN3tWoCrHYgt/Qdw0TEdABpPBoFKu7DBXOIC0YmbWldNN0Uu69Yr5N7rZvDSYsRTjg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oss.nxp.com; dmarc=pass action=none header.from=oss.nxp.com; dkim=pass header.d=oss.nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=NXP1.onmicrosoft.com; s=selector2-NXP1-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=SYlCFgTuvtNHMibcgf3vcP7TLphCQm3TcjRhEMJbZRI=; b=l6kVPfT/a0IhnNhiegjU/K0Z/7p7DgxtYFw+mHZY2Se2tDTnPc+4ZQCIptviCtt16SLrYKvsoS8wgNYABr/OU+OZcuArcFpy5vLZZQi64jnm27ok2/9A61mkOEFFFq3xahQZ7hBZPklT236TdpDKMzksPDdInyToy1Yol5uxTbI= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=oss.nxp.com; Received: from DU0PR04MB9417.eurprd04.prod.outlook.com (2603:10a6:10:358::11) by AM7PR04MB6935.eurprd04.prod.outlook.com (2603:10a6:20b:10e::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7316.36; Wed, 28 Feb 2024 06:13:27 +0000 Received: from DU0PR04MB9417.eurprd04.prod.outlook.com ([fe80::1232:ed97:118f:72fd]) by DU0PR04MB9417.eurprd04.prod.outlook.com ([fe80::1232:ed97:118f:72fd%4]) with mapi id 15.20.7316.034; Wed, 28 Feb 2024 06:13:27 +0000 From: "Peng Fan (OSS)" <peng.fan@oss.nxp.com> To: robh@kernel.org, saravanak@google.com, bhelgaas@google.com, pali@kernel.org Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Peng Fan <peng.fan@nxp.com> Subject: [PATCH] of: dynamic: notify before revert Date: Wed, 28 Feb 2024 14:21:38 +0800 Message-Id: <20240228062138.1275542-1-peng.fan@oss.nxp.com> X-Mailer: git-send-email 2.37.1 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: SI2PR01CA0048.apcprd01.prod.exchangelabs.com (2603:1096:4:193::17) To DU0PR04MB9417.eurprd04.prod.outlook.com (2603:10a6:10:358::11) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DU0PR04MB9417:EE_|AM7PR04MB6935:EE_ X-MS-Office365-Filtering-Correlation-Id: 1403f506-f7f0-4cdf-2481-08dc382460db X-MS-Exchange-SharedMailbox-RoutingAgent-Processed: True X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: WlAN5ffbR002cXXBxY1/5PwiYdePf2BZMjDOhYxyKLscgW375dxZ6SpXlZqjS1bagep2IuRHElpyAKDw88/ZvVmqlh1dGD/ZLQFGnxPM7ECPU9PAhi5q+p06cxmnrokYVBGOfKQJhqrx1EgwojfV5pYdy8D87M4dS9KWzLQOJKeFBvrbMb1v/t1CZtdCtbuwYGhJypY/ZN3KQHy/MIDrnkZp7vE7Nbt8HOyibO8rJ/vJZv3VOrjxwoOzGKqjpMtVmnKcFz2Tl7byA1Fpe3AV9MvCP1l2W4bTEUDJoseX0UyatuJUBso4o9ViX3UnkzeEtfQVc5FZZlfiRCM4NYQkzHK0Sn9HXlq6HSzGGdGDTkZCpJI4D9oDiqwtyedbVInlQrXXsZsyQZc7oLyb83eR4G75B1BkcJjp31lCsn37X8EiSVD+qGBtepn/4b419uD0mhMSgPtI60oEkhmMUOFmNu5Fm/0Wv+5abzailmQbzGTbTTUTJJikgrAkY/AIsv13HVWH4DHtTzjvY/+lylyinA7tU5pRXpPEhMkLCcvyfY1MBU0kFzJHRnHT2LvUqy0rIzBNu0gnU+uSnx528IZ0PucWsBo3cHOPgNyCwgqBc+t26M1SJkjkEXCo8TS8CL/+ekMXJX1igAhCOu+0c/W0ePthoSHJR5t5VqIYgJkDCfD4IUP9/9FCYEITeOgVGEWemb5RGM5wn2dAUdlZ05CK1Q== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DU0PR04MB9417.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(38350700005);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: 60FApJqnun5aABmXhNBCSobqBP5Wy9l/LZJAYq+/f/Vpx0UToy004gsR8ko1nGgF8WK8MzolagWPU+/Cjt8DptjN4JoPCp90Mc8wEjHIBsozycFJWfTBiGheLLebqOwbx78Fc2vlLpkoEpe5eYEG+B987Eqa2gUbIzzZ8Xd6y1aMQXEUMmhTSdUL6/adW6++PKPLu7ZX5pUKULt9gqhB6BR7zTVs3H/D7K4Zk44jEnk4M3DpXfi+COiIAC4npt6hRMOT0bIb/XfZCTCGWUGqCW8KAzAMCnNZtCeOb9JcNEok5ADMRr1Nflpgr4jY4RCLjJfzT82Bbnznjml29UiJeoJ9x5Rbiu3B3hDn2RkqbaGAKdXy3LHLS4hgft+PNkaC3bl3Ncy7HkTibv0XcYoiwmqnZ6D47Cr5OP4QPjHMi0eQQmWhA0jYClNifXjFerAcFEemZqBUkC4t5Dl5UpwQurQ58wGnSuaH6Ft6lknrJI/psLIbS028pYt56z0TjiNT6vqtqM+0ZW8IDm6JUf5x8y9mamqcPZCtrm6OvvA/59v6/cnfzI1WCsUXThewTlY7td8Uly38sIh47W/x4nak7dmdeKm8sHFUopa7R7Oaf2R9DhtjzNEa95TKRnvCJu2pWwn+PfXB02X9ESxkKuKb8WQyjW3HsRdNbNP9ACHmRD/oOM9SsSO6FfYnPzndGGb/zr5gtmVBZazwvjqrv6ELl9GrctfgtPbyIzkys/vcfBOEj1NU+/zK9aapB9Tqye7rW013VRxTdzMMQqL2qboZEV3utcGUuiwlqve6GkhPtiRrYMVcyVvtBUzpXyW64sK5xH9Bhqeb3a9ViaYJPsMg69XYBvtY5pGfeHvLV5jo9ZUGcvI9OVdAjRYo4dN4S1NLf2u7hfyNMmnmMKN2/oQFECC1XbD0qdoGhXiOZBhGrAPaGcuv7eHTHIXS4mNAHh1ZRrpaQ+f29QDDfEAZbnxVKCsrcvV981k5yl/2LgWINEgGaH8xQZMI14yejAa7hf89lrxGaIARbvphr2uUrHFO78uXQZF9ypDOFtrlRAI3pbWden0yJb4cJRkzVtWXAwbeqQbritz4yYpAePwkkGZSSIcpTHK8kc2Eh0AMih6C2vYgTdv3MsdDkf66M0WlmM75M6IqzaW2Auj00HEeX4p5dyBzfZ1tNRFFlRygRxa5cv7Ip2j6P7ClY2+dEjgodT01t3bPEVUbERNMAT/mg7ZMDgPNQ2Ua/NLGXmssNymaJiqWQX0zI6XcXfTYBwnq1HCnstwgCKlz4FRiH9NcQo4uQgQbt2Yysy4kmugrKSB12NIDGhpHcw6NiQUcAXFTQ760o4EZAMIhX8Gy+Iug7V4RN2g2vEBJLBS5kv/ExVUylI8OECfqxquDphfWIj5qGYhNJFcqczKDHbuunH4H0fGwMPyo1VVR4tpLLXQIwMetvqocZ6jI4CQiEBjdflgjvAqs89kTNOwS07BmBRpw566tj2U+KOE2+iZ/Un7SkO6ad2F6ccNc80I2wbGf+4LZ3K9Nj2f5Y4xvKjVv8ciWRM+JTkiYKMJuVNAa4oRj4fx0dUREiDSTUzfjTXO3PWMIZTRc X-OriginatorOrg: oss.nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1403f506-f7f0-4cdf-2481-08dc382460db X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Feb 2024 06:13:27.6109 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: pYiNydhIosOquIxOWChzoIO0aKILoHtlDfZSL0ouaZm5P/WRt8rM2sOkLYpLUBmKSrRf6WVikeR6R8xfOHdHdQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6935 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792122113890002478 X-GMAIL-MSGID: 1792122113890002478 |
Series |
of: dynamic: notify before revert
|
|
Commit Message
Peng Fan (OSS)
Feb. 28, 2024, 6:21 a.m. UTC
From: Peng Fan <peng.fan@nxp.com> When PCI node was created using an overlay and the overlay is reverted/destroyed, the "linux,pci-domain" property no longer exists, so of_get_pci_domain_nr will return failure. Then of_pci_bus_release_domain_nr will actually use the dynamic IDA, even if the IDA was allocated in static IDA. So move the notify before revert to fix the issue. Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") Signed-off-by: Peng Fan <peng.fan@nxp.com> --- The initial thread: https://lore.kernel.org/all/20230913115737.GA426735@bhelgaas/ drivers/of/dynamic.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Comments
On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > When PCI node was created using an overlay and the overlay is > reverted/destroyed, the "linux,pci-domain" property no longer > exists, so of_get_pci_domain_nr will return failure. Then > of_pci_bus_release_domain_nr will actually use the dynamic IDA, > even if the IDA was allocated in static IDA. That usage is broken to begin with unless there is a guarantee that static and dynamic domain numbers don't overlap. For example, a dynamic number is assigned and then you load an overlay with the same number defined in it. > So move the notify before revert to fix the issue. You can't just change the timing. Something might require notify to be after the revert. > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") I don't see where the notifier is even used. Rob
> Subject: Re: [PATCH] of: dynamic: notify before revert > > On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> > wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > When PCI node was created using an overlay and the overlay is > > reverted/destroyed, the "linux,pci-domain" property no longer exists, > > so of_get_pci_domain_nr will return failure. Then > > of_pci_bus_release_domain_nr will actually use the dynamic IDA, even > > if the IDA was allocated in static IDA. > > That usage is broken to begin with unless there is a guarantee that static and > dynamic domain numbers don't overlap. For example, a dynamic number is > assigned and then you load an overlay with the same number defined in it. I may not describe it clear, the code is here, because overlay property removed, of_get_pci_domain_nr will return failure, so the code path will goest into free a dynamic ida. But actually there is no such dynamic ida, so dump. So the problem is overlay was removed, but the notify callback may still use the property to do some work. static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *parent) { if (bus->domain_nr < 0) return; /* Release domain from IDA where it was allocated. */ if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr) ida_free(&pci_domain_nr_static_ida, bus->domain_nr); else ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr); } > > > So move the notify before revert to fix the issue. > > You can't just change the timing. Something might require notify to be after > the revert. > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") > > I don't see where the notifier is even used. The stack is as below: [ 595.150529] CPU: 1 PID: 582 Comm: jailhouse Tainted: G O 6.5.0-rc4-next-20230804-05021-g3d4cc14b42ef-dirty #355 [ 595.161998] Hardware name: NXP i.MX93 11X11 EVK board (DT) [ 595.167475] Call trace: [ 595.169908] dump_backtrace+0x94/0xec [ 595.173573] show_stack+0x18/0x24 [ 595.176884] dump_stack_lvl+0x48/0x60 [ 595.180541] dump_stack+0x18/0x24 [ 595.183843] pci_bus_release_domain_nr+0x34/0x84 [ 595.188453] pci_remove_root_bus+0xa0/0xa4 [ 595.192544] pci_host_common_remove+0x28/0x40 [ 595.196895] platform_remove+0x54/0x6c [ 595.200641] device_remove+0x4c/0x80 [ 595.204209] device_release_driver_internal+0x1d4/0x230 [ 595.209430] device_release_driver+0x18/0x24 [ 595.213691] bus_remove_device+0xcc/0x10c [ 595.217686] device_del+0x15c/0x41c [ 595.221170] platform_device_del.part.0+0x1c/0x88 [ 595.225861] platform_device_unregister+0x24/0x40 [ 595.230557] of_platform_device_destroy+0xfc/0x10c [ 595.235344] of_platform_notify+0x13c/0x178 [ 595.239518] blocking_notifier_call_chain+0x6c/0xa0 [ 595.244389] __of_changeset_entry_notify+0x148/0x16c [ 595.249346] of_changeset_revert+0xa8/0xcc [ 595.253437] jailhouse_pci_virtual_root_devices_remove+0x50/0x74 [jailhouse] [ 595.260484] jailhouse_cmd_disable+0x70/0x1ac [jailhouse] [ 595.265883] jailhouse_ioctl+0x60/0xf0 [jailhouse] [ 595.270674] __arm64_sys_ioctl+0xac/0xf0 [ 595.274595] invoke_syscall+0x48/0x114 [ 595.278336] el0_svc_common.constprop.0+0xc4/0xe4 Regards, Peng. > > Rob
On Thu, Feb 29, 2024 at 2:01 AM Peng Fan <peng.fan@nxp.com> wrote: > > > Subject: Re: [PATCH] of: dynamic: notify before revert > > > > On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> > > wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > When PCI node was created using an overlay and the overlay is > > > reverted/destroyed, the "linux,pci-domain" property no longer exists, > > > so of_get_pci_domain_nr will return failure. Then > > > of_pci_bus_release_domain_nr will actually use the dynamic IDA, even > > > if the IDA was allocated in static IDA. > > > > That usage is broken to begin with unless there is a guarantee that static and > > dynamic domain numbers don't overlap. For example, a dynamic number is > > assigned and then you load an overlay with the same number defined in it. > > I may not describe it clear, the code is here, because overlay property > removed, of_get_pci_domain_nr will return failure, so the code path > will goest into free a dynamic ida. But actually there is no such dynamic > ida, so dump. I understood the problem. Your usage of this is broken on applying your overlay. You just got lucky. > So the problem is overlay was removed, but the notify callback may > still use the property to do some work. > > static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *parent) > { > if (bus->domain_nr < 0) > return; > > /* Release domain from IDA where it was allocated. */ > if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr) > ida_free(&pci_domain_nr_static_ida, bus->domain_nr); > else > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr); > } > > > > > So move the notify before revert to fix the issue. > > > > You can't just change the timing. Something might require notify to be after > > the revert. Again ^^^ > > > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") > > > > I don't see where the notifier is even used. > > The stack is as below: > > [ 595.150529] CPU: 1 PID: 582 Comm: jailhouse Tainted: G O 6.5.0-rc4-next-20230804-05021-g3d4cc14b42ef-dirty #355 > [ 595.161998] Hardware name: NXP i.MX93 11X11 EVK board (DT) > [ 595.167475] Call trace: > [ 595.169908] dump_backtrace+0x94/0xec > [ 595.173573] show_stack+0x18/0x24 > [ 595.176884] dump_stack_lvl+0x48/0x60 > [ 595.180541] dump_stack+0x18/0x24 > [ 595.183843] pci_bus_release_domain_nr+0x34/0x84 > [ 595.188453] pci_remove_root_bus+0xa0/0xa4 > [ 595.192544] pci_host_common_remove+0x28/0x40 > [ 595.196895] platform_remove+0x54/0x6c > [ 595.200641] device_remove+0x4c/0x80 > [ 595.204209] device_release_driver_internal+0x1d4/0x230 > [ 595.209430] device_release_driver+0x18/0x24 > [ 595.213691] bus_remove_device+0xcc/0x10c > [ 595.217686] device_del+0x15c/0x41c > [ 595.221170] platform_device_del.part.0+0x1c/0x88 > [ 595.225861] platform_device_unregister+0x24/0x40 > [ 595.230557] of_platform_device_destroy+0xfc/0x10c > [ 595.235344] of_platform_notify+0x13c/0x178 > [ 595.239518] blocking_notifier_call_chain+0x6c/0xa0 > [ 595.244389] __of_changeset_entry_notify+0x148/0x16c > [ 595.249346] of_changeset_revert+0xa8/0xcc > [ 595.253437] jailhouse_pci_virtual_root_devices_remove+0x50/0x74 [jailhouse] $ git grep jailhouse_pci_virtual_root_devices_remove (END) Another out of tree overlay user. I have little interest in worrying about them. No one wants to step up and solve the problems with overlays, so I'm not going to worry about them either. Rob
> Subject: Re: [PATCH] of: dynamic: notify before revert > > On Thu, Feb 29, 2024 at 2:01 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > > Subject: Re: [PATCH] of: dynamic: notify before revert > > > > > > On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) > > > <peng.fan@oss.nxp.com> > > > wrote: > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > When PCI node was created using an overlay and the overlay is > > > > reverted/destroyed, the "linux,pci-domain" property no longer > > > > exists, so of_get_pci_domain_nr will return failure. Then > > > > of_pci_bus_release_domain_nr will actually use the dynamic IDA, > > > > even if the IDA was allocated in static IDA. > > > > > > That usage is broken to begin with unless there is a guarantee that > > > static and dynamic domain numbers don't overlap. For example, a > > > dynamic number is assigned and then you load an overlay with the same > number defined in it. > > > > I may not describe it clear, the code is here, because overlay > > property removed, of_get_pci_domain_nr will return failure, so the > > code path will goest into free a dynamic ida. But actually there is no > > such dynamic ida, so dump. > > I understood the problem. > > Your usage of this is broken on applying your overlay. You just got lucky. Would you please point me out where is broken on using overlay? https://github.com/siemens/jailhouse/blob/master/driver/pci.c#L458 > > > So the problem is overlay was removed, but the notify callback may > > still use the property to do some work. > > > > static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct > > device *parent) { > > if (bus->domain_nr < 0) > > return; > > > > /* Release domain from IDA where it was allocated. */ > > if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr) > > ida_free(&pci_domain_nr_static_ida, bus->domain_nr); > > else > > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr); > > } > > > > > > > So move the notify before revert to fix the issue. > > > > > > You can't just change the timing. Something might require notify to > > > be after the revert. > > Again ^^^ > > > > > > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") > > > > > > I don't see where the notifier is even used. > > > > The stack is as below: > > > > [ 595.150529] CPU: 1 PID: 582 Comm: jailhouse Tainted: G O > 6.5.0-rc4-next-20230804-05021-g3d4cc14b42ef-dirty #355 > > [ 595.161998] Hardware name: NXP i.MX93 11X11 EVK board (DT) [ > > 595.167475] Call trace: > > [ 595.169908] dump_backtrace+0x94/0xec [ 595.173573] > > show_stack+0x18/0x24 [ 595.176884] dump_stack_lvl+0x48/0x60 [ > > 595.180541] dump_stack+0x18/0x24 [ 595.183843] > > pci_bus_release_domain_nr+0x34/0x84 > > [ 595.188453] pci_remove_root_bus+0xa0/0xa4 [ 595.192544] > > pci_host_common_remove+0x28/0x40 [ 595.196895] > > platform_remove+0x54/0x6c [ 595.200641] device_remove+0x4c/0x80 [ > > 595.204209] device_release_driver_internal+0x1d4/0x230 > > [ 595.209430] device_release_driver+0x18/0x24 [ 595.213691] > > bus_remove_device+0xcc/0x10c [ 595.217686] device_del+0x15c/0x41c > > [ 595.221170] platform_device_del.part.0+0x1c/0x88 > > [ 595.225861] platform_device_unregister+0x24/0x40 > > [ 595.230557] of_platform_device_destroy+0xfc/0x10c > > [ 595.235344] of_platform_notify+0x13c/0x178 [ 595.239518] > > blocking_notifier_call_chain+0x6c/0xa0 > > [ 595.244389] __of_changeset_entry_notify+0x148/0x16c > > [ 595.249346] of_changeset_revert+0xa8/0xcc [ 595.253437] > > jailhouse_pci_virtual_root_devices_remove+0x50/0x74 [jailhouse] > > $ git grep jailhouse_pci_virtual_root_devices_remove > (END) > > Another out of tree overlay user. I have little interest in worrying about them. > No one wants to step up and solve the problems with overlays, so I'm not > going to worry about them either. Ok, but I think this is indeed an issue, if driver accessing property after property removed with overlay revert. Thanks, Peng. > > Rob
On Thu, Feb 29, 2024 at 9:04 PM Peng Fan <peng.fan@nxp.com> wrote: > > > Subject: Re: [PATCH] of: dynamic: notify before revert > > > > On Thu, Feb 29, 2024 at 2:01 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > > > > Subject: Re: [PATCH] of: dynamic: notify before revert > > > > > > > > On Wed, Feb 28, 2024 at 12:13 AM Peng Fan (OSS) > > > > <peng.fan@oss.nxp.com> > > > > wrote: > > > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > When PCI node was created using an overlay and the overlay is > > > > > reverted/destroyed, the "linux,pci-domain" property no longer > > > > > exists, so of_get_pci_domain_nr will return failure. Then > > > > > of_pci_bus_release_domain_nr will actually use the dynamic IDA, > > > > > even if the IDA was allocated in static IDA. > > > > > > > > That usage is broken to begin with unless there is a guarantee that > > > > static and dynamic domain numbers don't overlap. For example, a > > > > dynamic number is assigned and then you load an overlay with the same > > number defined in it. > > > > > > I may not describe it clear, the code is here, because overlay > > > property removed, of_get_pci_domain_nr will return failure, so the > > > code path will goest into free a dynamic ida. But actually there is no > > > such dynamic ida, so dump. > > > > I understood the problem. > > > > Your usage of this is broken on applying your overlay. You just got lucky. > > Would you please point me out where is broken on using overlay? > > https://github.com/siemens/jailhouse/blob/master/driver/pci.c#L458 I'm not saying your exact use is broken. Obviously, it worked for you. In general, it is fragile. What happens with the following combination?: base.dts: { pci@0 { linux,pci-domain = <0>; }; pci@1 { /* dynamic domain allocs domain 1 */ }; }; overlay.dts: { pci@0 { linux,pci-domain = <1>; }; }; The only way this can work is if the overlay linux,pci-domain is ignored if there's a conflict or you reserve first N ids for static and dynamic ones start at N where N is "should be more than anyone needs". > > > So the problem is overlay was removed, but the notify callback may > > > still use the property to do some work. > > > > > > static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct > > > device *parent) { > > > if (bus->domain_nr < 0) > > > return; > > > > > > /* Release domain from IDA where it was allocated. */ > > > if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr) > > > ida_free(&pci_domain_nr_static_ida, bus->domain_nr); > > > else > > > ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr); > > > } > > > > > > > > > So move the notify before revert to fix the issue. > > > > > > > > You can't just change the timing. Something might require notify to > > > > be after the revert. > > > > Again ^^^ > > > > > > > > > > > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()") > > > > > > > > I don't see where the notifier is even used. > > > > > > The stack is as below: > > > > > > [ 595.150529] CPU: 1 PID: 582 Comm: jailhouse Tainted: G O > > 6.5.0-rc4-next-20230804-05021-g3d4cc14b42ef-dirty #355 > > > [ 595.161998] Hardware name: NXP i.MX93 11X11 EVK board (DT) [ > > > 595.167475] Call trace: > > > [ 595.169908] dump_backtrace+0x94/0xec [ 595.173573] > > > show_stack+0x18/0x24 [ 595.176884] dump_stack_lvl+0x48/0x60 [ > > > 595.180541] dump_stack+0x18/0x24 [ 595.183843] > > > pci_bus_release_domain_nr+0x34/0x84 > > > [ 595.188453] pci_remove_root_bus+0xa0/0xa4 [ 595.192544] > > > pci_host_common_remove+0x28/0x40 [ 595.196895] > > > platform_remove+0x54/0x6c [ 595.200641] device_remove+0x4c/0x80 [ > > > 595.204209] device_release_driver_internal+0x1d4/0x230 > > > [ 595.209430] device_release_driver+0x18/0x24 [ 595.213691] > > > bus_remove_device+0xcc/0x10c [ 595.217686] device_del+0x15c/0x41c > > > [ 595.221170] platform_device_del.part.0+0x1c/0x88 > > > [ 595.225861] platform_device_unregister+0x24/0x40 > > > [ 595.230557] of_platform_device_destroy+0xfc/0x10c > > > [ 595.235344] of_platform_notify+0x13c/0x178 [ 595.239518] > > > blocking_notifier_call_chain+0x6c/0xa0 > > > [ 595.244389] __of_changeset_entry_notify+0x148/0x16c > > > [ 595.249346] of_changeset_revert+0xa8/0xcc [ 595.253437] > > > jailhouse_pci_virtual_root_devices_remove+0x50/0x74 [jailhouse] > > > > $ git grep jailhouse_pci_virtual_root_devices_remove > > (END) > > > > Another out of tree overlay user. I have little interest in worrying about them. > > No one wants to step up and solve the problems with overlays, so I'm not > > going to worry about them either. > > Ok, but I think this is indeed an issue, if driver accessing property after > property removed with overlay revert. A pre-remove notifier might be useful, but as I said, you can't just remove the post-remove notifier without some justification why no one needs it. Notifiers aren't the best construct in the kernel, so adding to them isn't really desired either. Also, I think there are 2 other issues with the design here: Generally, the code should read the DT once upfront and not need the DT later on. So needing to access the DT on removal is kind of suspect. That's like needing to read USB device descriptors after a USB device is unplugged. It also seems like the driver should hold a reference to the DT node(s) it needs to prevent removal until it no longer needs them. Not sure if the node refcount would be enough or we need something more. Rob
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c index 3bf27052832f..0a5ec2bda380 100644 --- a/drivers/of/dynamic.c +++ b/drivers/of/dynamic.c @@ -834,13 +834,15 @@ int __of_changeset_revert_notify(struct of_changeset *ocs) static int __of_changeset_revert(struct of_changeset *ocs) { - int ret, ret_reply; + int ret, ret_reply = 0; - ret_reply = 0; - ret = __of_changeset_revert_entries(ocs, &ret_reply); + ret = __of_changeset_revert_notify(ocs); + if (ret) + return ret; - if (!ret) - ret = __of_changeset_revert_notify(ocs); + ret = __of_changeset_revert_entries(ocs, &ret_reply); + if (ret && !ret_reply) + ret = __of_changeset_apply_notify(ocs); return ret; }