| Message ID | 20220420085303.100654-5-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fix some race conditions that exists between fbmem and sysfb | expand |
Hi Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas: > The platform devices registered in sysfb match with a firmware-based fbdev > or DRM driver, that are used to have early graphics using framebuffers set > up by the system firmware. > > Real DRM 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 two issues that this patch fixes: > > 1) It is a layering violation for the fbdev core to unregister a device > that was registered by sysfb. Why? We do this elsewhere and it works nicely. > > Instead, the sysfb_try_unregister() helper function can be called for > sysfb to attempt unregistering the device if is the one registered. And sysfb_try_unregister() is really just a glorified version of platform_device_unregister() IMHO. > > 2) The sysfb_init() function could be called after a DRM driver is probed > and requested to unregister devices for drivers with a conflicting fb. > > To prevent this, disable any future sysfb platform device registration > by calling sysfb_disable(), if a driver requested to remove conflicting > framebuffers with remove_conflicting_framebuffers(). As I mentioned in another comment, as soon as there's anything else than EFI/VESA using the sysfb code the unregistering step is likely to break in some way. Best regards Thomas > > There are video drivers (e.g: vga16fb) that register their own device and > don't use the sysfb infrastructure for that, so an unregistration has to > be forced by fbmem if sysfb_try_unregister() fails to do the unregister. > > 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> > --- > > (no changes since v2) > > 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 | 42 ++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 0bb459258df3..8098305879f8 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -19,6 +19,7 @@ > #include <linux/kernel.h> > #include <linux/major.h> > #include <linux/slab.h> > +#include <linux/sysfb.h> > #include <linux/mm.h> > #include <linux/mman.h> > #include <linux/vt.h> > @@ -1585,18 +1586,38 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > if (!device) { > pr_warn("fb%d: no device set\n", i); > do_unregister_framebuffer(registered_fb[i]); > - } else if (dev_is_platform(device)) { > + } else { > /* > * Drop the lock because if the device is unregistered, its > * driver will call to unregister_framebuffer(), that takes > * this lock. > */ > mutex_unlock(®istration_lock); > - platform_device_unregister(to_platform_device(device)); > + /* > + * First attempt the device to be unregistered by sysfb. > + */ > + if (!sysfb_try_unregister(device)) { > + if (dev_is_platform(device)) { > + /* > + * FIXME: sysfb didn't register this device, is a platform > + * device registered by a video driver (e.g: vga16fb), so > + * force its unregistration here. A proper fix would be to > + * move all device registration to the sysfb infrastructure > + * or platform code. > + */ > + platform_device_unregister(to_platform_device(device)); > + } else { > + /* > + * If is not a platform device, at least print a warning. A > + * fix would add to make the code that registered the device > + * to also unregister it. > + */ > + pr_warn("fb%d: cannot remove device\n", i); > + /* call unregister_framebuffer() since the lock was dropped */ > + unregister_framebuffer(registered_fb[i]); > + } > + } > mutex_lock(®istration_lock); > - } else { > - pr_warn("fb%d: cannot remove device\n", i); > - do_unregister_framebuffer(registered_fb[i]); > } > /* > * Restart the removal loop now that the device has been > @@ -1762,6 +1783,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, > do_free = true; > } > > + /* > + * 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(); > + > mutex_lock(®istration_lock); > do_remove_conflicting_framebuffers(a, name, primary); > mutex_unlock(®istration_lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 0bb459258df3..8098305879f8 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -19,6 +19,7 @@ #include <linux/kernel.h> #include <linux/major.h> #include <linux/slab.h> +#include <linux/sysfb.h> #include <linux/mm.h> #include <linux/mman.h> #include <linux/vt.h> @@ -1585,18 +1586,38 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, if (!device) { pr_warn("fb%d: no device set\n", i); do_unregister_framebuffer(registered_fb[i]); - } else if (dev_is_platform(device)) { + } else { /* * Drop the lock because if the device is unregistered, its * driver will call to unregister_framebuffer(), that takes * this lock. */ mutex_unlock(®istration_lock); - platform_device_unregister(to_platform_device(device)); + /* + * First attempt the device to be unregistered by sysfb. + */ + if (!sysfb_try_unregister(device)) { + if (dev_is_platform(device)) { + /* + * FIXME: sysfb didn't register this device, is a platform + * device registered by a video driver (e.g: vga16fb), so + * force its unregistration here. A proper fix would be to + * move all device registration to the sysfb infrastructure + * or platform code. + */ + platform_device_unregister(to_platform_device(device)); + } else { + /* + * If is not a platform device, at least print a warning. A + * fix would add to make the code that registered the device + * to also unregister it. + */ + pr_warn("fb%d: cannot remove device\n", i); + /* call unregister_framebuffer() since the lock was dropped */ + unregister_framebuffer(registered_fb[i]); + } + } mutex_lock(®istration_lock); - } else { - pr_warn("fb%d: cannot remove device\n", i); - do_unregister_framebuffer(registered_fb[i]); } /* * Restart the removal loop now that the device has been @@ -1762,6 +1783,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, do_free = true; } + /* + * 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(); + mutex_lock(®istration_lock); do_remove_conflicting_framebuffers(a, name, primary); mutex_unlock(®istration_lock);