dm: Add dm_remove_devices_active() for ordered device removal

This replaces dm_remove_devices_flags() calls in all boot
implementations to ensure non vital devices are consistently removed
first. All boot implementation except arch/arm/lib/bootm.c currently
just call dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL). This can result
in crashes when dependencies between devices exists. The driver model's
design document describes DM_FLAG_VITAL as "indicates that the device is
'vital' to the operation of other devices". Device removal at boot
should follow this.

Instead of adding dm_remove_devices_flags() with (DM_REMOVE_ACTIVE_ALL |
DM_REMOVE_NON_VITAL) everywhere add dm_remove_devices_active() which
does this.

Fixes a NULL pointer deref in the apple dart IOMMU driver during EFI
boot. The xhci-pci (driver which depends on the IOMMU to work) removes
its mapping on removal. This explodes when the IOMMU device was removed
first.

dm_remove_devices_flags() is kept since it is used for testing of
device_remove() calls in dm.

Signed-off-by: Janne Grunau <j@jannau.net>

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();
 }

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();
 }

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)

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)

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
 
 /**

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 */

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;