Re: [NOT A REGRESSION] firmware: framebuffer-coreboot: duplicate device name "simple-framebuffer.0"

From: Thomas Zimmermann
Date: Fri Sep 13 2024 - 02:53:58 EST

Hi Javier,

thanks for the patch.

Am 12.09.24 um 18:33 schrieb Javier Martinez Canillas:
Julius Werner <jwerner@xxxxxxxxxxxx> writes:

Hello Julius,

On Coreboot platforms, a system framebuffer may be provided to the Linux
kernel by filling a LB_TAG_FRAMEBUFFER entry in the Coreboot table. But
it seems SeaBIOS payload can also provide a VGA mode in the boot params.

[...]

To prevent the issue, make the framebuffer_core driver to disable sysfb
if there is system framebuffer data in the Coreboot table. That way only
this driver will register a device and sysfb would not attempt to do it
(or remove its registered device if was already executed before).
I wonder if the priority should be the other way around? coreboot's
framebuffer is generally only valid when coreboot exits to the payload
(e.g. SeaBIOS). Only if the payload doesn't touch the display
controller or if there is no payload and coreboot directly hands off
to a kernel does the kernel driver for LB_TAG_FRAMEBUFFER make sense.
But if there is some other framebuffer information passed to the
kernel from a firmware component running after coreboot, most likely
that one is more up to date and the framebuffer described by the
coreboot table doesn't work anymore (because the payload usually
doesn't modify the coreboot tables again, even if it changes hardware
state). So if there are two drivers fighting over which firmware
framebuffer description is the correct one, the coreboot driver should
probably give way.

That's a very good point. I'm actually not familiar with Coreboot and I
used an educated guess (in the case of DT for example, that's the main
source of truth and I didn't know if a Core table was in a similar vein).

Maybe something like the following (untested) patch then?

From de1c32017006f4671d91b695f4d6b4e99c073ab2 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@xxxxxxxxxx>
Date: Thu, 12 Sep 2024 18:31:55 +0200
Subject: [PATCH] firmware: coreboot: Don't register a pdev if screen_info data
is available

On Coreboot platforms, a system framebuffer may be provided to the Linux
kernel by filling a LB_TAG_FRAMEBUFFER entry in the Coreboot table. But
a Coreboot payload (e.g: SeaBIOS) could also provide this information to
the Linux kernel.

If that the case, early arch x86 boot code will fill the global struct
screen_info data and that data used by the Generic System Framebuffers
(sysfb) framework to add a platform device with platform data about the
system framebuffer.

But later then the framebuffer_coreboot driver will try to do the same
framebuffer (using the information from the Coreboot table), which will
lead to an error due a simple-framebuffer.0 device already registered:

sysfs: cannot create duplicate filename '/bus/platform/devices/simple-framebuffer.0'
...
coreboot: could not register framebuffer
framebuffer coreboot8: probe with driver framebuffer failed with error -17

To prevent the issue, make the framebuffer_core driver to not register a
platform device if the global struct screen_info data has been filled.

Reported-by: Brian Norris <briannorris@xxxxxxxxxxxx>
Link: https://lore.kernel.org/all/ZuCG-DggNThuF4pj@b20ea791c01f/T/#ma7fb65acbc1a56042258adac910992bb225a20d2
Suggested-by: Julius Werner <jwerner@xxxxxxxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
---
drivers/firmware/google/framebuffer-coreboot.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index daadd71d8ddd..4e50da17cd7e 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
+#include <linux/screen_info.h>
#include "coreboot_table.h"
@@ -27,6 +28,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
int i;
u32 length;
struct lb_framebuffer *fb = &dev->framebuffer;
+ struct screen_info *si = &screen_info;

Probably 'const'.

struct platform_device *pdev;
struct resource res;
struct simplefb_platform_data pdata = {
@@ -36,6 +38,20 @@ static int framebuffer_probe(struct coreboot_device *dev)
.format = NULL,
};
+ /*
+ * If the global screen_info data has been filled, the Generic
+ * System Framebuffers (sysfb) will already register a platform
+ * and pass the screen_info as platform_data to a driver that
+ * could scan-out using the system provided framebuffer.
+ *
+ * On Coreboot systems, the advertise LB_TAG_FRAMEBUFFER entry
+ * in the Coreboot table should only be used if the payload did
+ * not set video mode info and passed it to the Linux kernel.
+ */
+ if (si->orig_video_isVGA == VIDEO_TYPE_VLFB ||
+ si->orig_video_isVGA == VIDEO_TYPE_EFI)

Rather call screen_info_video_type(si) [1] to get the type. If it returns 0, the screen_info is unset and the corebios code can handle the framebuffer. In any other case, the framebuffer went through a bootloader, which might have modified it. This also handles awkward cases, such as if the bootloader programs a VGA text mode.

[1] https://elixir.bootlin.com/linux/v6.10.10/source/include/linux/screen_info.h#L92

With these changes:

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Best regards
Thomas

+ return -EINVAL;
+
if (!fb->physical_address)
return -ENODEV;

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)