Hi Heinrich,
On Wed, 13 Nov 2024 at 11:45, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <s...@chromium.org>:
> >Hi Heinrich,
> >
> >On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.g...@gmx.de>
> >wrote:
> >>
> >> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <s...@chromium.org>:
> >> >Hi,
> >> >
> >> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.g...@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 <kette...@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.
> 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?
>
> 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?
Regards,
Simon
