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 <[email protected]> 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 <[email protected]>
---
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)
