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

[Daniel Vetter]

On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
>
> 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@xxxxxxx>
> >>>> 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.

Ah yeah that could be an issue. I think the right fix is to replace
the platform dev unregister with a sysfb_unregister() function in
sysfb.c, which is synced with a common lock with the sysfb_init
function and a small boolean. I think I can type that up quickly for
v3.
-Daniel

>
> --
> Best regards,
>
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>


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