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

[Daniel Vetter]

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.vet...@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
> ---
> 
>  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.vet...@ffwll.ch>

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