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

[Javier Martinez Canillas <javierm@redhat.com>]

Brian Norris <briannorris@chromium.org> writes:

Hello Brian,

> (Tweaking subject; this indeed isn't related to the regression at all)
>
> Hi,
>
> On Mon, Sep 09, 2024 at 10:02:00AM +0200, Borislav Petkov wrote:
>> Looking at your log, the first warn is in framebuffer_coreboot. Some mess in
>> the sysfs platform devices registration.
>> 
>> Adding the relevant people for that:
>> 
>> Aug 20 20:29:36 luna kernel: sysfs: cannot create duplicate filename '/bus/platform/devices/simple-framebuffer.0'
>> Aug 20 20:29:36 luna kernel: CPU: 5 PID: 571 Comm: (udev-worker) Tainted: G           OE      6.10.6-arch1-1 #1 703d152c24f1971e36f16e505405e456fc9e23f8
>> Aug 20 20:29:36 luna kernel: Hardware name: Purism Librem 14/Librem 14, BIOS 4.14-Purism-1 06/18/2021
>> Aug 20 20:29:36 luna kernel: Call Trace:
>> Aug 20 20:29:36 luna kernel:  <TASK>
>> Aug 20 20:29:36 luna kernel:  dump_stack_lvl+0x5d/0x80
>> Aug 20 20:29:36 luna kernel:  sysfs_warn_dup.cold+0x17/0x23
>> Aug 20 20:29:36 luna kernel:  sysfs_do_create_link_sd+0xcf/0xe0
>> Aug 20 20:29:36 luna kernel:  bus_add_device+0x6b/0x130
>> Aug 20 20:29:36 luna kernel:  device_add+0x3b3/0x870
>> Aug 20 20:29:36 luna kernel:  platform_device_add+0xed/0x250
>> Aug 20 20:29:36 luna kernel:  platform_device_register_full+0xbb/0x140
>> Aug 20 20:29:36 luna kernel:  platform_device_register_resndata.constprop.0+0x54/0x80 [framebuffer_coreboot a587d2fc243ebaa0205c3badd33442a004d284e0]
>> Aug 20 20:29:36 luna kernel:  framebuffer_probe+0x165/0x1b0 [framebuffer_coreboot a587d2fc243ebaa0205c3badd33442a004d284e0]
>> Aug 20 20:29:36 luna kernel:  really_probe+0xdb/0x340
>> Aug 20 20:29:36 luna kernel:  ? pm_runtime_barrier+0x54/0x90
>> Aug 20 20:29:36 luna kernel:  ? __pfx___driver_attach+0x10/0x10
>> Aug 20 20:29:36 luna kernel:  __driver_probe_device+0x78/0x110
>> Aug 20 20:29:36 luna kernel:  driver_probe_device+0x1f/0xa0
>> Aug 20 20:29:36 luna kernel:  __driver_attach+0xba/0x1c0
>> Aug 20 20:29:36 luna kernel:  bus_for_each_dev+0x8c/0xe0
>> Aug 20 20:29:36 luna kernel:  bus_add_driver+0x112/0x1f0
>> Aug 20 20:29:36 luna kernel:  driver_register+0x72/0xd0
>> Aug 20 20:29:36 luna kernel:  ? __pfx_framebuffer_driver_init+0x10/0x10 [framebuffer_coreboot a587d2fc243ebaa0205c3badd33442a004d284e0]
>> Aug 20 20:29:36 luna kernel:  do_one_initcall+0x58/0x310
>> Aug 20 20:29:36 luna kernel:  do_init_module+0x60/0x220
>> Aug 20 20:29:36 luna kernel:  init_module_from_file+0x89/0xe0
>> Aug 20 20:29:36 luna kernel:  idempotent_init_module+0x121/0x320
>> Aug 20 20:29:36 luna kernel:  __x64_sys_finit_module+0x5e/0xb0
>> Aug 20 20:29:36 luna kernel:  do_syscall_64+0x82/0x190
>> Aug 20 20:29:36 luna kernel:  ? __do_sys_newfstatat+0x3c/0x80
>> Aug 20 20:29:36 luna kernel:  ? syscall_exit_to_user_mode+0x72/0x200
>> Aug 20 20:29:36 luna kernel:  ? do_syscall_64+0x8e/0x190
>> Aug 20 20:29:36 luna kernel:  ? do_sys_openat2+0x9c/0xe0
>> Aug 20 20:29:36 luna kernel:  ? syscall_exit_to_user_mode+0x72/0x200
>> Aug 20 20:29:36 luna kernel:  ? do_syscall_64+0x8e/0x190
>> Aug 20 20:29:36 luna kernel:  ? clear_bhb_loop+0x25/0x80
>> Aug 20 20:29:36 luna kernel:  ? clear_bhb_loop+0x25/0x80
>> Aug 20 20:29:36 luna kernel:  ? clear_bhb_loop+0x25/0x80
>> Aug 20 20:29:36 luna kernel:  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> Aug 20 20:29:36 luna kernel: RIP: 0033:0x7b1bee2f81fd
>
> Looks like it might be a conflict with
> drivers/firmware/sysfb_simplefb.c, which also uses the
> "simple-framebuffer" name with a constant ID of 0. It's possible both
> drivers should be switched to use PLATFORM_DEVID_AUTO? Or at least one
> of them. Or they should use different base names.
>

I'm unsure about PLATFORM_DEVID_AUTO because I don't know if there are
user-space programs that assume this to always be "simple-framebuffer.0".

> I'm not really sure what the best option is (does anyone rely on or care
> about the device naming?), and I don't actually use this driver. But
> here's an untested diff to try if you'd really like. If you test it,
> feel free to submit as a proper patch with my:
>


I've discussed this with Thomas Zimmermann (simpledrm maintainer) and
he suggests that the problem is the system framebuffer information to
be provided in both Coreboot table entry (AFAIU is LB_TAG_FRAMEBUFFER)
and in the boot_params, which leads to struct screen_info to be filled.

We had the same problem for EFI systems that passed DTB to the kernel
instead of ACPI, in those cases both a "simple-framebuffer" DT node and
an EFI-GOP table could be provided.

Commit 3310288f6135 "(of/platform: Disable sysfb if a simple-framebuffer
node is found") solved that issue. I've typed the same for Coreboot to
handle in the same way. Please let me know what you think:

From 6955149fb13af1c0cba2e5c1fbb1ac9367a09ae2 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Thu, 12 Sep 2024 12:55:29 +0200
Subject: [RFC PATCH] firmware: coreboot: Disable sysfb if Coreboot already
 provides a FB

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.

If that the case, early arch x86 boot code will fill the global struct
screen_info data.

The data is used by the Generic System Framebuffers (sysfb) framework to
add a platform device with platform data about the system framebuffer.

But if there is information about the system framebuffer in the Coreboot
table as well, the framebuffer_coreboot driver will also try to do the
same and add another device for the system framebuffer. This will fail
though because there's already a simple-framebuffer.0 device 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 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).

Reported-by: Brian Norris <briannorris@chromium.org>
Link: https://lore.kernel.org/all/ZuCG-DggNThuF4pj@b20ea791c01f/T/#ma7fb65acbc1a56042258adac910992bb225a20d2
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/google/framebuffer-coreboot.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index daadd71d8ddd..0a28aa5b17dc 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -61,6 +61,19 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (res.end <= res.start)
 		return -EINVAL;
 
+	/*
+	 * Since a "simple-framebuffer" device is already added
+	 * here, disable the Generic System Framebuffers (sysfb)
+	 * to prevent it from registering another device for the
+	 * system framebuffer later (e.g: using the screen_info
+	 * data that may had been filled as well).
+	 *
+	 * This can happen for example on Coreboot systems, that
+	 * advertise a LB_TAG_FRAMEBUFFER entry in their Coreboot
+	 * table and also a VESA mode by the BIOS used as payload.
+	 */
+	sysfb_disable();
+
 	pdev = platform_device_register_resndata(&dev->dev,
 						 "simple-framebuffer", 0,
 						 &res, 1, &pdata,
-- 
 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat