| Message ID | ZuCGkjoxKxpnhEh6@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [NOT,A,REGRESSION] firmware: framebuffer-coreboot: duplicate device name "simple-framebuffer.0" | expand |
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,
> 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.
Julius Werner <jwerner@chromium.org> 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@redhat.com> 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@chromium.org> Link: https://lore.kernel.org/all/ZuCG-DggNThuF4pj@b20ea791c01f/T/#ma7fb65acbc1a56042258adac910992bb225a20d2 Suggested-by: Julius Werner <jwerner@chromium.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- 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; 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) + return -EINVAL; + if (!fb->physical_address) return -ENODEV;
Hi Javier, thanks for the patch. Am 12.09.24 um 18:33 schrieb Javier Martinez Canillas: > Julius Werner <jwerner@chromium.org> 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@redhat.com> > 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@chromium.org> > Link: https://lore.kernel.org/all/ZuCG-DggNThuF4pj@b20ea791c01f/T/#ma7fb65acbc1a56042258adac910992bb225a20d2 > Suggested-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > 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@suse.de> Best regards Thomas > + return -EINVAL; > + > if (!fb->physical_address) > return -ENODEV; >
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Hi Javier, > > thanks for the patch. > Thanks for your feedback. > Am 12.09.24 um 18:33 schrieb Javier Martinez Canillas: >> Julius Werner <jwerner@chromium.org> writes: [...] >> --- >> 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'. > Ok. >> 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 Indeed. I missed that helper, I'll change 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@suse.de> > Thanks. I'll wait for others in this thread to comment and if all agree with the solution, I'll post a proper patch (addressing your comments). > Best regards > Thomas >
Hi Javier, On Thu, Sep 12, 2024 at 06:33:58PM +0200, Javier Martinez Canillas wrote: > 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? Julius is more familiar with the Coreboot + payload ecosystem than me, but his explanations make sense to me, as does this patch. > From de1c32017006f4671d91b695f4d6b4e99c073ab2 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javierm@redhat.com> > 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. Normally, these sorts of "early" and "later" ordering descriptions would set alarm bells when talking about independent drivers. But I suppose the "early arch" code has better ordering guaranteeds than drivers, so this should be fine. > 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@chromium.org> > Link: https://lore.kernel.org/all/ZuCG-DggNThuF4pj@b20ea791c01f/T/#ma7fb65acbc1a56042258adac910992bb225a20d2 > Suggested-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > 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; > 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 Did you mean 'platform_device'? > + * 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 s/advertise/advertised/ ? > + * in the Coreboot table should only be used if the payload did > + * not set video mode info and passed it to the Linux kernel. s/passed/pass/ > + */ > + if (si->orig_video_isVGA == VIDEO_TYPE_VLFB || > + si->orig_video_isVGA == VIDEO_TYPE_EFI) This line is using spaces for indentation. It should use a tab, and then spaces for alignment. But presumably this will change based on Thomas's suggestions anyway. > + return -EINVAL; Is EINVAL right? IIUC, that will print a noisier error to the logs. I believe the "expected" sorts of return codes are ENODEV or ENXIO. (See call_driver_probe().) ENODEV seems like a fine choice, similar to several of the other return codes already used here. Anyway, this seems along the right track. Thanks for tackling, and feel free to carry a: Reviewed-by: Brian Norris <briannorris@chromium.org> > + > if (!fb->physical_address) > return -ENODEV; > > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat >
Brian Norris <briannorris@chromium.org> writes: Hello Brian, > Hi Javier, > > On Thu, Sep 12, 2024 at 06:33:58PM +0200, Javier Martinez Canillas wrote: >> 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? > > Julius is more familiar with the Coreboot + payload ecosystem than me, > but his explanations make sense to me, as does this patch. > >> From de1c32017006f4671d91b695f4d6b4e99c073ab2 Mon Sep 17 00:00:00 2001 >> From: Javier Martinez Canillas <javierm@redhat.com> >> 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. > > Normally, these sorts of "early" and "later" ordering descriptions would > set alarm bells when talking about independent drivers. But I suppose > the "early arch" code has better ordering guaranteeds than drivers, so > this should be fine. > Yes, I didn't want to imply ordering here but just mentioning what code was registering a "simple-framebuffer" platform_device, that conflicted with this driver. >> 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: >> [...] >> >> + /* >> + * If the global screen_info data has been filled, the Generic >> + * System Framebuffers (sysfb) will already register a platform > > Did you mean 'platform_device'? > Ups, yeah I forgot to write device there. >> + * 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 > > s/advertise/advertised/ ? > Ok. >> + * in the Coreboot table should only be used if the payload did >> + * not set video mode info and passed it to the Linux kernel. > > s/passed/pass/ > Ok. >> + */ >> + if (si->orig_video_isVGA == VIDEO_TYPE_VLFB || >> + si->orig_video_isVGA == VIDEO_TYPE_EFI) > > This line is using spaces for indentation. It should use a tab, and then > spaces for alignment. But presumably this will change based on Thomas's > suggestions anyway. > Yes, I usually run checkpatch --strict before posting but didn't in this case because just shared the patch as a response. >> + return -EINVAL; > > Is EINVAL right? IIUC, that will print a noisier error to the logs. I > believe the "expected" sorts of return codes are ENODEV or ENXIO. (See > call_driver_probe().) ENODEV seems like a fine choice, similar to > several of the other return codes already used here. > You are right, -ENODEV is indeed a more suitable error code for this. > Anyway, this seems along the right track. Thanks for tackling, and feel > free to carry a: > > Reviewed-by: Brian Norris <briannorris@chromium.org> > Thanks and for your comments.
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c index daadd71d8ddd..3f1b8f664c3f 100644 --- a/drivers/firmware/google/framebuffer-coreboot.c +++ b/drivers/firmware/google/framebuffer-coreboot.c @@ -62,7 +62,8 @@ static int framebuffer_probe(struct coreboot_device *dev) return -EINVAL; pdev = platform_device_register_resndata(&dev->dev, - "simple-framebuffer", 0, + "simple-framebuffer", + PLATFORM_DEVID_AUTO, &res, 1, &pdata, sizeof(pdata)); if (IS_ERR(pdev))