[PATCH v4 0/2] firmware: Avoid coreboot and sysfb to register a pdev for same framebuffer

[PATCH v4 0/2] firmware: Avoid coreboot and sysfb to register a pdev for same framebuffer

[Javier Martinez Canillas]

Hello,

This is v4 of a fix to prevent both coreboot and sysfb drivers to register
a platform device to setup a system framebuffer. It has been converted to
a series since contains changes for both drivers, to prevent build issues
on architectures that don't define a global struct screen_info.

Patch #1 adds a sysfb_handles_screen_info() helper that can be used by
drivers to check whether sysfb can use the data set in screen_info or not.

Patch #2 makes the framebuffer_coreboot driver to use that helper to know
if has to setup the system framebuffer or delegate that action to sysfb.

I haven't dropped the collected tags from patch #2 due the logic being
basically the same than in v3.

The patches have only been compiled tested because I don't have access to
a coreboot machine. Please let me know if you plan to merge both patches
through the chrome-platforms tree or if you prefer to get merged through
the drm-misc tree.

Best regards,
Javier

Changes in v4:
- New patch to add sysfb_handles_screen_info() helper (Thomas Zimmermann).
- Use a sysfb_handles_screen_info() helper instead of screen_info_video_type()
  to fix build errors on platforms that don't define a struct screen_info
  (Thomas Zimmermann).

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).

Javier Martinez Canillas (2):
  firmware: sysfb: Add a sysfb_handles_screen_info() helper function
  firmware: coreboot: Don't register a pdev if screen_info data is
    present

 .../firmware/google/framebuffer-coreboot.c    | 14 ++++++++++++++
 drivers/firmware/sysfb.c                      | 19 +++++++++++++++++++
 include/linux/sysfb.h                         |  7 +++++++
 3 files changed, 40 insertions(+)

-- 
2.46.0

[PATCH v4 1/2] firmware: sysfb: Add a sysfb_handles_screen_info() helper function

[Javier Martinez Canillas]

That can be used by drivers to check if the Generic System Framebuffers
(sysfb) support can handle the data contained in the global screen_info.

Drivers might need this information to know if have to setup the system
framebuffer, or if they have to delegate this action to sysfb instead.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v4:
- New patch to add sysfb_handles_screen_info() helper (Thomas Zimmermann).

 drivers/firmware/sysfb.c | 19 +++++++++++++++++++
 include/linux/sysfb.h    |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 02a07d3d0d40..770e74be14f3 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -77,6 +77,25 @@ void sysfb_disable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
 
+/**
+ * sysfb_handles_screen_info() - reports if sysfb handles the global screen_info
+ *
+ * Callers can use sysfb_handles_screen_info() to determine whether the Generic
+ * System Framebuffers (sysfb) can handle the global screen_info data structure
+ * or not. Drivers might need this information to know if they have to setup the
+ * system framebuffer, or if they have to delegate this action to sysfb instead.
+ *
+ * Returns:
+ * True if sysfb handles the global screen_info data structure.
+ */
+bool sysfb_handles_screen_info(void)
+{
+	const struct screen_info *si = &screen_info;
+
+	return !!screen_info_video_type(si);
+}
+EXPORT_SYMBOL_GPL(sysfb_handles_screen_info);
+
 #if defined(CONFIG_PCI)
 static bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
 {
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index bef5f06a91de..07cbab516942 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -60,12 +60,19 @@ struct efifb_dmi_info {
 
 void sysfb_disable(struct device *dev);
 
+bool sysfb_handles_screen_info(void);
+
 #else /* CONFIG_SYSFB */
 
 static inline void sysfb_disable(struct device *dev)
 {
 }
 
+static inline bool sysfb_handles_screen_info(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_SYSFB */
 
 #ifdef CONFIG_EFI
-- 
2.46.0

Re: [PATCH v4 1/2] firmware: sysfb: Add a sysfb_handles_screen_info() helper function

[Thomas Zimmermann]

Am 16.09.24 um 13:00 schrieb Javier Martinez Canillas:
> That can be used by drivers to check if the Generic System Framebuffers
> (sysfb) support can handle the data contained in the global screen_info.
>
> Drivers might need this information to know if have to setup the system
> framebuffer, or if they have to delegate this action to sysfb instead.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>
> Changes in v4:
> - New patch to add sysfb_handles_screen_info() helper (Thomas Zimmermann).
>
>   drivers/firmware/sysfb.c | 19 +++++++++++++++++++
>   include/linux/sysfb.h    |  7 +++++++
>   2 files changed, 26 insertions(+)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 02a07d3d0d40..770e74be14f3 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -77,6 +77,25 @@ void sysfb_disable(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(sysfb_disable);
>   
> +/**
> + * sysfb_handles_screen_info() - reports if sysfb handles the global screen_info
> + *
> + * Callers can use sysfb_handles_screen_info() to determine whether the Generic
> + * System Framebuffers (sysfb) can handle the global screen_info data structure
> + * or not. Drivers might need this information to know if they have to setup the
> + * system framebuffer, or if they have to delegate this action to sysfb instead.
> + *
> + * Returns:
> + * True if sysfb handles the global screen_info data structure.
> + */
> +bool sysfb_handles_screen_info(void)
> +{
> +	const struct screen_info *si = &screen_info;
> +
> +	return !!screen_info_video_type(si);
> +}
> +EXPORT_SYMBOL_GPL(sysfb_handles_screen_info);
> +
>   #if defined(CONFIG_PCI)
>   static bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>   {
> diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
> index bef5f06a91de..07cbab516942 100644
> --- a/include/linux/sysfb.h
> +++ b/include/linux/sysfb.h
> @@ -60,12 +60,19 @@ struct efifb_dmi_info {
>   
>   void sysfb_disable(struct device *dev);
>   
> +bool sysfb_handles_screen_info(void);
> +
>   #else /* CONFIG_SYSFB */
>   
>   static inline void sysfb_disable(struct device *dev)
>   {
>   }
>   
> +static inline bool sysfb_handles_screen_info(void)
> +{
> +	return false;
> +}
> +
>   #endif /* CONFIG_SYSFB */
>   
>   #ifdef CONFIG_EFI

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH v4 2/2] firmware: coreboot: Don't register a pdev if screen_info data is present

[Javier Martinez Canillas]

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 v4:
- Use a sysfb_handles_screen_info() helper instead of screen_info_video_type()
  to fix build errors on platforms that don't define a struct screen_info
  (Thomas Zimmermann).

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index daadd71d8ddd..c68c9f56370f 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/sysfb.h>
 
 #include "coreboot_table.h"
 
@@ -36,6 +37,19 @@ 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.
+	 */
+	if (sysfb_handles_screen_info())
+		return -ENODEV;
+
 	if (!fb->physical_address)
 		return -ENODEV;
 
-- 
2.46.0

Re: [PATCH v4 0/2] firmware: Avoid coreboot and sysfb to register a pdev for same framebuffer

[Tzung-Bi Shih]

On Mon, Sep 16, 2024 at 01:00:24PM +0200, Javier Martinez Canillas wrote:
> The patches have only been compiled tested because I don't have access to
> a coreboot machine. Please let me know if you plan to merge both patches
> through the chrome-platforms tree or if you prefer to get merged through
> the drm-misc tree.
> 
> [...]
> Javier Martinez Canillas (2):
>   firmware: sysfb: Add a sysfb_handles_screen_info() helper function
>   firmware: coreboot: Don't register a pdev if screen_info data is
>     present

I'll queue both patches through the chrome-platform tree for v6.13 if there is
no objections.

Re: [PATCH v4 0/2] firmware: Avoid coreboot and sysfb to register a pdev for same framebuffer

[Javier Martinez Canillas]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

Hello Tzung-Bi,

> On Mon, Sep 16, 2024 at 01:00:24PM +0200, Javier Martinez Canillas wrote:
>> The patches have only been compiled tested because I don't have access to
>> a coreboot machine. Please let me know if you plan to merge both patches
>> through the chrome-platforms tree or if you prefer to get merged through
>> the drm-misc tree.
>> 
>> [...]
>> Javier Martinez Canillas (2):
>>   firmware: sysfb: Add a sysfb_handles_screen_info() helper function
>>   firmware: coreboot: Don't register a pdev if screen_info data is
>>     present
>
> I'll queue both patches through the chrome-platform tree for v6.13 if there is
> no objections.
>

That works for me. Thanks a lot.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 0/2] firmware: Avoid coreboot and sysfb to register a pdev for same framebuffer

[Tzung-Bi Shih]

On Mon, Sep 16, 2024 at 01:00:24PM +0200, Javier Martinez Canillas wrote:
> This is v4 of a fix to prevent both coreboot and sysfb drivers to register
> a platform device to setup a system framebuffer. It has been converted to
> a series since contains changes for both drivers, to prevent build issues
> on architectures that don't define a global struct screen_info.
> 
> Patch #1 adds a sysfb_handles_screen_info() helper that can be used by
> drivers to check whether sysfb can use the data set in screen_info or not.
> 
> [...]

Applied to

    https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-firmware-next

[1/2] firmware: sysfb: Add a sysfb_handles_screen_info() helper function
      commit: 6074e905023d09f64f2c896f475820a5623deb2c
[2/2] firmware: coreboot: Don't register a pdev if screen_info data is present
      commit: 67f488dff17e535ac3a8a52b47ff1363d8134983

Thanks!