linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] of: populate of_root node if bootloader doesn't
@ 2024-01-30  0:44 Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree() Stephen Boyd
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:44 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Anton Ivanov, Brendan Higgins,
	Catalin Marinas, David Gow, Frank Rowand, Johannes Berg,
	Richard Weinberger, Will Deacon, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	Saurabh Sengar

Arch maintainers, please ack/review patches.

This is a resend of a series from Frank last year[1]. I worked in Rob's
review comments to unconditionally call unflatten_device_tree() and
fixup/audit calls to of_have_populated_dt() so that behavior doesn't
change.

I need this series so I can add DT based tests in the clk framework.
Either I can merge it through the clk tree once everyone is happy, or
Rob can merge it through the DT tree and provide some branch so I can
base clk patches on it.

Changes from Frank's series[1]:
 * Add a DTB loaded kunit test
 * Make of_have_populated_dt() return false if the DTB isn't from the
   bootloader
 * Architecture calls made unconditional so that a root node is always
   made

Changes from v1 (https://lore.kernel.org/r/20240112200750.4062441-1-sboyd@kernel.org):
 * x86 patch included
 * arm64 knocks out initial dtb if acpi is in use
 * keep Kconfig hidden but def_bool enabled otherwise

Frank Rowand (2):
  of: Create of_root if no dtb provided by firmware
  of: unittest: treat missing of_root as error instead of fixing up

Stephen Boyd (5):
  arm64: Unconditionally call unflatten_device_tree()
  um: Unconditionally call unflatten_device_tree()
  x86/of: Unconditionally call unflatten_and_copy_device_tree()
  of: Always unflatten in unflatten_and_copy_device_tree()
  of: Add KUnit test to confirm DTB is loaded

 arch/arm64/kernel/setup.c    |  7 +++--
 arch/um/kernel/dtb.c         | 14 +++++-----
 arch/x86/kernel/devicetree.c | 24 +++++++++--------
 drivers/of/.kunitconfig      |  3 +++
 drivers/of/Kconfig           | 11 +++++++-
 drivers/of/Makefile          |  4 ++-
 drivers/of/empty_root.dts    |  6 +++++
 drivers/of/fdt.c             | 52 +++++++++++++++++++++++++-----------
 drivers/of/of_test.c         | 48 +++++++++++++++++++++++++++++++++
 drivers/of/platform.c        |  3 ---
 drivers/of/unittest.c        | 16 +++--------
 include/linux/of.h           | 25 ++++++++++-------
 12 files changed, 151 insertions(+), 62 deletions(-)
 create mode 100644 drivers/of/.kunitconfig
 create mode 100644 drivers/of/empty_root.dts
 create mode 100644 drivers/of/of_test.c

Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Gow <davidgow@google.com>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Weinberger <richard@nod.at>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: <x86@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Saurabh Sengar <ssengar@linux.microsoft.com>

[1] https://lore.kernel.org/r/20230317053415.2254616-1-frowand.list@gmail.com

base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree()
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-31 20:54   ` Rob Herring
  2024-01-30  0:45 ` [PATCH v2 2/7] um: " Stephen Boyd
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Catalin Marinas,
	Will Deacon, Mark Rutland

Call this function unconditionally so that we can populate an empty DTB
on platforms that don't boot with a firmware provided or builtin DTB.
Override 'initial_boot_params' to NULL when ACPI is in use but the
bootloader has loaded a DTB so that we don't allow both ACPI and DT to
be used during boot. If there isn't a valid initial_boot_params dtb then
unflatten_device_tree() returns early so this is fine.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 arch/arm64/kernel/setup.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 417a8a86b2db..ffb1942724ae 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -351,8 +351,11 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
 	/* Parse the ACPI tables for possible boot-time configuration */
 	acpi_boot_table_init();
 
-	if (acpi_disabled)
-		unflatten_device_tree();
+	/* Don't use the FDT from boot if ACPI is in use */
+	if (!acpi_disabled)
+		initial_boot_params = NULL;
+
+	unflatten_device_tree();
 
 	bootmem_init();
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/7] um: Unconditionally call unflatten_device_tree()
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree() Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree() Stephen Boyd
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Richard Weinberger,
	Anton Ivanov, Johannes Berg

Call this function unconditionally so that we can populate an empty DTB
on platforms that don't boot with a command line provided DTB.  There's
no harm in calling unflatten_device_tree() unconditionally. If there
isn't a valid initial_boot_params dtb then unflatten_device_tree()
returns early.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-um@lists.infradead.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 arch/um/kernel/dtb.c  | 14 +++++++-------
 drivers/of/unittest.c |  4 ----
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/um/kernel/dtb.c b/arch/um/kernel/dtb.c
index 484141b06938..4954188a6a09 100644
--- a/arch/um/kernel/dtb.c
+++ b/arch/um/kernel/dtb.c
@@ -16,16 +16,16 @@ void uml_dtb_init(void)
 	void *area;
 
 	area = uml_load_file(dtb, &size);
-	if (!area)
-		return;
+	if (area) {
+		if (!early_init_dt_scan(area)) {
+			pr_err("invalid DTB %s\n", dtb);
+			memblock_free(area, size);
+			return;
+		}
 
-	if (!early_init_dt_scan(area)) {
-		pr_err("invalid DTB %s\n", dtb);
-		memblock_free(area, size);
-		return;
+		early_init_fdt_scan_reserved_mem();
 	}
 
-	early_init_fdt_scan_reserved_mem();
 	unflatten_device_tree();
 }
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e9e90e96600e..a8b27dd16ecf 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -4075,10 +4075,6 @@ static int __init of_unittest(void)
 	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
 
 	/* adding data for unittest */
-
-	if (IS_ENABLED(CONFIG_UML))
-		unittest_unflatten_overlay_base();
-
 	res = unittest_data_add();
 	if (res)
 		return res;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree()
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree() Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 2/7] um: " Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-30 13:30   ` Saurabh Singh Sengar
  2024-01-30  0:45 ` [PATCH v2 4/7] of: Always unflatten in unflatten_and_copy_device_tree() Stephen Boyd
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	Saurabh Sengar

Call this function unconditionally so that we can populate an empty DTB
on platforms that don't boot with a firmware provided or builtin DTB.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: <x86@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Saurabh Sengar <ssengar@linux.microsoft.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 arch/x86/kernel/devicetree.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index afd09924094e..650752d112a6 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -283,22 +283,24 @@ void __init x86_flattree_get_config(void)
 	u32 size, map_len;
 	void *dt;
 
-	if (!initial_dtb)
-		return;
+	if (initial_dtb) {
+		map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
 
-	map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
+		dt = early_memremap(initial_dtb, map_len);
+		size = fdt_totalsize(dt);
+		if (map_len < size) {
+			early_memunmap(dt, map_len);
+			dt = early_memremap(initial_dtb, size);
+			map_len = size;
+		}
 
-	dt = early_memremap(initial_dtb, map_len);
-	size = fdt_totalsize(dt);
-	if (map_len < size) {
-		early_memunmap(dt, map_len);
-		dt = early_memremap(initial_dtb, size);
-		map_len = size;
+		early_init_dt_verify(dt);
 	}
 
-	early_init_dt_verify(dt);
 	unflatten_and_copy_device_tree();
-	early_memunmap(dt, map_len);
+
+	if (initial_dtb)
+		early_memunmap(dt, map_len);
 }
 #endif
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/7] of: Always unflatten in unflatten_and_copy_device_tree()
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
                   ` (2 preceding siblings ...)
  2024-01-30  0:45 ` [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree() Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 5/7] of: Create of_root if no dtb provided by firmware Stephen Boyd
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand

We want to populate an empty DT whenever CONFIG_OF is enabled so that
overlays can be applied and the DT unit tests can be run. Make
unflatten_and_copy_device_tree() stop printing a warning if the
'initial_boot_params' pointer is NULL. Instead, simply copy the dtb if
there is one and then unflatten it. If there isn't a DT to copy, then
the call to unflatten_device_tree() is largely a no-op, so nothing
really changes here.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/fdt.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..b488ad86d456 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1318,6 +1318,21 @@ bool __init early_init_dt_scan(void *params)
 	return true;
 }
 
+static void __init copy_device_tree(void)
+{
+	int size;
+	void *dt;
+
+	size = fdt_totalsize(initial_boot_params);
+	dt = early_init_dt_alloc_memory_arch(size,
+					     roundup_pow_of_two(FDT_V17_SIZE));
+
+	if (dt) {
+		memcpy(dt, initial_boot_params, size);
+		initial_boot_params = dt;
+	}
+}
+
 /**
  * unflatten_device_tree - create tree of device_nodes from flat blob
  *
@@ -1350,22 +1365,9 @@ void __init unflatten_device_tree(void)
  */
 void __init unflatten_and_copy_device_tree(void)
 {
-	int size;
-	void *dt;
+	if (initial_boot_params)
+		copy_device_tree();
 
-	if (!initial_boot_params) {
-		pr_warn("No valid device tree found, continuing without\n");
-		return;
-	}
-
-	size = fdt_totalsize(initial_boot_params);
-	dt = early_init_dt_alloc_memory_arch(size,
-					     roundup_pow_of_two(FDT_V17_SIZE));
-
-	if (dt) {
-		memcpy(dt, initial_boot_params, size);
-		initial_boot_params = dt;
-	}
 	unflatten_device_tree();
 }
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/7] of: Create of_root if no dtb provided by firmware
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
                   ` (3 preceding siblings ...)
  2024-01-30  0:45 ` [PATCH v2 4/7] of: Always unflatten in unflatten_and_copy_device_tree() Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 6/7] of: unittest: treat missing of_root as error instead of fixing up Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 7/7] of: Add KUnit test to confirm DTB is loaded Stephen Boyd
  6 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, linux-kernel, patches, linux-um, linux-arm-kernel,
	kunit-dev, linux-kselftest, devicetree

From: Frank Rowand <frowand.list@gmail.com>

When enabling CONFIG_OF on a platform where 'of_root' is not populated
by firmware, we end up without a root node. In order to apply overlays
and create subnodes of the root node, we need one. Create this root node
by unflattening an empty builtin dtb.

If firmware provides a flattened device tree (FDT) then the FDT is
unflattened via setup_arch(). Otherwise, the call to
unflatten(_and_copy)?_device_tree() will create an empty root node.

We make of_have_populated_dt() return true only if the DTB was loaded by
firmware so that existing callers don't change behavior after this
patch. The call in the of platform code is removed because it prevents
overlays from creating platform devices when the platform bus isn't
fully initialized.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
Link: https://lore.kernel.org/r/20230317053415.2254616-2-frowand.list@gmail.com
Cc: Rob Herring <robh+dt@kernel.org>
[sboyd@kernel.org: Update of_have_populated_dt() to treat this empty dtb
as not populated. Drop setup_of() initcall]
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/Kconfig        |  2 +-
 drivers/of/Makefile       |  2 +-
 drivers/of/empty_root.dts |  6 ++++++
 drivers/of/fdt.c          | 20 ++++++++++++++++++++
 drivers/of/platform.c     |  3 ---
 include/linux/of.h        | 25 +++++++++++++++----------
 6 files changed, 43 insertions(+), 15 deletions(-)
 create mode 100644 drivers/of/empty_root.dts

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index da9826accb1b..17733285b415 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -54,7 +54,7 @@ config OF_FLATTREE
 	select CRC32
 
 config OF_EARLY_FLATTREE
-	bool
+	def_bool OF && !SPARC
 	select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
 	select OF_FLATTREE
 
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index eff624854575..df305348d1cb 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,7 +2,7 @@
 obj-y = base.o cpu.o device.o module.o platform.o property.o
 obj-$(CONFIG_OF_KOBJ) += kobj.o
 obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
-obj-$(CONFIG_OF_FLATTREE) += fdt.o
+obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
new file mode 100644
index 000000000000..cf9e97a60f48
--- /dev/null
+++ b/drivers/of/empty_root.dts
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/dts-v1/;
+
+/ {
+
+};
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index b488ad86d456..f7dd7fd68d4d 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -32,6 +32,13 @@
 
 #include "of_private.h"
 
+/*
+ * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
+ * cmd_dt_S_dtb in scripts/Makefile.lib
+ */
+extern uint8_t __dtb_empty_root_begin[];
+extern uint8_t __dtb_empty_root_end[];
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -1343,6 +1350,19 @@ static void __init copy_device_tree(void)
  */
 void __init unflatten_device_tree(void)
 {
+	if (!initial_boot_params) {
+		initial_boot_params = (void *) __dtb_empty_root_begin;
+		/* fdt_totalsize() will be used for copy size */
+		if (fdt_totalsize(initial_boot_params) >
+		    __dtb_empty_root_end - __dtb_empty_root_begin) {
+			pr_err("invalid size in dtb_empty_root\n");
+			return;
+		}
+		of_fdt_crc32 = crc32_be(~0, initial_boot_params,
+					fdt_totalsize(initial_boot_params));
+		copy_device_tree();
+	}
+
 	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
 				early_init_dt_alloc_memory_arch, false);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 126d265aa7d8..20087bb8a46b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -549,9 +549,6 @@ static int __init of_platform_default_populate_init(void)
 
 	device_links_supplier_sync_state_pause();
 
-	if (!of_have_populated_dt())
-		return -ENODEV;
-
 	if (IS_ENABLED(CONFIG_PPC)) {
 		struct device_node *boot_display = NULL;
 		struct platform_device *dev;
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..52f6ad6a1c8c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -180,11 +180,6 @@ static inline bool is_of_node(const struct fwnode_handle *fwnode)
 			&__of_fwnode_handle_node->fwnode : NULL;	\
 	})
 
-static inline bool of_have_populated_dt(void)
-{
-	return of_root != NULL;
-}
-
 static inline bool of_node_is_root(const struct device_node *node)
 {
 	return node && (node->parent == NULL);
@@ -549,11 +544,6 @@ static inline struct device_node *of_find_node_with_property(
 
 #define of_fwnode_handle(node) NULL
 
-static inline bool of_have_populated_dt(void)
-{
-	return false;
-}
-
 static inline struct device_node *of_get_compatible_child(const struct device_node *parent,
 					const char *compatible)
 {
@@ -1634,6 +1624,21 @@ static inline bool of_device_is_system_power_controller(const struct device_node
 	return of_property_read_bool(np, "system-power-controller");
 }
 
+/**
+ * of_have_populated_dt() - Has DT been populated by bootloader
+ *
+ * Return: True if a DTB has been populated by the bootloader and it isn't the
+ * empty builtin one. False otherwise.
+ */
+static inline bool of_have_populated_dt(void)
+{
+#ifdef CONFIG_OF
+	return of_property_present(of_root, "compatible");
+#else
+	return false;
+#endif
+}
+
 /*
  * Overlay support
  */
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 6/7] of: unittest: treat missing of_root as error instead of fixing up
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
                   ` (4 preceding siblings ...)
  2024-01-30  0:45 ` [PATCH v2 5/7] of: Create of_root if no dtb provided by firmware Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  2024-01-30  0:45 ` [PATCH v2 7/7] of: Add KUnit test to confirm DTB is loaded Stephen Boyd
  6 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, linux-kernel, patches, linux-um, linux-arm-kernel,
	kunit-dev, linux-kselftest, devicetree

From: Frank Rowand <frowand.list@gmail.com>

unflatten_device_tree() now ensures that the 'of_root' node is populated
with the root of a default empty devicetree. Remove the unittest code
that created 'of_root' if it was missing. Verify that 'of_root' is valid
before attempting to attach the testcase-data subtree. Remove the
unittest code that unflattens the unittest overlay base if architecture
is UML because that is always done now.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
Link: https://lore.kernel.org/r/20230317053415.2254616-3-frowand.list@gmail.com
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/unittest.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index a8b27dd16ecf..742d919e8ab4 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1732,20 +1732,16 @@ static int __init unittest_data_add(void)
 		return -EINVAL;
 	}
 
+	/* attach the sub-tree to live tree */
 	if (!of_root) {
-		of_root = unittest_data_node;
-		for_each_of_allnodes(np)
-			__of_attach_node_sysfs(np);
-		of_aliases = of_find_node_by_path("/aliases");
-		of_chosen = of_find_node_by_path("/chosen");
-		of_overlay_mutex_unlock();
-		return 0;
+		pr_warn("%s: no live tree to attach sub-tree\n", __func__);
+		kfree(unittest_data);
+		return -ENODEV;
 	}
 
 	EXPECT_BEGIN(KERN_INFO,
 		     "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
 
-	/* attach the sub-tree to live tree */
 	np = unittest_data_node->child;
 	while (np) {
 		struct device_node *next = np->sibling;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 7/7] of: Add KUnit test to confirm DTB is loaded
  2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
                   ` (5 preceding siblings ...)
  2024-01-30  0:45 ` [PATCH v2 6/7] of: unittest: treat missing of_root as error instead of fixing up Stephen Boyd
@ 2024-01-30  0:45 ` Stephen Boyd
  6 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2024-01-30  0:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, David Gow,
	Brendan Higgins

Add a KUnit test that confirms a DTB has been loaded, i.e. there is a
root node, and that the of_have_populated_dt() API works properly.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: David Gow <davidgow@google.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/.kunitconfig |  3 +++
 drivers/of/Kconfig      |  9 ++++++++
 drivers/of/Makefile     |  2 ++
 drivers/of/of_test.c    | 48 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100644 drivers/of/.kunitconfig
 create mode 100644 drivers/of/of_test.c

diff --git a/drivers/of/.kunitconfig b/drivers/of/.kunitconfig
new file mode 100644
index 000000000000..5a8fee11978c
--- /dev/null
+++ b/drivers/of/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_OF=y
+CONFIG_OF_KUNIT_TEST=y
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 17733285b415..53d1b5dd89e8 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -37,6 +37,15 @@ config OF_UNITTEST
 
 	  If unsure, say N here. This option is not safe to enable.
 
+config OF_KUNIT_TEST
+	tristate "Devicetree KUnit Test" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This option builds KUnit unit tests for device tree infrastructure.
+
+	  If unsure, say N here, but this option is safe to enable.
+
 config OF_ALL_DTBS
 	bool "Build all Device Tree Blobs"
 	depends on COMPILE_TEST
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index df305348d1cb..251d33532148 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -19,4 +19,6 @@ obj-y	+= kexec.o
 endif
 endif
 
+obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
+
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
new file mode 100644
index 000000000000..71a767b42b43
--- /dev/null
+++ b/drivers/of/of_test.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for OF APIs
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <kunit/test.h>
+
+/*
+ * Test that the root node "/" can be found by path.
+ */
+static void dtb_root_node_found_by_path(struct kunit *test)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/");
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
+	of_node_put(np);
+}
+
+/*
+ * Test that the 'of_root' global variable is always populated when DT code is
+ * enabled.
+ */
+static void dtb_root_node_populates_of_root(struct kunit *test)
+{
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, of_root);
+}
+
+static struct kunit_case dtb_test_cases[] = {
+	KUNIT_CASE(dtb_root_node_found_by_path),
+	KUNIT_CASE(dtb_root_node_populates_of_root),
+	{}
+};
+
+/*
+ * Test suite to confirm a DTB is loaded.
+ */
+static struct kunit_suite dtb_suite = {
+	.name = "dtb",
+	.test_cases = dtb_test_cases,
+};
+
+kunit_test_suites(
+	&dtb_suite,
+);
+MODULE_LICENSE("GPL");
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree()
  2024-01-30  0:45 ` [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree() Stephen Boyd
@ 2024-01-30 13:30   ` Saurabh Singh Sengar
  0 siblings, 0 replies; 12+ messages in thread
From: Saurabh Singh Sengar @ 2024-01-30 13:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rob Herring, linux-kernel, patches, linux-um, linux-arm-kernel,
	kunit-dev, linux-kselftest, devicetree, Frank Rowand,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin

On Mon, Jan 29, 2024 at 04:45:02PM -0800, Stephen Boyd wrote:
> Call this function unconditionally so that we can populate an empty DTB
> on platforms that don't boot with a firmware provided or builtin DTB.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: <x86@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  arch/x86/kernel/devicetree.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index afd09924094e..650752d112a6 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -283,22 +283,24 @@ void __init x86_flattree_get_config(void)
>  	u32 size, map_len;
>  	void *dt;
>  
> -	if (!initial_dtb)
> -		return;
> +	if (initial_dtb) {
> +		map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
>  
> -	map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
> +		dt = early_memremap(initial_dtb, map_len);
> +		size = fdt_totalsize(dt);
> +		if (map_len < size) {
> +			early_memunmap(dt, map_len);
> +			dt = early_memremap(initial_dtb, size);
> +			map_len = size;
> +		}
>  
> -	dt = early_memremap(initial_dtb, map_len);
> -	size = fdt_totalsize(dt);
> -	if (map_len < size) {
> -		early_memunmap(dt, map_len);
> -		dt = early_memremap(initial_dtb, size);
> -		map_len = size;
> +		early_init_dt_verify(dt);
>  	}
>  
> -	early_init_dt_verify(dt);
>  	unflatten_and_copy_device_tree();
> -	early_memunmap(dt, map_len);
> +
> +	if (initial_dtb)
> +		early_memunmap(dt, map_len);
>  }
>  #endif
>  
> -- 
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


Tested the series in Hyper-V environment. Please feel free to add:
Tested-by: Saurabh Sengar <ssengar@linux.microsoft.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree()
  2024-01-30  0:45 ` [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree() Stephen Boyd
@ 2024-01-31 20:54   ` Rob Herring
  2024-01-31 22:59     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2024-01-31 20:54 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Catalin Marinas,
	Will Deacon, Mark Rutland

On Mon, Jan 29, 2024 at 04:45:00PM -0800, Stephen Boyd wrote:
> Call this function unconditionally so that we can populate an empty DTB
> on platforms that don't boot with a firmware provided or builtin DTB.
> Override 'initial_boot_params' to NULL when ACPI is in use but the
> bootloader has loaded a DTB so that we don't allow both ACPI and DT to
> be used during boot. If there isn't a valid initial_boot_params dtb then
> unflatten_device_tree() returns early so this is fine.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  arch/arm64/kernel/setup.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..ffb1942724ae 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -351,8 +351,11 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>  	/* Parse the ACPI tables for possible boot-time configuration */
>  	acpi_boot_table_init();
>  
> -	if (acpi_disabled)
> -		unflatten_device_tree();
> +	/* Don't use the FDT from boot if ACPI is in use */
> +	if (!acpi_disabled)
> +		initial_boot_params = NULL;

I still think this is a problem for kexec. See 
of_kexec_alloc_and_setup_fdt(). You see it uses initial_boot_params. At 
first glance it looks like it would just write out everything we need. 
But for UEFI boot, I think we need all the chosen properties like 
linux,uefi-mmap-start preserved from the current boot for the next 
kernel we kexec.

I think you'll have to check acpi_disabled in unflatten_device_tree() 
and unflatten the empty tree leaving initial_boot_params alone. That 
means our FDT and unflattened tree will be different DTs, but I think 
that's fine.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree()
  2024-01-31 20:54   ` Rob Herring
@ 2024-01-31 22:59     ` Stephen Boyd
  2024-02-02 16:30       ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2024-01-31 22:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Catalin Marinas,
	Will Deacon, Mark Rutland

Quoting Rob Herring (2024-01-31 12:54:05)
> On Mon, Jan 29, 2024 at 04:45:00PM -0800, Stephen Boyd wrote:
> > Call this function unconditionally so that we can populate an empty DTB
> > on platforms that don't boot with a firmware provided or builtin DTB.
> > Override 'initial_boot_params' to NULL when ACPI is in use but the
> > bootloader has loaded a DTB so that we don't allow both ACPI and DT to
> > be used during boot. If there isn't a valid initial_boot_params dtb then
> > unflatten_device_tree() returns early so this is fine.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: <linux-arm-kernel@lists.infradead.org>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >  arch/arm64/kernel/setup.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > index 417a8a86b2db..ffb1942724ae 100644
> > --- a/arch/arm64/kernel/setup.c
> > +++ b/arch/arm64/kernel/setup.c
> > @@ -351,8 +351,11 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> >       /* Parse the ACPI tables for possible boot-time configuration */
> >       acpi_boot_table_init();
> >  
> > -     if (acpi_disabled)
> > -             unflatten_device_tree();
> > +     /* Don't use the FDT from boot if ACPI is in use */
> > +     if (!acpi_disabled)
> > +             initial_boot_params = NULL;
> 
> I still think this is a problem for kexec. See 
> of_kexec_alloc_and_setup_fdt(). You see it uses initial_boot_params. At 
> first glance it looks like it would just write out everything we need. 
> But for UEFI boot, I think we need all the chosen properties like 
> linux,uefi-mmap-start preserved from the current boot for the next 
> kernel we kexec.

Ok, got it.

> 
> I think you'll have to check acpi_disabled in unflatten_device_tree() 
> and unflatten the empty tree leaving initial_boot_params alone. That 
> means our FDT and unflattened tree will be different DTs, but I think 
> that's fine.

It's sort of scary given that 'initial_boot_params' is an exported
global. Maybe that should be hidden away and accessed with a function
instead so that this mismatch doesn't break something later on?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree()
  2024-01-31 22:59     ` Stephen Boyd
@ 2024-02-02 16:30       ` Rob Herring
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2024-02-02 16:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-kernel, patches, linux-um, linux-arm-kernel, kunit-dev,
	linux-kselftest, devicetree, Frank Rowand, Catalin Marinas,
	Will Deacon, Mark Rutland

On Wed, Jan 31, 2024 at 02:59:53PM -0800, Stephen Boyd wrote:
> Quoting Rob Herring (2024-01-31 12:54:05)
> > On Mon, Jan 29, 2024 at 04:45:00PM -0800, Stephen Boyd wrote:
> > > Call this function unconditionally so that we can populate an empty DTB
> > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > Override 'initial_boot_params' to NULL when ACPI is in use but the
> > > bootloader has loaded a DTB so that we don't allow both ACPI and DT to
> > > be used during boot. If there isn't a valid initial_boot_params dtb then
> > > unflatten_device_tree() returns early so this is fine.
> > > 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: <linux-arm-kernel@lists.infradead.org>
> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > > ---
> > >  arch/arm64/kernel/setup.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > index 417a8a86b2db..ffb1942724ae 100644
> > > --- a/arch/arm64/kernel/setup.c
> > > +++ b/arch/arm64/kernel/setup.c
> > > @@ -351,8 +351,11 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> > >       /* Parse the ACPI tables for possible boot-time configuration */
> > >       acpi_boot_table_init();
> > >  
> > > -     if (acpi_disabled)
> > > -             unflatten_device_tree();
> > > +     /* Don't use the FDT from boot if ACPI is in use */
> > > +     if (!acpi_disabled)
> > > +             initial_boot_params = NULL;
> > 
> > I still think this is a problem for kexec. See 
> > of_kexec_alloc_and_setup_fdt(). You see it uses initial_boot_params. At 
> > first glance it looks like it would just write out everything we need. 
> > But for UEFI boot, I think we need all the chosen properties like 
> > linux,uefi-mmap-start preserved from the current boot for the next 
> > kernel we kexec.
> 
> Ok, got it.
> 
> > 
> > I think you'll have to check acpi_disabled in unflatten_device_tree() 
> > and unflatten the empty tree leaving initial_boot_params alone. That 
> > means our FDT and unflattened tree will be different DTs, but I think 
> > that's fine.
> 
> It's sort of scary given that 'initial_boot_params' is an exported
> global. Maybe that should be hidden away and accessed with a function
> instead so that this mismatch doesn't break something later on?

What you see now is after I did a pass of minimizing the number of 
users.

BTW, I've now got another use for unconditionally calling 
unflatten_device_tree() with this[1].

Rob

[1] https://lore.kernel.org/all/20240201194653.GA1328565-robh@kernel.org/

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-02-02 16:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-30  0:44 [PATCH v2 0/7] of: populate of_root node if bootloader doesn't Stephen Boyd
2024-01-30  0:45 ` [PATCH v2 1/7] arm64: Unconditionally call unflatten_device_tree() Stephen Boyd
2024-01-31 20:54   ` Rob Herring
2024-01-31 22:59     ` Stephen Boyd
2024-02-02 16:30       ` Rob Herring
2024-01-30  0:45 ` [PATCH v2 2/7] um: " Stephen Boyd
2024-01-30  0:45 ` [PATCH v2 3/7] x86/of: Unconditionally call unflatten_and_copy_device_tree() Stephen Boyd
2024-01-30 13:30   ` Saurabh Singh Sengar
2024-01-30  0:45 ` [PATCH v2 4/7] of: Always unflatten in unflatten_and_copy_device_tree() Stephen Boyd
2024-01-30  0:45 ` [PATCH v2 5/7] of: Create of_root if no dtb provided by firmware Stephen Boyd
2024-01-30  0:45 ` [PATCH v2 6/7] of: unittest: treat missing of_root as error instead of fixing up Stephen Boyd
2024-01-30  0:45 ` [PATCH v2 7/7] of: Add KUnit test to confirm DTB is loaded Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).