Hi Thomas On Wed, 26 Nov 2025 at 17:09, Thomas Zimmermann <[email protected]> wrote: > > Rename alloc_primary_display() and __alloc_primary_display(), clarify > free semantics to make interfaces easier to understand. > > Rename alloc_primary_display() to lookup_primary_display() as it > does not necessarily allocate. Then rename __alloc_primary_display() > to the new alloc_primary_display(). The helper belongs to > free_primary_display), so it should be named without underscores. > > The lookup helper does not necessarily allocate, so the output > parameter needs_free to indicate when free should be called.
I don't understand why we need this. Whether or not the helper allocates is a compile time decision, and in builds where it doesn't, the free helper doesn't do anything. I'm all for making things simpler, but I don't think this patch achieves that tbh. I've queued up this series now up until this patch - once we converge on the simplification, I'm happy to apply it on top. Thanks, > Pass > an argument through the calls to track this state. Put the free > handling into release_primary_display() for simplificy. > > Also move the comment fro primary_display.c to efi-stub-entry.c, > where it now describes lookup_primary_display(). > > Signed-off-by: Thomas Zimmermann <[email protected]> > --- > drivers/firmware/efi/libstub/efi-stub-entry.c | 23 +++++++++++++++++-- > drivers/firmware/efi/libstub/efi-stub.c | 22 ++++++++++++------ > drivers/firmware/efi/libstub/efistub.h | 2 +- > .../firmware/efi/libstub/primary_display.c | 17 +------------- > drivers/firmware/efi/libstub/zboot.c | 6 +++-- > 5 files changed, 42 insertions(+), 28 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-entry.c > b/drivers/firmware/efi/libstub/efi-stub-entry.c > index aa85e910fe59..3077b51fe0b2 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-entry.c > +++ b/drivers/firmware/efi/libstub/efi-stub-entry.c > @@ -14,10 +14,29 @@ static void *kernel_image_addr(void *addr) > return addr + kernel_image_offset; > } > > -struct sysfb_display_info *alloc_primary_display(void) > +/* > + * There are two ways of populating the core kernel's sysfb_primary_display > + * via the stub: > + * > + * - using a configuration table, which relies on the EFI init code to > + * locate the table and copy the contents; or > + * > + * - by linking directly to the core kernel's copy of the global symbol. > + * > + * The latter is preferred because it makes the EFIFB earlycon available very > + * early, but it only works if the EFI stub is part of the core kernel image > + * itself. The zboot decompressor can only use the configuration table > + * approach. > + */ > + > +struct sysfb_display_info *lookup_primary_display(bool *needs_free) > { > + *needs_free = true; > + > if (IS_ENABLED(CONFIG_ARM)) > - return __alloc_primary_display(); > + return alloc_primary_display(); > + > + *needs_free = false; > > if (IS_ENABLED(CONFIG_X86) || > IS_ENABLED(CONFIG_EFI_EARLYCON) || > diff --git a/drivers/firmware/efi/libstub/efi-stub.c > b/drivers/firmware/efi/libstub/efi-stub.c > index 42d6073bcd06..dc545f62c62b 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -51,14 +51,14 @@ static bool flat_va_mapping = (EFI_RT_VIRTUAL_OFFSET != > 0); > void __weak free_primary_display(struct sysfb_display_info *dpy) > { } > > -static struct sysfb_display_info *setup_primary_display(void) > +static struct sysfb_display_info *setup_primary_display(bool *dpy_needs_free) > { > struct sysfb_display_info *dpy; > struct screen_info *screen = NULL; > struct edid_info *edid = NULL; > efi_status_t status; > > - dpy = alloc_primary_display(); > + dpy = lookup_primary_display(dpy_needs_free); > if (!dpy) > return NULL; > screen = &dpy->screen; > @@ -68,15 +68,22 @@ static struct sysfb_display_info > *setup_primary_display(void) > > status = efi_setup_graphics(screen, edid); > if (status != EFI_SUCCESS) > - goto err_free_primary_display; > + goto err___free_primary_display; > > return dpy; > > -err_free_primary_display: > - free_primary_display(dpy); > +err___free_primary_display: > + if (*dpy_needs_free) > + free_primary_display(dpy); > return NULL; > } > > +static void release_primary_display(struct sysfb_display_info *dpy, bool > dpy_needs_free) > +{ > + if (dpy && dpy_needs_free) > + free_primary_display(dpy); > +} > + > static void install_memreserve_table(void) > { > struct linux_efi_memreserve *rsv; > @@ -156,13 +163,14 @@ efi_status_t efi_stub_common(efi_handle_t handle, > char *cmdline_ptr) > { > struct sysfb_display_info *dpy; > + bool dpy_needs_free; > efi_status_t status; > > status = check_platform_features(); > if (status != EFI_SUCCESS) > return status; > > - dpy = setup_primary_display(); > + dpy = setup_primary_display(&dpy_needs_free); > > efi_retrieve_eventlog(); > > @@ -182,7 +190,7 @@ efi_status_t efi_stub_common(efi_handle_t handle, > > status = efi_boot_kernel(handle, image, image_addr, cmdline_ptr); > > - free_primary_display(dpy); > + release_primary_display(dpy, dpy_needs_free); > > return status; > } > diff --git a/drivers/firmware/efi/libstub/efistub.h > b/drivers/firmware/efi/libstub/efistub.h > index 979a21818cc1..1503ffb82903 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -1176,8 +1176,8 @@ efi_enable_reset_attack_mitigation(void) { } > > void efi_retrieve_eventlog(void); > > +struct sysfb_display_info *lookup_primary_display(bool *needs_free); > struct sysfb_display_info *alloc_primary_display(void); > -struct sysfb_display_info *__alloc_primary_display(void); > void free_primary_display(struct sysfb_display_info *dpy); > > void efi_cache_sync_image(unsigned long image_base, > diff --git a/drivers/firmware/efi/libstub/primary_display.c > b/drivers/firmware/efi/libstub/primary_display.c > index cdaebab26514..34c54ac1e02a 100644 > --- a/drivers/firmware/efi/libstub/primary_display.c > +++ b/drivers/firmware/efi/libstub/primary_display.c > @@ -7,24 +7,9 @@ > > #include "efistub.h" > > -/* > - * There are two ways of populating the core kernel's sysfb_primary_display > - * via the stub: > - * > - * - using a configuration table, which relies on the EFI init code to > - * locate the table and copy the contents; or > - * > - * - by linking directly to the core kernel's copy of the global symbol. > - * > - * The latter is preferred because it makes the EFIFB earlycon available very > - * early, but it only works if the EFI stub is part of the core kernel image > - * itself. The zboot decompressor can only use the configuration table > - * approach. > - */ > - > static efi_guid_t primary_display_guid = > LINUX_EFI_PRIMARY_DISPLAY_TABLE_GUID; > > -struct sysfb_display_info *__alloc_primary_display(void) > +struct sysfb_display_info *alloc_primary_display(void) > { > struct sysfb_display_info *dpy; > efi_status_t status; > diff --git a/drivers/firmware/efi/libstub/zboot.c > b/drivers/firmware/efi/libstub/zboot.c > index 4b76f74c56da..c1fd1fdbcb08 100644 > --- a/drivers/firmware/efi/libstub/zboot.c > +++ b/drivers/firmware/efi/libstub/zboot.c > @@ -26,9 +26,11 @@ void __weak efi_cache_sync_image(unsigned long image_base, > // executable code loaded into memory to be safe for execution. > } > > -struct sysfb_display_info *alloc_primary_display(void) > +struct sysfb_display_info *lookup_primary_display(bool *needs_free) > { > - return __alloc_primary_display(); > + *needs_free = true; > + > + return alloc_primary_display(); > } > > asmlinkage efi_status_t __efiapi > -- > 2.51.1 >
