Date:   Mon, 25 Apr 2022 10:54:29 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org,
        Alex Deucher <alexander.deucher@....com>,
        Borislav Petkov <bp@...e.de>,
        Changcheng Deng <deng.changcheng@....com.cn>,
        Daniel Vetter <daniel@...ll.ch>,
        Hans de Goede <hdegoede@...hat.com>,
        Helge Deller <deller@....de>, Johan Hovold <johan@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Miaoqian Lin <linmq006@...il.com>,
        Peter Jones <pjones@...hat.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Yizhuo Zhai <yzhai003@....edu>,
        Zhen Lei <thunder.leizhen@...wei.com>,
        linux-doc@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 0/5] Fix some race conditions that exists between fbmem
 and sysfb

Hi

Am 20.04.22 um 10:52 schrieb Javier Martinez Canillas:
> Hello,
> 
> The patches in this series are mostly changes suggested by Daniel Vetter
> to fix some race conditions that exists between the fbdev core (fbmem)
> and sysfb with regard to device registration and removal.
> 
> For example, it is currently possible for sysfb to register a platform
> device after a real DRM driver was registered and requested to remove the
> conflicting framebuffers.
> 
> A symptom of this issue, was worked around with by commit fb561bf9abde
> ("fbdev: Prevent probing generic drivers if a FB is already registered")
> but that's really a hack and should be reverted.

As I mentioned on IRC, I think this series should be merged for the 
reasons I give in the other comments.

> 
> This series attempt to fix it more properly and revert the mentioned hack.
> That will also unblock a pending patch to not make the num_registered_fb
> variable visible to drivers anymore, since that's internal to fbdev core.

Here's as far as I understand the problem:

  1) build DRM/fbdev and sysfb code into the kernel
  2) during boot, load the DRM/fbdev modules and have them acquire I/O 
ranges
  3) afterwards load sysfb and have it register platform devices for the 
generic framebuffers
  4) these devices now conflict with the already-registered DRM/fbdev 
devices

If that is the problem here, let's simply set a sysfb_disable flag in 
sysfb code when the first DRM/fbdev driver first loads. With the flag 
set, sysfb won't create any platform devices. We assume that there are 
now DRM/fbdev drivers for the framebuffers and sysfb won't be needed.

We can set the flag internally from drm_aperture_detach_drivers() [1] 
and do_remove_conflicting_framebuffers() [2].

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.4/source/drivers/gpu/drm/drm_aperture.c#L253
[2] 
https://elixir.bootlin.com/linux/v5.17.4/source/drivers/video/fbdev/core/fbmem.c#L1559

> 
> Patch #1 is just a trivial preparatory change.
> 
> Patch #2 add sysfb_disable() and sysfb_try_unregister() helpers for fbmem
> to use them.
> 
> Patch #3 changes how is dealt with conflicting framebuffers unregistering,
> rather than having a variable to determine if a lock should be take, it
> just drops the lock before unregistering the platform device.
> 
> Patch #4 fixes the mentioned race conditions and finally patch #5 is the
> revert patch that was posted by Daniel before but he dropped from his set.
> 
> The patches were tested on a rpi4 using different video configurations:
> (simpledrm -> vc4 both builtin, only vc4 builtin, only simpledrm builtin
> and simpledrm builtin with vc4 built as a module).
> 
> Best regards,
> Javier
> 
> Changes in v3:
> - Rebase on top of latest drm-misc-next branch.
> 
> Changes in v2:
> - Rebase on top of latest drm-misc-next and fix conflicts (Daniel Vetter).
> - Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter).
> - 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).
> - Drop RFC prefix since patches were already reviewed by Daniel Vetter.
> - Add Daniel Reviewed-by tags to the patches.
> 
> Daniel Vetter (1):
>    Revert "fbdev: Prevent probing generic drivers if a FB is already
>      registered"
> 
> Javier Martinez Canillas (4):
>    firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer
>    firmware: sysfb: Add helpers to unregister a pdev and disable
>      registration
>    fbdev: Restart conflicting fb removal loop when unregistering devices
>    fbdev: Fix some race conditions between fbmem and sysfb
> 
>   .../driver-api/firmware/other_interfaces.rst  |  6 ++
>   drivers/firmware/sysfb.c                      | 77 +++++++++++++++++--
>   drivers/firmware/sysfb_simplefb.c             | 16 ++--
>   drivers/video/fbdev/core/fbmem.c              | 62 ++++++++++++---
>   drivers/video/fbdev/efifb.c                   | 11 ---
>   drivers/video/fbdev/simplefb.c                | 11 ---
>   include/linux/fb.h                            |  1 -
>   include/linux/sysfb.h                         | 29 +++++--
>   8 files changed, 158 insertions(+), 55 deletions(-)
> 

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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists