Re: [PATCH 1/8] firmware: google: Do sysfb test before creating coreboot framebuffer

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 08.01.26 um 17:55 schrieb Julius Werner:
> This seems less consistent to me, although tbh I don't understand the
> Linux device and driver framework that well. I think the original idea
> here was that the coreboot bus represents the coreboot table, and each
> entry is represented as one device (regardless of whether any driver
> actually wants to do anything with that entry). That's why we're
> creating a device for all the tags even though most of them aren't
> really interesting for kernel drivers. This also helps with inspecting
> things in sysfs.

We need to distinguish between coreboot and sysfb. Only one of these 
subsystems can handle the framebuffer. Having a framebuffer device on 
the coreboot bus, if the underlying framebuffer is managed by a 
different device seems even more incorrect.

>
> So the device with TAG_FRAMEBUFFER doesn't mean that we have a
> framebuffer, it just means that we have an entry in the table
> describing a framebuffer. Whether it should actually be used to set up
> a framebuffer should be up to the binding driver. That's why I think
> keeping this decision in the driver probe makes more sense, and
> excluding it from the devices on the bus is weird (because you're just
> randomly excluding one of the entries in the table from being
> represented like the others, just because of details about how its
> drivers would want to use it).
>
> If you want these devices to be bound by drivers outside this
> directory, then I think either that other driver needs to have the
> logic to decide when a coreboot framebuffer should actually be used,
> or maybe you should have a small stub driver in this directory that
> binds to the list entry device, makes the decision whether to actually
> use it, and if so creates a sub-device or something (if that's
> possible?) which the actual outside driver can find and bind to.

This is what the current code in framebuffer-coreboot.c does. It binds 
to the coreboot-framebuffer device and create another device for 
external DRM/fbdev drivers to handle. This is problematic, as

1) the additional device will be gone after the native hardware drivers 
takes over the display, so the pdev pointer at [1] is dangling. 
Apparently no one ever unloads the coreboot-framebuffer device to 
trigger this problem.

2) The corboot-framebuffer device sets itself as the external device's 
parent. This is incorrect. The framebuffer exists on top of a PCI 
graphics device, so that device should be the parent. Otherwise the user 
can hot-unplug the PCI hardware and the coreboot framebuffer operates on 
a bogus graphics aperture. Modeling the parent-child relationships 
correctly, avoids this problem.  We've seen these problems with 
UEFI/VESA framebuffers and fixed it accordingly. [2] Something similar 
should be done for coreboot. Coreboot devices can still be located on 
the coreboot bus.

[1] 
https://elixir.bootlin.com/linux/v6.18.4/source/drivers/firmware/google/framebuffer-coreboot.c#L92
[2] 
https://elixir.bootlin.com/linux/v6.18.4/source/drivers/firmware/sysfb.c#L160

Another question I have is why there's a separate device for the 
coreboot-table? Couldn't the kernel just create the coreboot bus and 
then populate it with the table entries? DT does that. If the 
coreboot-table device is really just the parent for the other deivce, 
that is incorrect, as I describe above.

The coreboot bus is only for convenience, I guess? Other subsystems 
(sysfb, DT) create platform devices directly.

Best regards
Thomas



>
> On Thu, Jan 8, 2026 at 3:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Test sysfb before creating the coreboot framebuffer device. Skip
>> device creation if the test fails, as this framebuffer does not exist.
>>
>> Depending on the system setup, the initial framebuffer can be provided
>> by the boot loader via screen_info boot parameters and handled by the
>> kernel's sysfb code in drivers/firmware/sysfb.c. With the sysfb test in
>> the coreboot-framebuffer probing, the coreboot device is present without
>> the framebuffer. Even after the sysfb device has been replaced with a
>> native PCI device, the coreboot device persists.
>>
>> Skipping device creation early avoids all these inconsistencies. It
>> further prepares coreboot to support graphics drivers besides the one
>> in framebuffer-coreboot.c.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/firmware/google/coreboot_table.c       | 17 +++++++++++++++++
>>   drivers/firmware/google/coreboot_table.h       |  1 +
>>   drivers/firmware/google/framebuffer-coreboot.c | 16 ----------------
>>   3 files changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
>> index 882db32e51be..c34426e5002d 100644
>> --- a/drivers/firmware/google/coreboot_table.c
>> +++ b/drivers/firmware/google/coreboot_table.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/of.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>> +#include <linux/sysfb.h>
>>
>>   #include "coreboot_table.h"
>>
>> @@ -118,6 +119,22 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
>>                          return -EINVAL;
>>                  }
>>
>> +               switch (entry->tag) {
>> +               case CB_TAG_FRAMEBUFFER:
>> +                       /*
>> +                        * On coreboot systems, the advertised CB_TAG_FRAMEBUFFER entry
>> +                        * in the coreboot table should only be used if the payload did
>> +                        * not pass a framebuffer information to the Linux kernel.
>> +                        *
>> +                        * If the global screen_info data has been filled, the generic
>> +                        * system framebuffers (sysfb) will already register a platform
>> +                        * device and pass that screen_info as platform_data to a driver
>> +                        * that can scan-out using the system-provided framebuffer.
>> +                        */
>> +                       if (sysfb_handles_screen_info())
>> +                               continue;
>> +               }
>> +
>>                  device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL);
>>                  if (!device)
>>                          return -ENOMEM;
>> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
>> index bb6f0f7299b4..e3c353676940 100644
>> --- a/drivers/firmware/google/coreboot_table.h
>> +++ b/drivers/firmware/google/coreboot_table.h
>> @@ -40,6 +40,7 @@ struct lb_cbmem_ref {
>>          u64 cbmem_addr;
>>   };
>>
>> +#define CB_TAG_FRAMEBUFFER 0x12
>>   #define LB_TAG_CBMEM_ENTRY 0x31
>>
>>   /* Corresponds to LB_TAG_CBMEM_ENTRY */
>> diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
>> index c68c9f56370f..bb53d1a47409 100644
>> --- a/drivers/firmware/google/framebuffer-coreboot.c
>> +++ b/drivers/firmware/google/framebuffer-coreboot.c
>> @@ -15,12 +15,9 @@
>>   #include <linux/module.h>
>>   #include <linux/platform_data/simplefb.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/sysfb.h>
>>
>>   #include "coreboot_table.h"
>>
>> -#define CB_TAG_FRAMEBUFFER 0x12
>> -
>>   static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
>>
>>   static int framebuffer_probe(struct coreboot_device *dev)
>> @@ -37,19 +34,6 @@ static int framebuffer_probe(struct coreboot_device *dev)
>>                  .format = NULL,
>>          };
>>
>> -       /*
>> -        * On coreboot systems, the advertised LB_TAG_FRAMEBUFFER entry
>> -        * in the coreboot table should only be used if the payload did
>> -        * not pass a framebuffer information to the Linux kernel.
>> -        *
>> -        * If the global screen_info data has been filled, the Generic
>> -        * System Framebuffers (sysfb) will already register a platform
>> -        * device and pass that screen_info as platform_data to a driver
>> -        * that can scan-out using the system provided framebuffer.
>> -        */
>> -       if (sysfb_handles_screen_info())
>> -               return -ENODEV;
>> -
>>          if (!fb->physical_address)
>>                  return -ENODEV;
>>
>> --
>> 2.52.0
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)