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

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> From: Simon Glass <sjg@chromium.org>
> Date: Wed, 13 Nov 2024 07:39:22 -0700
> 
> Hi,

Hi Simon,

> 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...

But I don't think it makes sense for this to be a per-architecture
policy (like it is now).

Also, not that outside of the testsuite, we currently either do:

  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);

or just:

  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);

and I'd argue that all instances of the latter should be converted
into the latter.  So we really would only have a single policy here...