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

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.

62 more lines

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 <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <redacted>
---

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

8 more lines

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

From: Daniel Vetter <redacted>

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 <redacted>
Cc: Jens Frederich <redacted>
Cc: Jon Nettleton <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>
Signed-off-by: Daniel Vetter <redacted>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Cc: Daniel Vetter <redacted>
Cc: Helge Deller <deller@gmx.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sam Ravnborg <redacted>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Zhen Lei <redacted>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Xiyu Yang <redacted>
Cc: <redacted>
Cc: Zheyu Ma <redacted>
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

From: Daniel Vetter <redacted>

This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.

With

commit 27599aacbaefcbf2af7b06b0029459bbf682000d
Author: Thomas Zimmermann [off-list ref]
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 <redacted>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Zack Rusin <redacted>
Cc: Hans de Goede <redacted>
Cc: Ilya Trukhanov <redacted>
Signed-off-by: Daniel Vetter <redacted>
Signed-off-by: Daniel Vetter <redacted>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Cc: Peter Jones <pjones@redhat.com>
Cc: <redacted>

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

Hi Javier

Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:

From: Daniel Vetter <redacted>

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

38 more quoted lines

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

Hello Thomas,

On 6/9/22 13:49, Thomas Zimmermann wrote:

Hi Javier

Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:

quoted

From: Daniel Vetter <redacted>

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

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

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:

quoted

Hi Javier

Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:

quoted

From: Daniel Vetter <redacted>

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]

4 more quoted lines

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

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

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 <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <redacted>
---

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

38 more quoted lines

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

37 more lines

Hello Zack,

On 6/16/22 21:29, Zack Rusin wrote:

On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:

quoted

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

On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:

Hello Zack,

On 6/16/22 21:29, Zack Rusin wrote:

quoted

On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:

quoted

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]

quoted

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.

quoted

 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

On 6/16/22 23:03, Zack Rusin wrote:

On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:

quoted

Hello Zack,

On 6/16/22 21:29, Zack Rusin wrote:

quoted

On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:

quoted

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]

quoted

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.
 

quoted

quoted

 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

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

On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:

On 6/17/22 00:18, Javier Martinez Canillas wrote:

quoted

On 6/16/22 23:03, Zack Rusin wrote:

[snip]

quoted

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

Hello Zack,

On 6/17/22 03:35, Zack Rusin wrote:

On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:

quoted

On 6/17/22 00:18, Javier Martinez Canillas wrote:

quoted

On 6/16/22 23:03, Zack Rusin wrote:

[snip]

quoted

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

On Fri, 2022-06-17 at 08:46 +0200, Javier Martinez Canillas wrote:

Hello Zack,

On 6/17/22 03:35, Zack Rusin wrote:

quoted

On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:

quoted

On 6/17/22 00:18, Javier Martinez Canillas wrote:

quoted

On 6/16/22 23:03, Zack Rusin wrote:

[snip]

quoted

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.

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

quoted

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.

Hello Xi,

On 7/4/22 12:29, Xi Ruoyao wrote:

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

quoted

quoted

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

On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:

Hello Xi,

quoted

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.

On 7/4/22 14:11, Xi Ruoyao wrote:

On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:

quoted

Hello Xi,

quoted

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

Hi

Am 04.07.22 um 14:11 schrieb Xi Ruoyao:

On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:

quoted

Hello Xi,

quoted

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