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.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.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
signature.asc
Description: PGP signature
