[PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Javier Martinez Canillas]

The Generic System Framebuffers support is built when the COMPILE_TEST
option is enabled. But this wrongly assumes that all the architectures
declare a struct screen_info.

This is true for most architectures, but at least the following do not:
arc, m68k, microblaze, openrisc, parisc and s390.

By attempting to make this compile testeable on all architectures, it
leads to linking errors as reported by the kernel test robot for parisc:

  All errors (new ones prefixed by >>):

     hppa-linux-ld: drivers/firmware/sysfb.o: in function `sysfb_init':
     (.init.text+0x24): undefined reference to `screen_info'
  >> hppa-linux-ld: (.init.text+0x28): undefined reference to `screen_info'

To prevent these errors only allow sysfb to be built on systems that are
going to need it, which are x86 BIOS and EFI.

The EFI Kconfig symbol is used instead of (ARM || ARM64 || RISC) because
some of these architectures only declare a struct screen_info if EFI is
enabled. And also, because the SYSFB code is only used for EFI on these
architectures. For !EFI the "simple-framebuffer" device is registered by
OF when parsing the Device Tree Blob (if a DT node for this was defined).

Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Add a Fixes tag to the changelog.

 drivers/firmware/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index af6719cc576b..897f5f25c64e 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
 config SYSFB
 	bool
 	default y
-	depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
+	depends on X86 || EFI
 
 config SYSFB_SIMPLEFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
-- 
2.31.1

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Daniel Vetter]

On Tue, Jul 27, 2021 at 11:30:15AM +0200, Javier Martinez Canillas wrote:
> The Generic System Framebuffers support is built when the COMPILE_TEST
> option is enabled. But this wrongly assumes that all the architectures
> declare a struct screen_info.
> 
> This is true for most architectures, but at least the following do not:
> arc, m68k, microblaze, openrisc, parisc and s390.
> 
> By attempting to make this compile testeable on all architectures, it
> leads to linking errors as reported by the kernel test robot for parisc:
> 
>   All errors (new ones prefixed by >>):
> 
>      hppa-linux-ld: drivers/firmware/sysfb.o: in function `sysfb_init':
>      (.init.text+0x24): undefined reference to `screen_info'
>   >> hppa-linux-ld: (.init.text+0x28): undefined reference to `screen_info'
> 
> To prevent these errors only allow sysfb to be built on systems that are
> going to need it, which are x86 BIOS and EFI.
> 
> The EFI Kconfig symbol is used instead of (ARM || ARM64 || RISC) because
> some of these architectures only declare a struct screen_info if EFI is
> enabled. And also, because the SYSFB code is only used for EFI on these
> architectures. For !EFI the "simple-framebuffer" device is registered by
> OF when parsing the Device Tree Blob (if a DT node for this was defined).
> 
> Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Whacked onto drm-next so we're welcome again in linux-next.
-Daniel

> ---
> 
> Changes in v2:
> - Add a Fixes tag to the changelog.
> 
>  drivers/firmware/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index af6719cc576b..897f5f25c64e 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>  config SYSFB
>  	bool
>  	default y
> -	depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
> +	depends on X86 || EFI
>  
>  config SYSFB_SIMPLEFB
>  	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Geert Uytterhoeven]

Hi Javier,

On Tue, Jul 27, 2021 at 11:33 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> The Generic System Framebuffers support is built when the COMPILE_TEST
> option is enabled. But this wrongly assumes that all the architectures
> declare a struct screen_info.
>
> This is true for most architectures, but at least the following do not:
> arc, m68k, microblaze, openrisc, parisc and s390.
>
> By attempting to make this compile testeable on all architectures, it
> leads to linking errors as reported by the kernel test robot for parisc:
>
>   All errors (new ones prefixed by >>):
>
>      hppa-linux-ld: drivers/firmware/sysfb.o: in function `sysfb_init':
>      (.init.text+0x24): undefined reference to `screen_info'
>   >> hppa-linux-ld: (.init.text+0x28): undefined reference to `screen_info'
>
> To prevent these errors only allow sysfb to be built on systems that are
> going to need it, which are x86 BIOS and EFI.
>
> The EFI Kconfig symbol is used instead of (ARM || ARM64 || RISC) because
> some of these architectures only declare a struct screen_info if EFI is
> enabled. And also, because the SYSFB code is only used for EFI on these
> architectures. For !EFI the "simple-framebuffer" device is registered by
> OF when parsing the Device Tree Blob (if a DT node for this was defined).
>
> Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for your patch!

> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>  config SYSFB
>         bool
>         default y
> -       depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
> +       depends on X86 || EFI

Thanks, much better.
Still, now this worm is crawling out of the X86 can, I'm wondering
why this option is so important that it has to default to y?
It is not just a dependency for SYSFB_SIMPLEFB, but also causes the
inclusion of drivers/firmware/sysfb.c.

>
>  config SYSFB_SIMPLEFB
>         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Javier Martinez Canillas]

Hello Geert,

On 7/27/21 12:03 PM, Geert Uytterhoeven wrote:

[snip]

> Thanks for your patch!
>

You are welcome.

>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>>  config SYSFB
>>         bool
>>         default y
>> -       depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
>> +       depends on X86 || EFI
> 
> Thanks, much better.
> Still, now this worm is crawling out of the X86 can, I'm wondering
> why this option is so important that it has to default to y?
> It is not just a dependency for SYSFB_SIMPLEFB, but also causes the
> inclusion of drivers/firmware/sysfb.c.
>

It defaults to yes because drivers/firmware/sysfb.c contains the logic
to register a "simple-framebuffer" device (or "efi-framebuffer" if the
CONFIG_SYSFB_SIMPLEFB Kconfig symbol is not enabled).

Not enabling this, would mean that a platform device to match a driver
supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
not be registered. Which will lead to not having an early framebuffer.

The logic used to be in drivers/firmware/efi/efi-init.c, that's built
in if CONFIG_EFI is enabled. We just consolidated both X86 and EFI:

https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8633ef82f101

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Geert Uytterhoeven]

Hi Javier,

On Tue, Jul 27, 2021 at 12:22 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 7/27/21 12:03 PM, Geert Uytterhoeven wrote:
> >> --- a/drivers/firmware/Kconfig
> >> +++ b/drivers/firmware/Kconfig
> >> @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> >>  config SYSFB
> >>         bool
> >>         default y
> >> -       depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
> >> +       depends on X86 || EFI
> >
> > Thanks, much better.
> > Still, now this worm is crawling out of the X86 can, I'm wondering
> > why this option is so important that it has to default to y?
> > It is not just a dependency for SYSFB_SIMPLEFB, but also causes the
> > inclusion of drivers/firmware/sysfb.c.
> >
>
> It defaults to yes because drivers/firmware/sysfb.c contains the logic
> to register a "simple-framebuffer" device (or "efi-framebuffer" if the
> CONFIG_SYSFB_SIMPLEFB Kconfig symbol is not enabled).
>
> Not enabling this, would mean that a platform device to match a driver
> supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
> not be registered. Which will lead to not having an early framebuffer.

Do all (embedded) EFI systems have a frame buffer?

Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA,
and FB_EFI?

> The logic used to be in drivers/firmware/efi/efi-init.c, that's built
> in if CONFIG_EFI is enabled. We just consolidated both X86 and EFI:
>
> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8633ef82f101

Thanks, I'm aware of that commit, as I was just about to reply to it,
when I saw the patch is this thread ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Javier Martinez Canillas]

On 7/27/21 1:17 PM, Geert Uytterhoeven wrote:

[snip]

>> Not enabling this, would mean that a platform device to match a driver
>> supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
>> not be registered. Which will lead to not having an early framebuffer.
> 
> Do all (embedded) EFI systems have a frame buffer?
>

That's a good question. I don't know if all EFI firmwares are expected
to provide a GOP or not. But even the u-boot EFI stub provides one, if
video output is supported by u-boot on that system.
 
> Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA,
> and FB_EFI?
> 

It's another option, yes. I just thought that the use of select was not
encouraged and using depends was less fragile / error prone.

>> The logic used to be in drivers/firmware/efi/efi-init.c, that's built
>> in if CONFIG_EFI is enabled. We just consolidated both X86 and EFI:
>>
>> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8633ef82f101
> 
> Thanks, I'm aware of that commit, as I was just about to reply to it,
> when I saw the patch is this thread ;-)
>

Ok :)
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Geert Uytterhoeven]

Hi Javier,

On Tue, Jul 27, 2021 at 1:32 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 7/27/21 1:17 PM, Geert Uytterhoeven wrote:
> >> Not enabling this, would mean that a platform device to match a driver
> >> supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
> >> not be registered. Which will lead to not having an early framebuffer.
> >
> > Do all (embedded) EFI systems have a frame buffer?
>
> That's a good question. I don't know if all EFI firmwares are expected
> to provide a GOP or not. But even the u-boot EFI stub provides one, if
> video output is supported by u-boot on that system.
>
> > Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA,
> > and FB_EFI?
>
> It's another option, yes. I just thought that the use of select was not
> encouraged and using depends was less fragile / error prone.

Select is very useful for config symbols that are invisible to the user (i.e.
cannot be enabled/disabled manually).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Tue, Jul 27, 2021 at 01:32:29PM +0200, Javier Martinez Canillas wrote:
> On 7/27/21 1:17 PM, Geert Uytterhoeven wrote:

> > Do all (embedded) EFI systems have a frame buffer?

> That's a good question. I don't know if all EFI firmwares are expected
> to provide a GOP or not. But even the u-boot EFI stub provides one, if
> video output is supported by u-boot on that system.

No, GOP is only mandatory for systems with a graphical console device
even assuming things fully conform to the spec.  Indeed I've got a
non-embedded EFI based system sitting next to my desk here which only
has a serial console, never mind anything embedded.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures

[Javier Martinez Canillas]

Hello Geert,

On 7/27/21 1:39 PM, Geert Uytterhoeven wrote:

[snip]

>>> Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA,
>>> and FB_EFI?
>>
>> It's another option, yes. I just thought that the use of select was not
>> encouraged and using depends was less fragile / error prone.
> 
> Select is very useful for config symbols that are invisible to the user (i.e.
> cannot be enabled/disabled manually).
> 

Got it. I don't have a strong opinion on this really. In fact, the first
version of the patch did use select for the arches but I got as feedback
that should use depends instead:

https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg351961.html

Granted, that was for the arches but you are proposing to do it for the
drivers that match against the platform devices registered by sysfb. So
it does make more sense to what I did in v1.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat