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(-)
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 */
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[];
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(®istration_lock); do_remove_conflicting_framebuffers(a, name, primary); mutex_unlock(®istration_lock);
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(®istration_lock); do_remove_conflicting_framebuffers(a, name, primary); mutex_unlock(®istration_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 ]---
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 ?
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
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
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.
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
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.
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;
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);
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/ol... [2] https://elixir.bootlin.com/linux/v5.18.2/source/drivers/staging/olpc_dcon/ol...
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]) {} elsebool 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);
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.
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
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.
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!
dri-devel@lists.freedesktop.org
-
Javier Martinez Canillas -
Sam Ravnborg -
Thomas Zimmermann -
Zack Rusin