Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi,
>
>On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/1/24 21:29, Mark Kettenis wrote:
>> >> From: Janne Grunau <j@jannau.net>
>> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
>> >>
>> >> DM_FLAG_VITAL marks devices which are essential for the operation of
>> >> other devices. Removing these devices before their users can result in
>> >> hangs or crashes.
>> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
>> >> devices (and with this series the dart iommu) are the only devices
>> >> markes as vital.
>> >> The arm boot code already handles devioce removal in this way.
>> >
>> > There is a typo in that last sentence of the commit message (devioce).
>> > Otherwise:
>> >
>> >> Signed-off-by: Janne Grunau <j@jannau.net>
>> >
>> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
>> >
>> >> ---
>> >>   lib/efi_loader/efi_boottime.c | 1 +
>> >>   1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> >> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
>> >> --- a/lib/efi_loader/efi_boottime.c
>> >> +++ b/lib/efi_loader/efi_boottime.c
>> >> @@ -2234,6 +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_NON_VITAL);
>> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>>
>> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
>> the same issue for bootm on arm. But what about about other architectures?
>>
>> This logic should be moved to drivers/core/root.c instead of replicating
>> code.
>
>We could have a common helper, but it should not be in driver/core as
>this ordering is more of a policy decision. Unless we can add a
>parameter telling dm exactly what to do...
>
>BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
>was supposed to check. But since it doesn't have the
>exit-boot-services piece at your request...
>
>Regards,
>Simon


Why can't we generally remove non-vital devices first if all are to be removed?

I cannot see anything device specific here.

Best regards

Heinrich