[PATCH v6 0/5] Fix some races between sysfb device registration and drivers probe

[PATCH v6 0/5] Fix some races between sysfb device registration and drivers probe

[Javier Martinez Canillas]

Hello,

The patches in this series contain mostly changes suggested by Daniel Vetter
Thomas Zimmermann. They aim to fix existing races between the Generic System
Framebuffer (sysfb) infrastructure and the fbdev and DRM device registration.

For example, it is currently possible for sysfb to register a platform
device after a real DRM driver was registered and requested to remove the
conflicting framebuffers. Or is possible for a simple{fb,drm} to match with
a device previously registered by sysfb, even after a real driver is present.

A symptom of this issue, was worked around with the commit fb561bf9abde
("fbdev: Prevent probing generic drivers if a FB is already registered")
but that's really a hack and should be reverted instead.

This series attempt to fix it more correctly and revert the mentioned hack.
That will also allow to make the num_registered_fb variable not visible to
drivers anymore, since that's internal to fbdev core.

Pach 1 is just a simple cleanup in preparation for later patches.

Patch 2 add a sysfb_disable() helper to allow disabling sysfb and unregister
devices registered by sysfb.

Patch 3 fixes the race that exists between sysfb devices registration and
fbdev framebuffer devices registration, by disabling the sysfb when a DRM
or fbdev driver requests to remove conflicting framebuffers.

Patch 4 is the revert patch that was posted by Daniel before but dropped
from his set and finally patch 5 is the one that makes num_registered_fb
private to fbmem.c, to not allow drivers to use it anymore.

The patches were tested on a rpi4 with the vc4, simpledrm and simplefb
drivers, using different combinations of built-in and as a module.

Best regards,
Javier

Changes in v6:
- Drop sysfb_try_unregister() helper since is no longer needed.
- Move the sysfb_disable() before the remove conflicting framebuffers
  loop (Daniel Vetter).
- Drop patch "fbdev: Make sysfb to unregister its own registered devices"
  since was no longer needed.

Changes in v5:
- Move the sysfb_disable() call at conflicting framebuffers again to
  avoid the need of a DRIVER_FIRMWARE capability flag.
- Add Daniel Vetter's Reviewed-by tag again since reverted to the old
  patch that he already reviewed in v2.
- Drop patches that added a DRM_FIRMWARE capability and use them
  since the case those prevented could be ignored (Daniel Vetter).

Changes in v4:
- Make sysfb_disable() to also attempt to unregister a device.
- Add patch to make registered_fb[] private.
- Add patches that introduce the DRM_FIRMWARE capability and usage.

Changes in v3:
- Add Thomas Zimmermann's Reviewed-by tag to patch #1.
- Call sysfb_disable() when a DRM dev and a fbdev are registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Call sysfb_disable() when a fbdev framebuffer is registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Drop Daniel Vetter's Reviewed-by tag since patch changed a lot.
- Rebase on top of latest drm-misc-next branch.

Changes in v2:
- Rebase on top of latest drm-misc-next and fix conflicts (Daniel Vetter).
- Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter).
- Explain in the commit message that fbmem has to unregister the device
  as fallback if a driver registered the device itself (Daniel Vetter).
- Also explain that fallback in a comment in the code (Daniel Vetter).
- Don't encode in fbmem the assumption that sysfb will always register
  platform devices (Daniel Vetter).
- Add a FIXME comment about drivers registering devices (Daniel Vetter).
- Drop RFC prefix since patches were already reviewed by Daniel Vetter.
- Add Daniel Reviewed-by tags to the patches.

Daniel Vetter (2):
  Revert "fbdev: Prevent probing generic drivers if a FB is already
    registered"
  fbdev: Make registered_fb[] private to fbmem.c

Javier Martinez Canillas (3):
  firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer
  firmware: sysfb: Add sysfb_disable() helper function
  fbdev: Disable sysfb device registration when removing conflicting FBs

 .../driver-api/firmware/other_interfaces.rst  |  6 ++
 drivers/firmware/sysfb.c                      | 58 ++++++++++++++++---
 drivers/firmware/sysfb_simplefb.c             | 16 ++---
 drivers/video/fbdev/core/fbmem.c              | 20 ++++++-
 drivers/video/fbdev/efifb.c                   | 11 ----
 drivers/video/fbdev/simplefb.c                | 11 ----
 include/linux/fb.h                            |  7 +--
 include/linux/sysfb.h                         | 23 ++++++--
 8 files changed, 103 insertions(+), 49 deletions(-)

-- 
2.36.1

[PATCH v6 1/5] firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer

[Javier Martinez Canillas]

This function just returned 0 on success or an errno code on error, but it
could be useful for sysfb_init() callers to have a pointer to the device.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

---

(no changes since v3)

Changes in v3:
- Add Thomas Zimmermann's Reviewed-by tag to patch #1.

Changes in v2:
- Rebase on top of latest drm-misc-next and fix conflicts (Daniel Vetter).

 drivers/firmware/sysfb.c          |  4 ++--
 drivers/firmware/sysfb_simplefb.c | 16 ++++++++--------
 include/linux/sysfb.h             | 10 +++++-----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 2bfbb05f7d89..b032f40a92de 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -46,8 +46,8 @@ static __init int sysfb_init(void)
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);
 	if (compatible) {
-		ret = sysfb_create_simplefb(si, &mode);
-		if (!ret)
+		pd = sysfb_create_simplefb(si, &mode);
+		if (!IS_ERR(pd))
 			return 0;
 	}
 
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index bda8712bfd8c..a353e27f83f5 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -57,8 +57,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 	return false;
 }
 
-__init int sysfb_create_simplefb(const struct screen_info *si,
-				 const struct simplefb_platform_data *mode)
+__init struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
+						     const struct simplefb_platform_data *mode)
 {
 	struct platform_device *pd;
 	struct resource res;
@@ -76,7 +76,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si,
 		base |= (u64)si->ext_lfb_base << 32;
 	if (!base || (u64)(resource_size_t)base != base) {
 		printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	/*
@@ -93,7 +93,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si,
 	length = mode->height * mode->stride;
 	if (length > size) {
 		printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 	length = PAGE_ALIGN(length);
 
@@ -104,11 +104,11 @@ __init int sysfb_create_simplefb(const struct screen_info *si,
 	res.start = base;
 	res.end = res.start + length - 1;
 	if (res.end <= res.start)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	pd = platform_device_alloc("simple-framebuffer", 0);
 	if (!pd)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	sysfb_apply_efi_quirks(pd);
 
@@ -124,10 +124,10 @@ __init int sysfb_create_simplefb(const struct screen_info *si,
 	if (ret)
 		goto err_put_device;
 
-	return 0;
+	return pd;
 
 err_put_device:
 	platform_device_put(pd);
 
-	return ret;
+	return ERR_PTR(ret);
 }
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index b0dcfa26d07b..708152e9037b 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -72,8 +72,8 @@ static inline void sysfb_apply_efi_quirks(struct platform_device *pd)
 
 bool sysfb_parse_mode(const struct screen_info *si,
 		      struct simplefb_platform_data *mode);
-int sysfb_create_simplefb(const struct screen_info *si,
-			  const struct simplefb_platform_data *mode);
+struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
+					      const struct simplefb_platform_data *mode);
 
 #else /* CONFIG_SYSFB_SIMPLE */
 
@@ -83,10 +83,10 @@ static inline bool sysfb_parse_mode(const struct screen_info *si,
 	return false;
 }
 
-static inline int sysfb_create_simplefb(const struct screen_info *si,
-					 const struct simplefb_platform_data *mode)
+static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
+							    const struct simplefb_platform_data *mode)
 {
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 #endif /* CONFIG_SYSFB_SIMPLE */
-- 
2.36.1

[PATCH v6 2/5] firmware: sysfb: Add sysfb_disable() helper function

[Javier Martinez Canillas]

This can be used by subsystems to unregister a platform device registered
by sysfb and also to disable future platform device registration in sysfb.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes in v6:
- Drop sysfb_try_unregister() helper since is no longer needed.

Changes in v4:
- Make sysfb_disable() to also attempt to unregister a device.

Changes in v2:
- Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter).

 .../driver-api/firmware/other_interfaces.rst  |  6 +++
 drivers/firmware/sysfb.c                      | 54 ++++++++++++++++---
 include/linux/sysfb.h                         | 13 +++++
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst
index b81794e0cfbb..06ac89adaafb 100644
--- a/Documentation/driver-api/firmware/other_interfaces.rst
+++ b/Documentation/driver-api/firmware/other_interfaces.rst
@@ -13,6 +13,12 @@ EDD Interfaces
 .. kernel-doc:: drivers/firmware/edd.c
    :internal:
 
+Generic System Framebuffers Interface
+-------------------------------------
+
+.. kernel-doc:: drivers/firmware/sysfb.c
+   :export:
+
 Intel Stratix10 SoC Service Layer
 ---------------------------------
 Some features of the Intel Stratix10 SoC require a level of privilege
diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index b032f40a92de..1f276f108cc9 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -34,21 +34,59 @@
 #include <linux/screen_info.h>
 #include <linux/sysfb.h>
 
+static struct platform_device *pd;
+static DEFINE_MUTEX(disable_lock);
+static bool disabled;
+
+static bool sysfb_unregister(void)
+{
+	if (IS_ERR_OR_NULL(pd))
+		return false;
+
+	platform_device_unregister(pd);
+	pd = NULL;
+
+	return true;
+}
+
+/**
+ * sysfb_disable() - disable the Generic System Framebuffers support
+ *
+ * This disables the registration of system framebuffer devices that match the
+ * generic drivers that make use of the system framebuffer set up by firmware.
+ *
+ * It also unregisters a device if this was already registered by sysfb_init().
+ *
+ * Context: The function can sleep. A @disable_lock mutex is acquired to serialize
+ *          against sysfb_init(), that registers a system framebuffer device.
+ */
+void sysfb_disable(void)
+{
+	mutex_lock(&disable_lock);
+	sysfb_unregister();
+	disabled = true;
+	mutex_unlock(&disable_lock);
+}
+EXPORT_SYMBOL_GPL(sysfb_disable);
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
 	struct simplefb_platform_data mode;
-	struct platform_device *pd;
 	const char *name;
 	bool compatible;
-	int ret;
+	int ret = 0;
+
+	mutex_lock(&disable_lock);
+	if (disabled)
+		goto unlock_mutex;
 
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);
 	if (compatible) {
 		pd = sysfb_create_simplefb(si, &mode);
 		if (!IS_ERR(pd))
-			return 0;
+			goto unlock_mutex;
 	}
 
 	/* if the FB is incompatible, create a legacy framebuffer device */
@@ -60,8 +98,10 @@ static __init int sysfb_init(void)
 		name = "platform-framebuffer";
 
 	pd = platform_device_alloc(name, 0);
-	if (!pd)
-		return -ENOMEM;
+	if (!pd) {
+		ret = -ENOMEM;
+		goto unlock_mutex;
+	}
 
 	sysfb_apply_efi_quirks(pd);
 
@@ -73,9 +113,11 @@ static __init int sysfb_init(void)
 	if (ret)
 		goto err;
 
-	return 0;
+	goto unlock_mutex;
 err:
 	platform_device_put(pd);
+unlock_mutex:
+	mutex_unlock(&disable_lock);
 	return ret;
 }
 
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index 708152e9037b..e9baee4ae361 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -55,6 +55,19 @@ struct efifb_dmi_info {
 	int flags;
 };
 
+#ifdef CONFIG_SYSFB
+
+void sysfb_disable(void);
+
+#else /* CONFIG_SYSFB */
+
+static inline void sysfb_disable(void)
+{
+
+}
+
+#endif /* CONFIG_SYSFB */
+
 #ifdef CONFIG_EFI
 
 extern struct efifb_dmi_info efifb_dmi_list[];
-- 
2.36.1

[PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

The platform devices registered by sysfb match with firmware-based DRM or
fbdev drivers, that are used to have early graphics using a framebuffer
provided by the system firmware.

DRM or fbdev drivers later are probed and remove all conflicting framebuffers,
leading to these platform devices for generic drivers to be unregistered.

But the current solution has a race, since the sysfb_init() function could
be called after a DRM or fbdev driver is probed and request to unregister
the devices for drivers with conflicting framebuffes.

To prevent this, disable any future sysfb platform device registration by
calling sysfb_disable(), if a driver requests to remove the conflicting
framebuffers.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes in v6:
- Move the sysfb_disable() before the remove conflicting framebuffers
  loop (Daniel Vetter).

Changes in v5:
- Move the sysfb_disable() call at conflicting framebuffers again to
  avoid the need of a DRIVER_FIRMWARE capability flag.
- Add Daniel Vetter's Reviewed-by tag again since reverted to the old
  patch that he already reviewed in v2.

Changes in v3:
- Call sysfb_disable() when a DRM dev and a fbdev are registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Call sysfb_disable() when a fbdev framebuffer is registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Drop Daniel Vetter's Reviewed-by tag since patch changed a lot.

Changes in v2:
- Explain in the commit message that fbmem has to unregister the device
  as fallback if a driver registered the device itself (Daniel Vetter).
- Also explain that fallback in a comment in the code (Daniel Vetter).
- Don't encode in fbmem the assumption that sysfb will always register
  platform devices (Daniel Vetter).
- Add a FIXME comment about drivers registering devices (Daniel Vetter).

 drivers/video/fbdev/core/fbmem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2fda5917c212..e0720fef0ee6 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/slab.h>
+#include <linux/sysfb.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/vt.h>
@@ -1764,6 +1765,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
 		do_free = true;
 	}
 
+	/*
+	 * If a driver asked to unregister a platform device registered by
+	 * sysfb, then can be assumed that this is a driver for a display
+	 * that is set up by the system firmware and has a generic driver.
+	 *
+	 * Drivers for devices that don't have a generic driver will never
+	 * ask for this, so let's assume that a real driver for the display
+	 * was already probed and prevent sysfb to register devices later.
+	 */
+	sysfb_disable();
+
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);
-- 
2.36.1

[PATCH v6 4/5] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"

[Javier Martinez Canillas]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.

With

commit 27599aacbaefcbf2af7b06b0029459bbf682000d
Author: Thomas Zimmermann <tzimmermann@suse.de>
Date:   Tue Jan 25 10:12:18 2022 +0100

    fbdev: Hot-unplug firmware fb devices on forced removal

this should be fixed properly and we can remove this somewhat hackish
check here (e.g. this won't catch drm drivers if fbdev emulation isn't
enabled).

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Zack Rusin <zackr@vmware.com>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Zack Rusin <zackr@vmware.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Ilya Trukhanov <lahvuun@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Cc: Peter Jones <pjones@redhat.com>
Cc: linux-fbdev@vger.kernel.org

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/video/fbdev/efifb.c    | 11 -----------
 drivers/video/fbdev/simplefb.c | 11 -----------
 2 files changed, 22 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..edca3703b964 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -351,17 +351,6 @@ static int efifb_probe(struct platform_device *dev)
 	char *option = NULL;
 	efi_memory_desc_t md;
 
-	/*
-	 * Generic drivers must not be registered if a framebuffer exists.
-	 * If a native driver was probed, the display hardware was already
-	 * taken and attempting to use the system framebuffer is dangerous.
-	 */
-	if (num_registered_fb > 0) {
-		dev_err(&dev->dev,
-			"efifb: a framebuffer is already registered\n");
-		return -EINVAL;
-	}
-
 	if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
 		return -ENODEV;
 
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..0ef41173325a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -413,17 +413,6 @@ static int simplefb_probe(struct platform_device *pdev)
 	struct simplefb_par *par;
 	struct resource *res, *mem;
 
-	/*
-	 * Generic drivers must not be registered if a framebuffer exists.
-	 * If a native driver was probed, the display hardware was already
-	 * taken and attempting to use the system framebuffer is dangerous.
-	 */
-	if (num_registered_fb > 0) {
-		dev_err(&pdev->dev,
-			"simplefb: a framebuffer is already registered\n");
-		return -EINVAL;
-	}
-
 	if (fb_get_options("simplefb", NULL))
 		return -ENODEV;
 
-- 
2.36.1

[PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Javier Martinez Canillas]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

Well except when the olpc dcon fbdev driver is enabled, that thing
digs around in there in rather unfixable ways.

Cc oldc_dcon maintainers as fyi.

v2: I typoed the config name (0day)

Cc: kernel test robot <lkp@intel.com>
Cc: Jens Frederich <jfrederich@gmail.com>
Cc: Jon Nettleton <jon.nettleton@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-staging@lists.linux.dev
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Helge Deller <deller@gmx.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Cc: linux-fbdev@vger.kernel.org
Cc: Zheyu Ma <zheyuma97@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/video/fbdev/core/fbmem.c | 8 ++++++--
 include/linux/fb.h               | 7 +++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index e0720fef0ee6..bdb08b665b43 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -50,10 +50,14 @@
 static DEFINE_MUTEX(registration_lock);
 
 struct fb_info *registered_fb[FB_MAX] __read_mostly;
-EXPORT_SYMBOL(registered_fb);
-
 int num_registered_fb __read_mostly;
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
+EXPORT_SYMBOL(registered_fb);
 EXPORT_SYMBOL(num_registered_fb);
+#endif
+#define for_each_registered_fb(i)		\
+	for (i = 0; i < FB_MAX; i++)		\
+		if (!registered_fb[i]) {} else
 
 bool fb_center_logo __read_mostly;
 
diff --git a/include/linux/fb.h b/include/linux/fb.h
index bbe1e4571899..c563e24b6293 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -632,16 +632,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
 extern int fb_get_options(const char *name, char **option);
 extern int fb_new_modelist(struct fb_info *info);
 
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
 extern struct fb_info *registered_fb[FB_MAX];
+
 extern int num_registered_fb;
+#endif
 extern bool fb_center_logo;
 extern int fb_logo_count;
 extern struct class *fb_class;
 
-#define for_each_registered_fb(i)		\
-	for (i = 0; i < FB_MAX; i++)		\
-		if (!registered_fb[i]) {} else
-
 static inline void lock_fb_info(struct fb_info *info)
 {
 	mutex_lock(&info->lock);
-- 
2.36.1

Re: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4447 bytes --]

Hi Javier

Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Well except when the olpc dcon fbdev driver is enabled, that thing
> digs around in there in rather unfixable ways.

There is fb_client_register() to set up a 'client' on top of an fbdev. 
The client would then get messages about modesetting, blanks, removals, 
etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
itself is somewhat unloved these days.

Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
even better, build a nicer interface for dcon.

The dcon driver appears to look only at the first entry. Maybe add 
fb_info_get_by_index() and fb_info_put() and export those. They would be 
trivial wrappers somewhere in fbmem.c:

#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
struct fb_info *fb_info_get_by_index(unsigned int index)
{
	return get_fb_info(index);
}
EXPORT_SYMBOL()
void fb_info_put(struct fb_info *fb_info)
{
	put_fb_info(fb_info);
}
EXPORT_SYMBOL()
#endif

In dcon itself, using the new interfaces will actually acquire a 
reference to keep the display alive. The code at [1] could be replaced. 
And a call to fb_info_put() needs to go into dcon_remove(). [2]

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.18.2/source/drivers/staging/olpc_dcon/olpc_dcon.c#L605
[2] 
https://elixir.bootlin.com/linux/v5.18.2/source/drivers/staging/olpc_dcon/olpc_dcon.c#L688

> 
> Cc oldc_dcon maintainers as fyi.
> 
> v2: I typoed the config name (0day)
> 
> Cc: kernel test robot <lkp@intel.com>
> Cc: Jens Frederich <jfrederich@gmail.com>
> Cc: Jon Nettleton <jon.nettleton@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-staging@lists.linux.dev
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Cc: Zhen Lei <thunder.leizhen@huawei.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Zheyu Ma <zheyuma97@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/video/fbdev/core/fbmem.c | 8 ++++++--
>   include/linux/fb.h               | 7 +++----
>   2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index e0720fef0ee6..bdb08b665b43 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -50,10 +50,14 @@
>   static DEFINE_MUTEX(registration_lock);
>   
>   struct fb_info *registered_fb[FB_MAX] __read_mostly;
> -EXPORT_SYMBOL(registered_fb);
> -
>   int num_registered_fb __read_mostly;
> +#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> +EXPORT_SYMBOL(registered_fb);
>   EXPORT_SYMBOL(num_registered_fb);
> +#endif
> +#define for_each_registered_fb(i)		\
> +	for (i = 0; i < FB_MAX; i++)		\
> +		if (!registered_fb[i]) {} else
>   
>   bool fb_center_logo __read_mostly;
>   
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index bbe1e4571899..c563e24b6293 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -632,16 +632,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
>   extern int fb_get_options(const char *name, char **option);
>   extern int fb_new_modelist(struct fb_info *info);
>   
> +#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
>   extern struct fb_info *registered_fb[FB_MAX];
> +
>   extern int num_registered_fb;
> +#endif
>   extern bool fb_center_logo;
>   extern int fb_logo_count;
>   extern struct class *fb_class;
>   
> -#define for_each_registered_fb(i)		\
> -	for (i = 0; i < FB_MAX; i++)		\
> -		if (!registered_fb[i]) {} else
> -
>   static inline void lock_fb_info(struct fb_info *info)
>   {
>   	mutex_lock(&info->lock);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Javier Martinez Canillas]

Hello Thomas,

On 6/9/22 13:49, Thomas Zimmermann wrote:
> Hi Javier
> 
> Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Well except when the olpc dcon fbdev driver is enabled, that thing
>> digs around in there in rather unfixable ways.
> 
> There is fb_client_register() to set up a 'client' on top of an fbdev. 
> The client would then get messages about modesetting, blanks, removals, 
> etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
> itself is somewhat unloved these days.
> 
> Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
> even better, build a nicer interface for dcon.
> 
> The dcon driver appears to look only at the first entry. Maybe add 
> fb_info_get_by_index() and fb_info_put() and export those. They would be 
> trivial wrappers somewhere in fbmem.c:
> 
> #if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> struct fb_info *fb_info_get_by_index(unsigned int index)
> {
> 	return get_fb_info(index);
> }
> EXPORT_SYMBOL()
> void fb_info_put(struct fb_info *fb_info)
> {
> 	put_fb_info(fb_info);
> }
> EXPORT_SYMBOL()
> #endif
> 
> In dcon itself, using the new interfaces will actually acquire a 
> reference to keep the display alive. The code at [1] could be replaced. 
> And a call to fb_info_put() needs to go into dcon_remove(). [2]
> 

Thanks for your suggestions, that makes sense to me. I'll drop this
patch from the set and post as a follow-up a different approach as
you suggested.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 0/5] Fix some races between sysfb device registration and drivers probe

[Javier Martinez Canillas]

On 6/7/22 20:23, Javier Martinez Canillas wrote:
> The patches in this series contain mostly changes suggested by Daniel Vetter
> Thomas Zimmermann. They aim to fix existing races between the Generic System
> Framebuffer (sysfb) infrastructure and the fbdev and DRM device registration.
> 
> For example, it is currently possible for sysfb to register a platform
> device after a real DRM driver was registered and requested to remove the
> conflicting framebuffers. Or is possible for a simple{fb,drm} to match with
> a device previously registered by sysfb, even after a real driver is present.
> 
> A symptom of this issue, was worked around with the commit fb561bf9abde
> ("fbdev: Prevent probing generic drivers if a FB is already registered")
> but that's really a hack and should be reverted instead.
> 
> This series attempt to fix it more correctly and revert the mentioned hack.
> That will also allow to make the num_registered_fb variable not visible to
> drivers anymore, since that's internal to fbdev core.
> 

Pushed patches 1-4 to drm-misc (drm-misc-next). Thanks!

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Mark olpc_dcon BROKEN [Was: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c]

[Sam Ravnborg]

Hi Javier.

On Thu, Jun 09, 2022 at 03:09:21PM +0200, Javier Martinez Canillas wrote:
> Hello Thomas,
> 
> On 6/9/22 13:49, Thomas Zimmermann wrote:
> > Hi Javier
> > 
> > Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
> >> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Well except when the olpc dcon fbdev driver is enabled, that thing
> >> digs around in there in rather unfixable ways.
> > 
> > There is fb_client_register() to set up a 'client' on top of an fbdev. 
> > The client would then get messages about modesetting, blanks, removals, 
> > etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
> > itself is somewhat unloved these days.
> > 
> > Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
> > even better, build a nicer interface for dcon.
> > 
> > The dcon driver appears to look only at the first entry. Maybe add 
> > fb_info_get_by_index() and fb_info_put() and export those. They would be 
> > trivial wrappers somewhere in fbmem.c:
> > 
> > #if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> > struct fb_info *fb_info_get_by_index(unsigned int index)
> > {
> > 	return get_fb_info(index);
> > }
> > EXPORT_SYMBOL()
> > void fb_info_put(struct fb_info *fb_info)
> > {
> > 	put_fb_info(fb_info);
> > }
> > EXPORT_SYMBOL()
> > #endif
> > 
> > In dcon itself, using the new interfaces will actually acquire a 
> > reference to keep the display alive. The code at [1] could be replaced. 
> > And a call to fb_info_put() needs to go into dcon_remove(). [2]
> > 
> 
> Thanks for your suggestions, that makes sense to me. I'll drop this
> patch from the set and post as a follow-up a different approach as
> you suggested.

To repeat myself from irc.
olpc_dcon is a staging driver and we should avoid inventing anything in
core code for to make staging drivers works.
Geert suggested EXPORT_SYMPBOL_NS_GPL() that could work and narrow it
down to olpc_dcon.
The better approach is to mark said driver BROKEN and then someone can
fix it it there is anyone who cares.
Last commit to olpc_dcon was in 2019: e40219d5e4b2177bfd4d885e7b64e3b236af40ac
and maybe Jerry Lin cares enough to fix it.

Added Jerry and Greg to the mail.

	Sam

Re: Mark olpc_dcon BROKEN [Was: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c]

[Javier Martinez Canillas]

Hello Sam,

On 6/9/22 19:23, Sam Ravnborg wrote:

[snip]

> 
> To repeat myself from irc.
> olpc_dcon is a staging driver and we should avoid inventing anything in
> core code for to make staging drivers works.
> Geert suggested EXPORT_SYMPBOL_NS_GPL() that could work and narrow it
> down to olpc_dcon.
> The better approach is to mark said driver BROKEN and then someone can
> fix it it there is anyone who cares.
> Last commit to olpc_dcon was in 2019: e40219d5e4b2177bfd4d885e7b64e3b236af40ac
> and maybe Jerry Lin cares enough to fix it.
> 
> Added Jerry and Greg to the mail.
> 
> 	Sam
> 

That does sound like the best approach indeed. And if the driver is kept
BROKEN for a few releases then it can just remove it from the kernel. If
someone still uses/cares about the driver, they can fix it as you said,
and it could even be ported to DRM if is something that's still useful.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Zack Rusin]

On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
> The platform devices registered by sysfb match with firmware-based DRM or
> fbdev drivers, that are used to have early graphics using a framebuffer
> provided by the system firmware.
> 
> DRM or fbdev drivers later are probed and remove all conflicting framebuffers,
> leading to these platform devices for generic drivers to be unregistered.
> 
> But the current solution has a race, since the sysfb_init() function could
> be called after a DRM or fbdev driver is probed and request to unregister
> the devices for drivers with conflicting framebuffes.
> 
> To prevent this, disable any future sysfb platform device registration by
> calling sysfb_disable(), if a driver requests to remove the conflicting
> framebuffers.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> 
> Changes in v6:
> - Move the sysfb_disable() before the remove conflicting framebuffers
>   loop (Daniel Vetter).
> 
> Changes in v5:
> - Move the sysfb_disable() call at conflicting framebuffers again to
>   avoid the need of a DRIVER_FIRMWARE capability flag.
> - Add Daniel Vetter's Reviewed-by tag again since reverted to the old
>   patch that he already reviewed in v2.
> 
> Changes in v3:
> - Call sysfb_disable() when a DRM dev and a fbdev are registered rather
>   than when conflicting framebuffers are removed (Thomas Zimmermann).
> - Call sysfb_disable() when a fbdev framebuffer is registered rather
>   than when conflicting framebuffers are removed (Thomas Zimmermann).
> - Drop Daniel Vetter's Reviewed-by tag since patch changed a lot.
> 
> Changes in v2:
> - Explain in the commit message that fbmem has to unregister the device
>   as fallback if a driver registered the device itself (Daniel Vetter).
> - Also explain that fallback in a comment in the code (Daniel Vetter).
> - Don't encode in fbmem the assumption that sysfb will always register
>   platform devices (Daniel Vetter).
> - Add a FIXME comment about drivers registering devices (Daniel Vetter).
> 
>  drivers/video/fbdev/core/fbmem.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 2fda5917c212..e0720fef0ee6 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/major.h>
>  #include <linux/slab.h>
> +#include <linux/sysfb.h>
>  #include <linux/mm.h>
>  #include <linux/mman.h>
>  #include <linux/vt.h>
> @@ -1764,6 +1765,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
>  		do_free = true;
>  	}
>  
> +	/*
> +	 * If a driver asked to unregister a platform device registered by
> +	 * sysfb, then can be assumed that this is a driver for a display
> +	 * that is set up by the system firmware and has a generic driver.
> +	 *
> +	 * Drivers for devices that don't have a generic driver will never
> +	 * ask for this, so let's assume that a real driver for the display
> +	 * was already probed and prevent sysfb to register devices later.
> +	 */
> +	sysfb_disable();
> +
>  	mutex_lock(&registration_lock);
>  	do_remove_conflicting_framebuffers(a, name, primary);
>  	mutex_unlock(&registration_lock);

Hi, Javier.

This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
you'd like .config or just have us test something directly for you):


 Unable to handle kernel NULL pointer dereference at virtual address
0000000000000008
 Mem abort info:
   ESR = 0x96000004
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x04: level 0 translation fault
 Data abort info:
   ISV = 0, ISS = 0x00000004
   CM = 0, WnR = 0
 user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
 [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
 Internal error: Oops: 96000004 [#1] SMP
 Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
 CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
#12
 Hardware name: VMware, Inc. VBSA/VBSA, BIOS VEFI 12/31/2020
 pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : kernfs_find_and_get_ns+0x2c/0x80
 lr : sysfs_unmerge_group+0x30/0x80
 sp : ffff80000b78b3f0
 x29: ffff80000b78b3f0 x28: ffff0000f7970910 x27: 0000000000000002
 x26: ffff0000f7970900 x25: ffff80000abca358 x24: ffff80000936cfa0
 x23: ffff80000ac8f458 x22: 0000000000000000 x21: ffff800009431938
 x20: ffff800009431800 x19: 0000000000000000 x18: 0000000000000000
 x17: 42555300302e7265 x16: 66667562656d6172 x15: 4d006d726f667461
 x14: 6c703d4d45545359 x13: 595342555300302e x12: 726566667562656d
 x11: 00323137323d4d55 x10: 4e51455300726566 x9 : ffff8000085f7400
 x8 : ffff0000f7970980 x7 : 0000000000000000 x6 : 0000000077ffffff
 x5 : 0000000070000000 x4 : ffff80000b78b548 x3 : ffff8000088b0420
 x2 : 0000000000000000 x1 : ffff800009431938 x0 : 0000000000000000
 Call trace:
  kernfs_find_and_get_ns+0x2c/0x80
  sysfs_unmerge_group+0x30/0x80
  dpm_sysfs_remove+0x3c/0x17c
  device_del+0xb0/0x3a0
  platform_device_del.part.0+0x24/0xb0
  platform_device_unregister+0x30/0x50
  sysfb_disable+0x4c/0x80
  remove_conflicting_framebuffers+0x40/0x100
  remove_conflicting_pci_framebuffers+0x128/0x240
  drm_aperture_remove_conflicting_pci_framebuffers+0xb8/0x170
  vmw_probe+0x50/0xd30 [vmwgfx]
  local_pci_probe+0x4c/0xc0
  pci_device_probe+0x1e8/0x230
  really_probe+0x18c/0x3f0
  __driver_probe_device+0x124/0x1c0
  driver_probe_device+0x44/0x140
  __driver_attach+0xe0/0x234
  bus_for_each_dev+0x7c/0xe0
  driver_attach+0x30/0x40
  bus_add_driver+0x158/0x250
  driver_register+0x84/0x140
  __pci_register_driver+0x50/0x5c
  vmw_pci_driver_init+0x44/0x1000 [vmwgfx]
  do_one_initcall+0x50/0x250
  do_init_module+0x50/0x260
  load_module+0x23e4/0x27c0
  __do_sys_finit_module+0xac/0x12c
  __arm64_sys_finit_module+0x2c/0x40
  invoke_syscall+0x78/0x100
  el0_svc_common.constprop.0+0x54/0x184
  do_el0_svc+0x34/0x9c
  el0_svc+0x54/0x1e0
  el0t_64_sync_handler+0xa4/0x130
  el0t_64_sync+0x1a0/0x1a4
 Code: aa0003f3 a9025bf5 aa0103f5 aa0203f6 (f9400400)
 ---[ end trace 0000000000000000 ]---

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

Hello Zack,

On 6/16/22 21:29, Zack Rusin wrote:
> On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
>> The platform devices registered by sysfb match with firmware-based DRM or
>> fbdev drivers, that are used to have early graphics using a framebuffer
>> provided by the system firmware.
>>

[snip]

> 
> Hi, Javier.
> 
> This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
> you'd like .config or just have us test something directly for you):
>

Yes please share your .config and I'll try to reproduce on an arm64 machine.

> 
>  Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008
>  Mem abort info:
>    ESR = 0x96000004
>    EC = 0x25: DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x04: level 0 translation fault
>  Data abort info:
>    ISV = 0, ISS = 0x00000004
>    CM = 0, WnR = 0
>  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
>  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
>  Internal error: Oops: 96000004 [#1] SMP
>  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
> sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
>  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
> #12

I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
is only in drm-misc-next now and will land in 5.20...

Did you backport it? Can you please try to reproduce with latest drm-tip ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Zack Rusin]

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

On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:
> Hello Zack,
> 
> On 6/16/22 21:29, Zack Rusin wrote:
> > On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
> > > The platform devices registered by sysfb match with firmware-based DRM or
> > > fbdev drivers, that are used to have early graphics using a framebuffer
> > > provided by the system firmware.
> > > 
> 
> [snip]
> 
> > 
> > Hi, Javier.
> > 
> > This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
> > you'd like .config or just have us test something directly for you):
> > 
> 
> Yes please share your .config and I'll try to reproduce on an arm64 machine.

Attached. It might be a little hard to reproduce unless you have an arm64 machine
with a dedicated gpu. You'll need a system that actually transitions from a generic
fb driver (e.g. efifb) to the dedicated one.

> > 
> >  Unable to handle kernel NULL pointer dereference at virtual address
> > 0000000000000008
> >  Mem abort info:
> >    ESR = 0x96000004
> >    EC = 0x25: DABT (current EL), IL = 32 bits
> >    SET = 0, FnV = 0
> >    EA = 0, S1PTW = 0
> >    FSC = 0x04: level 0 translation fault
> >  Data abort info:
> >    ISV = 0, ISS = 0x00000004
> >    CM = 0, WnR = 0
> >  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
> >  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
> >  Internal error: Oops: 96000004 [#1] SMP
> >  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
> > sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
> >  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
> > #12
> 
> I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
> is only in drm-misc-next now and will land in 5.20...
> 
> Did you backport it? Can you please try to reproduce with latest drm-tip ?

No, this is drm-misc-next as of yesterday. drm-misc-next was still on 5.18.0-rc5
yesterday.

z

[-- Attachment #2: arm64config.gz --]
[-- Type: application/gzip, Size: 45029 bytes --]

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

On 6/16/22 23:03, Zack Rusin wrote:
> On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:
>> Hello Zack,
>>
>> On 6/16/22 21:29, Zack Rusin wrote:
>>> On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
>>>> The platform devices registered by sysfb match with firmware-based DRM or
>>>> fbdev drivers, that are used to have early graphics using a framebuffer
>>>> provided by the system firmware.
>>>>
>>
>> [snip]
>>
>>>
>>> Hi, Javier.
>>>
>>> This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
>>> you'd like .config or just have us test something directly for you):
>>>
>>
>> Yes please share your .config and I'll try to reproduce on an arm64 machine.
> 
> Attached. It might be a little hard to reproduce unless you have an arm64 machine
> with a dedicated gpu. You'll need a system that actually transitions from a generic
> fb driver (e.g. efifb) to the dedicated one.
>

Yes, all my testing for this was done with a rpi4 so I should be able to reproduce
that case. I'm confused though because I tested efifb -> vc4, simplefb -> vc4 and
simpledrm -> vc4.
 
>>>
>>>  Unable to handle kernel NULL pointer dereference at virtual address
>>> 0000000000000008
>>>  Mem abort info:
>>>    ESR = 0x96000004
>>>    EC = 0x25: DABT (current EL), IL = 32 bits
>>>    SET = 0, FnV = 0
>>>    EA = 0, S1PTW = 0
>>>    FSC = 0x04: level 0 translation fault
>>>  Data abort info:
>>>    ISV = 0, ISS = 0x00000004
>>>    CM = 0, WnR = 0
>>>  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
>>>  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
>>>  Internal error: Oops: 96000004 [#1] SMP
>>>  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
>>> sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
>>>  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
>>> #12
>>
>> I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
>> is only in drm-misc-next now and will land in 5.20...
>>
>> Did you backport it? Can you please try to reproduce with latest drm-tip ?
> 
> No, this is drm-misc-next as of yesterday. drm-misc-next was still on 5.18.0-rc5
> yesterday.
> 

Right! I looked at the base for drm-tip but forgot that drm-misc was still on 5.18.

I'll look at this tomorrow but in the meantime, could you please look if the following
commits on top of drm-misc-next help ?

d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

On 6/17/22 00:18, Javier Martinez Canillas wrote:
> On 6/16/22 23:03, Zack Rusin wrote:

[snip]

> 
> I'll look at this tomorrow but in the meantime, could you please look if the following
> commits on top of drm-misc-next help ?
> 
> d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> 

Scratch that. I see in your config now that you are not using efifb but instead
simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.

Since you mentioned efifb I misunderstood that you are using it. Anyways, as
said I'll investigate this tomorrow.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Zack Rusin]

On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
> On 6/17/22 00:18, Javier Martinez Canillas wrote:
> > On 6/16/22 23:03, Zack Rusin wrote:
> 
> [snip]
> 
> > 
> > I'll look at this tomorrow but in the meantime, could you please look if the following
> > commits on top of drm-misc-next help ?
> > 
> > d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> > 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> > 
> 
> Scratch that. I see in your config now that you are not using efifb but instead
> simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
> 
> Since you mentioned efifb I misunderstood that you are using it. Anyways, as
> said I'll investigate this tomorrow.

Sounds good. Let me know if you'd like me to try it without SIMPLEFB.

z

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

Hello Zack,

On 6/17/22 03:35, Zack Rusin wrote:
> On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
>> On 6/17/22 00:18, Javier Martinez Canillas wrote:
>>> On 6/16/22 23:03, Zack Rusin wrote:
>>
>> [snip]
>>
>>>
>>> I'll look at this tomorrow but in the meantime, could you please look if the following
>>> commits on top of drm-misc-next help ?
>>>
>>> d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
>>> 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
>>>
>>
>> Scratch that. I see in your config now that you are not using efifb but instead
>> simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
>>
>> Since you mentioned efifb I misunderstood that you are using it. Anyways, as
>> said I'll investigate this tomorrow.
> 
> Sounds good. Let me know if you'd like me to try it without SIMPLEFB.
>

Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
enabled (so that "efi-framebuffer" is registered and efifb probed) or with
CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
is used too but with simplefb instead of simpledrm).
 
I'm not able to reproduce, it would be useful to have another data point.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Xi Ruoyao]

On Fri, 2022-06-17 at 08:46 +0200, Javier Martinez Canillas wrote:
> Hello Zack,
> 
> On 6/17/22 03:35, Zack Rusin wrote:
> > On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
> > > On 6/17/22 00:18, Javier Martinez Canillas wrote:
> > > > On 6/16/22 23:03, Zack Rusin wrote:
> > > 
> > > [snip]
> > > 
> > > > 
> > > > I'll look at this tomorrow but in the meantime, could you please look if the following
> > > > commits on top of drm-misc-next help ?
> > > > 
> > > > d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> > > > 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> > > > 
> > > 
> > > Scratch that. I see in your config now that you are not using efifb but instead
> > > simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
> > > 
> > > Since you mentioned efifb I misunderstood that you are using it. Anyways, as
> > > said I'll investigate this tomorrow.
> > 
> > Sounds good. Let me know if you'd like me to try it without SIMPLEFB.
> > 
> 
> Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
> enabled (so that "efi-framebuffer" is registered and efifb probed) or with
> CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
> is used too but with simplefb instead of simpledrm).
>  
> I'm not able to reproduce, it would be useful to have another data point.

Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
1065G7 (with iGPU).

Reverting this commit on top of 5.19-rc5 "fixes" the issue.

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Xi Ruoyao]

On Mon, 2022-07-04 at 17:36 +0800, Xi Ruoyao wrote:

> > Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
> > enabled (so that "efi-framebuffer" is registered and efifb probed) or with
> > CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
> > is used too but with simplefb instead of simpledrm).
> >  
> > I'm not able to reproduce, it would be useful to have another data point.
> 
> Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
> 1065G7 (with iGPU).
> 
> Reverting this commit on top of 5.19-rc5 "fixes" the issue.

With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
issue.

I guess it's something going wrong on a "drm -> drm" pass over.  For now
I'll continue to use simpledrm with this commit reverted.

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

Hello Xi,

On 7/4/22 12:29, Xi Ruoyao wrote:
> On Mon, 2022-07-04 at 17:36 +0800, Xi Ruoyao wrote:
> 
>>> Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
>>> enabled (so that "efi-framebuffer" is registered and efifb probed) or with
>>> CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
>>> is used too but with simplefb instead of simpledrm).
>>>  
>>> I'm not able to reproduce, it would be useful to have another data point.
>>
>> Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
>> 1065G7 (with iGPU).
>>
>> Reverting this commit on top of 5.19-rc5 "fixes" the issue.
> 
> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
> issue.
> 
> I guess it's something going wrong on a "drm -> drm" pass over.  For now
> I'll continue to use simpledrm with this commit reverted.
> 

Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
removal before internal helpers") now that the sysfb_disable() patches
are in v5.19-rc5.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Xi Ruoyao]

On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
> Hello Xi,
> > 
> > With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
> > issue.
> > 
> > I guess it's something going wrong on a "drm -> drm" pass over.  For now
> > I'll continue to use simpledrm with this commit reverted.
> > 
> 
> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
> removal before internal helpers") now that the sysfb_disable() patches
> are in v5.19-rc5.

I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Javier Martinez Canillas]

On 7/4/22 14:11, Xi Ruoyao wrote:
> On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
>> Hello Xi,
>>>
>>> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
>>> issue.
>>>
>>> I guess it's something going wrong on a "drm -> drm" pass over.  For now
>>> I'll continue to use simpledrm with this commit reverted.
>>>
>>
>> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
>> removal before internal helpers") now that the sysfb_disable() patches
>> are in v5.19-rc5.
> 
> I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.
> 

Thanks for testing it! Thomas is getting that patch through the drm-fixes
path, so it should make it to the v5.19-rc cycle at some point.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v6 3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 933 bytes --]

Hi

Am 04.07.22 um 14:11 schrieb Xi Ruoyao:
> On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
>> Hello Xi,
>>>
>>> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
>>> issue.
>>>
>>> I guess it's something going wrong on a "drm -> drm" pass over.  For now
>>> I'll continue to use simpledrm with this commit reverted.
>>>
>>
>> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
>> removal before internal helpers") now that the sysfb_disable() patches
>> are in v5.19-rc5.
> 
> I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.

I've cherry-picked the commit into drm-misc-fixes. It will show up in 
mainline soon.

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]