All of lore.kernel.org
 help / color / mirror / Atom feed
From: Javier Martinez Canillas <javierm@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Javier Martinez Canillas <javierm@redhat.com>,
	Helge Deller <deller@gmx.de>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: [PATCH v5 4/7] fbdev: Make sysfb to unregister its own registered devices
Date: Wed, 11 May 2022 13:31:25 +0200	[thread overview]
Message-ID: <20220511113125.1252660-1-javierm@redhat.com> (raw)
In-Reply-To: <20220511112438.1251024-1-javierm@redhat.com>

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 the problem that sysfb doesn't know when the
device that registered is unregistered. This means that is not able to do
any cleanup if needed since the device pointer may not be valid anymore.

Not all platforms use sysfb to register the simple framebuffer devices,
so an unregistration has to be forced by fbmem if sysfb_try_unregister()
does not succeed at unregister the device.

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

Changes in v4:
- Drop call to sysfb_disable() in fbmem since is done in other places now.

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 | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2fda5917c212..9b035ef4d552 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>
@@ -1587,18 +1588,35 @@ 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(&registration_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, the platform
+						 * device was registered in other 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(&registration_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
-- 
2.35.1


WARNING: multiple messages have this Message-ID (diff)
From: Javier Martinez Canillas <javierm@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Helge Deller <deller@gmx.de>,
	Javier Martinez Canillas <javierm@redhat.com>,
	dri-devel@lists.freedesktop.org,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: [PATCH v5 4/7] fbdev: Make sysfb to unregister its own registered devices
Date: Wed, 11 May 2022 13:31:25 +0200	[thread overview]
Message-ID: <20220511113125.1252660-1-javierm@redhat.com> (raw)
In-Reply-To: <20220511112438.1251024-1-javierm@redhat.com>

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 the problem that sysfb doesn't know when the
device that registered is unregistered. This means that is not able to do
any cleanup if needed since the device pointer may not be valid anymore.

Not all platforms use sysfb to register the simple framebuffer devices,
so an unregistration has to be forced by fbmem if sysfb_try_unregister()
does not succeed at unregister the device.

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

Changes in v4:
- Drop call to sysfb_disable() in fbmem since is done in other places now.

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 | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2fda5917c212..9b035ef4d552 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>
@@ -1587,18 +1588,35 @@ 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(&registration_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, the platform
+						 * device was registered in other 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(&registration_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
-- 
2.35.1


  parent reply	other threads:[~2022-05-11 11:31 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-11 11:24 [PATCH v5 0/7] Fix some races between sysfb device registration and drivers probe Javier Martinez Canillas
2022-05-11 11:24 ` Javier Martinez Canillas
2022-05-11 11:24 ` [PATCH v5 1/7] firmware: sysfb: Make sysfb_create_simplefb() return a pdev pointer Javier Martinez Canillas
2022-05-11 11:24   ` Javier Martinez Canillas
2022-05-11 12:04   ` Javier Martinez Canillas
2022-05-11 12:04     ` Javier Martinez Canillas
2022-05-11 11:24 ` [PATCH v5 2/7] firmware: sysfb: Add helpers to unregister a pdev and disable registration Javier Martinez Canillas
2022-05-11 11:24   ` Javier Martinez Canillas
2022-05-11 11:54   ` Thomas Zimmermann
2022-05-11 11:54     ` Thomas Zimmermann
2022-05-11 12:01     ` Javier Martinez Canillas
2022-05-11 12:01       ` Javier Martinez Canillas
2022-05-11 12:05       ` Thomas Zimmermann
2022-05-11 12:05         ` Thomas Zimmermann
2022-05-11 12:29         ` Javier Martinez Canillas
2022-05-11 12:29           ` Javier Martinez Canillas
2022-05-11 12:02   ` Thomas Zimmermann
2022-05-11 12:02     ` Thomas Zimmermann
2022-05-11 12:24     ` Javier Martinez Canillas
2022-05-11 12:24       ` Javier Martinez Canillas
2022-05-11 11:30 ` [PATCH v5 3/7] fbdev: Restart conflicting fb removal loop when unregistering devices Javier Martinez Canillas
2022-05-11 11:30   ` Javier Martinez Canillas
2022-05-11 11:47   ` Thomas Zimmermann
2022-05-11 11:47     ` Thomas Zimmermann
2022-05-11 11:57     ` Javier Martinez Canillas
2022-05-11 11:57       ` Javier Martinez Canillas
2022-05-13 12:28   ` Javier Martinez Canillas
2022-05-13 12:28     ` Javier Martinez Canillas
2022-05-11 11:31 ` Javier Martinez Canillas [this message]
2022-05-11 11:31   ` [PATCH v5 4/7] fbdev: Make sysfb to unregister its own registered devices Javier Martinez Canillas
2022-05-11 11:31 ` [PATCH v5 5/7] fbdev: Disable sysfb device registration when removing conflicting FBs Javier Martinez Canillas
2022-05-11 11:31   ` Javier Martinez Canillas
2022-06-07 15:01   ` Daniel Vetter
2022-06-07 15:01     ` Daniel Vetter
2022-06-07 15:41     ` Javier Martinez Canillas
2022-05-11 11:32 ` [PATCH v5 6/7] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Javier Martinez Canillas
2022-05-11 11:32   ` Javier Martinez Canillas
2022-05-11 11:32 ` [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c Javier Martinez Canillas
2022-05-11 11:32   ` Javier Martinez Canillas
2022-05-11 17:00   ` Sam Ravnborg
2022-05-11 17:00     ` Sam Ravnborg
2022-05-11 17:17     ` Guenter Roeck
2022-05-11 17:17       ` Guenter Roeck
2022-05-11 17:34       ` Javier Martinez Canillas
2022-05-11 17:34         ` Javier Martinez Canillas
2022-05-12 18:32         ` Sam Ravnborg
2022-05-12 18:32           ` Sam Ravnborg
2022-05-13 10:48 ` [PATCH v5 0/7] Fix some races between sysfb device registration and drivers probe Thomas Zimmermann
2022-05-13 10:48   ` Thomas Zimmermann
2022-05-13 11:10   ` Javier Martinez Canillas
2022-05-13 11:10     ` Javier Martinez Canillas
2022-05-13 11:32     ` Thomas Zimmermann
2022-05-13 11:32       ` Thomas Zimmermann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220511113125.1252660-1-javierm@redhat.com \
    --to=javierm@redhat.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=deller@gmx.de \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.