| Message ID | 20240913213246.1549213-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] firmware: coreboot: Don't register a pdev if screen_info data is present | expand |
Hi Javier,
kernel test robot noticed the following build errors:
[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com
patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present
config: csky-randconfig-002-20240915 (https://download.01.org/0day-ci/archive/20240915/202409150915.n7egvNYa-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409150915.n7egvNYa-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409150915.n7egvNYa-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/usb_debug.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/mxuport.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/symbolserial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_simpleondemand.o
>> ERROR: modpost: "screen_info" [drivers/firmware/google/framebuffer-coreboot.ko] undefined!
Hi Javier, kernel test robot noticed the following build errors: [auto build test ERROR on chrome-platform/for-next] [also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323 base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present config: riscv-randconfig-001-20240915 (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409151528.CIWZRPBq-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: screen_info >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a
On Fri, Sep 13, 2024 at 11:32:29PM +0200, Javier Martinez Canillas wrote: > @@ -27,8 +28,10 @@ static int framebuffer_probe(struct coreboot_device *dev) > int i; > u32 length; > struct lb_framebuffer *fb = &dev->framebuffer; > + const struct screen_info *si = &screen_info; > struct platform_device *pdev; > struct resource res; > + unsigned int type; > struct simplefb_platform_data pdata = { > .width = fb->x_resolution, > .height = fb->y_resolution, > @@ -36,6 +39,20 @@ 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. > + */ > + type = screen_info_video_type(si); > + if (type) > + return -ENODEV; Given that `type` and `si` aren't used in otherwhere, the local variables can be dropped. I haven't had chance to see how to fix the 0-day build errors properly. If you have chance to send the next versions, please drop the local variables.
Hi Am 15.09.24 um 09:44 schrieb kernel test robot: > Hi Javier, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on chrome-platform/for-next] > [also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323 > base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next > patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com > patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present > config: riscv-randconfig-001-20240915 (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/config) > compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409151528.CIWZRPBq-lkp@intel.com/ > > All errors (new ones prefixed by >>): > >>> ld.lld: error: undefined symbol: screen_info > >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) > >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a > >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) > >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a Not all platforms define screen_info. Maybe fix this by following Tzung-Bi's advice of removing the local variables and then guard the test by CONFIG_SYSFB. If SYSFB has been defined, screen_info has to be there. It's not a super pretty solution, though. Best regards Thomas >
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas and Tzung-Bi, > Hi > > Am 15.09.24 um 09:44 schrieb kernel test robot: >> Hi Javier, >> >> kernel test robot noticed the following build errors: >> >> [auto build test ERROR on chrome-platform/for-next] >> [also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch#_base_tree_information] >> >> url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next >> patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com >> patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present >> config: riscv-randconfig-001-20240915 (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/config) >> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) >> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/reproduce) >> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of >> the same patch/commit), kindly add following tags >> | Reported-by: kernel test robot <lkp@intel.com> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202409151528.CIWZRPBq-lkp@intel.com/ >> >> All errors (new ones prefixed by >>): >> >>>> ld.lld: error: undefined symbol: screen_info >> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a > > Not all platforms define screen_info. Maybe fix this by following Yes, after reading the build errors reported by the robot I remembered that we had similar issues with sysfb, for example commit 1260b9a7020 ("drivers/firmware: fix SYSFB depends to prevent build failures") fixed one of those. > Tzung-Bi's advice of removing the local variables and then guard the > test by CONFIG_SYSFB. If SYSFB has been defined, screen_info has to be > there. It's not a super pretty solution, though. > If possible I would prefer to avoid the ifdefery in the driver. I also believe that the local variables makes the code easier to read. But if you folks think that's better to drop them, I can do it in the next rev. Another option is to restrict the architectures where this driver could be build. As far as I understand it is mainly for x86 and ARM64 arches. These two have a screen_info defined so the driver will build correctly. I can include a preparatory patch that adds a "depends on x86 || ARM64". > Best regards > Thomas >
Hi Javier Am 16.09.24 um 10:36 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas and Tzung-Bi, > >> Hi >> >> Am 15.09.24 um 09:44 schrieb kernel test robot: >>> Hi Javier, >>> >>> kernel test robot noticed the following build errors: >>> >>> [auto build test ERROR on chrome-platform/for-next] >>> [also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913] >>> [If your patch is applied to the wrong git tree, kindly drop us a note. >>> And when submitting patch, we suggest to use '--base' as documented in >>> https://git-scm.com/docs/git-format-patch#_base_tree_information] >>> >>> url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323 >>> base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next >>> patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com >>> patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present >>> config: riscv-randconfig-001-20240915 (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/config) >>> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) >>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/reproduce) >>> >>> If you fix the issue in a separate patch/commit (i.e. not just a new version of >>> the same patch/commit), kindly add following tags >>> | Reported-by: kernel test robot <lkp@intel.com> >>> | Closes: https://lore.kernel.org/oe-kbuild-all/202409151528.CIWZRPBq-lkp@intel.com/ >>> >>> All errors (new ones prefixed by >>): >>> >>>>> ld.lld: error: undefined symbol: screen_info >>> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >>> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >> Not all platforms define screen_info. Maybe fix this by following > Yes, after reading the build errors reported by the robot I remembered > that we had similar issues with sysfb, for example commit 1260b9a7020 > ("drivers/firmware: fix SYSFB depends to prevent build failures") fixed > one of those. > >> Tzung-Bi's advice of removing the local variables and then guard the >> test by CONFIG_SYSFB. If SYSFB has been defined, screen_info has to be >> there. It's not a super pretty solution, though. >> > If possible I would prefer to avoid the ifdefery in the driver. I also > believe that the local variables makes the code easier to read. But if > you folks think that's better to drop them, I can do it in the next rev. > > Another option is to restrict the architectures where this driver could > be build. As far as I understand it is mainly for x86 and ARM64 arches. > > These two have a screen_info defined so the driver will build correctly. > I can include a preparatory patch that adds a "depends on x86 || ARM64". That feels arbitrary, as the dependency is not really in coreboot, but in sysbf. What you'd want is a HAVE_SCREEN_INFO define on the architectures that provide it. IIRC earlier attempts to add this have failed. :/ If you don't want the ifdef-ery in coreboot.c, you could add a helper to sysfb. Let's say bool sysfb_handles_screen_info(void) returns the result of the test. If you put it next to sysfb_disable(), you could add an empty wrapper into the sysfb.h header as well. [1] (There's still the possibility that screen_info is available, but sysfb has been disabled. But that's not different from how it currently works.) [1] https://elixir.bootlin.com/linux/v6.10.10/source/include/linux/sysfb.h#L65 Best regards Thomas > >> Best regards >> Thomas >>
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Hi Javier > > Am 16.09.24 um 10:36 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >> Hello Thomas and Tzung-Bi, >> >>> Hi >>> >>> Am 15.09.24 um 09:44 schrieb kernel test robot: >>>> Hi Javier, >>>> >>>> kernel test robot noticed the following build errors: >>>> >>>> [auto build test ERROR on chrome-platform/for-next] >>>> [also build test ERROR on chrome-platform/for-firmware-next linus/master v6.11-rc7 next-20240913] >>>> [If your patch is applied to the wrong git tree, kindly drop us a note. >>>> And when submitting patch, we suggest to use '--base' as documented in >>>> https://git-scm.com/docs/git-format-patch#_base_tree_information] >>>> >>>> url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/firmware-coreboot-Don-t-register-a-pdev-if-screen_info-data-is-present/20240914-053323 >>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next >>>> patch link: https://lore.kernel.org/r/20240913213246.1549213-1-javierm%40redhat.com >>>> patch subject: [PATCH v3] firmware: coreboot: Don't register a pdev if screen_info data is present >>>> config: riscv-randconfig-001-20240915 (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/config) >>>> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) >>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151528.CIWZRPBq-lkp@intel.com/reproduce) >>>> >>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of >>>> the same patch/commit), kindly add following tags >>>> | Reported-by: kernel test robot <lkp@intel.com> >>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202409151528.CIWZRPBq-lkp@intel.com/ >>>> >>>> All errors (new ones prefixed by >>): >>>> >>>>>> ld.lld: error: undefined symbol: screen_info >>>> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>>> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >>>> >>> referenced by framebuffer-coreboot.c:27 (drivers/firmware/google/framebuffer-coreboot.c:27) >>>> >>> drivers/firmware/google/framebuffer-coreboot.o:(framebuffer_probe) in archive vmlinux.a >>> Not all platforms define screen_info. Maybe fix this by following >> Yes, after reading the build errors reported by the robot I remembered >> that we had similar issues with sysfb, for example commit 1260b9a7020 >> ("drivers/firmware: fix SYSFB depends to prevent build failures") fixed >> one of those. >> >>> Tzung-Bi's advice of removing the local variables and then guard the >>> test by CONFIG_SYSFB. If SYSFB has been defined, screen_info has to be >>> there. It's not a super pretty solution, though. >>> >> If possible I would prefer to avoid the ifdefery in the driver. I also >> believe that the local variables makes the code easier to read. But if >> you folks think that's better to drop them, I can do it in the next rev. >> >> Another option is to restrict the architectures where this driver could >> be build. As far as I understand it is mainly for x86 and ARM64 arches. >> >> These two have a screen_info defined so the driver will build correctly. >> I can include a preparatory patch that adds a "depends on x86 || ARM64". > > That feels arbitrary, as the dependency is not really in coreboot, but > in sysbf. What you'd want is a HAVE_SCREEN_INFO define on the > architectures that provide it. IIRC earlier attempts to add this have > failed. :/ > > If you don't want the ifdef-ery in coreboot.c, you could add a helper to > sysfb. Let's say > > bool sysfb_handles_screen_info(void) > > returns the result of the test. If you put it next to sysfb_disable(), > you could add an empty wrapper into the sysfb.h header as well. [1] > > (There's still the possibility that screen_info is available, but sysfb > has been disabled. But that's not different from how it currently works.) > I like that. And when SYSFB is not enabled, sysfb_handles_screen_info() could just be defined as "return false;" which will indicate to drivers that would have to handle the system frambuffer themselves. I'll try to type the patches later today.
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c index daadd71d8ddd..f722292e7684 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,8 +28,10 @@ static int framebuffer_probe(struct coreboot_device *dev) int i; u32 length; struct lb_framebuffer *fb = &dev->framebuffer; + const struct screen_info *si = &screen_info; struct platform_device *pdev; struct resource res; + unsigned int type; struct simplefb_platform_data pdata = { .width = fb->x_resolution, .height = fb->y_resolution, @@ -36,6 +39,20 @@ 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. + */ + type = screen_info_video_type(si); + if (type) + return -ENODEV; + if (!fb->physical_address) return -ENODEV;
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 its own framebuffer information to the Linux kernel. If that's the case, arch x86 boot code will fill the global screen_info data and this used by the Generic System Framebuffers (sysfb) framework, to register a platform device with pdata about the system's framebuffer. But later, the framebuffer_coreboot driver will try to do the same and attempt to register a "simple-framebuffer" platform device (using the information from the coreboot table), which will lead to an error due a device with the same name already being 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 this 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> Closes: 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> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org> --- Changes in v3: - Fix coreboot spelling to be all in lowercase (Julius Werner). Changes in v2: - Declare the struct screen_info as constant variable (Thomas Zimmermann). - Use screen_info_video_type() instead of checking the screen_info video types directly (Thomas Zimmermann). - Fix missing "device" word in a comment (Brian Norris). - Fix some mispellings in a comment (Brian Norris). - Change error code returned from -EINVAL to -ENODEV (Brian Norris). drivers/firmware/google/framebuffer-coreboot.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)