Merge patch series "Fix device removal order for Apple dart iommu"

Janne Grunau <j@jannau.net> says:

Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
trigger a NULL pointer dereference in the Apple dart iommu driver. The
iommu device is removed before its user. The sparsely used DM_FLAG_VITAL
flag is intended to describe this dependency. Add it to the driver.

Adding this flag is unfortunately not enough since the boot routines
except the arm one simply remove all drivers. Add and use a new function
which calls
    dm_remove_devioce_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
    dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
to ensure this order dependency is head consistently.

Link: https://lore.kernel.org/r/20241123-iommu_apple_dart_ordering-v2-0-cc2ade6dde97@jannau.net

8 files changed, 74 insertions, 8 deletions

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 192c120a7d2..974cbfe8400 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -73,11 +73,10 @@ static void announce_and_cleanup(int fake)
 	 * Call remove function of all devices with a removal flag set.
 	 * This may be useful for last-stage operations, like cancelling
 	 * of DMA operation or releasing device internal buffers.
+	 * dm_remove_devices_active() ensures that vital devices are removed in
+	 * a second round.
 	 */
-	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
-
-	/* Remove all active vital devices next */
-	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+	dm_remove_devices_active();
 
 	cleanup_before_linux();
 }
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
index 82502972eec..76c610bcee0 100644
--- a/arch/riscv/lib/bootm.c
+++ b/arch/riscv/lib/bootm.c
@@ -57,7 +57,7 @@ static void announce_and_cleanup(int fake)
 	 * This may be useful for last-stage operations, like cancelling
 	 * of DMA operation or releasing device internal buffers.
 	 */
-	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+	dm_remove_devices_active();
 
 	cleanup_before_linux();
 }
diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index 55f581836df..0f79a5d5495 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -49,7 +49,7 @@ void bootm_announce_and_cleanup(void)
 	 * This may be useful for last-stage operations, like cancelling
 	 * of DMA operation or releasing device internal buffers.
 	 */
-	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+	dm_remove_devices_active();
 }
 
 #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 7a714f5478a..c7fb58285ca 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -147,6 +147,13 @@ int dm_remove_devices_flags(uint flags)
 
 	return 0;
 }
+
+void dm_remove_devices_active(void)
+{
+	/* Remove non-vital devices first */
+	device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
+	device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL);
+}
 #endif
 
 int dm_scan_plat(bool pre_reloc_only)
diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
index 3e9e7819e51..bfd4ad20105 100644
--- a/drivers/iommu/apple_dart.c
+++ b/drivers/iommu/apple_dart.c
@@ -322,5 +322,5 @@ U_BOOT_DRIVER(apple_dart) = {
 	.ops = &apple_dart_ops,
 	.probe = apple_dart_probe,
 	.remove = apple_dart_remove,
-	.flags	= DM_FLAG_OS_PREPARE
+	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_VITAL
 };
diff --git a/include/dm/root.h b/include/dm/root.h
index b2f30a842f5..5651b868c8b 100644
--- a/include/dm/root.h
+++ b/include/dm/root.h
@@ -167,8 +167,18 @@ int dm_uninit(void);
  * Return: 0 if OK, -ve on error
  */
 int dm_remove_devices_flags(uint flags);
+
+/**
+ * dm_remove_devices_active - Call remove function of all active drivers heeding
+ *                            device dependencies as far as know, i.e. removing
+ *                            devices marked with DM_FLAG_VITAL last.
+ *
+ * All active devices will be removed
+ */
+void dm_remove_devices_active(void);
 #else
 static inline int dm_remove_devices_flags(uint flags) { return 0; }
+static inline void dm_remove_devices_active(void) { }
 #endif
 
 /**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 4f52284b4c6..080e7f78ae3 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2234,7 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 		if (IS_ENABLED(CONFIG_USB_DEVICE))
 			udc_disconnect();
 		board_quiesce_devices();
-		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+		dm_remove_devices_active();
 	}
 
 	/* Patch out unsupported runtime function */
diff --git a/test/dm/core.c b/test/dm/core.c
index 7371d3ff426..c59ffc6f611 100644
--- a/test/dm/core.c
+++ b/test/dm/core.c
@@ -999,6 +999,56 @@ static int dm_test_remove_vital(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_remove_vital, 0);
 
+/* Test removal of 'active' devices */
+static int dm_test_remove_active(struct unit_test_state *uts)
+{
+	struct udevice *normal, *dma, *vital, *dma_vital;
+
+	/* Skip the behaviour in test_post_probe() */
+	uts->skip_post_probe = 1;
+
+	ut_assertok(device_bind_by_name(uts->root, false, &driver_info_manual,
+					&normal));
+	ut_assertnonnull(normal);
+
+	ut_assertok(device_bind_by_name(uts->root, false, &driver_info_act_dma,
+					&dma));
+	ut_assertnonnull(dma);
+
+	ut_assertok(device_bind_by_name(uts->root, false,
+					&driver_info_vital_clk, &vital));
+	ut_assertnonnull(vital);
+
+	ut_assertok(device_bind_by_name(uts->root, false,
+					&driver_info_act_dma_vital_clk,
+					&dma_vital));
+	ut_assertnonnull(dma_vital);
+
+	/* Probe the devices */
+	ut_assertok(device_probe(normal));
+	ut_assertok(device_probe(dma));
+	ut_assertok(device_probe(vital));
+	ut_assertok(device_probe(dma_vital));
+
+	/* Check that devices are active right now */
+	ut_asserteq(true, device_active(normal));
+	ut_asserteq(true, device_active(dma));
+	ut_asserteq(true, device_active(vital));
+	ut_asserteq(true, device_active(dma_vital));
+
+	/* Remove active devices in an ordered way */
+	dm_remove_devices_active();
+
+	/* Check that all devices are inactive right now */
+	ut_asserteq(true, device_active(normal));
+	ut_asserteq(false, device_active(dma));
+	ut_asserteq(true, device_active(vital));
+	ut_asserteq(false, device_active(dma_vital));
+
+	return 0;
+}
+DM_TEST(dm_test_remove_active, 0);
+
 static int dm_test_uclass_before_ready(struct unit_test_state *uts)
 {
 	struct uclass *uc;