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

[Daniel Vetter <daniel@xxxxxxxx>]

On Wed, Apr 06, 2022 at 11:39:18PM +0200, Javier Martinez Canillas wrote:
> 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.
> 
>    Instead, the sysfb_try_unregister() helper function can be called for
>    sysfb to attempt unregistering the device if is the one registered.
> 
> 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().
> 
> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> ---
> 
>  drivers/video/fbdev/core/fbmem.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index c1bfb8df9cba..acf641b05d11 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>
> @@ -1588,7 +1589,10 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>  				 * unregister_framebuffer() function that takes it.
>  				 */
>  				mutex_unlock(&registration_lock);
> -				platform_device_unregister(to_platform_device(device));
> +				if (!sysfb_try_unregister(device)) {
> +					/* sysfb didn't register this device, unregister it */

Maybe explain in the commit message that this is still needed for drivers
which set up their platform_dev themselves, like vga16fb.

Also I'm not sure we want to have an assumption encoded in fbmem.c here
that the sysfb device is always a platform device. I think it would be
better to call sysfb_try_unregister on any device, and then fall back to
the forced removal on our own if it's a platform device.

Also maybe change the comment to /* FIXME: Not all platform fb drivers use sysfb yet */

> +					platform_device_unregister(to_platform_device(device));
> +				}
>  				mutex_lock(&registration_lock);
>  				/*
>  				 * Restart the removal now that the platform device
> @@ -1781,6 +1785,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.
> +	 */

Yeah it's disappointing, but no worse than the piles of hacks we have now.

With the bikesheds addressed above:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> +	sysfb_disable();
> +
>  	mutex_lock(&registration_lock);
>  	do_remove_conflicting_framebuffers(a, name, primary);
>  	mutex_unlock(&registration_lock);
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch