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

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241114/1d6b7ec9/attachment.sig>