[PATCH v2 0/4] dm: net: Assorted patches related to networking and DMA

[PATCH v2 0/4] dm: net: Assorted patches related to networking and DMA

[Simon Glass]

This series collects together a few patches related to U-Boot leaving
DMA running when booting.

Changes in v2:
- Use the designware_eth_remove() method directly
- Add gmac_rockchip too
- Reword the commit message to mention the uclass' actions
- Add a new patch to mark drivers as having active DMA

Simon Glass (4):
  designware: Use the remove() method with related drivers
  net: designware: Mark drivers as having active DMA
  efi_loader: Move device-removal later in exit-boot-services
  dm: core: Rename dm_remove_devices_active()

 arch/arm/lib/bootm.c          |  4 ++--
 arch/riscv/lib/bootm.c        |  2 +-
 arch/x86/lib/bootm.c          |  2 +-
 drivers/core/root.c           |  2 +-
 drivers/net/designware.c      |  4 ++--
 drivers/net/designware.h      | 12 ++++++++++++
 drivers/net/dwmac_meson8b.c   |  3 ++-
 drivers/net/dwmac_s700.c      |  3 ++-
 drivers/net/dwmac_socfpga.c   |  3 ++-
 drivers/net/gmac_rockchip.c   |  3 ++-
 include/dm/root.h             |  6 +++---
 lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
 test/dm/core.c                |  2 +-
 13 files changed, 44 insertions(+), 23 deletions(-)

-- 
2.43.0

base-commit: d47b5693e21f78d8cd8caa8fcd9e64fd6ae85759
branch: eth2

[PATCH v2 1/4] designware: Use the remove() method with related drivers

[Simon Glass]

Several drivers make use of the designware Ethernet driver but do not
implement the remove() method. Add this to keep things tidy.

Note that the Ethernet uclass calls each driver's stop() method on
removal, so there is no need to do that here.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
---

Changes in v2:
- Use the designware_eth_remove() method directly
- Add gmac_rockchip too
- Reword the commit message to mention the uclass' actions

 drivers/net/designware.c    |  2 +-
 drivers/net/designware.h    | 12 ++++++++++++
 drivers/net/dwmac_meson8b.c |  1 +
 drivers/net/dwmac_s700.c    |  1 +
 drivers/net/dwmac_socfpga.c |  1 +
 drivers/net/gmac_rockchip.c |  1 +
 6 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 07b0f49ef58..5385849ea6b 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -805,7 +805,7 @@ clk_err:
 	return err;
 }
 
-static int designware_eth_remove(struct udevice *dev)
+int designware_eth_remove(struct udevice *dev)
 {
 	struct dw_eth_dev *priv = dev_get_priv(dev);
 
diff --git a/drivers/net/designware.h b/drivers/net/designware.h
index e47101ccaf6..8c9d0190e03 100644
--- a/drivers/net/designware.h
+++ b/drivers/net/designware.h
@@ -247,6 +247,18 @@ struct dw_eth_dev {
 
 int designware_eth_of_to_plat(struct udevice *dev);
 int designware_eth_probe(struct udevice *dev);
+
+/**
+ * designware_eth_remove() - Remove the device
+ *
+ * Disables DMA and marks the device as remove. This must be called before
+ * booting an OS, to ensure that DMA is inactive.
+ *
+ * @dev: Device to remove
+ * Return 0 if OK, -ve on error
+ */
+int designware_eth_remove(struct udevice *dev);
+
 extern const struct eth_ops designware_eth_ops;
 
 struct dw_eth_pdata {
diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
index fde4aabbace..bf94e50842d 100644
--- a/drivers/net/dwmac_meson8b.c
+++ b/drivers/net/dwmac_meson8b.c
@@ -158,6 +158,7 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
 	.of_match	= dwmac_meson8b_ids,
 	.of_to_plat = dwmac_meson8b_of_to_plat,
 	.probe		= dwmac_meson8b_probe,
+	.remove		= designware_eth_remove,
 	.ops		= &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct dwmac_meson8b_plat),
diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c
index 969d247b4f3..e78193f3376 100644
--- a/drivers/net/dwmac_s700.c
+++ b/drivers/net/dwmac_s700.c
@@ -60,6 +60,7 @@ U_BOOT_DRIVER(dwmac_s700) = {
 	.of_match = dwmac_s700_ids,
 	.of_to_plat = dwmac_s700_of_to_plat,
 	.probe  = dwmac_s700_probe,
+	.remove	= designware_eth_remove,
 	.ops    = &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct eth_pdata),
diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
index a9e2d8c0972..dda33f90d65 100644
--- a/drivers/net/dwmac_socfpga.c
+++ b/drivers/net/dwmac_socfpga.c
@@ -141,6 +141,7 @@ U_BOOT_DRIVER(dwmac_socfpga) = {
 	.of_match	= dwmac_socfpga_ids,
 	.of_to_plat = dwmac_socfpga_of_to_plat,
 	.probe		= dwmac_socfpga_probe,
+	.remove		= designware_eth_remove,
 	.ops		= &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct dwmac_socfpga_plat),
diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
index 8cfeeffe95b..5c542d62bd8 100644
--- a/drivers/net/gmac_rockchip.c
+++ b/drivers/net/gmac_rockchip.c
@@ -756,6 +756,7 @@ U_BOOT_DRIVER(eth_gmac_rockchip) = {
 	.of_match = rockchip_gmac_ids,
 	.of_to_plat = gmac_rockchip_of_to_plat,
 	.probe	= gmac_rockchip_probe,
+	.remove	= designware_eth_remove,
 	.ops	= &gmac_rockchip_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct gmac_rockchip_plat),
-- 
2.43.0

[PATCH v2 2/4] net: designware: Mark drivers as having active DMA

[Simon Glass]

Network devices which use this driver have DMA running while receiving
packages. Add the required flag to driver model so that they will be
removed before starting the OS.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add a new patch to mark drivers as having active DMA

 drivers/net/designware.c    | 2 +-
 drivers/net/dwmac_meson8b.c | 2 +-
 drivers/net/dwmac_s700.c    | 2 +-
 drivers/net/dwmac_socfpga.c | 2 +-
 drivers/net/gmac_rockchip.c | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 5385849ea6b..9b2b0a5d602 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -885,7 +885,7 @@ U_BOOT_DRIVER(eth_designware) = {
 	.ops	= &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct dw_eth_pdata),
-	.flags = DM_FLAG_ALLOC_PRIV_DMA,
+	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
 };
 
 static struct pci_device_id supported[] = {
diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
index bf94e50842d..95a99a98e99 100644
--- a/drivers/net/dwmac_meson8b.c
+++ b/drivers/net/dwmac_meson8b.c
@@ -162,5 +162,5 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
 	.ops		= &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct dwmac_meson8b_plat),
-	.flags		= DM_FLAG_ALLOC_PRIV_DMA,
+	.flags		= DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
 };
diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c
index e78193f3376..21df8e3ce81 100644
--- a/drivers/net/dwmac_s700.c
+++ b/drivers/net/dwmac_s700.c
@@ -64,5 +64,5 @@ U_BOOT_DRIVER(dwmac_s700) = {
 	.ops    = &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct eth_pdata),
-	.flags = DM_FLAG_ALLOC_PRIV_DMA,
+	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
 };
diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
index dda33f90d65..9f262aaee60 100644
--- a/drivers/net/dwmac_socfpga.c
+++ b/drivers/net/dwmac_socfpga.c
@@ -145,5 +145,5 @@ U_BOOT_DRIVER(dwmac_socfpga) = {
 	.ops		= &designware_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct dwmac_socfpga_plat),
-	.flags		= DM_FLAG_ALLOC_PRIV_DMA,
+	.flags		= DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
 };
diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
index 5c542d62bd8..52be3a5687f 100644
--- a/drivers/net/gmac_rockchip.c
+++ b/drivers/net/gmac_rockchip.c
@@ -760,5 +760,5 @@ U_BOOT_DRIVER(eth_gmac_rockchip) = {
 	.ops	= &gmac_rockchip_eth_ops,
 	.priv_auto	= sizeof(struct dw_eth_dev),
 	.plat_auto	= sizeof(struct gmac_rockchip_plat),
-	.flags = DM_FLAG_ALLOC_PRIV_DMA,
+	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
 };
-- 
2.43.0

[PATCH v2 3/4] efi_loader: Move device-removal later in exit-boot-services

[Simon Glass]

This removal should be the last thing done, so that U-Boot does no more
memory allocations afterwards, thus avoiding potentially allocating
memory which has been freed by a device that fails to de-activate its
DMA.

Of course, devices should be marked with DM_FLAG_ACTIVE_DMA or
DM_FLAG_OS_PREPARE but this change is good practice, in any case.

It also matches the code in announce_and_cleanup(), which we should at
some point unify with EFI_LOADER

So move the code and add a comment.

Note that the TCG2 log is updated after this call, but I cannot see any
allocations there.

Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index ffe43accd1e..e525662f82f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2250,14 +2250,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 			list_del(&evt->link);
 	}
 
-	if (!efi_st_keep_devices) {
-		bootm_disable_interrupts();
-		if (IS_ENABLED(CONFIG_USB_DEVICE))
-			udc_disconnect();
-		board_quiesce_devices();
-		dm_remove_devices_active();
-	}
-
 	/* Patch out unsupported runtime function */
 	efi_runtime_detach();
 
@@ -2279,6 +2271,19 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	/* Give the payload some time to boot */
 	efi_set_watchdog(0);
 	schedule();
+
+	/*
+	 * this should be the last thing done, to avoid memory allocations
+	 * between removing devices and the OS taking over
+	 */
+	if (!efi_st_keep_devices) {
+		bootm_disable_interrupts();
+		if (IS_ENABLED(CONFIG_USB_DEVICE))
+			udc_disconnect();
+		board_quiesce_devices();
+		dm_remove_devices_active();
+	}
+
 out:
 	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
 		if (ret != EFI_SUCCESS)
-- 
2.43.0

[PATCH v2 4/4] dm: core: Rename dm_remove_devices_active()

[Simon Glass]

This function doesn't have a great name, since we talk of devices being
active once they are probed.

Rename it to dm_remove_dma_devices().

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Jonas Karlman <jonas@kwiboo.se>
---

(no changes since v1)

 arch/arm/lib/bootm.c          | 4 ++--
 arch/riscv/lib/bootm.c        | 2 +-
 arch/x86/lib/bootm.c          | 2 +-
 drivers/core/root.c           | 2 +-
 include/dm/root.h             | 6 +++---
 lib/efi_loader/efi_boottime.c | 2 +-
 test/dm/core.c                | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 688c2f3f29b..ef859a2cd4c 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -74,10 +74,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
+	 * dm_remove_dma_devices() ensures that vital devices are removed in
 	 * a second round.
 	 */
-	dm_remove_devices_active();
+	dm_remove_dma_devices();
 
 	cleanup_before_linux();
 }
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
index 76c610bcee0..9aecaafef43 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_active();
+	dm_remove_dma_devices();
 
 	cleanup_before_linux();
 }
diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index 3c420b00936..91e8de4e1ad 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -50,7 +50,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_active();
+	dm_remove_dma_devices();
 }
 
 #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
diff --git a/drivers/core/root.c b/drivers/core/root.c
index c7fb58285ca..101cc4d43fa 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -148,7 +148,7 @@ int dm_remove_devices_flags(uint flags)
 	return 0;
 }
 
-void dm_remove_devices_active(void)
+void dm_remove_dma_devices(void)
 {
 	/* Remove non-vital devices first */
 	device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
diff --git a/include/dm/root.h b/include/dm/root.h
index 5651b868c8b..39a069cd465 100644
--- a/include/dm/root.h
+++ b/include/dm/root.h
@@ -169,16 +169,16 @@ int dm_uninit(void);
 int dm_remove_devices_flags(uint flags);
 
 /**
- * dm_remove_devices_active - Call remove function of all active drivers heeding
+ * dm_remove_dma_devices - 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);
+void dm_remove_dma_devices(void);
 #else
 static inline int dm_remove_devices_flags(uint flags) { return 0; }
-static inline void dm_remove_devices_active(void) { }
+static inline void dm_remove_dma_devices(void) { }
 #endif
 
 /**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index e525662f82f..8f78fcdf7c8 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2281,7 +2281,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_active();
+		dm_remove_dma_devices();
 	}
 
 out:
diff --git a/test/dm/core.c b/test/dm/core.c
index 959b834576f..1194e3250b5 100644
--- a/test/dm/core.c
+++ b/test/dm/core.c
@@ -1057,7 +1057,7 @@ static int dm_test_remove_active(struct unit_test_state *uts)
 	ut_asserteq(true, device_active(dma_vital));
 
 	/* Remove active devices in an ordered way */
-	dm_remove_devices_active();
+	dm_remove_dma_devices();
 
 	/* Check that all devices are inactive right now */
 	ut_asserteq(true, device_active(normal));
-- 
2.43.0

Re: [PATCH v2 1/4] designware: Use the remove() method with related drivers

[neil.armstrong]

On 07/04/2025 03:35, Simon Glass wrote:
> Several drivers make use of the designware Ethernet driver but do not
> implement the remove() method. Add this to keep things tidy.
> 
> Note that the Ethernet uclass calls each driver's stop() method on
> removal, so there is no need to do that here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
> ---
> 
> Changes in v2:
> - Use the designware_eth_remove() method directly
> - Add gmac_rockchip too
> - Reword the commit message to mention the uclass' actions
> 
>   drivers/net/designware.c    |  2 +-
>   drivers/net/designware.h    | 12 ++++++++++++
>   drivers/net/dwmac_meson8b.c |  1 +
>   drivers/net/dwmac_s700.c    |  1 +
>   drivers/net/dwmac_socfpga.c |  1 +
>   drivers/net/gmac_rockchip.c |  1 +
>   6 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 07b0f49ef58..5385849ea6b 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -805,7 +805,7 @@ clk_err:
>   	return err;
>   }
>   
> -static int designware_eth_remove(struct udevice *dev)
> +int designware_eth_remove(struct udevice *dev)
>   {
>   	struct dw_eth_dev *priv = dev_get_priv(dev);
>   
> diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> index e47101ccaf6..8c9d0190e03 100644
> --- a/drivers/net/designware.h
> +++ b/drivers/net/designware.h
> @@ -247,6 +247,18 @@ struct dw_eth_dev {
>   
>   int designware_eth_of_to_plat(struct udevice *dev);
>   int designware_eth_probe(struct udevice *dev);
> +
> +/**
> + * designware_eth_remove() - Remove the device
> + *
> + * Disables DMA and marks the device as remove. This must be called before
> + * booting an OS, to ensure that DMA is inactive.
> + *
> + * @dev: Device to remove
> + * Return 0 if OK, -ve on error
> + */
> +int designware_eth_remove(struct udevice *dev);
> +
>   extern const struct eth_ops designware_eth_ops;
>   
>   struct dw_eth_pdata {
> diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
> index fde4aabbace..bf94e50842d 100644
> --- a/drivers/net/dwmac_meson8b.c
> +++ b/drivers/net/dwmac_meson8b.c
> @@ -158,6 +158,7 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
>   	.of_match	= dwmac_meson8b_ids,
>   	.of_to_plat = dwmac_meson8b_of_to_plat,
>   	.probe		= dwmac_meson8b_probe,
> +	.remove		= designware_eth_remove,
>   	.ops		= &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct dwmac_meson8b_plat),
> diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c
> index 969d247b4f3..e78193f3376 100644
> --- a/drivers/net/dwmac_s700.c
> +++ b/drivers/net/dwmac_s700.c
> @@ -60,6 +60,7 @@ U_BOOT_DRIVER(dwmac_s700) = {
>   	.of_match = dwmac_s700_ids,
>   	.of_to_plat = dwmac_s700_of_to_plat,
>   	.probe  = dwmac_s700_probe,
> +	.remove	= designware_eth_remove,
>   	.ops    = &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct eth_pdata),
> diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
> index a9e2d8c0972..dda33f90d65 100644
> --- a/drivers/net/dwmac_socfpga.c
> +++ b/drivers/net/dwmac_socfpga.c
> @@ -141,6 +141,7 @@ U_BOOT_DRIVER(dwmac_socfpga) = {
>   	.of_match	= dwmac_socfpga_ids,
>   	.of_to_plat = dwmac_socfpga_of_to_plat,
>   	.probe		= dwmac_socfpga_probe,
> +	.remove		= designware_eth_remove,
>   	.ops		= &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct dwmac_socfpga_plat),
> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
> index 8cfeeffe95b..5c542d62bd8 100644
> --- a/drivers/net/gmac_rockchip.c
> +++ b/drivers/net/gmac_rockchip.c
> @@ -756,6 +756,7 @@ U_BOOT_DRIVER(eth_gmac_rockchip) = {
>   	.of_match = rockchip_gmac_ids,
>   	.of_to_plat = gmac_rockchip_of_to_plat,
>   	.probe	= gmac_rockchip_probe,
> +	.remove	= designware_eth_remove,
>   	.ops	= &gmac_rockchip_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct gmac_rockchip_plat),

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2 2/4] net: designware: Mark drivers as having active DMA

[neil.armstrong]

On 07/04/2025 03:35, Simon Glass wrote:
> Network devices which use this driver have DMA running while receiving
> packages. Add the required flag to driver model so that they will be
> removed before starting the OS.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Add a new patch to mark drivers as having active DMA
> 
>   drivers/net/designware.c    | 2 +-
>   drivers/net/dwmac_meson8b.c | 2 +-
>   drivers/net/dwmac_s700.c    | 2 +-
>   drivers/net/dwmac_socfpga.c | 2 +-
>   drivers/net/gmac_rockchip.c | 2 +-
>   5 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 5385849ea6b..9b2b0a5d602 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -885,7 +885,7 @@ U_BOOT_DRIVER(eth_designware) = {
>   	.ops	= &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct dw_eth_pdata),
> -	.flags = DM_FLAG_ALLOC_PRIV_DMA,
> +	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
>   };
>   
>   static struct pci_device_id supported[] = {
> diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
> index bf94e50842d..95a99a98e99 100644
> --- a/drivers/net/dwmac_meson8b.c
> +++ b/drivers/net/dwmac_meson8b.c
> @@ -162,5 +162,5 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
>   	.ops		= &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct dwmac_meson8b_plat),
> -	.flags		= DM_FLAG_ALLOC_PRIV_DMA,
> +	.flags		= DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
>   };
> diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c
> index e78193f3376..21df8e3ce81 100644
> --- a/drivers/net/dwmac_s700.c
> +++ b/drivers/net/dwmac_s700.c
> @@ -64,5 +64,5 @@ U_BOOT_DRIVER(dwmac_s700) = {
>   	.ops    = &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct eth_pdata),
> -	.flags = DM_FLAG_ALLOC_PRIV_DMA,
> +	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
>   };
> diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
> index dda33f90d65..9f262aaee60 100644
> --- a/drivers/net/dwmac_socfpga.c
> +++ b/drivers/net/dwmac_socfpga.c
> @@ -145,5 +145,5 @@ U_BOOT_DRIVER(dwmac_socfpga) = {
>   	.ops		= &designware_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct dwmac_socfpga_plat),
> -	.flags		= DM_FLAG_ALLOC_PRIV_DMA,
> +	.flags		= DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
>   };
> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
> index 5c542d62bd8..52be3a5687f 100644
> --- a/drivers/net/gmac_rockchip.c
> +++ b/drivers/net/gmac_rockchip.c
> @@ -760,5 +760,5 @@ U_BOOT_DRIVER(eth_gmac_rockchip) = {
>   	.ops	= &gmac_rockchip_eth_ops,
>   	.priv_auto	= sizeof(struct dw_eth_dev),
>   	.plat_auto	= sizeof(struct gmac_rockchip_plat),
> -	.flags = DM_FLAG_ALLOC_PRIV_DMA,
> +	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_ACTIVE_DMA,
>   };

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2 3/4] efi_loader: Move device-removal later in exit-boot-services

[Heinrich Schuchardt]

On 07.04.25 03:35, Simon Glass wrote:
> This removal should be the last thing done, so that U-Boot does no more
> memory allocations afterwards, thus avoiding potentially allocating
> memory which has been freed by a device that fails to de-activate its
> DMA.

The EFI application that is calling ExitBootServices() has been reading
the EFI memory map with GetMemoryMap() before. This is checked by
comparing the MapKey parameter.

Whatever allocations are done or not in ExitBootServices() is not
visible to the EFI application.

DMA has to be stopped in all cases.

I don't understand the virtue of the proposed change.

Best regards

Heinrich

>
> Of course, devices should be marked with DM_FLAG_ACTIVE_DMA or
> DM_FLAG_OS_PREPARE but this change is good practice, in any case.
>
> It also matches the code in announce_and_cleanup(), which we should at
> some point unify with EFI_LOADER
>
> So move the code and add a comment.
>
> Note that the TCG2 log is updated after this call, but I cannot see any
> allocations there.
>
> Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
> Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>   lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index ffe43accd1e..e525662f82f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2250,14 +2250,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   			list_del(&evt->link);
>   	}
>
> -	if (!efi_st_keep_devices) {
> -		bootm_disable_interrupts();
> -		if (IS_ENABLED(CONFIG_USB_DEVICE))
> -			udc_disconnect();
> -		board_quiesce_devices();
> -		dm_remove_devices_active();
> -	}
> -
>   	/* Patch out unsupported runtime function */
>   	efi_runtime_detach();
>
> @@ -2279,6 +2271,19 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   	/* Give the payload some time to boot */
>   	efi_set_watchdog(0);
>   	schedule();
> +
> +	/*
> +	 * this should be the last thing done, to avoid memory allocations
> +	 * between removing devices and the OS taking over
> +	 */
> +	if (!efi_st_keep_devices) {
> +		bootm_disable_interrupts();
> +		if (IS_ENABLED(CONFIG_USB_DEVICE))
> +			udc_disconnect();
> +		board_quiesce_devices();
> +		dm_remove_devices_active();
> +	}
> +
>   out:
>   	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>   		if (ret != EFI_SUCCESS)

Re: [PATCH v2 4/4] dm: core: Rename dm_remove_devices_active()

[Heinrich Schuchardt]

On 07.04.25 03:35, Simon Glass wrote:
> This function doesn't have a great name, since we talk of devices being
> active once they are probed.
>
> Rename it to dm_remove_dma_devices().
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>
> (no changes since v1)
>
>   arch/arm/lib/bootm.c          | 4 ++--
>   arch/riscv/lib/bootm.c        | 2 +-
>   arch/x86/lib/bootm.c          | 2 +-
>   drivers/core/root.c           | 2 +-
>   include/dm/root.h             | 6 +++---
>   lib/efi_loader/efi_boottime.c | 2 +-
>   test/dm/core.c                | 2 +-
>   7 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 688c2f3f29b..ef859a2cd4c 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -74,10 +74,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
> +	 * dm_remove_dma_devices() ensures that vital devices are removed in
>   	 * a second round.
>   	 */
> -	dm_remove_devices_active();
> +	dm_remove_dma_devices();
>
>   	cleanup_before_linux();
>   }
> diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
> index 76c610bcee0..9aecaafef43 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_active();
> +	dm_remove_dma_devices();
>
>   	cleanup_before_linux();
>   }
> diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
> index 3c420b00936..91e8de4e1ad 100644
> --- a/arch/x86/lib/bootm.c
> +++ b/arch/x86/lib/bootm.c
> @@ -50,7 +50,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_active();
> +	dm_remove_dma_devices();
>   }
>
>   #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index c7fb58285ca..101cc4d43fa 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -148,7 +148,7 @@ int dm_remove_devices_flags(uint flags)
>   	return 0;
>   }
>
> -void dm_remove_devices_active(void)
> +void dm_remove_dma_devices(void)
>   {
>   	/* Remove non-vital devices first */
>   	device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> diff --git a/include/dm/root.h b/include/dm/root.h
> index 5651b868c8b..39a069cd465 100644
> --- a/include/dm/root.h
> +++ b/include/dm/root.h
> @@ -169,16 +169,16 @@ int dm_uninit(void);
>   int dm_remove_devices_flags(uint flags);
>
>   /**
> - * dm_remove_devices_active - Call remove function of all active drivers heeding
> + * dm_remove_dma_devices - 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);
> +void dm_remove_dma_devices(void);

We have to remove all active devices. Not only DMA devices but also any
device that creates interrupts.

The renaming is misleading.

Best regards

Heinrich

>   #else
>   static inline int dm_remove_devices_flags(uint flags) { return 0; }
> -static inline void dm_remove_devices_active(void) { }
> +static inline void dm_remove_dma_devices(void) { }
>   #endif
>
>   /**
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index e525662f82f..8f78fcdf7c8 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2281,7 +2281,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_active();
> +		dm_remove_dma_devices();
>   	}
>
>   out:
> diff --git a/test/dm/core.c b/test/dm/core.c
> index 959b834576f..1194e3250b5 100644
> --- a/test/dm/core.c
> +++ b/test/dm/core.c
> @@ -1057,7 +1057,7 @@ static int dm_test_remove_active(struct unit_test_state *uts)
>   	ut_asserteq(true, device_active(dma_vital));
>
>   	/* Remove active devices in an ordered way */
> -	dm_remove_devices_active();
> +	dm_remove_dma_devices();
>
>   	/* Check that all devices are inactive right now */
>   	ut_asserteq(true, device_active(normal));

Re: [PATCH v2 3/4] efi_loader: Move device-removal later in exit-boot-services

[Simon Glass]

Hi Heinrich,

On Mon, 7 Apr 2025 at 19:55, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 07.04.25 03:35, Simon Glass wrote:
> > This removal should be the last thing done, so that U-Boot does no more
> > memory allocations afterwards, thus avoiding potentially allocating
> > memory which has been freed by a device that fails to de-activate its
> > DMA.
>
> The EFI application that is calling ExitBootServices() has been reading
> the EFI memory map with GetMemoryMap() before. This is checked by
> comparing the MapKey parameter.
>
> Whatever allocations are done or not in ExitBootServices() is not
> visible to the EFI application.
>
> DMA has to be stopped in all cases.

Yes, DMA must be stopped.

>
> I don't understand the virtue of the proposed change.

It is described in the next two paragraphs:

>
> Best regards
>
> Heinrich
>
> >
> > Of course, devices should be marked with DM_FLAG_ACTIVE_DMA or
> > DM_FLAG_OS_PREPARE but this change is good practice, in any case.
> >
> > It also matches the code in announce_and_cleanup(), which we should at
> > some point unify with EFI_LOADER

See above. Also, what do you think about unifying with announce_and_cleanup() ?

Regards,
Simon


> >
> > So move the code and add a comment.
> >
> > Note that the TCG2 log is updated after this call, but I cannot see any
> > allocations there.
> >
> > Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
> > Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >   lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
> >   1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index ffe43accd1e..e525662f82f 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -2250,14 +2250,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> >                       list_del(&evt->link);
> >       }
> >
> > -     if (!efi_st_keep_devices) {
> > -             bootm_disable_interrupts();
> > -             if (IS_ENABLED(CONFIG_USB_DEVICE))
> > -                     udc_disconnect();
> > -             board_quiesce_devices();
> > -             dm_remove_devices_active();
> > -     }
> > -
> >       /* Patch out unsupported runtime function */
> >       efi_runtime_detach();
> >
> > @@ -2279,6 +2271,19 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> >       /* Give the payload some time to boot */
> >       efi_set_watchdog(0);
> >       schedule();
> > +
> > +     /*
> > +      * this should be the last thing done, to avoid memory allocations
> > +      * between removing devices and the OS taking over
> > +      */
> > +     if (!efi_st_keep_devices) {
> > +             bootm_disable_interrupts();
> > +             if (IS_ENABLED(CONFIG_USB_DEVICE))
> > +                     udc_disconnect();
> > +             board_quiesce_devices();
> > +             dm_remove_devices_active();
> > +     }
> > +
> >   out:
> >       if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> >               if (ret != EFI_SUCCESS)
>

Re: [PATCH v2 4/4] dm: core: Rename dm_remove_devices_active()

[Simon Glass]

Hi Heinrich,

On Mon, 7 Apr 2025 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 07.04.25 03:35, Simon Glass wrote:
> > This function doesn't have a great name, since we talk of devices being
> > active once they are probed.
> >
> > Rename it to dm_remove_dma_devices().
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >
> > (no changes since v1)
> >
> >   arch/arm/lib/bootm.c          | 4 ++--
> >   arch/riscv/lib/bootm.c        | 2 +-
> >   arch/x86/lib/bootm.c          | 2 +-
> >   drivers/core/root.c           | 2 +-
> >   include/dm/root.h             | 6 +++---
> >   lib/efi_loader/efi_boottime.c | 2 +-
> >   test/dm/core.c                | 2 +-
> >   7 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> > index 688c2f3f29b..ef859a2cd4c 100644
> > --- a/arch/arm/lib/bootm.c
> > +++ b/arch/arm/lib/bootm.c
> > @@ -74,10 +74,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
> > +      * dm_remove_dma_devices() ensures that vital devices are removed in
> >        * a second round.
> >        */
> > -     dm_remove_devices_active();
> > +     dm_remove_dma_devices();
> >
> >       cleanup_before_linux();
> >   }
> > diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
> > index 76c610bcee0..9aecaafef43 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_active();
> > +     dm_remove_dma_devices();
> >
> >       cleanup_before_linux();
> >   }
> > diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
> > index 3c420b00936..91e8de4e1ad 100644
> > --- a/arch/x86/lib/bootm.c
> > +++ b/arch/x86/lib/bootm.c
> > @@ -50,7 +50,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_active();
> > +     dm_remove_dma_devices();
> >   }
> >
> >   #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
> > diff --git a/drivers/core/root.c b/drivers/core/root.c
> > index c7fb58285ca..101cc4d43fa 100644
> > --- a/drivers/core/root.c
> > +++ b/drivers/core/root.c
> > @@ -148,7 +148,7 @@ int dm_remove_devices_flags(uint flags)
> >       return 0;
> >   }
> >
> > -void dm_remove_devices_active(void)
> > +void dm_remove_dma_devices(void)
> >   {
> >       /* Remove non-vital devices first */
> >       device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> > diff --git a/include/dm/root.h b/include/dm/root.h
> > index 5651b868c8b..39a069cd465 100644
> > --- a/include/dm/root.h
> > +++ b/include/dm/root.h
> > @@ -169,16 +169,16 @@ int dm_uninit(void);
> >   int dm_remove_devices_flags(uint flags);
> >
> >   /**
> > - * dm_remove_devices_active - Call remove function of all active drivers heeding
> > + * dm_remove_dma_devices - 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);
> > +void dm_remove_dma_devices(void);
>
> We have to remove all active devices. Not only DMA devices but also any
> device that creates interrupts.
>
> The renaming is misleading.

Yes, but so is the existing name, as mentioned.

How about dm_remove_for_boot() ?

Regards,
Simon

Re: [PATCH v2 3/4] efi_loader: Move device-removal later in exit-boot-services

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Mon, 7 Apr 2025 22:49:05 +1200

Hi Simon,

Since I brought up the same objection as Heinrich...

> Hi Heinrich,
> 
> On Mon, 7 Apr 2025 at 19:55, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 07.04.25 03:35, Simon Glass wrote:
> > > This removal should be the last thing done, so that U-Boot does no more
> > > memory allocations afterwards, thus avoiding potentially allocating
> > > memory which has been freed by a device that fails to de-activate its
> > > DMA.
> >
> > The EFI application that is calling ExitBootServices() has been reading
> > the EFI memory map with GetMemoryMap() before. This is checked by
> > comparing the MapKey parameter.
> >
> > Whatever allocations are done or not in ExitBootServices() is not
> > visible to the EFI application.
> >
> > DMA has to be stopped in all cases.
> 
> Yes, DMA must be stopped.
> 
> >
> > I don't understand the virtue of the proposed change.
> 
> It is described in the next two paragraphs:
> 
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > Of course, devices should be marked with DM_FLAG_ACTIVE_DMA or
> > > DM_FLAG_OS_PREPARE but this change is good practice, in any case.

I disagree.  Stopping DMA is early as possible is good practice.

> > > It also matches the code in announce_and_cleanup(), which we should at
> > > some point unify with EFI_LOADER

As far as I can see there is nothing that happens in between the old
location and your new location in efi_exit_boot_services() that
matches anything that is done in announce_and_cleanup().

> See above. Also, what do you think about unifying with
> announce_and_cleanup() ?
> 
> Regards,
> Simon
> 
> 
> > >
> > > So move the code and add a comment.
> > >
> > > Note that the TCG2 log is updated after this call, but I cannot see any
> > > allocations there.
> > >
> > > Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
> > > Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >   lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
> > >   1 file changed, 13 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > index ffe43accd1e..e525662f82f 100644
> > > --- a/lib/efi_loader/efi_boottime.c
> > > +++ b/lib/efi_loader/efi_boottime.c
> > > @@ -2250,14 +2250,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > >                       list_del(&evt->link);
> > >       }
> > >
> > > -     if (!efi_st_keep_devices) {
> > > -             bootm_disable_interrupts();
> > > -             if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > -                     udc_disconnect();
> > > -             board_quiesce_devices();
> > > -             dm_remove_devices_active();
> > > -     }
> > > -
> > >       /* Patch out unsupported runtime function */
> > >       efi_runtime_detach();
> > >
> > > @@ -2279,6 +2271,19 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > >       /* Give the payload some time to boot */
> > >       efi_set_watchdog(0);
> > >       schedule();
> > > +
> > > +     /*
> > > +      * this should be the last thing done, to avoid memory allocations
> > > +      * between removing devices and the OS taking over
> > > +      */
> > > +     if (!efi_st_keep_devices) {
> > > +             bootm_disable_interrupts();
> > > +             if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > +                     udc_disconnect();
> > > +             board_quiesce_devices();
> > > +             dm_remove_devices_active();
> > > +     }
> > > +
> > >   out:
> > >       if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > >               if (ret != EFI_SUCCESS)
> >
> 

Re: [PATCH v2 4/4] dm: core: Rename dm_remove_devices_active()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4588 bytes --]

On Mon, Apr 07, 2025 at 10:49:11PM +1200, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 7 Apr 2025 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 07.04.25 03:35, Simon Glass wrote:
> > > This function doesn't have a great name, since we talk of devices being
> > > active once they are probed.
> > >
> > > Rename it to dm_remove_dma_devices().
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >   arch/arm/lib/bootm.c          | 4 ++--
> > >   arch/riscv/lib/bootm.c        | 2 +-
> > >   arch/x86/lib/bootm.c          | 2 +-
> > >   drivers/core/root.c           | 2 +-
> > >   include/dm/root.h             | 6 +++---
> > >   lib/efi_loader/efi_boottime.c | 2 +-
> > >   test/dm/core.c                | 2 +-
> > >   7 files changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> > > index 688c2f3f29b..ef859a2cd4c 100644
> > > --- a/arch/arm/lib/bootm.c
> > > +++ b/arch/arm/lib/bootm.c
> > > @@ -74,10 +74,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
> > > +      * dm_remove_dma_devices() ensures that vital devices are removed in
> > >        * a second round.
> > >        */
> > > -     dm_remove_devices_active();
> > > +     dm_remove_dma_devices();
> > >
> > >       cleanup_before_linux();
> > >   }
> > > diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
> > > index 76c610bcee0..9aecaafef43 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_active();
> > > +     dm_remove_dma_devices();
> > >
> > >       cleanup_before_linux();
> > >   }
> > > diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
> > > index 3c420b00936..91e8de4e1ad 100644
> > > --- a/arch/x86/lib/bootm.c
> > > +++ b/arch/x86/lib/bootm.c
> > > @@ -50,7 +50,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_active();
> > > +     dm_remove_dma_devices();
> > >   }
> > >
> > >   #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
> > > diff --git a/drivers/core/root.c b/drivers/core/root.c
> > > index c7fb58285ca..101cc4d43fa 100644
> > > --- a/drivers/core/root.c
> > > +++ b/drivers/core/root.c
> > > @@ -148,7 +148,7 @@ int dm_remove_devices_flags(uint flags)
> > >       return 0;
> > >   }
> > >
> > > -void dm_remove_devices_active(void)
> > > +void dm_remove_dma_devices(void)
> > >   {
> > >       /* Remove non-vital devices first */
> > >       device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> > > diff --git a/include/dm/root.h b/include/dm/root.h
> > > index 5651b868c8b..39a069cd465 100644
> > > --- a/include/dm/root.h
> > > +++ b/include/dm/root.h
> > > @@ -169,16 +169,16 @@ int dm_uninit(void);
> > >   int dm_remove_devices_flags(uint flags);
> > >
> > >   /**
> > > - * dm_remove_devices_active - Call remove function of all active drivers heeding
> > > + * dm_remove_dma_devices - 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);
> > > +void dm_remove_dma_devices(void);
> >
> > We have to remove all active devices. Not only DMA devices but also any
> > device that creates interrupts.
> >
> > The renaming is misleading.
> 
> Yes, but so is the existing name, as mentioned.
> 
> How about dm_remove_for_boot() ?

I think the name itself is fine and maybe just a bit more
documentation? It's dealing with the DM_REMOVE_ACTIVE_ALL flag
after all.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 4/4] dm: core: Rename dm_remove_devices_active()

[Simon Glass]

Hi Tom,

On Mon, 7 Apr 2025 at 07:30, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Apr 07, 2025 at 10:49:11PM +1200, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 7 Apr 2025 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 07.04.25 03:35, Simon Glass wrote:
> > > > This function doesn't have a great name, since we talk of devices being
> > > > active once they are probed.
> > > >
> > > > Rename it to dm_remove_dma_devices().
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >   arch/arm/lib/bootm.c          | 4 ++--
> > > >   arch/riscv/lib/bootm.c        | 2 +-
> > > >   arch/x86/lib/bootm.c          | 2 +-
> > > >   drivers/core/root.c           | 2 +-
> > > >   include/dm/root.h             | 6 +++---
> > > >   lib/efi_loader/efi_boottime.c | 2 +-
> > > >   test/dm/core.c                | 2 +-
> > > >   7 files changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> > > > index 688c2f3f29b..ef859a2cd4c 100644
> > > > --- a/arch/arm/lib/bootm.c
> > > > +++ b/arch/arm/lib/bootm.c
> > > > @@ -74,10 +74,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
> > > > +      * dm_remove_dma_devices() ensures that vital devices are removed in
> > > >        * a second round.
> > > >        */
> > > > -     dm_remove_devices_active();
> > > > +     dm_remove_dma_devices();
> > > >
> > > >       cleanup_before_linux();
> > > >   }
> > > > diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
> > > > index 76c610bcee0..9aecaafef43 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_active();
> > > > +     dm_remove_dma_devices();
> > > >
> > > >       cleanup_before_linux();
> > > >   }
> > > > diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
> > > > index 3c420b00936..91e8de4e1ad 100644
> > > > --- a/arch/x86/lib/bootm.c
> > > > +++ b/arch/x86/lib/bootm.c
> > > > @@ -50,7 +50,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_active();
> > > > +     dm_remove_dma_devices();
> > > >   }
> > > >
> > > >   #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
> > > > diff --git a/drivers/core/root.c b/drivers/core/root.c
> > > > index c7fb58285ca..101cc4d43fa 100644
> > > > --- a/drivers/core/root.c
> > > > +++ b/drivers/core/root.c
> > > > @@ -148,7 +148,7 @@ int dm_remove_devices_flags(uint flags)
> > > >       return 0;
> > > >   }
> > > >
> > > > -void dm_remove_devices_active(void)
> > > > +void dm_remove_dma_devices(void)
> > > >   {
> > > >       /* Remove non-vital devices first */
> > > >       device_remove(dm_root(), DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> > > > diff --git a/include/dm/root.h b/include/dm/root.h
> > > > index 5651b868c8b..39a069cd465 100644
> > > > --- a/include/dm/root.h
> > > > +++ b/include/dm/root.h
> > > > @@ -169,16 +169,16 @@ int dm_uninit(void);
> > > >   int dm_remove_devices_flags(uint flags);
> > > >
> > > >   /**
> > > > - * dm_remove_devices_active - Call remove function of all active drivers heeding
> > > > + * dm_remove_dma_devices - 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);
> > > > +void dm_remove_dma_devices(void);
> > >
> > > We have to remove all active devices. Not only DMA devices but also any
> > > device that creates interrupts.
> > >
> > > The renaming is misleading.
> >
> > Yes, but so is the existing name, as mentioned.
> >
> > How about dm_remove_for_boot() ?
>
> I think the name itself is fine and maybe just a bit more
> documentation? It's dealing with the DM_REMOVE_ACTIVE_ALL flag
> after all.

Yes,  it is. I would like to more clearly separate the naming, but I
haven't come up with a great option. So let's drop this patch.

Regards,
SImon