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

[Javier Martinez Canillas <javierm@redhat.com>]

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

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

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> CC: stable@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