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 platformDid 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 entrys/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.