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

[Simon Glass]

Hi Tom,

On Thu, 14 Nov 2024 at 07:26, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Nov 13, 2024 at 08:53:17PM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 13 Nov 2024 at 11:45, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > >
> > > Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <sjg at chromium.org>:
> > > >Hi Heinrich,
> > > >
> > > >On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > >wrote:
> > > >>
> > > >> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg at chromium.org>:
> > > >> >Hi,
> > > >> >
> > > >> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > >wrote:
> > > >> >>
> > > >> >> On 11/1/24 21:29, Mark Kettenis wrote:
> > > >> >> >> From: Janne Grunau <j at 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 at jannau.net>
> > > >> >> >
> > > >> >> > Reviewed-by: Mark Kettenis <kettenis at 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.
> > > >
> > > >No objection to that...but it needs to be in a new function, not become the
> > > >default behaviour of dm_remove_devices_flags(), which is supposed to do
> > > >what it is told and in one pass.
> > >
> > > dm_remove_device_flags() makes no specific promises concerning the deletion sequence and its internal working.
> >
> > Do you mean in the function comment in the header file? We can
> > certainly update that.
>
> That seems like "fixing" the problem by documenting that things should
> work in the seemingly broken way.
>
> > > But obviously it will fail if non-vital redources are not deleted first.
> >
> > Either I don't understand that, or I don't agree. Can you give a
> > specific example?
>
> The Apple IOMMU? That's what got this thread started.
>
> > > Leaving this broken function exported and writing a new one has no user benefit.
> >
> > This function works fine. It is not broken.
> >
> > >
> > > We could copy the old function to a new static function that we call as needed from the old function name.
> >
> > Yes, that's fine with me. Also, did you see my note about the
> > bootflow_efi() test?
>
> But since your test passed (I assume) and real platforms are failing and
> requiring a fix, I'm not sure this is a strong argument? Or did you have
> a patch in a later series that fixed the problem here as well?

My comments were to Heinrich, not this patch. This patch is fine.

Heinrich, if you want to clean this up, please create a new function
which does these two steps separately.

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon