Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Daniel,

On 4/5/22 10:40, Daniel Vetter wrote:
> On Tue, Apr 05, 2022 at 10:36:35AM +0200, Daniel Vetter wrote:
>> On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
>>> On 2/8/22 22:08, Daniel Vetter wrote:
>>>> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
>>>>
>>>> With
>>>>
>>>> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
>>>> Author: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Date:   Tue Jan 25 10:12:18 2022 +0100
>>>>
>>>>     fbdev: Hot-unplug firmware fb devices on forced removal
>>>>
>>>> this should be fixed properly and we can remove this somewhat hackish
>>>> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
>>>> enabled).
>>>>
>>>
>>> Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
>>> of platform devices matched with fbdev drivers to be properly unregistered if
>>> a DRM driver attempts to remove all the conflicting framebuffers.
>>>
>>> But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
>>> a FB is already registered") worked around is different. It happens when the
>>> DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
>>> kicking out of conflicting framebuffers already happened and these drivers
>>> will be allowed to probe even when a DRM driver is already present.
>>>
>>> We need a clearer way to prevent it, but can't revert fb561bf9abde until that.
>>
>> Yeah that entire area is a mess still, ideally we'd have something else
>> creating the platform devices, and efifb/offb and all these would just
>> bind against them.
>>
>> Hm one idea that just crossed my mind: Could we have a flag in fb_info for
>> fw drivers, and check this in framebuffer_register? Then at least all the
>> logic would be in the fbdev core.
>

I can't answer right away since I've since forgotten this part of the code
and will require to do a detailed read to refresh my memory.

I'll answer later but preferred to mention the other question ASAP.
 
> Ok coffee just kicked in, how exactly does your scenario work?
> 
> This code I'm reverting here is in the platform_dev->probe function.
> Thomas' patch removes the platform_dev. How exactly can you still probe
> against a platform dev if that platform dev is gone?
>

Because the platform was not even registered by the time the DRM driver
probed and all the devices for the conflicting drivers were unregistered.
 
> Iow, now that I reponder your case after a few weeks I'm no longer sure
> things work like you claim.
>

This is how I think that work, please let me know if you see something
wrong in my logic:

1) A PCI device of OF device is registered for the GPU, this attempt to
   match a registered driver but no driver was registered that match yet.

2) The efifb driver is built-in, will be initialized according to the link
   order of the objects under drivers/video and the fbdev driver is registered.

   There is no platform device or PCI/OF device registered that matches.

3) The DRM driver is built-in, will be initialized according to the link
   order of the objects under drivers/gpu and the DRM driver is registered.
   
   This matches the device registered in (1) and the DRM driver probes.

4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
   before registering the DRM device.

   There are no conflicting drivers or platform device at this point.

5) Latter at some point the drivers/firmware/sysfb.c init function is
   executed, and this registers a platform device for the generic fb.

   This device matches the efifb driver registered in (2) and the fbdev
   driver probes.
   
   Since that happens *after* the DRM driver already matched, probed
   and registered the DRM device, that is a bug and what the reverted
   patch worked around.

So we need to prevent (5) if (1) and (3) already happened. Having a flag
set in the fbdev core somewhere when remove_conflicting_framebuffers()
is called could be a solution indeed.

That is, the fbdev core needs to know that a DRM driver already probed
and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE

I can attempt to write a patch for that.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat