Re: [PATCH v3 0/5] Fix some race conditions that exists between fbmem and sysfb

From: Javier Martinez Canillas
Date: Mon Apr 25 2022 - 05:50:19 EST

Hello Thomas,

Thanks for the feedback. It was very useful.

On 4/25/22 11:15, Thomas Zimmermann wrote:
> Hi
> Am 25.04.22 um 10:54 schrieb Thomas Zimmermann:
>> 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.

You meant that should *not* get merged, as we discussed over IRC.

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

That's correct, yes.

>> 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].
> And further thinking about it, it would be better to set such a flag
> after successfully registering a DRM/fbdev device. So we know that
> there's at least one working display in the system. We don't have to
> rely on generic framebuffers after that.

Exactly, should be done when the device is registered rather than when
the driver is registered or a call is made to remove the conflicting FB.

I'll rework this series with only the bits for sysfb_disable() and drop
the rest. We can go back to the discussion of the remaining parts later
if that makes sense (I still think that patch 3/5 is a better approach,
but let's defer that for a different series).

Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat