Re: [PATCH v3 4/5] fbdev: Fix some race conditions between fbmem and sysfb

[Thomas Zimmermann]

Hi

Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas:
> 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 two issues that this patch fixes:
>
> 1) It is a layering violation for the fbdev core to unregister a device
>     that was registered by sysfb.

Why? We do this elsewhere and it works nicely.

>     Instead, the sysfb_try_unregister() helper function can be called for
>     sysfb to attempt unregistering the device if is the one registered.

And sysfb_try_unregister() is really just a glorified version of 
platform_device_unregister() IMHO.

> 2) The sysfb_init() function could be called after a DRM driver is probed
>     and requested to unregister devices for drivers with a conflicting fb.
>
>     To prevent this, disable any future sysfb platform device registration
>     by calling sysfb_disable(), if a driver requested to remove conflicting
>     framebuffers with remove_conflicting_framebuffers().

As I mentioned in another comment, as soon as there's anything else than 
EFI/VESA using the sysfb code the unregistering step is likely to break 
in some way.

Best regards
Thomas

> There are video drivers (e.g: vga16fb) that register their own device and
> don't use the sysfb infrastructure for that, so an unregistration has to
> be forced by fbmem if sysfb_try_unregister() fails to do the unregister.
>
> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>
> (no changes since v2)
>
> 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 | 42 ++++++++++++++++++++++++++++----
>   1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 0bb459258df3..8098305879f8 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -19,6 +19,7 @@
>   #include <linux/kernel.h>
>   #include <linux/major.h>
>   #include <linux/slab.h>
> +#include <linux/sysfb.h>
>   #include <linux/mm.h>
>   #include <linux/mman.h>
>   #include <linux/vt.h>
> @@ -1585,18 +1586,38 @@ 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(&registration_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, is a platform
> +						 * device registered by a video driver (e.g: vga16fb), so
> +						 * force its unregistration here. A proper fix would be to
> +						 * move all device registration to the sysfb infrastructure
> +						 * or 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(&registration_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
> @@ -1762,6 +1783,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
>   		do_free = true;
>   	}
>   
> +	/*
> +	 * If a driver asked to unregister a platform device registered by
> +	 * sysfb, then can be assumed that this is a driver for a display
> +	 * that is set up by the system firmware and has a generic driver.
> +	 *
> +	 * Drivers for devices that don't have a generic driver will never
> +	 * ask for this, so let's assume that a real driver for the display
> +	 * was already probed and prevent sysfb to register devices later.
> +	 */
> +	sysfb_disable();
> +
>   	mutex_lock(&registration_lock);
>   	do_remove_conflicting_framebuffers(a, name, primary);
>   	mutex_unlock(&registration_lock);

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature