Hi

Am 24.01.22 um 14:52 schrieb 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-zack@kde.org/
>>
> 
> I would add a Reported-by tag here for Zack.

Indeed, I simply forgot about it.

> 
>> 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

'dev' here refers to 'fb_info->device', which is the underlying device 
created by the sysfb code.  fb_info->dev is something different.

> 
>> +				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() ?

Doing the hot-unplug will end up in unregister_framebuffer(), but we 
already hold the lock from the do_remove_conflicting_framebuffer() code.

Best regards
Thomas

> 
> Best regards,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev