Hello Thomas, Thanks for the patch. On 1/24/22 13:36, Thomas Zimmermann wrote: > Hot-unplug all firmware-framebuffer devices as part of removing > them via remove_conflicting_framebuffers() et al. Releases all > memory regions to be acquired by native drivers. > > Firmware, such as EFI, install a framebuffer while posting the > computer. After removing the firmware-framebuffer device from fbdev, > a native driver takes over the hardware and the firmware framebuffer > becomes invalid. > > Firmware-framebuffer drivers, specifically simplefb, don't release > their device from Linux' device hierarchy. It still owns the firmware > framebuffer and blocks the native drivers from loading. This has been > observed in the vmwgfx driver. [1] > > Initiating a device removal (i.e., hot unplug) as part of > remove_conflicting_framebuffers() removes the underlying device and > returns the memory range to the system. > > [1] https://lore.kernel.org/dri-devel/20220117180359.18114-1-zack@xxxxxxx/ > I would add a Reported-by tag here for Zack. > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> > CC: stable@xxxxxxxxxxxxxxx # v5.11+ > --- > drivers/video/fbdev/core/fbmem.c | 29 ++++++++++++++++++++++++++--- > include/linux/fb.h | 1 + > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 0fa7ede94fa6..f73f8415b8cb 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -25,6 +25,7 @@ > #include <linux/init.h> > #include <linux/linux_logo.h> > #include <linux/proc_fs.h> > +#include <linux/platform_device.h> > #include <linux/seq_file.h> > #include <linux/console.h> > #include <linux/kmod.h> > @@ -1557,18 +1558,36 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > /* check all firmware fbs and kick off if the base addr overlaps */ > for_each_registered_fb(i) { > struct apertures_struct *gen_aper; > + struct device *dev; > > if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE)) > continue; > > gen_aper = registered_fb[i]->apertures; > + dev = registered_fb[i]->device; > if (fb_do_apertures_overlap(gen_aper, a) || > (primary && gen_aper && gen_aper->count && > gen_aper->ranges[0].base == VGA_FB_PHYS)) { > > printk(KERN_INFO "fb%d: switching to %s from %s\n", > i, name, registered_fb[i]->fix.id); > - do_unregister_framebuffer(registered_fb[i]); > + > + /* > + * If we kick-out a firmware driver, we also want to remove > + * the underlying platform device, such as simple-framebuffer, > + * VESA, EFI, etc. A native driver will then be able to > + * allocate the memory range. > + * > + * If it's not a platform device, at least print a warning. A > + * fix would add code to remove the device from the system. > + */ > + if (dev_is_platform(dev)) { In do_register_framebuffer() creating the fb%d is not a fatal error. It would be safer to do if (!IS_ERR_OR_NULL(dev) && dev_is_platform(dev)) instead here. https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fbmem.c#L1605 > + registered_fb[i]->forced_out = true; > + platform_device_unregister(to_platform_device(dev)); > + } else { > + pr_warn("fb%d: cannot remove device\n", i); > + do_unregister_framebuffer(registered_fb[i]); > + } > } > } > } > @@ -1898,9 +1917,13 @@ EXPORT_SYMBOL(register_framebuffer); > void > unregister_framebuffer(struct fb_info *fb_info) > { > - mutex_lock(®istration_lock); > + bool forced_out = fb_info->forced_out; > + > + if (!forced_out) > + mutex_lock(®istration_lock); > do_unregister_framebuffer(fb_info); > - mutex_unlock(®istration_lock); > + if (!forced_out) > + mutex_unlock(®istration_lock); > } I'm not sure to follow the logic here. The forced_out bool is set when the platform device is unregistered in do_remove_conflicting_framebuffers(), but shouldn't the struct platform_driver .remove callback be executed even in this case ? That is, the platform_device_unregister() will trigger the call to the .remove callback that in turn will call unregister_framebuffer(). Shouldn't we always hold the mutex when calling do_unregister_framebuffer() ? Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
