| Message ID | 20220511113144.1252729-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Fix some races between sysfb device registration and drivers probe | expand |
On Wed, May 11, 2022 at 01:31:44PM +0200, Javier Martinez Canillas wrote: > The platform devices registered by sysfb match with firmware-based DRM or > fbdev drivers, that are used to have early graphics using a framebuffer > provided by the system firmware. > > DRM or fbdev 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 a race, since the sysfb_init() function could > be called after a DRM or fbdev driver is probed and request to unregister > the devices for drivers with conflicting framebuffes. > > To prevent this, disable any future sysfb platform device registration by > calling sysfb_disable(), if a driver requests to remove the conflicting > framebuffers. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > Changes in v5: > - Move the sysfb_disable() call at conflicting framebuffers again to > avoid the need of a DRIVER_FIRMWARE capability flag. > - Add Daniel Vetter's Reviewed-by tag again since reverted to the old > patch that he already reviewed in v2. > > Changes in v3: > - Call sysfb_disable() when a DRM dev and a fbdev are registered rather > than when conflicting framebuffers are removed (Thomas Zimmermann). > - Call sysfb_disable() when a fbdev framebuffer is registered rather > than when conflicting framebuffers are removed (Thomas Zimmermann). > - Drop Daniel Vetter's Reviewed-by tag since patch changed a lot. > > Changes in v2: > - 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). > > drivers/video/fbdev/core/fbmem.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 9b035ef4d552..265efa189bcc 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1789,6 +1789,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, > if (do_free) > kfree(a); > > + /* > + * 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. > + */ > + sysfb_disable(); So the og version had (or should have had at least) the sysfb_disable() call before we go through the loop and try to unregister stuff. I think this needs to be done before we call do_remove_conflicting_framebuffer() instead. With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > return 0; > } > EXPORT_SYMBOL(remove_conflicting_framebuffers); > -- > 2.35.1 >
Hello Daniel, On 6/7/22 17:01, Daniel Vetter wrote: [snip] >> >> drivers/video/fbdev/core/fbmem.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c >> index 9b035ef4d552..265efa189bcc 100644 >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -1789,6 +1789,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, >> if (do_free) >> kfree(a); >> >> + /* >> + * 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. >> + */ >> + sysfb_disable(); > > So the og version had (or should have had at least) the sysfb_disable() > call before we go through the loop and try to unregister stuff. I think > this needs to be done before we call do_remove_conflicting_framebuffer() > instead. With that: > Yes, the original version did that but also the original version didn't attempt to remove the devices registered by sysfb on sysfb_disable(). I was going to answer that this has to be done after the loop because that way fbmem could first ask sysfb to remove the devices, but then I realized that you are correct That this wouldn't be needed if sysfb does the disable (and unregistration) before the loop. So by doing it before the loop, we should be able to drop [PATCH v5 4/7] fbdev: Make sysfb to unregister its own registered devices: https://lists.freedesktop.org/archives/dri-devel/2022-May/355201.html Since by the time the loop is executed, no registered_fb associated with a device that was registered by sysfb should be present in that array.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 9b035ef4d552..265efa189bcc 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1789,6 +1789,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, if (do_free) kfree(a); + /* + * 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. + */ + sysfb_disable(); + return 0; } EXPORT_SYMBOL(remove_conflicting_framebuffers);