[RFC PATCH v4 07/11] fbdev: Make sysfb to unregister its own registered devices
The platform devices registered in sysfb match with a firmware-based fbdev
or DRM driver, that are used to have early graphics using framebuffers set
up by the system firmware.
Real DRM 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 the problem that sysfb doesn't know when the
device that registered is unregistered. This means that is not able to do
any cleanup if needed since the device pointer may not be valid anymore.
Not all platforms use sysfb to register the simple framebuffer devices,
so an unregistration has to be forced by fbmem if sysfb_try_unregister()
does not succeed at unregister the device.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Vetter
---
Changes in v4:
- Drop call to sysfb_disable() in fbmem since is done in other places now.
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 | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0bb459258df3..d6ae33990f40 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1585,18 +1585,35 @@ static void do_remove_conflicting_framebuffers(struct
apertures_struct *a,
if (!device) {
pr_warn("fb%d: no device set\n", i);
do_unregister_framebuffer(registered_fb[i]);
- } else if (dev_is_platform(device)) {
+ } else {
/*
* Drop the lock because if the device is
unregistered, its
* driver will call to
unregister_framebuffer(), that takes
* this lock.
*/
mutex_unlock(®istration_lock);
-
platform_device_unregister(to_platform_device(device));
+ /*
+* First attempt the device to be unregistered
by sysfb.
+*/
+ if (!sysfb_try_unregister(device)) {
+ if (dev_is_platform(device)) {
+ /*
+* FIXME: sysfb didn't register
this device, the platform
+* device was registered in
other platform code.
+*/
+
platform_device_unregister(to_platform_device(device));
+ } else {
+ /*
+* If is not a platform device,
at least print a warning. A
+* fix would add to make the
code that registered the device
+* to also unregister it.
+*/
+ pr_warn("fb%d: cannot remove
device\n", i);
+ /* call
unregister_framebuffer() since the lock was dropped */
+
unregister_framebuffer(registered_fb[i]);
+ }
+ }
mutex_lock(®istration_lock);
- } else {
- pr_warn("fb%d: cannot remove device\n", i);
- do_unregister_framebuffer(registered_fb[i]);
}
/*
* Restart the removal loop now that the device has been
--
2.35.1
[RFC PATCH v4 10/11] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
From: Daniel Vetter
This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
With
commit 27599aacbaefcbf2af7b06b0029459bbf682000d
Author: Thomas Zimmermann
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
Cc: Zack Rusin
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Hans de Goede
Cc: Ilya Trukhanov
Signed-off-by: Daniel Vetter
Signed-off-by: Daniel Vetter
Reviewed-by: Javier Martinez Canillas
Cc: Peter Jones
Cc: linux-fb...@vger.kernel.org
Signed-off-by: Javier Martinez Canillas
---
(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.35.1
[RFC PATCH v4 11/11] fbdev: Make registered_fb[] private to fbmem.c
From: Daniel Vetter
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
Cc: Jens Frederich
Cc: Jon Nettleton
Cc: Greg Kroah-Hartman
Cc: linux-stag...@lists.linux.dev
Signed-off-by: Daniel Vetter
Signed-off-by: Daniel Vetter
Reviewed-by: Javier Martinez Canillas
Cc: Daniel Vetter
Cc: Helge Deller
Cc: Matthew Wilcox
Cc: Sam Ravnborg
Cc: Tetsuo Handa
Cc: Zhen Lei
Cc: Alex Deucher
Cc: Xiyu Yang
Cc: linux-fb...@vger.kernel.org
Cc: Zheyu Ma
Cc: Guenter Roeck
Signed-off-by: Javier Martinez Canillas
---
(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 7583296481b0..1ce5b0f3abd4 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 b781bc721113..208bca693b33 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -623,16 +623,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.35.1
Re: [RFC PATCH v4 02/11] drm/fb-helper: Set FBINFO_MISC_FIRMWARE flag for DRIVER_FIRMWARE fb
Hello Thomas, On 4/29/22 11:14, Thomas Zimmermann wrote: > Hi > > Am 29.04.22 um 10:42 schrieb Javier Martinez Canillas: >> The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer >> that was initialized and provided by the system firmware for scanout. >> >> Indicate to the fbdev subsystem that the registered framebuffer is a >> FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example, >> wold hot-unplug the associated device if asked to remove conflicting >> framebuffers. >> >> Suggested-by: Thomas Zimmermann >> Signed-off-by: Javier Martinez Canillas >> --- >> >> (no changes since v1) >> >> drivers/gpu/drm/drm_fb_helper.c | 4 >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c >> b/drivers/gpu/drm/drm_fb_helper.c >> index d265a73313c9..76dd11888621 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct >> drm_fb_helper *fb_helper, >> /* don't leak any physical addresses to userspace */ >> info->flags |= FBINFO_HIDE_SMEM_START; >> >> +/* Indicate that the framebuffer is provided by the firmware */ >> +if (drm_core_check_feature(dev, DRIVER_FIRMWARE)) >> +info->flags |= FBINFO_MISC_FIRMWARE; >> + > > Patches 1 to 3 should be squashed into one before landing. > I actually considered this but then decided to go with the each change goes into its own patch approach. But I'll squash it in next revisions. > We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we > could also used the existing final parameter of > drm_fbdev_generic_setup() to pass a flag that designates a firmware device. > By existing final parameter you mean @preferred_bpp ? That doesn't seem correct. I also like that by using DRIVER_FIRMWARE it is completely data driven and transparent to the DRM driver. Or do you envision a case where a driver would be DRIVER_FIRMWARE but we wouldn't want the emulated fbdev to also be FBINFO_MISC_FIRMWARE ? -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH v4 02/11] drm/fb-helper: Set FBINFO_MISC_FIRMWARE flag for DRIVER_FIRMWARE fb
On 4/29/22 12:20, Thomas Zimmermann wrote: > Hi Javier [snip] >> >>> We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we >>> could also used the existing final parameter of >>> drm_fbdev_generic_setup() to pass a flag that designates a firmware device. >>> >> >> By existing final parameter you mean @preferred_bpp ? That doesn't seem >> correct. I also like that by using DRIVER_FIRMWARE it is completely data >> driven and transparent to the DRM driver. > > DRIVER_FIRMWARE is an indirection and only used here. (Just like > FBINFO_MISC_FIRMWARE is a bad interface for marking framebuffers that > can be unplugged.) If a driver supports hot-unplugging, it should simply > register itself with aperture helpers, regardless of whether it's a > firmware framebuffer or not. > That's fair, and if in practice will only be used by one driver (simpledrm) then that would also allow us to drop patches 1 and 2 from this series. IOW, we wouldn't really need a DRIVER_FIRMWARE capability flag. > Of preferred_bpp, we really only use the lowest byte. All other bits are > up for grabbing. The argument is a workaround for handling > mode_config.prefered_depth correctly. > Yeah, but I didn't want to abuse that argument or package data in an int. > Eventually, preferred_depth should be replaced by something like > 'preferred_format', which will hold the driver's preferred format in > 4CC. We won't need preferred_bpp then. So we could turn preferred_bpp > into a flags argument. > That's a good point, maybe we could start from there and do this cleanup as a preparatory change of this series ? Then the patches would only be 1) renaming preferred_bpp (that would be unused at this point) to flags and 2) make simpledrm to set FBDEV_FIRMWARE flag or something like that. Another option is to add a third flags param to drm_fbdev_generic_setup() and make all drivers to set 0 besides simpledrm. That way marking the fb as FBINFO_MISC_FIRMWARE won't be blocked by the preferred_depth cleanup. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH 0/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello, This series contain patches suggested by Thomas Zimmermannas a feedback for "[RFC PATCH v4 00/11] Fix some race between sysfb device registration and drivers probe" [0]. Since other changes in [0] were more controversial, I decided to just split this part in a new patch-set and revisit the rest of the patches later. Patch #1 is just a cleanup since when working on this noticed that some DRM drivers were passing as preferred bits per pixel to drm_fbdev_generic_setup() the value that is the default anyways. Patch #2 renames the 'preferred_bpp' drm_fbdev_generic_setup() parameter to 'options', and make this a multi field parameter so that it can be extended later to pass other options as well. Patch #3 finally adds the new DRM_FB_FW option and makes simpledrm to use it so that the registered framebuffer device is also marked as firmware provided. [0]: https://lore.kernel.org/lkml/20220429084253.1085911-1-javi...@redhat.com/ Javier Martinez Canillas (3): drm: Remove superfluous arg when calling to drm_fbdev_generic_setup() drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter drm: Allow simpledrm to setup its emulated FB as firmware provided drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 +++-- drivers/gpu/drm/arm/hdlcd_drv.c | 2 +- drivers/gpu/drm/arm/malidp_drv.c | 2 +- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +- drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 25 --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +- drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +- drivers/gpu/drm/imx/imx-drm-core.c| 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/mcde/mcde_drv.c | 2 +- drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +- drivers/gpu/drm/meson/meson_drv.c | 2 +- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +- drivers/gpu/drm/pl111/pl111_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- drivers/gpu/drm/sti/sti_drv.c | 2 +- drivers/gpu/drm/stm/drv.c | 2 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/drm/tiny/arcpgu.c | 2 +- drivers/gpu/drm/tiny/bochs.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- drivers/gpu/drm/tiny/simpledrm.c | 2 +- drivers/gpu/drm/tve200/tve200_drv.c | 2 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +- include/drm/drm_fb_helper.h | 22 36 files changed, 80 insertions(+), 39 deletions(-) -- 2.35.1
[PATCH 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
The drm_fbdev_generic_setup() function already sets the preferred bits per pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp value is zero. Passing the same value to the function is unnecessary. Let's cleanup that in the two drivers that do it. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index fe4269c5aa0a..ace92459e462 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -349,7 +349,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, goto err_unload; } - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c index c8e791840862..ed5a2e14894a 100644 --- a/drivers/gpu/drm/tiny/cirrus.c +++ b/drivers/gpu/drm/tiny/cirrus.c @@ -601,7 +601,7 @@ static int cirrus_pci_probe(struct pci_dev *pdev, if (ret) return ret; - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; } -- 2.35.1
[PATCH 2/3] drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter
By default the bits per pixel for the emulated framebuffer device is set
to dev->mode_config.preferred_depth, but some devices need another value.
Since this second parameter is only used by a few drivers, and to allow
drivers to use it for passing other configurations when registering the
fbdev, rename @preferred_bpp to @options and make it a multi-field param.
The DRM_FB_SET_OPTION() and DRM_FB_GET_OPTION() macros are provided for
drivers to set and get an option respectively. For now, only DRM_FB_BPP
option exists but other options would be added later.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
drivers/gpu/drm/arm/malidp_drv.c| 2 +-
drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c| 2 +-
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/gpu/drm/drm_fb_helper.c | 16
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +-
drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +-
drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +-
drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
drivers/gpu/drm/mcde/mcde_drv.c | 2 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +-
drivers/gpu/drm/meson/meson_drv.c | 2 +-
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +-
drivers/gpu/drm/sti/sti_drv.c | 2 +-
drivers/gpu/drm/stm/drv.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +-
drivers/gpu/drm/tidss/tidss_drv.c | 2 +-
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
drivers/gpu/drm/tiny/arcpgu.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tve200/tve200_drv.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 2 +-
drivers/gpu/drm/vc4/vc4_drv.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +-
include/drm/drm_fb_helper.h | 12
33 files changed, 58 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b03663f42cc9..f5fae3838cdc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2118,9 +2118,11 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
!list_empty(&adev_to_drm(adev)->mode_config.connector_list)) {
/* select 8 bpp console on low vram cards */
if (adev->gmc.real_vram_size <= (32*1024*1024))
- drm_fbdev_generic_setup(adev_to_drm(adev), 8);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_SET_OPTION(DRM_FB_BPP,
8));
else
- drm_fbdev_generic_setup(adev_to_drm(adev), 32);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_SET_OPTION(DRM_FB_BPP,
32));
}
ret = amdgpu_debugfs_init(adev);
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e89ae0ec60eb..636b0e989398 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -321,7 +321,7 @@ static int hdlcd_drm_bind(struct device *dev)
if (ret)
goto err_register;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_SET_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d5aef21426cf..8f7c4f3136d5 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -863,7 +863,7 @@ static int malidp_bind(struct device *dev)
if (ret)
goto register_fail;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_SET_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 7780b72de9e8..c16837fd1f8d 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -343,7 +343,7 @@ static int aspeed_gfx_probe(struct platform_device *pdev)
if (ret)
goto err_unload;
- drm_fbdev_generic_setup(&priv->drm, 32);
+ drm_fbdev_g
[PATCH 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Indicate to fbdev subsystem that the registered framebuffer is provided by
the system firmware, so that it can handle accordingly. For example, would
unregister the FB devices if asked to remove the conflicting framebuffers.
Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter.
Drivers can use this to indicate the FB helper initialization that the FB
registered is provided by the firmware, so it can be configured as such.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/drm_fb_helper.c | 9 +
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
include/drm/drm_fb_helper.h | 10 ++
3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f626947bb9b9..c2ff986f064d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct
drm_fb_helper *fb_helper,
/* don't leak any physical addresses to userspace */
info->flags |= FBINFO_HIDE_SMEM_START;
+ /* Indicate that the framebuffer is provided by the firmware */
+ if (fb_helper->firmware)
+ info->flags |= FBINFO_MISC_FIRMWARE;
+
/* Need to drop locks to avoid recursive deadlock in
* register_framebuffer. This is ok because the only thing left to do is
* register the fbdev emulation instance in kernel_fb_helper_list. */
@@ -2511,6 +2515,8 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
*
* * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
* @dev->mode_config.preferred_depth is used instead.
+ * * DRM_FB_FW: if the framebuffer for the device is provided by the
+ * system firmware.
*
* This function sets up generic fbdev emulation for drivers that supports
* dumb buffers with a virtual address and that can be mmap'ed.
@@ -2537,6 +2543,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int options)
{
struct drm_fb_helper *fb_helper;
unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options);
+ bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
int ret;
drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
@@ -2569,6 +2576,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int options)
preferred_bpp = 32;
fb_helper->preferred_bpp = preferred_bpp;
+ fb_helper->firmware = firmware;
+
ret = drm_fbdev_client_hotplug(&fb_helper->client);
if (ret)
drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index f5b8e864a5cd..5dcc21ea6180 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev)
if (ret)
return ret;
- drm_fbdev_generic_setup(dev, 0);
+ drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
return 0;
}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 1da3ef76f499..0eec500e0784 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -44,6 +44,7 @@ enum mode_set_atomic {
};
#define DRM_FB_BPP_MASK GENMASK(7, 0)
+#define DRM_FB_FW_MASK GENMASK(8, 8)
/* Using the GNU statement expression extension */
#define DRM_FB_SET_OPTION(option, value) \
@@ -197,6 +198,15 @@ struct drm_fb_helper {
* See also: @deferred_setup
*/
int preferred_bpp;
+
+ /**
+* @firmware:
+*
+* Set if the driver indicates to the FB helper initialization that the
+* framebuffer for the device being registered is provided by firmware,
+* so that it can pass this on when registering the framebuffer device.
+*/
+ bool firmware;
};
static inline struct drm_fb_helper *
--
2.35.1
Re: [PATCH 0/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello Thomas, On 5/2/22 12:35, Thomas Zimmermann wrote: > Hi Javier > > Am 02.05.22 um 10:48 schrieb Javier Martinez Canillas: >> Hello, >> >> This series contain patches suggested by Thomas Zimmermannas a feedback for Ups, I missed a space here. I meant to write "Zimmermann as a feedback..." >> "[RFC PATCH v4 00/11] Fix some race between sysfb device registration and >> drivers probe" [0]. >> >> Since other changes in [0] were more controversial, I decided to just split >> this part in a new patch-set and revisit the rest of the patches later. >> >> Patch #1 is just a cleanup since when working on this noticed that some DRM >> drivers were passing as preferred bits per pixel to drm_fbdev_generic_setup() >> the value that is the default anyways. >> >> Patch #2 renames the 'preferred_bpp' drm_fbdev_generic_setup() parameter to >> 'options', and make this a multi field parameter so that it can be extended >> later to pass other options as well. >> >> Patch #3 finally adds the new DRM_FB_FW option and makes simpledrm to use it >> so that the registered framebuffer device is also marked as firmware >> provided. > > For the whole patchset: > > Reviewed-by: Thomas Zimmermann > > Thanks a lot! > Thanks for the prompt review! -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH 0/2] fbdev: Fix a NULL pointer dereference in fb_release()
Hello, This small series contains fixes for two bugs I found in fbmem.c, that may explain a bug reported in the rpi4 [0] when using simplefb and vc4 drivers. I was not able to reproduce the mentioned bug, but looking at the code I think that it might explain the issue. I've tested these patches in a rpi4 with both simplefb and vc4 drivers built-in and did not find any regression. [0]: https://github.com/raspberrypi/linux/issues/5011 Best regards, Javier Javier Martinez Canillas (2): fbdev: Check in file_fb_info() if the fb_info was already been freed fbdev: Make fb_release() return -ENODEV if fbdev was unregistered drivers/video/fbdev/core/fbmem.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) -- 2.35.1
[PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed
If real driver probes, the fbdev core kicks out all drivers that are using a framebuffer that were provided by the system firmware. But it could be a user-space process still has a file descriptor for the fbdev device node. This can lead to a NULL pointer dereference, if the framebuffer device is unregistered and associated data freed, but later in the .release callback is attempted to access its struct fb_info. To prevent this, make file_fb_info() to also check the fb_info reference counter and just return NULL if this equals zero. Since that means it has already been freed. Signed-off-by: Javier Martinez Canillas --- drivers/video/fbdev/core/fbmem.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 84427470367b..20d8929df79f 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -751,8 +751,13 @@ static struct fb_info *file_fb_info(struct file *file) int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx]; - if (info != file->private_data) - info = NULL; + if (!info) + return NULL; + + /* check that the fb_info has not changed or was already freed */ + if (info != file->private_data || refcount_read(&info->count) == 0) + return NULL; + return info; } -- 2.35.1
[PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.
This can happen for example if the fbdev driver was one that is using a
framebuffer provided by the system firmware. In that case, the fbdev core
could unregister the framebuffer device if a real video driver is probed.
Reported-by: Maxime Ripard
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbmem.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 20d8929df79f..d68097105f93 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1439,7 +1439,10 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file->private_data;
+ struct fb_info * const info = file_fb_info(file);
+
+ if (!info)
+ return -ENODEV;
lock_fb_info(info);
if (info->fbops->fb_release)
--
2.35.1
Re: [PATCH 2/3] drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter
Hello Laurent,
On 5/2/22 13:34, Laurent Pinchart wrote:
> Hi Javier,
>
> Thank you for the patch.
>
Thanks a lot for your feedback.
[snip]
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2501,8 +2501,16 @@ static const struct drm_client_funcs
>> drm_fbdev_client_funcs = {
>> /**
>> * drm_fbdev_generic_setup() - Setup generic fbdev emulation
>> * @dev: DRM device
>> - * @preferred_bpp: Preferred bits per pixel for the device.
>> - * @dev->mode_config.preferred_depth is used if this is
>> zero.
>> + * @options: options for the registered framebuffer.
>> + *
>> + * The @options parameter is a multi-field parameter that can contain
>> + * different options for the emulated framebuffer device registered.
>> + *
>> + * The options must be set using DRM_FB_SET_OPTION() and obtained using
>> + * DRM_FB_GET_OPTION(). The options field are the following:
>> + *
>> + * * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
>> + * @dev->mode_config.preferred_depth is used instead.
>
> Do I assume correctly that a driver that would need to set multiple
> options would do something like
>
> drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_BPP, 32) |
> DRM_FB_SET_OPTION(DRM_FB_FW, 1));
>
That's correct, yes.
> ? If so, I would rename DRM_FB_SET_OPTION() to DRM_FB_OPTION() as it's
> computing the value of the option bitfield, it doesn't actually set it.
> Apart from that,
>
Right. I'll rename it.
> Reviewed-by: Laurent Pinchart
>
Thanks!
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed
Hello Thomas, On 5/2/22 15:26, Thomas Zimmermann wrote: > Hi > > Am 02.05.22 um 15:09 schrieb Javier Martinez Canillas: >> If real driver probes, the fbdev core kicks out all drivers that are using >> a framebuffer that were provided by the system firmware. But it could be a >> user-space process still has a file descriptor for the fbdev device node. >> >> This can lead to a NULL pointer dereference, if the framebuffer device is >> unregistered and associated data freed, but later in the .release callback >> is attempted to access its struct fb_info. >> >> To prevent this, make file_fb_info() to also check the fb_info reference >> counter and just return NULL if this equals zero. Since that means it has >> already been freed. >> >> Signed-off-by: Javier Martinez Canillas >> --- >> >> drivers/video/fbdev/core/fbmem.c | 9 +++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/video/fbdev/core/fbmem.c >> b/drivers/video/fbdev/core/fbmem.c >> index 84427470367b..20d8929df79f 100644 >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -751,8 +751,13 @@ static struct fb_info *file_fb_info(struct file *file) >> int fbidx = iminor(inode); >> struct fb_info *info = registered_fb[fbidx]; >> >> -if (info != file->private_data) >> -info = NULL; >> +if (!info) >> +return NULL; >> + >> +/* check that the fb_info has not changed or was already freed */ >> +if (info != file->private_data || refcount_read(&info->count) == 0) >> +return NULL; >> + > > Acked-by: Thomas Zimmermann > > However, I'm having problems with the semantics of these variables: if > we have an info from registered_fb[fbinx] and the refcount in > info->count is still 0, isn't that a consistency problem? If so, we > should print a WARN_ON(). > That's a good point. Maybe we are being too paranoid here? If the fb_info was set to NULL then the existing if (info != file->private_data) check will already catch that issue. In other words, now that fb_release() is getting the fb_info with the file_fb_info() function instead of file->private_data directly, the NULL pointer dereference should not happen anymore. I think that will just drop this patch, the less we touch the fbdev code the better IMO. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
Hello Thomas,
On 5/2/22 15:20, Thomas Zimmermann wrote:
>
>
> Am 02.05.22 um 15:09 schrieb Javier Martinez Canillas:
>> A reference to the framebuffer device struct fb_info is stored in the file
>> private data, but this reference could no longer be valid and must not be
>> accessed directly. Instead, the file_fb_info() accessor function must be
>> used since it does sanity checking to make sure that the fb_info is valid.
>>
>> This can happen for example if the fbdev driver was one that is using a
>> framebuffer provided by the system firmware. In that case, the fbdev core
>> could unregister the framebuffer device if a real video driver is probed.
>>
>> Reported-by: Maxime Ripard
>> Signed-off-by: Javier Martinez Canillas
>
> Reviewed-by: Thomas Zimmermann
>
Thanks.
> This seems like the correct thing to do in any case. Thanks for the
Agreed, it's certainly a bug if not the same that was already reported.
> patch. Before merging, you should also add
>
> Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced
> removal")
I thought about that but I don't think that's accurate since the bug is
not related to that commit. That might make easier to reproduce it but
is something that would happen anyway if for example someone attempted
to remove a module or unbind the device using the sysfs entries.
Maybe I can comment in the commit message that this change made it more
likely to occur and for that reason I'm adding a fixes tag.
> Reported-by: Junxiao Chang
>
Indeed.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH v2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.
This can happen for example if the registered framebuffer device is for a
driver that just uses a framebuffer provided by the system firmware. In
that case, the fbdev core would unregister the framebuffer device when a
real video driver is probed and ask to remove conflicting framebuffers.
The bug has been present for a long time but commit 27599aacbaef ("fbdev:
Hot-unplug firmware fb devices on forced removal") unmasked it since the
fbdev core started unregistering the framebuffers' devices associated.
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
Reported-by: Maxime Ripard
Reported-by: Junxiao Chang
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
Changes in v2:
- Drop patch 1/2 since patch 2/2 should be enough to fix the issue.
- Add missing Fixes and Reported-by tags (Thomas Zimmermann).
- Add Thomas Zimmermann's Reviewed-by tag.
drivers/video/fbdev/core/fbmem.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 84427470367b..82d4318ba8f7 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1434,7 +1434,10 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file->private_data;
+ struct fb_info * const info = file_fb_info(file);
+
+ if (!info)
+ return -ENODEV;
lock_fb_info(info);
if (info->fbops->fb_release)
--
2.35.1
[PATCH v2 2/3] drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter
By default the bits per pixel for the emulated framebuffer device is set
to dev->mode_config.preferred_depth, but some devices need another value.
Since this second parameter is only used by a few drivers, and to allow
drivers to use it for passing other configurations when registering the
fbdev, rename @preferred_bpp to @options and make it a multi-field param.
The DRM_FB_OPTION() and DRM_FB_GET_OPTION() macros are provided to drivers
for computing options bitfield values and getting the values respectively
For now, only the DRM_FB_BPP option exists but other options can be added.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Laurent Pinchart
---
Changes in v2:
- Rename DRM_FB_SET_OPTION() to DRM_FB_SET() and make more clear in the
kernel-doc what this macro does (Laurent Pinchart).
- Fix some kernel-doc issues I didn't notice in v1.
- Add Reviewed-by tags from Thomas and Laurent.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
drivers/gpu/drm/arm/malidp_drv.c| 2 +-
drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c| 2 +-
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/gpu/drm/drm_fb_helper.c | 17 +
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +-
drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +-
drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +-
drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
drivers/gpu/drm/mcde/mcde_drv.c | 2 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +-
drivers/gpu/drm/meson/meson_drv.c | 2 +-
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +-
drivers/gpu/drm/sti/sti_drv.c | 2 +-
drivers/gpu/drm/stm/drv.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +-
drivers/gpu/drm/tidss/tidss_drv.c | 2 +-
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
drivers/gpu/drm/tiny/arcpgu.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tve200/tve200_drv.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 2 +-
drivers/gpu/drm/vc4/vc4_drv.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +-
include/drm/drm_fb_helper.h | 12
33 files changed, 59 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b03663f42cc9..0c54470975e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2118,9 +2118,11 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
!list_empty(&adev_to_drm(adev)->mode_config.connector_list)) {
/* select 8 bpp console on low vram cards */
if (adev->gmc.real_vram_size <= (32*1024*1024))
- drm_fbdev_generic_setup(adev_to_drm(adev), 8);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_OPTION(DRM_FB_BPP, 8));
else
- drm_fbdev_generic_setup(adev_to_drm(adev), 32);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_OPTION(DRM_FB_BPP, 32));
}
ret = amdgpu_debugfs_init(adev);
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e89ae0ec60eb..b69b1e5be379 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -321,7 +321,7 @@ static int hdlcd_drm_bind(struct device *dev)
if (ret)
goto err_register;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d5aef21426cf..25685b579a05 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -863,7 +863,7 @@ static int malidp_bind(struct device *dev)
if (ret)
goto register_fail;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 7780b72de9e8..dccc
[PATCH v2 0/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello, This series contain patches suggested by Thomas Zimmermann as a feedback for "[RFC PATCH v4 00/11] Fix some race between sysfb device registration and drivers probe" [0]. Since other changes in [0] were more controversial, I decided to just split this part in a new patch-set and revisit the rest of the patches later. This is a v2 that addresses issues pointed out in v1. Patch #1 is just a cleanup since when working on this noticed that some DRM drivers were passing as preferred bits per pixel to drm_fbdev_generic_setup() the value that is the default anyways. Patch #2 renames the 'preferred_bpp' drm_fbdev_generic_setup() parameter to 'options', and make this a multi field parameter so that it can be extended later to pass other options as well. Patch #3 finally adds the new DRM_FB_FW option and makes simpledrm to use it so that the registered framebuffer device is also marked as firmware provided. [0]: https://lore.kernel.org/lkml/20220429084253.1085911-1-javi...@redhat.com/ Changes in v2: - Rename DRM_FB_SET_OPTION() to DRM_FB_SET() and make more clear in the kernel-doc what this macro does (Laurent Pinchart). - Fix some kernel-doc issues I didn't notice in v1. - Add Reviewed-by tags from Thomas and Laurent. Javier Martinez Canillas (3): drm: Remove superfluous arg when calling to drm_fbdev_generic_setup() drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter drm: Allow simpledrm to setup its emulated FB as firmware provided drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 +++-- drivers/gpu/drm/arm/hdlcd_drv.c | 2 +- drivers/gpu/drm/arm/malidp_drv.c | 2 +- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +- drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 26 --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +- drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +- drivers/gpu/drm/imx/imx-drm-core.c| 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/mcde/mcde_drv.c | 2 +- drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +- drivers/gpu/drm/meson/meson_drv.c | 2 +- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +- drivers/gpu/drm/pl111/pl111_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- drivers/gpu/drm/sti/sti_drv.c | 2 +- drivers/gpu/drm/stm/drv.c | 2 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/drm/tiny/arcpgu.c | 2 +- drivers/gpu/drm/tiny/bochs.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- drivers/gpu/drm/tiny/simpledrm.c | 2 +- drivers/gpu/drm/tve200/tve200_drv.c | 2 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +- include/drm/drm_fb_helper.h | 22 36 files changed, 81 insertions(+), 39 deletions(-) -- 2.35.1
[PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Indicate to fbdev subsystem that the registered framebuffer is provided by
the system firmware, so that it can handle accordingly. For example, would
unregister the FB devices if asked to remove the conflicting framebuffers.
Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter.
Drivers can use this to indicate the FB helper initialization that the FB
registered is provided by the firmware, so it can be configured as such.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
(no changes since v1)
drivers/gpu/drm/drm_fb_helper.c | 9 +
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
include/drm/drm_fb_helper.h | 10 ++
3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index fd0084ad77c3..775e47c5de1f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct
drm_fb_helper *fb_helper,
/* don't leak any physical addresses to userspace */
info->flags |= FBINFO_HIDE_SMEM_START;
+ /* Indicate that the framebuffer is provided by the firmware */
+ if (fb_helper->firmware)
+ info->flags |= FBINFO_MISC_FIRMWARE;
+
/* Need to drop locks to avoid recursive deadlock in
* register_framebuffer. This is ok because the only thing left to do is
* register the fbdev emulation instance in kernel_fb_helper_list. */
@@ -2512,6 +2516,8 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
*
* * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
* @dev->mode_config.preferred_depth is used instead.
+ * * DRM_FB_FW: if the framebuffer for the device is provided by the
+ * system firmware.
*
* This function sets up generic fbdev emulation for drivers that supports
* dumb buffers with a virtual address and that can be mmap'ed.
@@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int options)
{
struct drm_fb_helper *fb_helper;
unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options);
+ bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
int ret;
drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
@@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int options)
preferred_bpp = 32;
fb_helper->preferred_bpp = preferred_bpp;
+ fb_helper->firmware = firmware;
+
ret = drm_fbdev_client_hotplug(&fb_helper->client);
if (ret)
drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index f5b8e864a5cd..5dcc21ea6180 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev)
if (ret)
return ret;
- drm_fbdev_generic_setup(dev, 0);
+ drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
return 0;
}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 740f87560102..77a72d55308d 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -44,6 +44,7 @@ enum mode_set_atomic {
};
#define DRM_FB_BPP_MASK GENMASK(7, 0)
+#define DRM_FB_FW_MASK GENMASK(8, 8)
/* Using the GNU statement expression extension */
#define DRM_FB_OPTION(option, value) \
@@ -197,6 +198,15 @@ struct drm_fb_helper {
* See also: @deferred_setup
*/
int preferred_bpp;
+
+ /**
+* @firmware:
+*
+* Set if the driver indicates to the FB helper initialization that the
+* framebuffer for the device being registered is provided by firmware,
+* so that it can pass this on when registering the framebuffer device.
+*/
+ bool firmware;
};
static inline struct drm_fb_helper *
--
2.35.1
[PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
The drm_fbdev_generic_setup() function already sets the preferred bits per pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp value is zero. Passing the same value to the function is unnecessary. Let's cleanup that in the two drivers that do it. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann --- (no changes since v1) drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index fe4269c5aa0a..ace92459e462 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -349,7 +349,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, goto err_unload; } - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c index c8e791840862..ed5a2e14894a 100644 --- a/drivers/gpu/drm/tiny/cirrus.c +++ b/drivers/gpu/drm/tiny/cirrus.c @@ -601,7 +601,7 @@ static int cirrus_pci_probe(struct pci_dev *pdev, if (ret) return ret; - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; } -- 2.35.1
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Hello Laurent, On 5/2/22 18:06, Laurent Pinchart wrote: > Hi Javier, > > Thank you for the patch. > > On Mon, May 02, 2022 at 05:38:58PM +0200, Javier Martinez Canillas wrote: >> The drm_fbdev_generic_setup() function already sets the preferred bits per >> pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp >> value is zero. >> >> Passing the same value to the function is unnecessary. Let's cleanup that >> in the two drivers that do it. > > This looks fine, so > > Reviewed-by: Laurent Pinchart > > but why do we have two different mechanisms to set the preferred depth ? > Could we get all drivers to set dev->mode_config.preferred_depth and Yes, that's the plan and the reason why when we were discussing with Thomas about how to pass this option to the FB helper layer, we agreed on reusing the @preferred_bpp parameter rather than adding a third parameter to drm_fbdev_generic_setup(). Since in the future drivers shouldn't pass that information to the FB helper and just get it from the default mode config. But doing that would require more auditing to all drivers and it could add regressions while patches 1/2 and 2/2 in this series shouldn't cause any behavioral changes. > drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME > comment in drm_fbdev_generic_setup() that could be related. > A FIXME makes sense, I'll add that to when posting a v3. Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 2/3] drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter
On 5/2/22 18:07, Laurent Pinchart wrote: > Hi Javier, > > Thank you for the patch. > > On Mon, May 02, 2022 at 05:38:59PM +0200, Javier Martinez Canillas wrote: >> By default the bits per pixel for the emulated framebuffer device is set >> to dev->mode_config.preferred_depth, but some devices need another value. >> >> Since this second parameter is only used by a few drivers, and to allow >> drivers to use it for passing other configurations when registering the >> fbdev, rename @preferred_bpp to @options and make it a multi-field param. >> >> The DRM_FB_OPTION() and DRM_FB_GET_OPTION() macros are provided to drivers >> for computing options bitfield values and getting the values respectively >> >> For now, only the DRM_FB_BPP option exists but other options can be added. >> >> Suggested-by: Thomas Zimmermann >> Signed-off-by: Javier Martinez Canillas >> Reviewed-by: Thomas Zimmermann >> Reviewed-by: Laurent Pinchart >> --- >> >> Changes in v2: >> - Rename DRM_FB_SET_OPTION() to DRM_FB_SET() and make more clear in the > > I assume you meant DRM_FB_OPTION() here, not DRM_FB_SET(). > >> kernel-doc what this macro does (Laurent Pinchart). >> Right, that's a typo. The patch description and content are correct though. I'll fix the patch history log in v3. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello Laurent,
On 5/2/22 18:17, Laurent Pinchart wrote:
> Hi Javier,
>
> Thank you for the patch.
>
> On Mon, May 02, 2022 at 05:39:00PM +0200, Javier Martinez Canillas wrote:
>> Indicate to fbdev subsystem that the registered framebuffer is provided by
>> the system firmware, so that it can handle accordingly. For example, would
>> unregister the FB devices if asked to remove the conflicting framebuffers.
>>
>> Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter.
>> Drivers can use this to indicate the FB helper initialization that the FB
>> registered is provided by the firmware, so it can be configured as such.
>>
>> Suggested-by: Thomas Zimmermann
>> Signed-off-by: Javier Martinez Canillas
>> Reviewed-by: Thomas Zimmermann
>> ---
>>
>> (no changes since v1)
>>
>> drivers/gpu/drm/drm_fb_helper.c | 9 +
>> drivers/gpu/drm/tiny/simpledrm.c | 2 +-
>> include/drm/drm_fb_helper.h | 10 ++
>> 3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index fd0084ad77c3..775e47c5de1f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct
>> drm_fb_helper *fb_helper,
>> /* don't leak any physical addresses to userspace */
>> info->flags |= FBINFO_HIDE_SMEM_START;
>>
>> +/* Indicate that the framebuffer is provided by the firmware */
>> +if (fb_helper->firmware)
>> +info->flags |= FBINFO_MISC_FIRMWARE;
>> +
>> /* Need to drop locks to avoid recursive deadlock in
>> * register_framebuffer. This is ok because the only thing left to do is
>> * register the fbdev emulation instance in kernel_fb_helper_list. */
>> @@ -2512,6 +2516,8 @@ static const struct drm_client_funcs
>> drm_fbdev_client_funcs = {
>> *
>> * * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
>> * @dev->mode_config.preferred_depth is used instead.
>> + * * DRM_FB_FW: if the framebuffer for the device is provided by the
>> + * system firmware.
>> *
>> * This function sets up generic fbdev emulation for drivers that supports
>> * dumb buffers with a virtual address and that can be mmap'ed.
>> @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>> unsigned int options)
>> {
>> struct drm_fb_helper *fb_helper;
>> unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options);
>> +bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
>> int ret;
>>
>> drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
>> @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>> unsigned int options)
>> preferred_bpp = 32;
>> fb_helper->preferred_bpp = preferred_bpp;
>>
>> +fb_helper->firmware = firmware;
>
> I'd get rid of the local variable and write
>
I actually considered that but then decided to add a local variable to
have both options set in the same place, since I thought that would be
easier to read and also consistent with how preferred_bpp is handled.
Maybe I could go the other way around and rework patch 2/3 to also not
require a preferred_bpp local variable ? That patch won't be as small
as it's now though. --
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
On 5/2/22 18:55, Javier Martinez Canillas wrote:
[snip]
>
>> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
>> comment in drm_fbdev_generic_setup() that could be related.
>>
>
> A FIXME makes sense, I'll add that to when posting a v3.
There's actually a FIXME already in drm_fbdev_generic_setup(), so it's
a documented issue [0]:
void drm_fbdev_generic_setup(struct drm_device *dev,
unsigned int preferred_bpp)
{
...
/*
* FIXME: This mixes up depth with bpp, which results in a glorious
* mess, resulting in some drivers picking wrong fbdev defaults and
* others wrong preferred_depth defaults.
*/
if (!preferred_bpp)
preferred_bpp = dev->mode_config.preferred_depth;
if (!preferred_bpp)
preferred_bpp = 32;
fb_helper->preferred_bpp = preferred_bpp;
...
}
[0]:
https://elixir.bootlin.com/linux/v5.18-rc5/source/drivers/gpu/drm/drm_fb_helper.c#L2553
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
On 5/2/22 20:36, Laurent Pinchart wrote: > On Mon, May 02, 2022 at 07:15:16PM +0200, Javier Martinez Canillas wrote: >> On 5/2/22 18:55, Javier Martinez Canillas wrote: >> >> [snip] >> >>>> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME >>>> comment in drm_fbdev_generic_setup() that could be related. >>> >>> A FIXME makes sense, I'll add that to when posting a v3. >> >> There's actually a FIXME already in drm_fbdev_generic_setup(), so it's >> a documented issue [0]: > > That's what I meant by "there's a FIXME" :-) It doesn't have to be > addressed by this series, but it would be good to fix it. > doh, I misread your original email. Yes, it's the same issue as you said and something that I plan to look at some point as a follow-up. I hope that we could just replace fbcon with a kms/systemd-consoled/foo user-space implementation before fixing all the stuff in the DRM fbdev emulation layer :) -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello Laurent, On 5/2/22 20:01, Laurent Pinchart wrote: > Hi Javier, [snip] >>>> + fb_helper->firmware = firmware; >>> >>> I'd get rid of the local variable and write >>> >> >> I actually considered that but then decided to add a local variable to >> have both options set in the same place, since I thought that would be >> easier to read and also consistent with how preferred_bpp is handled. >> >> Maybe I could go the other way around and rework patch 2/3 to also not >> require a preferred_bpp local variable ? That patch won't be as small >> as it's now though. -- > > Up to you, or you could ignore the comment, it's minor. If you want to > keep the variable, you could also make it const, it's a good practice to > show it isn't intended to be modified. > Right. I'll also do the same for the preferred_bpp variable in patch 2/3 if I choose to keep them in v3. Thanks again for your feedback and comments! -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v3 0/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello, This series contain patches suggested by Thomas Zimmermann as a feedback for "[RFC PATCH v4 00/11] Fix some race between sysfb device registration and drivers probe" [0]. Since other changes in [0] were more controversial, I decided to just split this part in a new patch-set and revisit the rest of the patches later. This is a v2 that addresses issues pointed out in v1. Patch #1 is just a cleanup since when working on this noticed that some DRM drivers were passing as preferred bits per pixel to drm_fbdev_generic_setup() the value that is the default anyways. Patch #2 renames the 'preferred_bpp' drm_fbdev_generic_setup() parameter to 'options', and make this a multi field parameter so that it can be extended later to pass other options as well. Patch #3 finally adds the new DRM_FB_FW option and makes simpledrm to use it so that the registered framebuffer device is also marked as firmware provided. The patches were tested on a rpi4 board with the vc4 DRM driver built as a module and either simplefb or simpledrm built-in. [0]: https://lore.kernel.org/lkml/20220429084253.1085911-1-javi...@redhat.com/ Changes in v3: - Drop the preferred_bpp local variable (Laurent Pinchart). - Add a const qualifier to options parameter (Laurent Pinchart). - Drop the firmware local variable (Laurent Pinchart). - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot). Changes in v2: - Rename DRM_FB_SET_OPTION() to DRM_FB_OPTION() and make more clear in the kernel-doc what this macro does (Laurent Pinchart). - Fix some kernel-doc issues I didn't notice in v1. - Add Reviewed-by tags from Thomas and Laurent. Javier Martinez Canillas (3): drm: Remove superfluous arg when calling to drm_fbdev_generic_setup() drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter drm: Allow simpledrm to setup its emulated FB as firmware provided drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++-- drivers/gpu/drm/arm/hdlcd_drv.c | 2 +- drivers/gpu/drm/arm/malidp_drv.c | 2 +- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +- drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 34 ++- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +- drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +- drivers/gpu/drm/imx/imx-drm-core.c| 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/mcde/mcde_drv.c | 2 +- drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +- drivers/gpu/drm/meson/meson_drv.c | 2 +- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +- drivers/gpu/drm/pl111/pl111_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- drivers/gpu/drm/sti/sti_drv.c | 2 +- drivers/gpu/drm/stm/drv.c | 2 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/drm/tiny/arcpgu.c | 2 +- drivers/gpu/drm/tiny/bochs.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- drivers/gpu/drm/tiny/simpledrm.c | 2 +- drivers/gpu/drm/tve200/tve200_drv.c | 2 +- drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.c | 2 +- drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +- include/drm/drm_fb_helper.h | 24 - 36 files changed, 85 insertions(+), 45 deletions(-) -- 2.35.1
[PATCH v3 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
The drm_fbdev_generic_setup() function already sets the preferred bits per pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp value is zero. Passing the same value to the function is unnecessary. Let's cleanup that in the two drivers that do it. Signed-off-by: Javier Martinez Canillas Reviewed-by: Thomas Zimmermann Reviewed-by: Laurent Pinchart --- (no changes since v1) drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index fe4269c5aa0a..ace92459e462 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -349,7 +349,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, goto err_unload; } - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c index c8e791840862..ed5a2e14894a 100644 --- a/drivers/gpu/drm/tiny/cirrus.c +++ b/drivers/gpu/drm/tiny/cirrus.c @@ -601,7 +601,7 @@ static int cirrus_pci_probe(struct pci_dev *pdev, if (ret) return ret; - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); + drm_fbdev_generic_setup(dev, 0); return 0; } -- 2.35.1
[PATCH v3 2/3] drm/fb-helper: Rename preferred_bpp drm_fbdev_generic_setup() parameter
By default the bits per pixel for the emulated framebuffer device is set
to dev->mode_config.preferred_depth, but some devices need another value.
Since this second parameter is only used by a few drivers, and to allow
drivers to use it for passing other configurations when registering the
fbdev, rename @preferred_bpp to @options and make it a multi-field param.
The DRM_FB_OPTION() and DRM_FB_GET_OPTION() macros are provided to drivers
for computing options bitfield values and getting the values respectively
For now, only the DRM_FB_BPP option exists but other options can be added.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Laurent Pinchart
---
Changes in v3:
- Drop the preferred_bpp local variable (Laurent Pinchart).
- Add a const qualifier to options parameter (Laurent Pinchart).
Changes in v2:
- Rename DRM_FB_SET_OPTION() to DRM_FB_OPTION() and make more clear in
the kernel-doc what this macro does (Laurent Pinchart).
- Fix some kernel-doc issues I didn't notice in v1.
- Add Reviewed-by tags from Thomas and Laurent.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 +++--
drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
drivers/gpu/drm/arm/malidp_drv.c | 2 +-
drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/gpu/drm/drm_fb_helper.c | 26 ---
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 2 +-
.../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +-
drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 +-
drivers/gpu/drm/imx/imx-drm-core.c| 2 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
drivers/gpu/drm/mcde/mcde_drv.c | 2 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +-
drivers/gpu/drm/meson/meson_drv.c | 2 +-
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +-
drivers/gpu/drm/sti/sti_drv.c | 2 +-
drivers/gpu/drm/stm/drv.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +-
drivers/gpu/drm/tidss/tidss_drv.c | 2 +-
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
drivers/gpu/drm/tiny/arcpgu.c | 2 +-
drivers/gpu/drm/tiny/bochs.c | 2 +-
drivers/gpu/drm/tve200/tve200_drv.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c | 2 +-
drivers/gpu/drm/vc4/vc4_drv.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 2 +-
include/drm/drm_fb_helper.h | 14 +-
33 files changed, 64 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b03663f42cc9..0c54470975e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2118,9 +2118,11 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
!list_empty(&adev_to_drm(adev)->mode_config.connector_list)) {
/* select 8 bpp console on low vram cards */
if (adev->gmc.real_vram_size <= (32*1024*1024))
- drm_fbdev_generic_setup(adev_to_drm(adev), 8);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_OPTION(DRM_FB_BPP, 8));
else
- drm_fbdev_generic_setup(adev_to_drm(adev), 32);
+ drm_fbdev_generic_setup(adev_to_drm(adev),
+ DRM_FB_OPTION(DRM_FB_BPP, 32));
}
ret = amdgpu_debugfs_init(adev);
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e89ae0ec60eb..b69b1e5be379 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -321,7 +321,7 @@ static int hdlcd_drm_bind(struct device *dev)
if (ret)
goto err_register;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d5aef21426cf..25685b579a05 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -863,7 +863,7 @@ static int malidp_bind(struct device *dev)
if (ret)
goto register_fail;
- drm_fbdev_generic_setup(drm, 32);
+ drm_fbdev_generic_setup(drm, DRM_FB_OPTION(DRM_FB_BPP, 32));
return 0;
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv
[PATCH v3 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Indicate to fbdev subsystem that the registered framebuffer is provided by
the system firmware, so that it can handle accordingly. For example, would
unregister the FB devices if asked to remove the conflicting framebuffers.
Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter.
Drivers can use this to indicate the FB helper initialization that the FB
registered is provided by the firmware, so it can be configured as such.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Laurent Pinchart
---
Changes in v3:
- Drop the firmware local variable (Laurent Pinchart).
- Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot).
drivers/gpu/drm/drm_fb_helper.c | 8
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
include/drm/drm_fb_helper.h | 10 ++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 52042ba1e4cf..28b21858b726 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct
drm_fb_helper *fb_helper,
/* don't leak any physical addresses to userspace */
info->flags |= FBINFO_HIDE_SMEM_START;
+ /* Indicate that the framebuffer is provided by the firmware */
+ if (fb_helper->firmware)
+ info->flags |= FBINFO_MISC_FIRMWARE;
+
/* Need to drop locks to avoid recursive deadlock in
* register_framebuffer. This is ok because the only thing left to do is
* register the fbdev emulation instance in kernel_fb_helper_list. */
@@ -2512,6 +2516,8 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
*
* * DRM_FB_BPP: bits per pixel for the device. If the field is not set,
* @dev->mode_config.preferred_depth is used instead.
+ * * DRM_FB_FW: if the framebuffer for the device is provided by the
+ * system firmware.
*
* This function sets up generic fbdev emulation for drivers that supports
* dumb buffers with a virtual address and that can be mmap'ed.
@@ -2569,6 +2575,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
const unsigned int options)
if (!fb_helper->preferred_bpp)
fb_helper->preferred_bpp = 32;
+ fb_helper->firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options);
+
ret = drm_fbdev_client_hotplug(&fb_helper->client);
if (ret)
drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index f5b8e864a5cd..f6f1c5e108b2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev)
if (ret)
return ret;
- drm_fbdev_generic_setup(dev, 0);
+ drm_fbdev_generic_setup(dev, DRM_FB_OPTION(DRM_FB_FW, 1));
return 0;
}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5fc41cf0c987..5a17af423944 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -44,6 +44,7 @@ enum mode_set_atomic {
};
#define DRM_FB_BPP_MASK GENMASK(7, 0)
+#define DRM_FB_FW_MASK GENMASK(8, 8)
/* Using the GNU statement expression extension */
#define DRM_FB_OPTION(option, value) \
@@ -197,6 +198,15 @@ struct drm_fb_helper {
* See also: @deferred_setup
*/
int preferred_bpp;
+
+ /**
+* @firmware:
+*
+* Set if the driver indicates to the FB helper initialization that the
+* framebuffer for the device being registered is provided by firmware,
+* so that it can pass this on when registering the framebuffer device.
+*/
+ bool firmware;
};
static inline struct drm_fb_helper *
--
2.35.1
Re: [PATCH 1/4] drm/format-helper: Implement drm_fb_swab() with per-line helpers
Hello Thomas, On 4/27/22 16:14, Thomas Zimmermann wrote: > Replace the inner loop of drm_fb_swab() with helper functions that > swap the bytes in each pixel. This will allow to share the outer > loop with other conversion helpers. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas I've a meta question though. Something that I never fully understood in the conversion helpers is if there is some convention about the name of the parameters. Since it seems that in some places we use dbuf, sbuf but in others we use src and dst, and so forth. If is just that the naming haven't been used consistently, maybe this rework (or a follow-up) could be an opportunity to add consistency in that regard. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/4] drm/format-helper: Remove optional byte-swap from line convertion
Hello Thomas, On 4/27/22 16:14, Thomas Zimmermann wrote: > Implement per-pixel byte swapping in a separate conversion helper > for the single function that requires it. Select the correct helper > for each conversion. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas I couldn't spot any issues but I believe these conversion helpers are not easy to read and one could miss subtle issues so it would be nice to have unit tests for these using the KUnit infrastructure: https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html I'm not asking to do it as a part of this series but something that we may do as a follow-up though, just mentioning. I'll post a patch to add that task to Documentation/gpu/todo.rst, so it is tracked somewhere. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 3/4] drm/format-helper: Unify the parameters of all per-line conversion helpers
On 4/27/22 16:14, Thomas Zimmermann wrote: > Give each per-line conversion helper pointers of type void and the > number of pixels in the line. Remove the unused swab parameters. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 4/4] drm/format-helper: Share implementation among conversion helpers
On 4/27/22 16:14, Thomas Zimmermann wrote: > Provide format-independent conversion helpers for system and I/O > memory. Implement most existing helpers on top of it. The source and > destination formats of each conversion is handled by a per-line > helper that is given to the generic implementation. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
On 5/2/22 15:50, Javier Martinez Canillas wrote:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> The bug has been present for a long time but commit 27599aacbaef ("fbdev:
> Hot-unplug firmware fb devices on forced removal") unmasked it since the
> fbdev core started unregistering the framebuffers' devices associated.
>
> Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced
> removal")
> Reported-by: Maxime Ripard
> Reported-by: Junxiao Chang
> Signed-off-by: Javier Martinez Canillas
> Reviewed-by: Thomas Zimmermann
> ---
Applied to drm-misc (drm-misc-fixes).
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH] fbdev: Use helper to get fb_info in all file operations
A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.
This can happen for example if the registered framebuffer device is for a
driver that just uses a framebuffer provided by the system firmware. In
that case, the fbdev core would unregister the framebuffer device when a
real video driver is probed and ask to remove conflicting framebuffers.
Most fbdev file operations already use the helper to get the fb_info but
get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
exported. Rename it and add a fb_ prefix to denote that is public now.
Reported-by: Junxiao Chang
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_defio.c | 5 -
drivers/video/fbdev/core/fbmem.c| 24 +++-
include/linux/fb.h | 1 +
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index 842c66b3e33d..ac1c88b3fbb5 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int
datasync)
{
- struct fb_info *info = file->private_data;
+ struct fb_info *info = fb_file_fb_info(file->private_data);
struct inode *inode = file_inode(file);
int err = file_write_and_wait_range(file, start, end);
if (err)
return err;
+ if (!info)
+ return -ENODEV;
+
/* Skip if deferred io is compiled-in but disabled on this fbdev */
if (!info->fbdefio)
return 0;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 97eb0dee411c..f924fda89dd5 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused
proc_fb_seq_ops = {
* So look up the fb_info using the inode minor number,
* and just verify it against the reference we have.
*/
-static struct fb_info *file_fb_info(struct file *file)
+struct fb_info *fb_file_fb_info(struct file *file)
{
struct inode *inode = file_inode(file);
int fbidx = iminor(inode);
@@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
info = NULL;
return info;
}
+EXPORT_SYMBOL(fb_file_fb_info);
static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *dst;
u8 __iomem *src;
int c, cnt = 0, err = 0;
@@ -825,7 +826,7 @@ static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *src;
u8 __iomem *dst;
int c, cnt = 0, err = 0;
@@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned
int cmd,
static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info,
unsigned int cmd,
static long fb_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
const struct fb_ops *fb;
long ret = -ENOIOCTLCMD;
@@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned
int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
unsigned long mmio_pgoff;
unsigned long start;
@@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file_fb_info(file);
+ struct fb_info * const info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp,
unsigned
[PATCH v2] fbdev: Use helper to get fb_info in all file operations
A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.
This can happen for example if the registered framebuffer device is for a
driver that just uses a framebuffer provided by the system firmware. In
that case, the fbdev core would unregister the framebuffer device when a
real video driver is probed and ask to remove conflicting framebuffers.
Most fbdev file operations already use the helper to get the fb_info but
get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
exported. Rename it and add a fb_ prefix to denote that is public now.
Reported-by: Junxiao Chang
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Fix copy & paste error passing file->private_data instead of file
to fb_file_fb_info() function (Sam Ravnborg).
drivers/video/fbdev/core/fb_defio.c | 5 -
drivers/video/fbdev/core/fbmem.c| 24 +++-
include/linux/fb.h | 1 +
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index 842c66b3e33d..ccdf903c48bd 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int
datasync)
{
- struct fb_info *info = file->private_data;
+ struct fb_info *info = fb_file_fb_info(file);
struct inode *inode = file_inode(file);
int err = file_write_and_wait_range(file, start, end);
if (err)
return err;
+ if (!info)
+ return -ENODEV;
+
/* Skip if deferred io is compiled-in but disabled on this fbdev */
if (!info->fbdefio)
return 0;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 97eb0dee411c..ba2c14a1087d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused
proc_fb_seq_ops = {
* So look up the fb_info using the inode minor number,
* and just verify it against the reference we have.
*/
-static struct fb_info *file_fb_info(struct file *file)
+struct fb_info *fb_file_fb_info(struct file *file)
{
struct inode *inode = file_inode(file);
int fbidx = iminor(inode);
@@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
info = NULL;
return info;
}
+EXPORT_SYMBOL(fb_file_fb_info);
static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *dst;
u8 __iomem *src;
int c, cnt = 0, err = 0;
@@ -825,7 +826,7 @@ static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *src;
u8 __iomem *dst;
int c, cnt = 0, err = 0;
@@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned
int cmd,
static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info,
unsigned int cmd,
static long fb_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
const struct fb_ops *fb;
long ret = -ENOIOCTLCMD;
@@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned
int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
unsigned long mmio_pgoff;
unsigned long start;
@@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file_fb_info(file);
+ struct fb_info * const info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -
Re: [PATCH] fbdev: Use helper to get fb_info in all file operations
Hello Sam,
On 5/3/22 19:14, Sam Ravnborg wrote:
> Hi Javier,
>
[snip]
>>
>> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int
>> datasync)
>> {
>> -struct fb_info *info = file->private_data;
>> +struct fb_info *info = fb_file_fb_info(file->private_data);
> This looks wrong. I assume you wanted to write:
>> +struct fb_info *info = fb_file_fb_info(file);
Right I did. Thanks for pointing out.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH] drm/todo: Add entry for using kunit in the subsystem
The Kernel Unit Testing (KUnit) framework provides a common framework for unit tests within the Linux kernel. Having a test suite would allow to identify regressions earlier. Signed-off-by: Javier Martinez Canillas --- Documentation/gpu/todo.rst | 14 ++ 1 file changed, 14 insertions(+) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 127e76ee0b2d..10bfb50908d1 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -603,6 +603,20 @@ Level: Advanced Better Testing == +Add unit tests using the Kernel Unit Testing (KUnit) framework +-- + +The `KUnit <https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html>`_ +provides a common framework for unit tests within the Linux kernel. Having a +test suite would allow to identify regressions earlier. + +A good candidate for the first unit tests are the format-conversion helpers in +``drm_format_helper.c``. + +Contact: Javier Martinez Canillas + +Level: Intermediate + Enable trinity for DRM -- -- 2.35.1
Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations
Hello Thomas, On 5/4/22 10:15, Thomas Zimmermann wrote: > Hi > > Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas: >> A reference to the framebuffer device struct fb_info is stored in the file >> private data, but this reference could no longer be valid and must not be >> accessed directly. Instead, the file_fb_info() accessor function must be >> used since it does sanity checking to make sure that the fb_info is valid. >> >> This can happen for example if the registered framebuffer device is for a >> driver that just uses a framebuffer provided by the system firmware. In >> that case, the fbdev core would unregister the framebuffer device when a >> real video driver is probed and ask to remove conflicting framebuffers. >> >> Most fbdev file operations already use the helper to get the fb_info but >> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two. >> >> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be >> exported. Rename it and add a fb_ prefix to denote that is public now. >> >> Reported-by: Junxiao Chang >> Signed-off-by: Javier Martinez Canillas > > Reviewed-by: Thomas Zimmermann > Thanks. > Please see my comment below. [snip] >> >> +if (!info) >> +return -ENODEV; >> + > > This is consistent with other functions, but it's probably not the > correct errno code. It means that a device is not available for opening. > > But the situation here is rather as with close() on a > disconnected-network file. The call to close() returns EIO in this case. > Maybe we should consider changing this in a separate patch. > Indeed. Agree that -EIO makes more sense here. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/todo: Add entry for using kunit in the subsystem
Hello Maxime, Thanks for your feedback. On 5/4/22 10:10, Maxime Ripard wrote: > Hi, > > On Wed, May 04, 2022 at 10:02:12AM +0200, Javier Martinez Canillas wrote: >> The Kernel Unit Testing (KUnit) framework provides a common framework for >> unit tests within the Linux kernel. Having a test suite would allow to >> identify regressions earlier. >> >> Signed-off-by: Javier Martinez Canillas >> --- >> >> Documentation/gpu/todo.rst | 14 ++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst >> index 127e76ee0b2d..10bfb50908d1 100644 >> --- a/Documentation/gpu/todo.rst >> +++ b/Documentation/gpu/todo.rst >> @@ -603,6 +603,20 @@ Level: Advanced >> Better Testing >> == >> >> +Add unit tests using the Kernel Unit Testing (KUnit) framework >> +-- >> + >> +The `KUnit >> <https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html>`_ >> +provides a common framework for unit tests within the Linux kernel. Having a >> +test suite would allow to identify regressions earlier. >> + >> +A good candidate for the first unit tests are the format-conversion helpers >> in >> +``drm_format_helper.c``. >> + >> +Contact: Javier Martinez Canillas >> + >> +Level: Intermediate > > Kunit is fairly easy to grasp if you have some knowledge of other unit > testing frameworks already (pytest, cmocka, etc.) > Yes, I didn't set to intermediate due kunit but rather due the format conversions, since the functions are not easy to read and understand. And the person writing the unit tests will have to get familiar with the different formats to verify that conversions are done correctly. > Another good candidate would be to convert (some ?) selftests to kunit. > I'm not sure the others, but at least test-drm_cmdline_parser should be > fairly easy to convert. > Indeed. Maybe I would add it as a separate entr though, as a follow-up. > Maxime -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations
Hello Daniel,
On 5/4/22 11:02, Daniel Vetter wrote:
> On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
>> A reference to the framebuffer device struct fb_info is stored in the file
>> private data, but this reference could no longer be valid and must not be
>> accessed directly. Instead, the file_fb_info() accessor function must be
>> used since it does sanity checking to make sure that the fb_info is valid.
>>
>> This can happen for example if the registered framebuffer device is for a
>> driver that just uses a framebuffer provided by the system firmware. In
>> that case, the fbdev core would unregister the framebuffer device when a
>> real video driver is probed and ask to remove conflicting framebuffers.
>>
>> Most fbdev file operations already use the helper to get the fb_info but
>> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>>
>> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
>> exported. Rename it and add a fb_ prefix to denote that is public now.
>>
>> Reported-by: Junxiao Chang
>> Signed-off-by: Javier Martinez Canillas
>
> Note that fb_file_info is hilariously racy since there's nothing
> preventing a concurrenct framebuffer_unregister. Or at least I'm not
> seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
> context, maybe reference that commit here in your patch.
>
> Either way this doesn't really make anything worse, so
> Acked-by: Daniel Vetter
>
Yes, I noticed is racy but at least checking this makes less likely to
occur. And thanks, I'll reference that commit in the description of v3.
BTW, I also noticed that the same race that happens with open(),read(),
close(), etc happens with the VM operations:
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
...
vma->vm_private_data = info;
...
}
static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
{
...
struct fb_info *info = vmf->vma->vm_private_data;
...
}
static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
{
...
struct fb_info *info = vmf->vma->vm_private_data;
...
}
So something similar to fb_file_fb_info() is needed to check if
the vm_private_data is still valid. I guess that could be done
by using the vmf->vma->vm_file and attempting the same trick that
fb_file_fb_info() does ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
Hello Daniel, On 5/4/22 11:47, Daniel Vetter wrote: > On Mon, May 02, 2022 at 03:09:44PM +0200, Javier Martinez Canillas wrote: >> A reference to the framebuffer device struct fb_info is stored in the file >> private data, but this reference could no longer be valid and must not be >> accessed directly. Instead, the file_fb_info() accessor function must be >> used since it does sanity checking to make sure that the fb_info is valid. >> >> This can happen for example if the fbdev driver was one that is using a >> framebuffer provided by the system firmware. In that case, the fbdev core >> could unregister the framebuffer device if a real video driver is probed. >> >> Reported-by: Maxime Ripard >> Signed-off-by: Javier Martinez Canillas > > Doesn't this mean we just leak the references? Also anything the driver > might refcount in fb_open would be leaked too. > It maybe result in leaks, that's true. But I don't think we can do anything at this point since the fb_info just went away and the file->private_data reference is no longer valid... > I'm not sure what exactly you're trying to fix here, but this looks a bit > wrong. > This is fixing a NULL pointer deref that at least 3 people reported, i.e: https://github.com/raspberrypi/linux/issues/5011 Because if a real DRM driver is probed, then the registered framebuffer is unregistered and the fb_info just freed. But user-space has no way to know and on close the kernel will try to dereference a NULL pointer. > Maybe stepping back what fbdev would need, but doesn't have (see the > commit reference I dropped on the previous version) is drm_dev_enter/exit > around hw access. the file_fb_info check essentially provides that, but > with races and everything. > Yes, but I don't know how that could work since user-space can just open the fbdev, mmap it, write to the mmap'ed memory and then close it. The only way that this could be done safely AFAICT is if we prevent the real video drivers to be registered if the fbdev is currently mmap'ed. Otherwise, the firmware initialized framebuffer will go away anyways and things will break for the user-space process that's currently using it. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] Revert "fbdev: Make fb_release() return -ENODEV if fbdev was unregistered"
This reverts commit aafa025c76dcc7d1a8c8f0bdefcbe4eb480b2f6a. That commit
attempted to fix a NULL pointer dereference, caused by the struct fb_info
associated with a framebuffer device to not longer be valid when the file
descriptor was closed.
But the solution was wrong since it was just papering over the issue, and
also would leak any resources that might be reference counted in fb_open.
Instead, the fbdev drivers that are releasing the fb_info too soon should
be fixed to prevent this situation to happen.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbmem.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 97eb0dee411c..a6bb0e438216 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1434,10 +1434,7 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file_fb_info(file);
-
- if (!info)
- return -ENODEV;
+ struct fb_info * const info = file->private_data;
lock_fb_info(info);
if (info->fbops->fb_release)
--
2.35.1
Re: [PATCH v3 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
On 5/4/22 12:58, Daniel Vetter wrote: > On Tue, May 03, 2022 at 09:15:40AM +0200, Javier Martinez Canillas wrote: >> Indicate to fbdev subsystem that the registered framebuffer is provided by >> the system firmware, so that it can handle accordingly. For example, would >> unregister the FB devices if asked to remove the conflicting framebuffers. >> >> Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter. >> Drivers can use this to indicate the FB helper initialization that the FB >> registered is provided by the firmware, so it can be configured as such. >> >> Suggested-by: Thomas Zimmermann >> Signed-off-by: Javier Martinez Canillas >> Reviewed-by: Thomas Zimmermann >> Reviewed-by: Laurent Pinchart >> --- >> >> Changes in v3: >> - Drop the firmware local variable (Laurent Pinchart). >> - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test >> robot). > > Just for the record what I brought up on irc already: > > FBINFO_MISC_FIRMWARE is purely an internal flag with no uapi impact, and > it's only to control whether we nuke this from > remove_conflicting_framebuffer or not. Since simpledrm only ever binds > against sysfb I think it'd be cleaner to only rely on that, and relegate That's not actually true. The OF subsystem also registers "simple-framebuffer" devices when there are Device Tree nodes that contain a "simple-framebuffer" compatible string. In that case these pdev will also bind against simpledrm. > that entire FBINFO_MISC_FIRMWARE misc hack to the fbdev dungeons and let > it quietly wither away there. > > Also I'm not a huge fan of these midlayer flags in general :-) And while I agree with you that these midlayer flags are horrible, that is what any other fbdev that makes use of a firmware-provided framebuffer set, so simpledrm emulated fbdev shouldn't be the exception IMO. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations
Hello Thomas, On 5/4/22 13:08, Thomas Zimmermann wrote: [snip] >>> So something similar to fb_file_fb_info() is needed to check if >>> the vm_private_data is still valid. I guess that could be done >>> by using the vmf->vma->vm_file and attempting the same trick that >>> fb_file_fb_info() does ? >> >> Yeah should work, except if the ptes are set up already there's kinda not >> much that this will prevent. We'd need to tear down mappings and SIGBUS or >> alternatively have something else in place there so userspace doesn't blow >> up in funny ways (which is what we're doing on the drm side, or at least >> trying to). >> >> I'm also not sure how much we should care, since ideally for drm drivers >> this is all taken care of by drm_dev_enter in the right places. It does >> mean though that fbdev mmap either needs to have it's own memory or be >> fully redirected to the drm gem mmap. >> >> And then we can afford to just not care to fix fbdev itself. > > While the problem has been there ever since, the bug didn't happen until > we fixed hot-unplugging for fbdev. Not doing anything is probably not > the right thing. > Actually, this issue shouldn't happen if the fbdev drivers are not buggy and do the proper cleanup at .fb_release() time rather than at .remove(). I'll post patches for simplefb and efifb which are the drivers that we mostly care at this point. So we should be good and not need more fixes. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v2] Revert "fbdev: Make fb_release() return -ENODEV if fbdev was unregistered"
This reverts commit aafa025c76dcc7d1a8c8f0bdefcbe4eb480b2f6a. That commit
attempted to fix a NULL pointer dereference, caused by the struct fb_info
associated with a framebuffer device to not longer be valid when the file
descriptor was closed.
The issue was exposed by commit 27599aacbaef ("fbdev: Hot-unplug firmware
fb devices on forced removal"), which added a new path that goes through
the struct device removal instead of directly unregistering the fb.
Most fbdev drivers have issues with the fb_info lifetime, because call to
framebuffer_release() from their driver's .remove callback, rather than
doing from fbops.fb_destroy callback. This meant that due to this switch,
the fb_info was now destroyed too early, while references still existed,
while before it was simply leaked.
The patch we're reverting here reinstated that leak, hence "fixed" the
regression. But the proper solution is to fix the drivers to not release
the fb_info too soon.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Vetter
---
Changes in v2:
- Add more info in the commit message about why it's crashing and how
the reverted commit was papering over the issue (Daniel Vetter).
- Add Daniel Vetter's Reviewed-by tag.
drivers/video/fbdev/core/fbmem.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 97eb0dee411c..a6bb0e438216 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1434,10 +1434,7 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file_fb_info(file);
-
- if (!info)
- return -ENODEV;
+ struct fb_info * const info = file->private_data;
lock_fb_info(info);
if (info->fbops->fb_release)
--
2.35.1
Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"
Hello Alex,
On 5/4/22 15:48, Alex Deucher wrote:
> This reverts commit b95dc06af3e683d6b7ddbbae178b2b2a21ee8b2b.
>
> This workaround is no longer necessary. We have a better workaround
> in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
> displays attached (v3)").
>
I would write this line instead as:
in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
displays attached (v3)").
> Signed-off-by: Alex Deucher
> ---
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] Revert "fbdev: fbmem: add a helper to determine if an aperture is used by a fw fb"
On 5/4/22 15:48, Alex Deucher wrote:
> This reverts commit 9a45ac2320d0a6ae01880a30d4b86025fce4061b.
>
> This was added a helper for amdgpu to workaround a runtime pm regression
> caused by a runtime pm fix in efifb. We now have a better workarouund
s/workarouund/workaround
> in amdgpu in
> commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
> displays attached (v3)")
Again I would write it as:
commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
displays attached (v3)")
> so this workaround is no longer necessary. Since amdgpu was the only
> user of this interface, we can remove it.
>
> Signed-off-by: Alex Deucher
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"
On 5/4/22 18:50, Alex Deucher wrote:
> On Wed, May 4, 2022 at 12:46 PM Javier Martinez Canillas
> wrote:
>>
>> Hello Alex,
>>
>> On 5/4/22 15:48, Alex Deucher wrote:
>>> This reverts commit b95dc06af3e683d6b7ddbbae178b2b2a21ee8b2b.
>>>
>>> This workaround is no longer necessary. We have a better workaround
>>> in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
>>> displays attached (v3)").
>>>
>>
>> I would write this line instead as:
>>
>> in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are
>> displays attached (v3)").
>
> When you do it that way checkpatch and some maintainers complain about
> splitting up a commit line across multiple lines.
>
It does indeed, how silly. Scratch my comment then.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers
Hello, This series contains patches suggested by Daniel Vetter to fix a use-after-free error in the fb_release() function, due a fb_info associated with a fbdev being freed too early while a user-space process still has the fbdev dev node opened. The is cuused by a wrong management of the struct fb_info lifetime in drivers, but the fbdev core can also be made more resilient about it an leak This can easily be reproduced with the simplefb driver doing the following: $ cat < /dev/fb0 & $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind $ kill %1 [ 257.490471] [ cut here ] ... [ 257.495125] refcount_t: underflow; use-after-free. [ 257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144 ... [ 257.637482] pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 257.61] pc : refcount_warn_saturate+0xf4/0x144 [ 257.649226] lr : refcount_warn_saturate+0xf4/0x144 [ 257.654009] sp : 8a06bbf0 [ 257.657315] x29: 8a06bbf0 x28: 000a x27: 000a [ 257.664448] x26: x25: 470b88c6a180 x24: 000a [ 257.671581] x23: 470b81706480 x22: 470b808c2160 x21: 470b8922ba20 [ 257.678713] x20: 470b891f5810 x19: 470b891f5800 x18: [ 257.685846] x17: 3a725f7463656a62 x16: bb18c6465fd4 x15: 0720072007200720 [ 257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375 [ 257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : bb18c58f6c90 [ 257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0001 [ 257.714373] x5 : 470bff8ec418 x4 : x3 : 0027 [ 257.721506] x2 : x1 : 0027 x0 : 0026 [ 257.728638] Call trace: [ 257.731075] refcount_warn_saturate+0xf4/0x144 [ 257.735513] put_fb_info+0x70/0x7c [ 257.738916] fb_release+0x60/0x74 [ 257.742225] __fput+0x88/0x240 [ 257.745276] fput+0x1c/0x30 [ 257.748410] task_work_run+0xc4/0x21c [ 257.752066] do_exit+0x170/0x370 [ 257.755288] do_group_exit+0x40/0xb4 [ 257.758858] get_signal+0x8e0/0x90c [ 257.762339] do_signal+0x1a0/0x280 [ 257.765733] do_notify_resume+0xc8/0x390 [ 257.769650] el0_da+0xe8/0xf0 [ 257.772613] el0t_64_sync_handler+0xe8/0x130 [ 257.776877] el0t_64_sync+0x190/0x194 [ 257.780534] ---[ end trace ]--- Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free to happen. Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to free the resources at the correct place. Daniel Vetter (1): fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas (2): fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove drivers/video/fbdev/core/fbsysfs.c | 4 drivers/video/fbdev/efifb.c| 9 - drivers/video/fbdev/simplefb.c | 8 +++- 3 files changed, 19 insertions(+), 2 deletions(-) -- 2.35.1
[PATCH 1/3] fbdev: Prevent possible use-after-free in fb_release()
From: Daniel Vetter
Most fbdev drivers have issues with the fb_info lifetime, because call to
framebuffer_release() from their driver's .remove callback, rather than
doing from fbops.fb_destroy callback.
Doing that will destroy the fb_info too early, while references to it may
still exist, leading to a use-after-free error.
To prevent this, check the fb_info reference counter when attempting to
kfree the data structure in framebuffer_release(). That will leak it but
at least will prevent the mentioned error.
Signed-off-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbsysfs.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/video/fbdev/core/fbsysfs.c
b/drivers/video/fbdev/core/fbsysfs.c
index 26892940c213..82e31a2d845e 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
{
if (!info)
return;
+
+ if (WARN_ON(refcount_read(&info->count)))
+ return;
+
kfree(info->apertures);
kfree(info);
}
--
2.35.1
[PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/simplefb.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
static void simplefb_clocks_destroy(struct simplefb_par *par);
static void simplefb_regulators_destroy(struct simplefb_par *par);
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void simplefb_destroy(struct fb_info *info)
{
struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
if (info->screen_base)
iounmap(info->screen_base);
+ framebuffer_release(info);
+
if (mem)
release_mem_region(mem->start, resource_size(mem));
}
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* simplefb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH 3/3] fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/efifb.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
static inline void efifb_show_boot_graphics(struct fb_info *info) {}
#endif
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void efifb_destroy(struct fb_info *info)
{
if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
else
memunmap(info->screen_base);
}
+
+ framebuffer_release(info);
+
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* efifb_destroy takes care of info cleanup */
unregister_framebuffer(info);
sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
- framebuffer_release(info);
return 0;
}
--
2.35.1
Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
Hello Thomas,
On 5/5/22 09:29, Thomas Zimmermann wrote:
[snip]
>> static void simplefb_destroy(struct fb_info *info)
>> {
>> struct simplefb_par *par = info->par;
>> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>> if (info->screen_base)
>> iounmap(info->screen_base);
>>
>> +framebuffer_release(info);
>> +
>> if (mem)
>> release_mem_region(mem->start, resource_size(mem));
>
> The original problem with fbdev hot-unplug was that vmwgfx needed the
> framebuffer region to be released. If we release it only after userspace
> closed it's final file descriptor, vmwgfx could have already failed.
>
> I still don't fully get why this code apparently works or at least
> doesn't blow up occasionally. Any ideas?
>
I believe that vmwgfx doesn't fail to probe (or any other DRM driver)
only when there are not user-space processes with a fbdev node opened
since otherwise as you said the memory wouldn't be released yet.
unregister_framebuffer() is called from the driver's .remove handler
and that decrement the fb_info refcount, so if reaches zero it will
call to the fb fops .destroy() handler and release the I/O memory.
In other words, in most cases (i.e: only fbcon bound to the fbdev)
the driver's removal/ device unbind and the memory release will be
at the same time.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
Hello Thomas,
On 5/5/22 10:05, Thomas Zimmermann wrote:
[snip]
>>
>> In other words, in most cases (i.e: only fbcon bound to the fbdev)
>> the driver's removal/ device unbind and the memory release will be
>> at the same time.
>>
>
> We're one the same page here, but it's still sort of a mystery to me why
> this works in practice.
>
> I'm specifically talking about pci_request_regions() in vmwgfx [1]. IIRC
> this would fail if simplefb still owns the framebuffer region. Lots of
> systems run Plymouth during boot and this should result in failures
> occasionally. Still, we never heard about anything.
>
Yes, I think is because Plymouth IIUC waits for a /dev/dri/card? to be
present and only uses a /dev/fb? as a fallback if a timeout expires.
At least in Fedora (even before the efifb -> simpledrm change) it will
use KMS/DRM since the DRM kernel module for the graphics device in the
machine would be in the intird.
So efifb was only used for fbcon and plymouth would only use DRM/KMS
and not its fbdev backend.
This seems to be sort of a corner case when you have {efi,simple}fb
in the early boot but the real DRM module only in the rootfs after the
initrd has done a pivot_root(2).
> Of course, it's always been broken (even long before real fbdev
> hotunplugging). Switching to simpledrm resolves the problem.
>
Indeed. My opinion after dealing with these fbdev problems is that we
shouldn't try to fix all possible corner cases and just try to get rid
of fbdev as soon as possible.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers
Hello Thomas, On 5/5/22 10:16, Thomas Zimmermann wrote: [snip] >> Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the >> use-after-free >> to happen. >> >> Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to >> free the resources at the correct place. > > From a quick look, vesafb seems to be affected as well. > Right, I wrongly assumed that we only cared about efifb and simplefb but forgot that vesafb is used when setting a VESA mode with vga=foo. I'll add it in a v2. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/todo: Add entry for using kunit in the subsystem
On 5/4/22 10:02, Javier Martinez Canillas wrote: > The Kernel Unit Testing (KUnit) framework provides a common framework for > unit tests within the Linux kernel. Having a test suite would allow to > identify regressions earlier. > > Signed-off-by: Javier Martinez Canillas > --- Pushed to drm-misc (drm-misc-next). Thanks all! -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v2 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers
Hello, This series contains patches suggested by Daniel Vetter to fix a use-after-free error in the fb_release() function, due a fb_info associated with a fbdev being freed too early while a user-space process still has the fbdev dev node opened. That is caused by a wrong management of the struct fb_info lifetime in drivers, but the fbdev core can also be made more resilient about it an leak This can easily be reproduced with the simplefb driver doing the following: $ cat < /dev/fb0 & $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind $ kill %1 [ 257.490471] [ cut here ] ... [ 257.495125] refcount_t: underflow; use-after-free. [ 257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144 ... [ 257.637482] pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 257.61] pc : refcount_warn_saturate+0xf4/0x144 [ 257.649226] lr : refcount_warn_saturate+0xf4/0x144 [ 257.654009] sp : 8a06bbf0 [ 257.657315] x29: 8a06bbf0 x28: 000a x27: 000a [ 257.664448] x26: x25: 470b88c6a180 x24: 000a [ 257.671581] x23: 470b81706480 x22: 470b808c2160 x21: 470b8922ba20 [ 257.678713] x20: 470b891f5810 x19: 470b891f5800 x18: [ 257.685846] x17: 3a725f7463656a62 x16: bb18c6465fd4 x15: 0720072007200720 [ 257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375 [ 257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : bb18c58f6c90 [ 257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0001 [ 257.714373] x5 : 470bff8ec418 x4 : x3 : 0027 [ 257.721506] x2 : x1 : 0027 x0 : 0026 [ 257.728638] Call trace: [ 257.731075] refcount_warn_saturate+0xf4/0x144 [ 257.735513] put_fb_info+0x70/0x7c [ 257.738916] fb_release+0x60/0x74 [ 257.742225] __fput+0x88/0x240 [ 257.745276] fput+0x1c/0x30 [ 257.748410] task_work_run+0xc4/0x21c [ 257.752066] do_exit+0x170/0x370 [ 257.755288] do_group_exit+0x40/0xb4 [ 257.758858] get_signal+0x8e0/0x90c [ 257.762339] do_signal+0x1a0/0x280 [ 257.765733] do_notify_resume+0xc8/0x390 [ 257.769650] el0_da+0xe8/0xf0 [ 257.772613] el0t_64_sync_handler+0xe8/0x130 [ 257.776877] el0t_64_sync+0x190/0x194 [ 257.780534] ---[ end trace ]--- Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free to happen. Patch #2, #3 and #4 fix the simplefb, efifb and vesafb drivers respectively, to free the resources at the correct place. Changes in v2: - Also do the change for vesafb (Thomas Zimmermann). Daniel Vetter (1): fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas (3): fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove drivers/video/fbdev/core/fbsysfs.c | 4 drivers/video/fbdev/efifb.c| 9 - drivers/video/fbdev/simplefb.c | 8 +++- drivers/video/fbdev/vesafb.c | 14 +++--- 4 files changed, 30 insertions(+), 5 deletions(-) -- 2.35.1
[PATCH v2 1/4] fbdev: Prevent possible use-after-free in fb_release()
From: Daniel Vetter
Most fbdev drivers have issues with the fb_info lifetime, because call to
framebuffer_release() from their driver's .remove callback, rather than
doing from fbops.fb_destroy callback.
Doing that will destroy the fb_info too early, while references to it may
still exist, leading to a use-after-free error.
To prevent this, check the fb_info reference counter when attempting to
kfree the data structure in framebuffer_release(). That will leak it but
at least will prevent the mentioned error.
Signed-off-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
(no changes since v1)
drivers/video/fbdev/core/fbsysfs.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/video/fbdev/core/fbsysfs.c
b/drivers/video/fbdev/core/fbsysfs.c
index 8c1ee9ecec3d..c2a60b187467 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
{
if (!info)
return;
+
+ if (WARN_ON(refcount_read(&info->count)))
+ return;
+
kfree(info->apertures);
kfree(info);
}
--
2.35.1
[PATCH v2 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
(no changes since v1)
drivers/video/fbdev/simplefb.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
static void simplefb_clocks_destroy(struct simplefb_par *par);
static void simplefb_regulators_destroy(struct simplefb_par *par);
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void simplefb_destroy(struct fb_info *info)
{
struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
if (info->screen_base)
iounmap(info->screen_base);
+ framebuffer_release(info);
+
if (mem)
release_mem_region(mem->start, resource_size(mem));
}
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* simplefb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH v2 3/4] fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
(no changes since v1)
drivers/video/fbdev/efifb.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
static inline void efifb_show_boot_graphics(struct fb_info *info) {}
#endif
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void efifb_destroy(struct fb_info *info)
{
if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
else
memunmap(info->screen_base);
}
+
+ framebuffer_release(info);
+
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* efifb_destroy takes care of info cleanup */
unregister_framebuffer(info);
sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Also do the change for vesafb (Thomas Zimmermann).
drivers/video/fbdev/vesafb.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
index df6de5a9dd4c..1f03a449e505 100644
--- a/drivers/video/fbdev/vesafb.c
+++ b/drivers/video/fbdev/vesafb.c
@@ -179,6 +179,10 @@ static int vesafb_setcolreg(unsigned regno, unsigned red,
unsigned green,
return err;
}
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void vesafb_destroy(struct fb_info *info)
{
struct vesafb_par *par = info->par;
@@ -187,7 +191,13 @@ static void vesafb_destroy(struct fb_info *info)
arch_phys_wc_del(par->wc_cookie);
if (info->screen_base)
iounmap(info->screen_base);
+
+ if (((struct vesafb_par *)(info->par))->region)
+ release_region(0x3c0, 32);
+
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
+
+ framebuffer_release(info);
}
static struct fb_ops vesafb_ops = {
@@ -484,10 +494,8 @@ static int vesafb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* vesafb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- if (((struct vesafb_par *)(info->par))->region)
- release_region(0x3c0, 32);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH] fbdev: vesafb: Allow to be built if COMPILE_TEST is enabled
The driver has runtime but no build time dependency with X86, so it can be built for testing purposes if the COMPILE_TEST option is enabled. This is useful to have more build coverage and make sure that the driver is not affected by changes that could cause build regressions. Signed-off-by: Javier Martinez Canillas --- drivers/video/fbdev/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index f2a6b81e45c4..bd849013f16f 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -616,7 +616,7 @@ config FB_UVESA config FB_VESA bool "VESA VGA graphics support" - depends on (FB = y) && X86 + depends on (FB = y) && (X86 || COMPILE_TEST) select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT -- 2.35.1
Re: [PATCH v3 3/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
Hello Daniel, On 5/5/22 14:11, Daniel Vetter wrote: [snip] >> >> And while I agree with you that these midlayer flags are horrible, that is >> what any other fbdev that makes use of a firmware-provided framebuffer set, >> so simpledrm emulated fbdev shouldn't be the exception IMO. > > So we discussed this a pile more on irc, and at least my take is that > people who run simpledrm but want to combine that with fbdev drivers and > expect it to all work nicely we can probably ignore. At least until all > this sysfb stuff is nicely unified, and at that point we shouldn't need > special flags anymore. I'm OK with this take and happy to just drop this patch-set then. My worry was just that someone could complain that we broke their uncommon setup [0]. [0]: https://xkcd.com/1172/. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
Hello Daniel,
On 5/5/22 14:52, Daniel Vetter wrote:
> On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote:
>> The driver is calling framebuffer_release() in its .remove callback, but
>> this will cause the struct fb_info to be freed too early. Since it could
>> be that a reference is still hold to it if user-space opened the fbdev.
>>
>> This would lead to a use-after-free error if the framebuffer device was
>> unregistered but later a user-space process tries to close the fbdev fd.
>>
>> The correct thing to do is to only unregister the framebuffer in the
>> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
>>
>> Suggested-by: Daniel Vetter
>> Signed-off-by: Javier Martinez Canillas
>
> I think this should have a Fixes: line for the patch from Thomas which
> changed the remove_conflicting_fb code:
>
> 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
>
Ok.
> I think we should also mention that strictly speaking the code flow is now
> wrong, because hw cleanup (like iounmap) should be done from ->remove
> while sw cleanup (like calling framebuffer_release()) is the only thing
> that should be done from ->fb_destroy. But the current code matches what
> was happening before 27599aacbaef so more minimal "fix"
>
Yes, I tried to make it as small as possible. Thomas pointed out that vesafb
has the same issue and I included in v2. I had move things around more there
though.
> With those details added Reviewed-by: Daniel Vetter
>
> Same for the next patch.
Thanks. I'll post a v3 adding what you suggested but probably not today.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
Hello Daniel,
On 5/5/22 15:02, Daniel Vetter wrote:
[snip]
>> static void vesafb_destroy(struct fb_info *info)
>> {
>> struct vesafb_par *par = info->par;
>> @@ -187,7 +191,13 @@ static void vesafb_destroy(struct fb_info *info)
>> arch_phys_wc_del(par->wc_cookie);
>> if (info->screen_base)
>> iounmap(info->screen_base);
>> +
>> +if (((struct vesafb_par *)(info->par))->region)
>> +release_region(0x3c0, 32);
>
> This move seems rather iffy, so maybe justify it with "makes the code
> exactly as busted before 27599aacbaef ("fbdev: Hot-unplug firmware fb
> devices on forced removal")"
>
I think that will just drop this change. While being here I wanted the release
order to be the inverse of the order in which the driver acquires them. But I
will only move the framebuffer_release() that is the problematic bit.
Someone if care enough could fix the rest of the driver.
> Also same comments as on v1 about adding more details about what/how this
> fixes, with that: Reviewed-by: Daniel Vetter
>
Yes, I'll do that too. Thanks again for your comments and feedback.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH] MAINTAINERS: Add simpledrm driver co-maintainer
Thomas asked me to serve as co-maintainer for the simpledrm driver. Signed-off-by: Javier Martinez Canillas --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 1a04950c1a8f..bfe43560f9d5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6389,6 +6389,7 @@ F:include/uapi/drm/savage_drm.h DRM DRIVER FOR SIMPLE FRAMEBUFFERS M: Thomas Zimmermann +M: Javier Martinez Canillas L: dri-devel@lists.freedesktop.org S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc -- 2.35.1
[PATCH v3 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers
Hello, This series contains patches suggested by Daniel Vetter to fix a use-after-free error in the fb_release() function, due a fb_info associated with a fbdev being freed too early while a user-space process still has the fbdev dev node opened. That is caused by a wrong management of the struct fb_info lifetime in drivers, but the fbdev core can also be made more resilient about it an leak This can easily be reproduced with the simplefb driver doing the following: $ cat < /dev/fb0 & $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind $ kill %1 [ 257.490471] [ cut here ] ... [ 257.495125] refcount_t: underflow; use-after-free. [ 257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144 ... [ 257.637482] pstate: 8045 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 257.61] pc : refcount_warn_saturate+0xf4/0x144 [ 257.649226] lr : refcount_warn_saturate+0xf4/0x144 [ 257.654009] sp : 8a06bbf0 [ 257.657315] x29: 8a06bbf0 x28: 000a x27: 000a [ 257.664448] x26: x25: 470b88c6a180 x24: 000a [ 257.671581] x23: 470b81706480 x22: 470b808c2160 x21: 470b8922ba20 [ 257.678713] x20: 470b891f5810 x19: 470b891f5800 x18: [ 257.685846] x17: 3a725f7463656a62 x16: bb18c6465fd4 x15: 0720072007200720 [ 257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375 [ 257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : bb18c58f6c90 [ 257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0001 [ 257.714373] x5 : 470bff8ec418 x4 : x3 : 0027 [ 257.721506] x2 : x1 : 0027 x0 : 0026 [ 257.728638] Call trace: [ 257.731075] refcount_warn_saturate+0xf4/0x144 [ 257.735513] put_fb_info+0x70/0x7c [ 257.738916] fb_release+0x60/0x74 [ 257.742225] __fput+0x88/0x240 [ 257.745276] fput+0x1c/0x30 [ 257.748410] task_work_run+0xc4/0x21c [ 257.752066] do_exit+0x170/0x370 [ 257.755288] do_group_exit+0x40/0xb4 [ 257.758858] get_signal+0x8e0/0x90c [ 257.762339] do_signal+0x1a0/0x280 [ 257.765733] do_notify_resume+0xc8/0x390 [ 257.769650] el0_da+0xe8/0xf0 [ 257.772613] el0t_64_sync_handler+0xe8/0x130 [ 257.776877] el0t_64_sync+0x190/0x194 [ 257.780534] ---[ end trace ]--- Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free to happen. Patch #2, #3 and #4 fix the simplefb, efifb and vesafb drivers respectively, to free the resources at the correct place. Changes in v3: - Add Fixes: tag (Daniel Vetter). - Include in commit message a note that drivers are still broken but at least reverts to the previous behavior (Daniel Vetter). - Only move framebuffer_release() and don't do any other change (Daniel Vetter). Changes in v2: - Also do the change for vesafb (Thomas Zimmermann). Daniel Vetter (1): fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas (3): fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove drivers/video/fbdev/core/fbsysfs.c | 4 drivers/video/fbdev/efifb.c| 9 - drivers/video/fbdev/simplefb.c | 8 +++- drivers/video/fbdev/vesafb.c | 8 +++- 4 files changed, 26 insertions(+), 3 deletions(-) -- 2.35.1
[PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
From: Daniel Vetter
Most fbdev drivers have issues with the fb_info lifetime, because call to
framebuffer_release() from their driver's .remove callback, rather than
doing from fbops.fb_destroy callback.
Doing that will destroy the fb_info too early, while references to it may
still exist, leading to a use-after-free error.
To prevent this, check the fb_info reference counter when attempting to
kfree the data structure in framebuffer_release(). That will leak it but
at least will prevent the mentioned error.
Signed-off-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
---
(no changes since v1)
drivers/video/fbdev/core/fbsysfs.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/video/fbdev/core/fbsysfs.c
b/drivers/video/fbdev/core/fbsysfs.c
index 8c1ee9ecec3d..c2a60b187467 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
{
if (!info)
return;
+
+ if (WARN_ON(refcount_read(&info->count)))
+ return;
+
kfree(info->apertures);
kfree(info);
}
--
2.35.1
[PATCH v3 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
To prevent this, move the framebuffer_release() call to fb_ops.fb_destroy
instead of doing it in the driver's .remove callback.
Strictly speaking, the code flow in the driver is still wrong because all
the hardware cleanupd (i.e: iounmap) should be done in .remove while the
software cleanup (i.e: releasing the framebuffer) should be done in the
.fb_destroy handler. But this at least makes to match the behavior before
commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal").
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
---
(no changes since v1)
drivers/video/fbdev/simplefb.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
static void simplefb_clocks_destroy(struct simplefb_par *par);
static void simplefb_regulators_destroy(struct simplefb_par *par);
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void simplefb_destroy(struct fb_info *info)
{
struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
if (info->screen_base)
iounmap(info->screen_base);
+ framebuffer_release(info);
+
if (mem)
release_mem_region(mem->start, resource_size(mem));
}
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* simplefb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH v3 3/4] fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
To prevent this, move the framebuffer_release() call to fb_ops.fb_destroy
instead of doing it in the driver's .remove callback.
Strictly speaking, the code flow in the driver is still wrong because all
the hardware cleanupd (i.e: iounmap) should be done in .remove while the
software cleanup (i.e: releasing the framebuffer) should be done in the
.fb_destroy handler. But this at least makes to match the behavior before
commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal").
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
---
(no changes since v1)
drivers/video/fbdev/efifb.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
static inline void efifb_show_boot_graphics(struct fb_info *info) {}
#endif
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void efifb_destroy(struct fb_info *info)
{
if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
else
memunmap(info->screen_base);
}
+
+ framebuffer_release(info);
+
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* efifb_destroy takes care of info cleanup */
unregister_framebuffer(info);
sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH v3 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
To prevent this, move the framebuffer_release() call to fb_ops.fb_destroy
instead of doing it in the driver's .remove callback.
Strictly speaking, the code flow in the driver is still wrong because all
the hardware cleanupd (i.e: iounmap) should be done in .remove while the
software cleanup (i.e: releasing the framebuffer) should be done in the
.fb_destroy handler. But this at least makes to match the behavior before
commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal").
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
---
Changes in v3:
- Only move framebuffer_release() and don't do any other change
(Daniel Vetter).
Changes in v2:
- Also do the change for vesafb (Thomas Zimmermann).
drivers/video/fbdev/vesafb.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
index df6de5a9dd4c..e25e8de5ff67 100644
--- a/drivers/video/fbdev/vesafb.c
+++ b/drivers/video/fbdev/vesafb.c
@@ -179,6 +179,10 @@ static int vesafb_setcolreg(unsigned regno, unsigned red,
unsigned green,
return err;
}
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void vesafb_destroy(struct fb_info *info)
{
struct vesafb_par *par = info->par;
@@ -188,6 +192,8 @@ static void vesafb_destroy(struct fb_info *info)
if (info->screen_base)
iounmap(info->screen_base);
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
+
+ framebuffer_release(info);
}
static struct fb_ops vesafb_ops = {
@@ -484,10 +490,10 @@ static int vesafb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* vesafb_destroy takes care of info cleanup */
unregister_framebuffer(info);
if (((struct vesafb_par *)(info->par))->region)
release_region(0x3c0, 32);
- framebuffer_release(info);
return 0;
}
--
2.35.1
[PATCH v4] drm/msm: Make .remove and .shutdown HW shutdown consistent
1f d281 aa0103e3 (c8e37c02)
---[ end trace ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
Kernel Offset: 0x495d77c0 from 0x8800
PHYS_OFFSET: 0xcd85
CPU features: 0x800,00c2a015,19801c82
Memory Limit: none
---[ end Kernel panic - not syncing: Attempted to kill init!
exitcode=0x000b ]---
Fixes: 9d5cbf5fe46e ("drm/msm: add shutdown support for display
platform_driver")
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Abhinav Kumar
---
Changes in v4:
- Remove the kernel log timestamps and just keep the stacktrace (Abhinav Kumar).
- Add Abhinav Kumar Reviewed-by tag.
Changes in v3:
- Drop the msm_shutdown_hw() wrapper and just call drm_atomic_helper_shutdown()
in both callbacks (Dmitry Baryshkov).
- Copy the comment in msm_drm_uninit() to msm_drv_shutdown() (Dmitry Baryshkov).
Changes in v2:
- Take the registered check out of the msm_shutdown_hw() and make callers to
check instead.
- Make msm_shutdown_hw() an inline function.
- Add a Fixes: tag.
drivers/gpu/drm/msm/msm_drv.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 1d0bafedd585..226d8d4629d2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1242,10 +1242,15 @@ void msm_drv_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
- if (!priv || !priv->kms)
- return;
-
- drm_atomic_helper_shutdown(drm);
+ /*
+* Shutdown the hw if we're far enough along where things might be on.
+* If we run this too early, we'll end up panicking in any variety of
+* places. Since we don't register the drm device until late in
+* msm_drm_init, drm_dev->registered is used as an indicator that the
+* shutdown will be successful.
+*/
+ if (drm && drm->registered)
+ drm_atomic_helper_shutdown(drm);
}
static struct platform_driver msm_platform_driver = {
--
2.37.1
Re: [PATCH v2] Revert "fbdev: Make fb_release() return -ENODEV if fbdev was unregistered"
On 5/4/22 13:59, Javier Martinez Canillas wrote:
> This reverts commit aafa025c76dcc7d1a8c8f0bdefcbe4eb480b2f6a. That commit
> attempted to fix a NULL pointer dereference, caused by the struct fb_info
> associated with a framebuffer device to not longer be valid when the file
> descriptor was closed.
>
> The issue was exposed by commit 27599aacbaef ("fbdev: Hot-unplug firmware
> fb devices on forced removal"), which added a new path that goes through
> the struct device removal instead of directly unregistering the fb.
>
> Most fbdev drivers have issues with the fb_info lifetime, because call to
> framebuffer_release() from their driver's .remove callback, rather than
> doing from fbops.fb_destroy callback. This meant that due to this switch,
> the fb_info was now destroyed too early, while references still existed,
> while before it was simply leaked.
>
> The patch we're reverting here reinstated that leak, hence "fixed" the
> regression. But the proper solution is to fix the drivers to not release
> the fb_info too soon.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Javier Martinez Canillas
> Reviewed-by: Daniel Vetter
> ---
>
Pushed this to drm-misc (drm-misc-fixes).
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers
On 5/5/22 23:59, Javier Martinez Canillas wrote: > Hello, > > This series contains patches suggested by Daniel Vetter to fix a > use-after-free > error in the fb_release() function, due a fb_info associated with a fbdev > being > freed too early while a user-space process still has the fbdev dev node > opened. > Pushed this to drm-misc (drm-misc-fixes). -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 3/4] fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
Hello Andrzej, On 5/6/22 15:07, Andrzej Hajda wrote: > On 06.05.2022 00:05, Javier Martinez Canillas wrote: [snip] >> + >> +framebuffer_release(info); >> + >> if (request_mem_succeeded) >> release_mem_region(info->apertures->ranges[0].base, >> info->apertures->ranges[0].size); > > You are releasing info, then you are using it. > > I suspect it is responsible for multiple failures of Intel CI [1]. > Yes, it is :( sorry about the mess. Ville already reported this to me. I'll post a patch in a minute. I wonder how this didn't happen before since .remove() happens before .fb_destroy() AFAIU... -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
than .remove") attempted to fix a use-after-free error due driver freeing
the fb_info in the .remove handler instead of doing it in .fb_destroy.
But ironically that change introduced yet another use-after-free since the
fb_info was still used after the free.
This should fix for good by freeing the fb_info at the end of the handler.
Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than
.remove")
Reported-by: Ville Syrjälä
Reported-by: Andrzej Hajda
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/efifb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index cfa3dc0b4eee..b3d5f884c544 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
memunmap(info->screen_base);
}
- framebuffer_release(info);
-
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
fb_dealloc_cmap(&info->cmap);
+
+ framebuffer_release(info);
}
static const struct fb_ops efifb_ops = {
--
2.35.1
Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
Hello Lucas,
On 5/7/22 18:20, Lucas De Marchi wrote:
> On Fri, May 06, 2022 at 03:22:25PM +0200, Javier Martinez Canillas wrote:
>> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>> than .remove") attempted to fix a use-after-free error due driver freeing
>> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>>
>> But ironically that change introduced yet another use-after-free since the
>> fb_info was still used after the free.
>>
>> This should fix for good by freeing the fb_info at the end of the handler.
>>
>> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>> than .remove")
>
> are these patches going through any CI before being applied? Maybe would
> be a good idea to cc intel-gfx mailing list on these fixes to have Intel
> CI to pick them up for some tests?
>
I Cc'ed intel-gfx for this particular patch. I should had done it for the
previous patches too, but I wasn't aware that Cc'ing that list would make
it run on your CI.
I tested locally the offending patch on an EFI platform before applying it
and I don't know why it didn't fail there. Sorry all for the inconvenience.
> pushed to drm-misc-fixes where the previous patch was applied.
>
Thanks.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] drm/vmwgfx: Fix passing partly uninitialized drm_mode_fb_cmd2 struct
Hello Hans,
On 5/9/22 13:04, Hans de Goede wrote:
> vmw_fb_kms_framebuffer() declares a drm_mode_fb_cmd2 struct on the stack
> without zero-ing it and then continues with initializing only some fields.
>
> This leads to drm_mode_fb_cmd2.modifiers[0] containing garbage,
> which eventually gets used by drm_helper_mode_fill_fb_struct() to
> set fb->modifier when leads to the following atomic-check failure:
>
> vmwgfx :00:02.0: [drm:drm_atomic_check_only] [PLANE:34:plane-0]
> invalid pixel format XR24 little-endian (0x34325258),
> modifier 0x94d64719e000
> fbcon_init: detected unhandled fb_set_par error, error code -22
>
> Which causes the fbdev emulation and thus also fbcon to not work.
>
> Initialize the struct with all zeros to fix this.
>
> Fixes: dabdcdc9822a ("drm/vmwgfx: Switch to mode_cmd2")
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2072556
> Signed-off-by: Hans de Goede
> ---
Zack fixed this already:
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=5405d25b9e8e6
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] drm/vmwgfx: Fix passing partly uninitialized drm_mode_fb_cmd2 struct
On 5/9/22 13:55, Hans de Goede wrote:
[snip]
>>>
>>> Fixes: dabdcdc9822a ("drm/vmwgfx: Switch to mode_cmd2")
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2072556
>>> Signed-off-by: Hans de Goede
>>> ---
>>
>> Zack fixed this already:
>>
>> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=5405d25b9e8e6
>
> I see, but it seems that this was never pushed to drm-misc-fixes,
> so this is still broken in 5.18-rc#
>
Indeed. Agreed that should be cherry-picked in -fixes as well.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH] drm/todo: Add entry for converting kselftests to kunit
Many of the kselftests in DRM can be converted to kunit tests instead, since that framework is more suitable for unit testing. Suggested-by: Maxime Ripard Signed-off-by: Javier Martinez Canillas --- Documentation/gpu/todo.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 10bfb50908d1..513b20ccef1e 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -617,6 +617,17 @@ Contact: Javier Martinez Canillas Level: Intermediate +Convert Kernel Selftests (kselftest) to KUnit tests when appropriate + + +Many of the `Kselftest <https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html>`_ +tests in DRM could be converted to Kunit tests instead, since that framework +is more suitable for unit testing. + +Contact: Javier Martinez Canillas + +Level: Starter + Enable trinity for DRM -- -- 2.35.1
Re: [PATCH v2] Revert "fbdev: Make fb_release() return -ENODEV if fbdev was unregistered"
Hello Tim, On 5/9/22 16:01, pub...@timruffing.de wrote: > Thanks for this patch. Do you think this can be backported to LTS 5.17.y and You are welcome. > 5.15.y, which are still buggy? It's not a big deal for me but others might > profit. > > Background: > The patch solves a regression from 5.17.1 to 5.17.2 or 5.15.32 and > 5.15.33 I was about to report. On my Thinkpad T570, I got random "BUG", "Oops" > or even panics when during booting with efifb and plymouthd (and then > sometimes > also problems when shutting down because). I had bisected the issue to commit > 27599aacbaef. I could provide more info but I don't think it's necessary given > that either aafa025c76dcc7d1a8c8f0bdefcbe4eb480b2f6a or your better patch now > fixes the issue (I tested both, both work for me). > The patches to fix the fbdev hot-unplug regression will get merged in mainline soon and since all have a Fixes tag, they should get picked for stable as well. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
Hello Andrzej,
On 5/9/22 16:56, Andrzej Hajda wrote:
> On 06.05.2022 00:04, Javier Martinez Canillas wrote:
>> From: Daniel Vetter
>>
>> Most fbdev drivers have issues with the fb_info lifetime, because call to
>> framebuffer_release() from their driver's .remove callback, rather than
>> doing from fbops.fb_destroy callback.
>>
>> Doing that will destroy the fb_info too early, while references to it may
>> still exist, leading to a use-after-free error.
>>
>> To prevent this, check the fb_info reference counter when attempting to
>> kfree the data structure in framebuffer_release(). That will leak it but
>> at least will prevent the mentioned error.
>>
>> Signed-off-by: Daniel Vetter
>> Signed-off-by: Javier Martinez Canillas
>> Reviewed-by: Thomas Zimmermann
>> ---
>>
>> (no changes since v1)
>>
>> drivers/video/fbdev/core/fbsysfs.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbsysfs.c
>> b/drivers/video/fbdev/core/fbsysfs.c
>> index 8c1ee9ecec3d..c2a60b187467 100644
>> --- a/drivers/video/fbdev/core/fbsysfs.c
>> +++ b/drivers/video/fbdev/core/fbsysfs.c
>> @@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
>> {
>> if (!info)
>> return;
>> +
>> +if (WARN_ON(refcount_read(&info->count)))
>> +return;
>> +
>
> Regarding drm:
> What about drm_fb_helper_fini? It calls also framebuffer_release and is
> called often from _remove paths (checked intel/radeon/nouveau). I guess
> it should be fixed as well. Do you plan to fix it?
>
I think you are correct. Maybe we need something like the following?
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..b09598f7af28 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (info) {
if (info->cmap.len)
fb_dealloc_cmap(&info->cmap);
- framebuffer_release(info);
}
fb_helper->fbdev = NULL;
@@ -2112,6 +2111,7 @@ static void drm_fbdev_release(struct drm_fb_helper
*fb_helper)
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
drm_fbdev_release(info->par);
+ framebuffer_release(info);
}
static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>
> Regarding fb drivers, just for stats:
> git grep -p framebuffer_release | grep _remove | wc -l
> Suggests there is at least 70 incorrect users of this :)
>
Yes, Daniel already mentioned that most of them get this wrong but I was
mostly interested in {simple,efi,vesa}fb since are used with "nomodeset".
But given that I only touched those tree and still managed to introduce
a regression, I won't attempt to fix the others.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
On 5/9/22 17:51, Andrzej Hajda wrote:
[snip]
>>>> +
>>> Regarding drm:
>>> What about drm_fb_helper_fini? It calls also framebuffer_release and is
>>> called often from _remove paths (checked intel/radeon/nouveau). I guess
>>> it should be fixed as well. Do you plan to fix it?
>>>
>> I think you are correct. Maybe we need something like the following?
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index d265a73313c9..b09598f7af28 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>> if (info) {
>> if (info->cmap.len)
>> fb_dealloc_cmap(&info->cmap);
>> - framebuffer_release(info);
>
> What about cmap? I am not an fb expert, but IMO cmap can be accessed
> from userspace as well.
>
I actually thought about the same but then remembered what Daniel said in [0]
(AFAIU at least) that these should be done in .remove() so the current code
looks like matches that and only framebuffer_release() should be moved.
For vesafb a previous patch proposed to also move a release_region() call to
.fb_destroy() and Daniel also said that it was iffy and shouldn't be done [1].
But I'm also not fb expert so happy to move fb_dealloc_cmap() as well if that
is the correct thing to do.
[0]: https://www.spinics.net/lists/dri-devel/msg346257.html
[1]: https://www.spinics.net/lists/dri-devel/msg346261.html
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
Hello Thomas,
On 5/9/22 20:32, Thomas Zimmermann wrote:
> Hi
>
> Am 09.05.22 um 18:33 schrieb Javier Martinez Canillas:
>> On 5/9/22 17:51, Andrzej Hajda wrote:
>>
>> [snip]
>>
>>>>>> +
>>>>> Regarding drm:
>>>>> What about drm_fb_helper_fini? It calls also framebuffer_release and is
>>>>> called often from _remove paths (checked intel/radeon/nouveau). I guess
>>>>> it should be fixed as well. Do you plan to fix it?
>>>>>
>>>> I think you are correct. Maybe we need something like the following?
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>> index d265a73313c9..b09598f7af28 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper
>>>> *fb_helper)
>>>> if (info) {
>>>> if (info->cmap.len)
>>>> fb_dealloc_cmap(&info->cmap);
>>>> - framebuffer_release(info);
>
> After reviewing that code, drm_fb_helper_fini() appears to be called
> from .fb_destroy (see drm_fbdev_release). The code is hard to follow
> though. If there another way of releasing the framebuffer here?
>
Andrzej mentioned intel/radeon/nouveau as example, I only looked at i915
and the call chain is the following as far as I can tell:
struct pci_driver i915_pci_driver = {
...
.remove = i915_pci_remove,
...
};
i915_driver_remove
intel_modeset_driver_remove_noirq
intel_fbdev_fini
intel_fbdev_destroy
drm_fb_helper_fini
framebuffer_release
So my underdestanding is that if a program has the emulated fbdev device
opened and the i915 module is removed, then a use-after-free would be
triggered on drm_fbdev_fb_destroy() once the program closes the device:
drm_fbdev_fb_destroy
drm_fbdev_release(info->par); <-- info was already freed on .remove
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
On 5/9/22 20:12, Thomas Zimmermann wrote:
[snip]
>> I actually thought about the same but then remembered what Daniel said in [0]
>> (AFAIU at least) that these should be done in .remove() so the current code
>> looks like matches that and only framebuffer_release() should be moved.
>>
>> For vesafb a previous patch proposed to also move a release_region() call to
>> .fb_destroy() and Daniel also said that it was iffy and shouldn't be done
>> [1].
>>
>> But I'm also not fb expert so happy to move fb_dealloc_cmap() as well if that
>> is the correct thing to do.
>
> The cmap data structure is software state that can be accessed via icotl
> as long as the devfile is open. Drivers update the hardware from it. See
> [1]. Moving that cleanup into fb_destroy seems appropriate to me.
>
I see, that makes sense. Then something like the following instead?
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..ce0d89c49e42 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
cancel_work_sync(&fb_helper->resume_work);
cancel_work_sync(&fb_helper->damage_work);
- info = fb_helper->fbdev;
- if (info) {
- if (info->cmap.len)
- fb_dealloc_cmap(&info->cmap);
- framebuffer_release(info);
- }
fb_helper->fbdev = NULL;
mutex_lock(&kernel_fb_helper_lock);
@@ -2111,7 +2105,11 @@ static void drm_fbdev_release(struct drm_fb_helper
*fb_helper)
*/
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
+ if (info->cmap.len)
+ fb_dealloc_cmap(&info->cmap);
+
drm_fbdev_release(info->par);
+ framebuffer_release(info);
}
static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
On 5/10/22 00:22, Andrzej Hajda wrote:
[snip]
>> static void drm_fbdev_fb_destroy(struct fb_info *info)
>> {
>> + if (info->cmap.len)
>> + fb_dealloc_cmap(&info->cmap);
>> +
>> drm_fbdev_release(info->par);
>> + framebuffer_release(info);
>
> I would put drm_fbdev_release at the beginning - it cancels workers
> which could expect cmap to be still valid.
>
Indeed, you are correct again. [0] is the final version of the patch I've
but don't have an i915 test machine to give it a try. I'll test tomorrow
on my test systems to verify that it doesn't cause any regressions since
with other DRM drivers.
I think that besides this patch, drivers shouldn't need to call to the
drm_fb_helper_fini() function directly. Since that would be called during
drm_fbdev_fb_destroy() anyways.
We should probably remove that call in all drivers and make this helper
function static and just private to drm_fb_helper functions.
Or am I missing something here ?
[0]:
>From 5170cafcf2936da8f1c53231e3baa7d7a2b16c61 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Tue May 10 00:39:55 2022 +0200
Subject: [RFT PATCH] drm/fb-helper: Don't deallocate fb colormap and free fb
info
too early
Currently these are done in drm_fb_helper_fini() but this helper is called
by drivers in their .remove callback, which could lead to a use-after-free
if a process has opened the emulated fbdev node while a driver is removed.
For example, in i915 driver the call chain during remove is the following:
struct pci_driver i915_pci_driver = {
...
.remove = i915_pci_remove,
...
};
i915_pci_remove
i915_driver_remove
intel_modeset_driver_remove_noirq
intel_fbdev_fini
intel_fbdev_destroy
drm_fb_helper_fini
framebuffer_release
Later the process will close the fbdev node file descriptor leading to the
mentioned use-after-free bug in drm_fbdev_fb_destroy(), due the following:
drm_fbdev_fb_destroy
drm_fbdev_release(info->par); <-- info was already freed on .remove
To prevent that, let's move the framebuffer_release() call to the end of
the drm_fbdev_fb_destroy() function.
Also, the call to fb_dealloc_cmap() in drm_fb_helper_fini() is too early
and is more correct to do it in drm_fbdev_fb_destroy() as well. After a
call to drm_fbdev_release() has been made.
Reported-by: Andrzej Hajda
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/drm_fb_helper.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..7288fbd26bcc 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
cancel_work_sync(&fb_helper->resume_work);
cancel_work_sync(&fb_helper->damage_work);
- info = fb_helper->fbdev;
- if (info) {
- if (info->cmap.len)
- fb_dealloc_cmap(&info->cmap);
- framebuffer_release(info);
- }
fb_helper->fbdev = NULL;
mutex_lock(&kernel_fb_helper_lock);
@@ -2112,6 +2106,9 @@ static void drm_fbdev_release(struct drm_fb_helper
*fb_helper)
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
drm_fbdev_release(info->par);
+ if (info->cmap.len)
+ fb_dealloc_cmap(&info->cmap);
+ framebuffer_release(info);
}
static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
--
2.35.1
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
On 5/10/22 09:19, Andrzej Hajda wrote:
>
>
> On 10.05.2022 00:42, Javier Martinez Canillas wrote:
>> On 5/10/22 00:22, Andrzej Hajda wrote:
>>
>> [snip]
>>
>>>>static void drm_fbdev_fb_destroy(struct fb_info *info)
>>>>{
>>>> + if (info->cmap.len)
>>>> + fb_dealloc_cmap(&info->cmap);
>>>> +
>>>> drm_fbdev_release(info->par);
>>>> + framebuffer_release(info);
>>> I would put drm_fbdev_release at the beginning - it cancels workers
>>> which could expect cmap to be still valid.
>>>
>> Indeed, you are correct again. [0] is the final version of the patch I've
>> but don't have an i915 test machine to give it a try. I'll test tomorrow
>> on my test systems to verify that it doesn't cause any regressions since
>> with other DRM drivers.
>>
>> I think that besides this patch, drivers shouldn't need to call to the
>> drm_fb_helper_fini() function directly. Since that would be called during
>> drm_fbdev_fb_destroy() anyways.
>>
>> We should probably remove that call in all drivers and make this helper
>> function static and just private to drm_fb_helper functions.
>>
>> Or am I missing something here ?
>
> This is question for experts :)
Fair. I'm definitely not one of them :)
> I do not know what are user API/ABI expectations regarding removal of
> fbdev driver, I wonder if they are documented somewhere :)
I don't know. At least I haven't found them.
> Apparently we have some process of 'zombification' here - we need to
> remove the driver without waiting for userspace closing framebuffer(???)
> (to unbind ops-es and remove references to driver related things), but
> we need to leave some structures to fool userspace, 'info' seems to be
> one of them.
That's correct, yes. I think that any driver that provides a .mmap file
operation would have the same issue. But drivers keep an internal state
and just return -ENODEV or whatever on read/write/close after a removal.
The fbdev subsystem is different though since as you said it, the fbdev
core unconditionally calls to the driver .fb_release() callback with a
struct fb_info reference as argument.
I tried to prevent that with commit aafa025c76dc ("fbdev: Make fb_release()
return -ENODEV if fbdev was unregistered") but Daniel pointed out that
is was wrong since could leak memory allocated and was expected to be
freed on release.
That's why I instead fixed the issue in the fbdev drivers and just added
a warn on fb_release(), that is $SUBJECT.
> So I guess there should be something called on driver's _remove path,
> and sth on destroy path.
>
That was my question actually, do we need something to be called in the
destroy path ? Since that could just be internal to the DRM fb helpers.
In other words, drivers should only care about setting a generic fbdev
by calling drm_fbdev_generic_setup(), and then do any HW cleanup in the
removal path, but let the fb helpers to handle the SW cleanup in destroy.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
Hello Thomas,
On 5/10/22 10:04, Thomas Zimmermann wrote:
> Hi
>
> Am 10.05.22 um 00:42 schrieb Javier Martinez Canillas:
>> On 5/10/22 00:22, Andrzej Hajda wrote:
>>
>> [snip]
>>
>>>>static void drm_fbdev_fb_destroy(struct fb_info *info)
>>>>{
>>>> + if (info->cmap.len)
>>>> + fb_dealloc_cmap(&info->cmap);
>>>> +
>>>> drm_fbdev_release(info->par);
>>>> + framebuffer_release(info);
>>>
>>> I would put drm_fbdev_release at the beginning - it cancels workers
>>> which could expect cmap to be still valid.
>>>
>>
>> Indeed, you are correct again. [0] is the final version of the patch I've
>> but don't have an i915 test machine to give it a try. I'll test tomorrow
>> on my test systems to verify that it doesn't cause any regressions since
>> with other DRM drivers.
>
> You have to go through all DRM drivers that call drm_fb_helper_fini()
> and make sure that they free fb_info. For example armada appears to be
> leaking now. [1]
>
But shouldn't fb_info be freed when unregister_framebuffer() is called
through drm_dev_unregister() ? AFAICT the call chain is the following:
drm_put_dev()
drm_dev_unregister()
drm_client_dev_unregister()
drm_fbdev_client_unregister()
drm_fb_helper_unregister_fbi()
unregister_framebuffer()
do_unregister_framebuffer()
put_fb_info()
drm_fbdev_fb_destroy()
framebuffer_release()
which is the reason why I believe that drm_fb_helper_fini() should be
an internal static function and only called from drm_fbdev_fb_destroy().
Drivers shouldn't really explicitly call this helper in my opinion.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
Hello Thomas, On 5/10/22 10:50, Thomas Zimmermann wrote: [snip] >>> Drivers shouldn't really explicitly call this helper in my opinion. > > One more stupid question: does armada actually use > drm_fbdev_fb_destroy()? It's supposed to be a callback for struct > fb_ops. Armada uses it's own instance of fb_ops, which apparently > doesn't contain fb_destroy. [1] > No stupid question at all. You are correct on this. So I guess we still need this call in the drivers that don't provide a .fb_destroy() handler. I see many options here: 1) Document in drm_fb_helper_alloc_fbi() that drivers only need to call drm_fb_helper_fini() explicitly if they are not setting up a fbdev with drm_fbdev_generic_setup(), otherwise is not needed. 2) Make drm_fbdev_fb_destroy() an exported symbol so drivers that have custom fb_ops can use it. 3) Set .fb_destroy to drm_fbdev_fb_destroy() if isn't set by drivers when they call drm_fb_helper_initial_config() or drm_fb_helper_fill_info(). I'm leaning towards option (3). Then the fb_info release will be automatic whether drivers are using the generic setup or a custom one. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()
On 5/10/22 11:39, Thomas Zimmermann wrote: [snip] >> >> 3) Set .fb_destroy to drm_fbdev_fb_destroy() if isn't set by drivers when >> they call drm_fb_helper_initial_config() or drm_fb_helper_fill_info(). >> >> I'm leaning towards option (3). Then the fb_info release will be automatic >> whether drivers are using the generic setup or a custom one. > > IMHO this would just be another glitch to paper over all the broken > code. And if you follow through drm_fbdev_fb_helper(), [1] it'll call > _fini at some point and probably blow up in some other way. Instances of > struct fb_ops are also usually const. > > The only reliable way AFAICT is to do what generic fbdev does: use > unregister_framebuffer and do the software cleanup somewhere within > fb_destroy. And then fix all drivers to use that pattern. > Right. We can't really abstract this away from drivers that are not using the generic fbdev helpers. So then they will have to provide their own .fb_destroy() callback and do the cleanup. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] MAINTAINERS: Add simpledrm driver co-maintainer
On 5/5/22 19:26, Javier Martinez Canillas wrote: > Thomas asked me to serve as co-maintainer for the simpledrm driver. > > Signed-off-by: Javier Martinez Canillas > --- Pushed this to drm-misc (drm-misc-next). -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/todo: Add entry for converting kselftests to kunit
On 5/9/22 15:08, Javier Martinez Canillas wrote: > Many of the kselftests in DRM can be converted to kunit tests instead, > since that framework is more suitable for unit testing. > > Suggested-by: Maxime Ripard > Signed-off-by: Javier Martinez Canillas > --- Pushed this to drm-misc (drm-misc-next). -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v5 0/7] 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() and sysfb_try_unregister() helpers to allow
disabling sysfb and attempt to unregister registered devices respectively.
Patch 3 changes how is dealt with conflicting framebuffers unregistering,
rather than having a variable to determine if a lock should be take, it
just drops the lock before unregistering the platform device.
Patch 4 changes the fbdev core to not attempt to unregister devices that
were registered by sysfb, let the same code doing the registration to also
handle the unregistration.
Patch 5 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 6 is the revert patch that was posted by Daniel before but dropped
from his set and finally patch 7 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.
For example, having simpledrm as a module and loading it after the vc4
driver probed would not register a DRM device, which happens now without
the patches from this series.
Best regards,
Javier
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.
- Drop call to sysfb_disable() in fbmem since is done in other places now.
- 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).
- 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 (5):
firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer
firmware: sysfb: Add helpers to unregister a pdev and disable
registration
fbdev: Restart conflicting fb removal loop when unregistering devices
fbdev: Make sysfb to unregister i
[PATCH v5 1/7] firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer
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
Reviewed-by: Daniel Vetter
Reviewed-by: Thomas Zimmermann
---
(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.35.1
[PATCH v5 2/7] firmware: sysfb: Add helpers to unregister a pdev and disable registration
These 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
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Vetter
---
(no changes since v4)
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 | 87 +--
include/linux/sysfb.h | 19
3 files changed, 106 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..6768968949e6 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -34,21 +34,92 @@
#include
#include
+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
and
+ * sysfb_try_unregister(), that tries to unregister a framebuffer
device.
+ */
+void sysfb_disable(void)
+{
+ mutex_lock(&disable_lock);
+ sysfb_unregister();
+ disabled = true;
+ mutex_unlock(&disable_lock);
+}
+EXPORT_SYMBOL_GPL(sysfb_disable);
+
+/**
+ * sysfb_try_unregister() - attempt to unregister a system framebuffer device
+ * @dev: device to unregister
+ *
+ * This tries to unregister a system framebuffer device if this was registered
+ * by the Generic System Framebuffers. The device will only be unregistered if
+ * it was registered by sysfb_init(), otherwise it will not be unregistered.
+ *
+ * Context: The function can sleep. a @load_lock mutex is acquired to serialize
+ * against sysfb_init(), that registers a simple framebuffer device
and
+ * sysfb_disable(), that disables the Generic System Framebuffers
support.
+ *
+ * Return:
+ * * true - the device was unregistered successfully
+ * * false - the device was not unregistered
+ */
+bool sysfb_try_unregister(struct device *dev)
+{
+ bool ret = false;
+
+ mutex_lock(&disable_lock);
+ if (IS_ERR_OR_NULL(pd) || pd != to_platform_device(dev))
+ goto unlock_mutex;
+
+ ret = sysfb_unregister();
+
+unlock_mutex:
+ mutex_unlock(&disable_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sysfb_try_unregister);
+
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 +131,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 +146,11 @@ static __init int sysfb_init(void)
if (ret)
goto err;
- return 0;
+ goto unlock_m
[PATCH v5 3/7] fbdev: Restart conflicting fb removal loop when unregistering devices
Drivers that want to remove registered conflicting framebuffers prior to
register their own framebuffer, calls remove_conflicting_framebuffers().
This function takes the registration_lock mutex, to prevent a races when
drivers register framebuffer devices. But if a conflicting framebuffer
device is found, the underlaying platform device is unregistered and this
will lead to the platform driver .remove callback to be called, which in
turn will call to the unregister_framebuffer() that takes the same lock.
To prevent this, a struct fb_info.forced_out field was used as indication
to unregister_framebuffer() whether the mutex has to be grabbed or not.
A cleaner solution is to drop the lock before platform_device_unregister()
so unregister_framebuffer() can take it when called from the fbdev driver,
and just grab the lock again after the device has been registered and do
a removal loop restart.
Since the framebuffer devices will already be removed, the loop would just
finish when no more conflicting framebuffers are found.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Vetter
---
(no changes since v1)
drivers/video/fbdev/core/fbmem.c | 22 +++---
include/linux/fb.h | 1 -
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index b445a7a00def..2fda5917c212 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1555,6 +1555,7 @@ static void do_remove_conflicting_framebuffers(struct
apertures_struct *a,
{
int i;
+restart_removal:
/* check all firmware fbs and kick off if the base addr overlaps */
for_each_registered_fb(i) {
struct apertures_struct *gen_aper;
@@ -1587,12 +1588,23 @@ static void do_remove_conflicting_framebuffers(struct
apertures_struct *a,
pr_warn("fb%d: no device set\n", i);
do_unregister_framebuffer(registered_fb[i]);
} else if (dev_is_platform(device)) {
- registered_fb[i]->forced_out = true;
+ /*
+* Drop the lock because if the device is
unregistered, its
+* driver will call to
unregister_framebuffer(), that takes
+* this lock.
+*/
+ mutex_unlock(®istration_lock);
platform_device_unregister(to_platform_device(device));
+ mutex_lock(®istration_lock);
} else {
pr_warn("fb%d: cannot remove device\n", i);
do_unregister_framebuffer(registered_fb[i]);
}
+ /*
+* Restart the removal loop now that the device has been
+* unregistered and its associated framebuffer gone.
+*/
+ goto restart_removal;
}
}
}
@@ -1899,13 +1911,9 @@ EXPORT_SYMBOL(register_framebuffer);
void
unregister_framebuffer(struct fb_info *fb_info)
{
- bool forced_out = fb_info->forced_out;
-
- if (!forced_out)
- mutex_lock(®istration_lock);
+ mutex_lock(®istration_lock);
do_unregister_framebuffer(fb_info);
- if (!forced_out)
- mutex_unlock(®istration_lock);
+ mutex_unlock(®istration_lock);
}
EXPORT_SYMBOL(unregister_framebuffer);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 69c67c70fa78..bbe1e4571899 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -511,7 +511,6 @@ struct fb_info {
} *apertures;
bool skip_vt_switch; /* no VT switch on suspend/resume required */
- bool forced_out; /* set when being removed by another driver */
};
static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
--
2.35.1
[PATCH v5 4/7] fbdev: Make sysfb to unregister its own registered devices
The platform devices registered in sysfb match with a firmware-based fbdev
or DRM driver, that are used to have early graphics using framebuffers set
up by the system firmware.
Real DRM 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 the problem that sysfb doesn't know when the
device that registered is unregistered. This means that is not able to do
any cleanup if needed since the device pointer may not be valid anymore.
Not all platforms use sysfb to register the simple framebuffer devices,
so an unregistration has to be forced by fbmem if sysfb_try_unregister()
does not succeed at unregister the device.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Daniel Vetter
---
(no changes since v4)
Changes in v4:
- Drop call to sysfb_disable() in fbmem since is done in other places now.
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 | 28 +++-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2fda5917c212..9b035ef4d552 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1587,18 +1588,35 @@ static void do_remove_conflicting_framebuffers(struct
apertures_struct *a,
if (!device) {
pr_warn("fb%d: no device set\n", i);
do_unregister_framebuffer(registered_fb[i]);
- } else if (dev_is_platform(device)) {
+ } else {
/*
* Drop the lock because if the device is
unregistered, its
* driver will call to
unregister_framebuffer(), that takes
* this lock.
*/
mutex_unlock(®istration_lock);
-
platform_device_unregister(to_platform_device(device));
+ /*
+* First attempt the device to be unregistered
by sysfb.
+*/
+ if (!sysfb_try_unregister(device)) {
+ if (dev_is_platform(device)) {
+ /*
+* FIXME: sysfb didn't register
this device, the platform
+* device was registered in
other platform code.
+*/
+
platform_device_unregister(to_platform_device(device));
+ } else {
+ /*
+* If is not a platform device,
at least print a warning. A
+* fix would add to make the
code that registered the device
+* to also unregister it.
+*/
+ pr_warn("fb%d: cannot remove
device\n", i);
+ /* call
unregister_framebuffer() since the lock was dropped */
+
unregister_framebuffer(registered_fb[i]);
+ }
+ }
mutex_lock(®istration_lock);
- } else {
- pr_warn("fb%d: cannot remove device\n", i);
- do_unregister_framebuffer(registered_fb[i]);
}
/*
* Restart the removal loop now that the device has been
--
2.35.1
