Re: [PATCH 1/5] fbdev: Hot-unplug firmware fb devices on forced removal

[Javier Martinez Canillas]

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-z...@kde.org/
> 

I would add a Reported-by tag here for Zack.

> Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
> CC: sta...@vger.kernel.org # 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(&registration_lock);
> +     bool forced_out = fb_info->forced_out;
> +
> +     if (!forced_out)
> +             mutex_lock(&registration_lock);
>       do_unregister_framebuffer(fb_info);
> -     mutex_unlock(&registration_lock);
> +     if (!forced_out)
> +             mutex_unlock(&registration_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