[PATCH v3] x86: sysfb: remove sysfb when probing real hw

[PATCH v3] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
resource-conflicts and drivers will refuse to load. A call to
request_mem_region() will fail, if the region overlaps with the mem-region
used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
are not affected as they don't reserve their resources, but some others
do, including (nvidiafb, cirrus, ..).

The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
platform-device during bootup but never release it. Probing simplefb on
this platform-device is fine, but the handover to real-hw via
remove_conflicting_framebuffers() will only unload the fbdev driver, but
keep the platform-device around. Thus, if the real hw-driver calls
request_mem_region() and friends on the same PCI region, we will get a
resource conflict and most drivers refuse to load. Users will see
errors like:
  "nvidiafb: cannot reserve video memory at <xyz>"

vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
and falling back to those drivers will avoid the bug. With sysfb enabled,
we need to properly unload the simple-framebuffer devices before probing
real hw-drivers.

This patch adds sysfb_unregister() for that purpose. It can be called from
any context (except from the platform-device ->probe and ->remove callback
path), synchronously unloads any global sysfb and prevents new sysfbs from
getting registered. Thus, you can call it even before any sysfb has been
loaded. Note that for now we only do that for simple-framebuffer devices,
as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
They're not affected by the resource-conflicts, so we can fix those later.

This also changes remove_conflicting_framebuffer() to call this helper
*before* trying its heuristic to remove conflicting drivers. This way, we
unload sysfbs properly on any conflict. But to avoid dead-locks in
register_framebuffer(), we must not call sysfb_unregister() for
framebuffers probing on sysfb devices. Hence, we simply remove any
aperture from simplefb and we're good to go. simplefb is unregistered by
sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
there currently is no handover from simplefb, so we're fine. If it's
added, they need to provide something like sysfb_unregister() too)

Cc: <stable@vger.kernel.org> # 3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
v3: add two comments to explain how sysfb_unregister() works

 arch/x86/include/asm/sysfb.h     |  6 ++++
 arch/x86/kernel/sysfb.c          | 68 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/sysfb_simplefb.c |  9 ++----
 drivers/video/fbmem.c            | 19 +++++++++++
 drivers/video/simplefb.c         |  8 -----
 5 files changed, 95 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..0877cf1 100644
--- a/arch/x86/include/asm/sysfb.h
+++ b/arch/x86/include/asm/sysfb.h
@@ -11,6 +11,7 @@
  * any later version.
  */
 
+#include <linux/fb.h>
 #include <linux/kernel.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/screen_info.h>
@@ -59,6 +60,11 @@ struct efifb_dmi_info {
 	int flags;
 };
 
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size);
+void sysfb_unregister(const struct apertures_struct *apert, bool primary);
+
 #ifdef CONFIG_EFI
 
 extern struct efifb_dmi_info efifb_dmi_list[];
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
index 193ec2c..882dc7c 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,79 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/mutex.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 #include <asm/sysfb.h>
 
+static DEFINE_MUTEX(sysfb_lock);
+static struct platform_device *sysfb_dev;
+
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size)
+{
+	struct platform_device *pd;
+	int ret = 0;
+
+	mutex_lock(&sysfb_lock);
+	if (!sysfb_dev) {
+		pd = platform_device_register_resndata(NULL, name, id,
+						       res, res_num,
+						       data, data_size);
+		if (IS_ERR(pd))
+			ret = PTR_ERR(pd);
+		else
+			sysfb_dev = pd;
+	}
+	mutex_unlock(&sysfb_lock);
+
+	return ret;
+}
+
+static bool sysfb_match(const struct apertures_struct *apert, bool primary)
+{
+	struct screen_info *si = &screen_info;
+	unsigned int i;
+	const struct aperture *a;
+
+	if (!apert || primary)
+		return true;
+
+	for (i = 0; i < apert->count; ++i) {
+		a = &apert->ranges[i];
+		if (a->base >= si->lfb_base &&
+		    a->base < si->lfb_base + ((u64)si->lfb_size << 16))
+			return true;
+		if (si->lfb_base >= a->base &&
+		    si->lfb_base < a->base + a->size)
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
+ * called from any context except recursively or from sysfb_register().
+ * Used by remove_conflicting_framebuffers() and friends.
+ */
+void sysfb_unregister(const struct apertures_struct *apert, bool primary)
+{
+	mutex_lock(&sysfb_lock);
+	if (!IS_ERR(sysfb_dev) && sysfb_dev) {
+		if (sysfb_match(apert, primary)) {
+			platform_device_unregister(sysfb_dev);
+			sysfb_dev = ERR_PTR(-EALREADY);
+		}
+	} else {
+		/* if !sysfb_dev, set err so no new sysfb is probed later */
+		sysfb_dev = ERR_PTR(-EALREADY);
+	}
+	mutex_unlock(&sysfb_lock);
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..a760d47 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
 __init int create_simplefb(const struct screen_info *si,
 			   const struct simplefb_platform_data *mode)
 {
-	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
 
@@ -86,10 +85,6 @@ __init int create_simplefb(const struct screen_info *si,
 	if (res.end <= res.start)
 		return -EINVAL;
 
-	pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
-					       &res, 1, mode, sizeof(*mode));
-	if (IS_ERR(pd))
-		return PTR_ERR(pd);
-
-	return 0;
+	return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
+			      sizeof(*mode));
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..590a46a 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
 
 #include <asm/fb.h>
 
+#if IS_ENABLED(CONFIG_X86)
+#include <asm/sysfb.h>
+#endif
 
     /*
      *  Frame buffer device initialization and setup routines
@@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	}
 }
 
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+				     bool primary)
+{
+	if (!apert)
+		return;
+
+#if IS_ENABLED(CONFIG_X86)
+	sysfb_unregister(apert, primary);
+#endif
+}
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i;
@@ -1742,6 +1756,8 @@ EXPORT_SYMBOL(unlink_framebuffer);
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {
+	remove_conflicting_sysfb(a, primary);
+
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);
@@ -1762,6 +1778,9 @@ register_framebuffer(struct fb_info *fb_info)
 {
 	int ret;
 
+	remove_conflicting_sysfb(fb_info->apertures,
+				 fb_is_primary_device(fb_info));
+
 	mutex_lock(&registration_lock);
 	ret = do_register_framebuffer(fb_info);
 	mutex_unlock(&registration_lock);
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..9f4a0cf 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->var.blue = params.format->blue;
 	info->var.transp = params.format->transp;
 
-	info->apertures = alloc_apertures(1);
-	if (!info->apertures) {
-		framebuffer_release(info);
-		return -ENOMEM;
-	}
-	info->apertures->ranges[0].base = info->fix.smem_start;
-	info->apertures->ranges[0].size = info->fix.smem_len;
-
 	info->fbops = &simplefb_ops;
 	info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
 	info->screen_base = ioremap_wc(info->fix.smem_start,
-- 
1.8.5.1

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

* David Herrmann <dh.herrmann@gmail.com> wrote:

> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -35,6 +35,9 @@
>  
>  #include <asm/fb.h>
>  
> +#if IS_ENABLED(CONFIG_X86)
> +#include <asm/sysfb.h>
> +#endif

I think this can be written as:

#ifdef CONFIG_X86
# include <asm/sysfb.h>
#endif

also ... the dependency on a large, unrelated option like CONFIG_X86 
looks pretty ugly here - is there no other, more specific CONFIG_ 
option that can be used here - such as CONFIG_FB_SIMPLE or 
CONFIG_X86_SYSFB?

> +#if IS_ENABLED(CONFIG_X86)
> +	sysfb_unregister(apert, primary);
> +#endif

Ditto.

Thanks,

	Ingo 

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -35,6 +35,9 @@
>>
>>  #include <asm/fb.h>
>>
>> +#if IS_ENABLED(CONFIG_X86)
>> +#include <asm/sysfb.h>
>> +#endif
>
> I think this can be written as:
>
> #ifdef CONFIG_X86
> # include <asm/sysfb.h>
> #endif
>
> also ... the dependency on a large, unrelated option like CONFIG_X86
> looks pretty ugly here - is there no other, more specific CONFIG_
> option that can be used here - such as CONFIG_FB_SIMPLE or
> CONFIG_X86_SYSFB?
>
>> +#if IS_ENABLED(CONFIG_X86)
>> +     sysfb_unregister(apert, primary);
>> +#endif
>
> Ditto.

CONFIG_X86 is probably never 'm'.. will fix that.
It was CONFIG_X86_SYSFB before and that works, too, but the broader
X86 seemed more appropriate as sysfb is available on all x86.

Note that I have patches here locally which move
sysfb_register/unregister to drivers/video/sysfb.c and add
include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected to
avoid the #ifdef. This will allow other architectures to do
gfx-hand-over, too. They seem too big for stable, though. That's why I
split them up and added it to x86/kernel/sysfb.c first.

Thanks
David

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

* David Herrmann <dh.herrmann@gmail.com> wrote:

> Hi
> 
> On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> --- a/drivers/video/fbmem.c
> >> +++ b/drivers/video/fbmem.c
> >> @@ -35,6 +35,9 @@
> >>
> >>  #include <asm/fb.h>
> >>
> >> +#if IS_ENABLED(CONFIG_X86)
> >> +#include <asm/sysfb.h>
> >> +#endif
> >
> > I think this can be written as:
> >
> > #ifdef CONFIG_X86
> > # include <asm/sysfb.h>
> > #endif
> >
> > also ... the dependency on a large, unrelated option like CONFIG_X86
> > looks pretty ugly here - is there no other, more specific CONFIG_
> > option that can be used here - such as CONFIG_FB_SIMPLE or
> > CONFIG_X86_SYSFB?
> >
> >> +#if IS_ENABLED(CONFIG_X86)
> >> +     sysfb_unregister(apert, primary);
> >> +#endif
> >
> > Ditto.
> 
> CONFIG_X86 is probably never 'm'.. will fix that. It was 
> CONFIG_X86_SYSFB before and that works, too, but the broader X86 
> seemed more appropriate as sysfb is available on all x86.

Well, sysfb is available if CONFIG_X86_SYSFB is set, right? So on 
!CONFIG_X86_SYSFB x86 kernels this code should not run, right?

> Note that I have patches here locally which move 
> sysfb_register/unregister to drivers/video/sysfb.c and add 
> include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected 
> to avoid the #ifdef. This will allow other architectures to do 
> gfx-hand-over, too. They seem too big for stable, though. That's why 
> I split them up and added it to x86/kernel/sysfb.c first.

Yeah, it's fine to do those cleanups after the minimal fix. But using 
a sensible config option must still be done - we cannot just slap 
broad CONFIG_X86 dependencies into random code.

Thanks,

	Ingo

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 6:09 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * David Herrmann <dh.herrmann@gmail.com> wrote:
>> >
>> >> --- a/drivers/video/fbmem.c
>> >> +++ b/drivers/video/fbmem.c
>> >> @@ -35,6 +35,9 @@
>> >>
>> >>  #include <asm/fb.h>
>> >>
>> >> +#if IS_ENABLED(CONFIG_X86)
>> >> +#include <asm/sysfb.h>
>> >> +#endif
>> >
>> > I think this can be written as:
>> >
>> > #ifdef CONFIG_X86
>> > # include <asm/sysfb.h>
>> > #endif
>> >
>> > also ... the dependency on a large, unrelated option like CONFIG_X86
>> > looks pretty ugly here - is there no other, more specific CONFIG_
>> > option that can be used here - such as CONFIG_FB_SIMPLE or
>> > CONFIG_X86_SYSFB?
>> >
>> >> +#if IS_ENABLED(CONFIG_X86)
>> >> +     sysfb_unregister(apert, primary);
>> >> +#endif
>> >
>> > Ditto.
>>
>> CONFIG_X86 is probably never 'm'.. will fix that. It was
>> CONFIG_X86_SYSFB before and that works, too, but the broader X86
>> seemed more appropriate as sysfb is available on all x86.
>
> Well, sysfb is available if CONFIG_X86_SYSFB is set, right? So on
> !CONFIG_X86_SYSFB x86 kernels this code should not run, right?

No. The sysfb code is always enabled. It provides platform-devices for
firmware-framebuffers which efifb/vesafb pick up. The X86_SYSFB option
only controls whether generic system-framebuffers should be used
instead of the specific efi/vesa FBs (to be compatible to other
architectures and keep efi/vesa specifics in arch/x86/).

But the system-framebuffer conversion (to a format compatible to
simplefb/simple-framebuffer) is what may break as the legacy fbs don't
reserve resources. Hence, putting it enclosed into X86_SYSFB works for
now. But if we start registering efifb/vesafb with sysfb, too, then we
need to change it to CONFIG_X86 as all fbs are affected then.

I think using X86_SYSFB (as in v1 of this patch) is the way to go. I
will fix it up and resend v4. The broader #ifdef can be used once we
do the more complex cleanups, in which case it will go away entirely.

>> Note that I have patches here locally which move
>> sysfb_register/unregister to drivers/video/sysfb.c and add
>> include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected
>> to avoid the #ifdef. This will allow other architectures to do
>> gfx-hand-over, too. They seem too big for stable, though. That's why
>> I split them up and added it to x86/kernel/sysfb.c first.
>
> Yeah, it's fine to do those cleanups after the minimal fix. But using
> a sensible config option must still be done - we cannot just slap
> broad CONFIG_X86 dependencies into random code.

Ok.

Thanks
David

[PATCH v4] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
resource-conflicts and drivers will refuse to load. A call to
request_mem_region() will fail, if the region overlaps with the mem-region
used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
are not affected as they don't reserve their resources, but some others
do, including (nvidiafb, cirrus, ..).

The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
platform-device during bootup but never release it. Probing simplefb on
this platform-device is fine, but the handover to real-hw via
remove_conflicting_framebuffers() will only unload the fbdev driver, but
keep the platform-device around. Thus, if the real hw-driver calls
request_mem_region() and friends on the same PCI region, we will get a
resource conflict and most drivers refuse to load. Users will see
errors like:
  "nvidiafb: cannot reserve video memory at <xyz>"

vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
and falling back to those drivers will avoid the bug. With sysfb enabled,
we need to properly unload the simple-framebuffer devices before probing
real hw-drivers.

This patch adds sysfb_unregister() for that purpose. It can be called from
any context (except from the platform-device ->probe and ->remove callback
path), synchronously unloads any global sysfb and prevents new sysfbs from
getting registered. Thus, you can call it even before any sysfb has been
loaded. Note that for now we only do that for simple-framebuffer devices,
as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
They're not affected by the resource-conflicts, so we can fix those later.

This also changes remove_conflicting_framebuffer() to call this helper
*before* trying its heuristic to remove conflicting drivers. This way, we
unload sysfbs properly on any conflict. But to avoid dead-locks in
register_framebuffer(), we must not call sysfb_unregister() for
framebuffers probing on sysfb devices. Hence, we simply remove any
aperture from simplefb and we're good to go. simplefb is unregistered by
sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
there currently is no handover from simplefb, so we're fine. If it's
added, they need to provide something like sysfb_unregister() too)

Cc: <stable@vger.kernel.org> # v3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
v4:
 - change #ifdef to CONFIG_X86_SYSFB again

 arch/x86/include/asm/sysfb.h     |  6 ++++
 arch/x86/kernel/sysfb.c          | 68 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/sysfb_simplefb.c |  9 ++----
 drivers/video/fbmem.c            | 19 +++++++++++
 drivers/video/simplefb.c         |  8 -----
 5 files changed, 95 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..0877cf1 100644
--- a/arch/x86/include/asm/sysfb.h
+++ b/arch/x86/include/asm/sysfb.h
@@ -11,6 +11,7 @@
  * any later version.
  */
 
+#include <linux/fb.h>
 #include <linux/kernel.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/screen_info.h>
@@ -59,6 +60,11 @@ struct efifb_dmi_info {
 	int flags;
 };
 
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size);
+void sysfb_unregister(const struct apertures_struct *apert, bool primary);
+
 #ifdef CONFIG_EFI
 
 extern struct efifb_dmi_info efifb_dmi_list[];
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
index 193ec2c..882dc7c 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,79 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/mutex.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 #include <asm/sysfb.h>
 
+static DEFINE_MUTEX(sysfb_lock);
+static struct platform_device *sysfb_dev;
+
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size)
+{
+	struct platform_device *pd;
+	int ret = 0;
+
+	mutex_lock(&sysfb_lock);
+	if (!sysfb_dev) {
+		pd = platform_device_register_resndata(NULL, name, id,
+						       res, res_num,
+						       data, data_size);
+		if (IS_ERR(pd))
+			ret = PTR_ERR(pd);
+		else
+			sysfb_dev = pd;
+	}
+	mutex_unlock(&sysfb_lock);
+
+	return ret;
+}
+
+static bool sysfb_match(const struct apertures_struct *apert, bool primary)
+{
+	struct screen_info *si = &screen_info;
+	unsigned int i;
+	const struct aperture *a;
+
+	if (!apert || primary)
+		return true;
+
+	for (i = 0; i < apert->count; ++i) {
+		a = &apert->ranges[i];
+		if (a->base >= si->lfb_base &&
+		    a->base < si->lfb_base + ((u64)si->lfb_size << 16))
+			return true;
+		if (si->lfb_base >= a->base &&
+		    si->lfb_base < a->base + a->size)
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
+ * called from any context except recursively or from sysfb_register().
+ * Used by remove_conflicting_framebuffers() and friends.
+ */
+void sysfb_unregister(const struct apertures_struct *apert, bool primary)
+{
+	mutex_lock(&sysfb_lock);
+	if (!IS_ERR(sysfb_dev) && sysfb_dev) {
+		if (sysfb_match(apert, primary)) {
+			platform_device_unregister(sysfb_dev);
+			sysfb_dev = ERR_PTR(-EALREADY);
+		}
+	} else {
+		/* if !sysfb_dev, set err so no new sysfb is probed later */
+		sysfb_dev = ERR_PTR(-EALREADY);
+	}
+	mutex_unlock(&sysfb_lock);
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..a760d47 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
 __init int create_simplefb(const struct screen_info *si,
 			   const struct simplefb_platform_data *mode)
 {
-	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
 
@@ -86,10 +85,6 @@ __init int create_simplefb(const struct screen_info *si,
 	if (res.end <= res.start)
 		return -EINVAL;
 
-	pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
-					       &res, 1, mode, sizeof(*mode));
-	if (IS_ERR(pd))
-		return PTR_ERR(pd);
-
-	return 0;
+	return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
+			      sizeof(*mode));
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..7dc0491 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
 
 #include <asm/fb.h>
 
+#ifdef CONFIG_X86_SYSFB
+#include <asm/sysfb.h>
+#endif
 
     /*
      *  Frame buffer device initialization and setup routines
@@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	}
 }
 
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+				     bool primary)
+{
+	if (!apert)
+		return;
+
+#ifdef CONFIG_X86_SYSFB
+	sysfb_unregister(apert, primary);
+#endif
+}
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i;
@@ -1742,6 +1756,8 @@ EXPORT_SYMBOL(unlink_framebuffer);
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {
+	remove_conflicting_sysfb(a, primary);
+
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);
@@ -1762,6 +1778,9 @@ register_framebuffer(struct fb_info *fb_info)
 {
 	int ret;
 
+	remove_conflicting_sysfb(fb_info->apertures,
+				 fb_is_primary_device(fb_info));
+
 	mutex_lock(&registration_lock);
 	ret = do_register_framebuffer(fb_info);
 	mutex_unlock(&registration_lock);
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..9f4a0cf 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->var.blue = params.format->blue;
 	info->var.transp = params.format->transp;
 
-	info->apertures = alloc_apertures(1);
-	if (!info->apertures) {
-		framebuffer_release(info);
-		return -ENOMEM;
-	}
-	info->apertures->ranges[0].base = info->fix.smem_start;
-	info->apertures->ranges[0].size = info->fix.smem_len;
-
 	info->fbops = &simplefb_ops;
 	info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
 	info->screen_base = ioremap_wc(info->fix.smem_start,
-- 
1.8.5.1

Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

* David Herrmann <dh.herrmann@gmail.com> wrote:

> +/*
> + * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
> + * called from any context except recursively or from sysfb_register().
> + * Used by remove_conflicting_framebuffers() and friends.
> + */
> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> +{
> +	mutex_lock(&sysfb_lock);
> +	if (!IS_ERR(sysfb_dev) && sysfb_dev) {
> +		if (sysfb_match(apert, primary)) {
> +			platform_device_unregister(sysfb_dev);
> +			sysfb_dev = ERR_PTR(-EALREADY);
> +		}
> +	} else {
> +		/* if !sysfb_dev, set err so no new sysfb is probed later */
> +		sysfb_dev = ERR_PTR(-EALREADY);

Just a small detail: we can get into this 'else' branch not just with 
NULL, but also with IS_ERR(sysfb_dev). In that case we override 
whatever error code is contained in sysfb_dev and overwrite it with 
ERR_PTR(-EALREADY).

(Probably not a big deal, because we don't actually ever seem to 
extract the error code from the pointer, but wanted to mention it.)

> +#ifdef CONFIG_X86_SYSFB
> +#include <asm/sysfb.h>
> +#endif

Pet peeve, this looks sexier:

#ifdef CONFIG_X86_SYSFB
# include <asm/sysfb.h>
#endif

> @@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>  	}
>  }
>  
> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> +				     bool primary)
> +{
> +	if (!apert)
> +		return;
> +
> +#ifdef CONFIG_X86_SYSFB
> +	sysfb_unregister(apert, primary);
> +#endif
> +}

So why not make sysfb_unregister() accept a !apert parameter (it would 
simply return), at which point remove_conflicting_sysfb() could be 
eliminated and just be replaced with a direct sysfb_unregister() call 
- with no #ifdefs.

We only need #ifdefs for the sysfb_unregister() declaration in the .h 
file.

At first sight this looks simpler and cleaner for the fix itself - no 
need for extra cleanups for this detail.

Thanks,

	Ingo

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Stephen Warren]

On 12/19/2013 03:13 AM, David Herrmann wrote:
> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> resource-conflicts and drivers will refuse to load. A call to
> request_mem_region() will fail, if the region overlaps with the mem-region
> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> are not affected as they don't reserve their resources, but some others
> do, including (nvidiafb, cirrus, ..).

I have validated that this doesn't cause any regressions on the/a
non-x86 platform using simplefb, although given the main point of this
patch is to fix issues on x86, I'm rather hesitant to give a tested-by
tag in case someone looking back interprets it incorrectly:-)

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

* Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 12/19/2013 03:13 AM, David Herrmann wrote:
> > With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> > resource-conflicts and drivers will refuse to load. A call to
> > request_mem_region() will fail, if the region overlaps with the mem-region
> > used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> > are not affected as they don't reserve their resources, but some others
> > do, including (nvidiafb, cirrus, ..).
> 
> I have validated that this doesn't cause any regressions on the/a
> non-x86 platform using simplefb, although given the main point of this
> patch is to fix issues on x86, I'm rather hesitant to give a tested-by
> tag in case someone looking back interprets it incorrectly:-)

Tested-on-ARM-by: Stephen Warren <swarren@wwwdotorg.org>

?

Thanks,

	Ingo

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Stephen Warren]

On 12/19/2013 11:55 AM, Ingo Molnar wrote:
> 
> * Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> On 12/19/2013 03:13 AM, David Herrmann wrote:
>>> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
>>> resource-conflicts and drivers will refuse to load. A call to
>>> request_mem_region() will fail, if the region overlaps with the mem-region
>>> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
>>> are not affected as they don't reserve their resources, but some others
>>> do, including (nvidiafb, cirrus, ..).
>>
>> I have validated that this doesn't cause any regressions on the/a
>> non-x86 platform using simplefb, although given the main point of this
>> patch is to fix issues on x86, I'm rather hesitant to give a tested-by
>> tag in case someone looking back interprets it incorrectly:-)
> 
> Tested-on-ARM-by: Stephen Warren <swarren@wwwdotorg.org>

I suppose with CC: stable there's some precedent for a comment after the
tag, so perhaps:

Tested-by: Stephen Warren <swarren@wwwdotorg.org> # on ARM only

Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

* Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 12/19/2013 11:55 AM, Ingo Molnar wrote:
> > 
> > * Stephen Warren <swarren@wwwdotorg.org> wrote:
> > 
> >> On 12/19/2013 03:13 AM, David Herrmann wrote:
> >>> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> >>> resource-conflicts and drivers will refuse to load. A call to
> >>> request_mem_region() will fail, if the region overlaps with the mem-region
> >>> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> >>> are not affected as they don't reserve their resources, but some others
> >>> do, including (nvidiafb, cirrus, ..).
> >>
> >> I have validated that this doesn't cause any regressions on the/a
> >> non-x86 platform using simplefb, although given the main point of this
> >> patch is to fix issues on x86, I'm rather hesitant to give a tested-by
> >> tag in case someone looking back interprets it incorrectly:-)
> > 
> > Tested-on-ARM-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> I suppose with CC: stable there's some precedent for a comment after the
> tag, so perhaps:
> 
> Tested-by: Stephen Warren <swarren@wwwdotorg.org> # on ARM only

That works too.

Thanks,

	Ingo

Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 7:33 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> +/*
>> + * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
>> + * called from any context except recursively or from sysfb_register().
>> + * Used by remove_conflicting_framebuffers() and friends.
>> + */
>> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
>> +{
>> +     mutex_lock(&sysfb_lock);
>> +     if (!IS_ERR(sysfb_dev) && sysfb_dev) {
>> +             if (sysfb_match(apert, primary)) {
>> +                     platform_device_unregister(sysfb_dev);
>> +                     sysfb_dev = ERR_PTR(-EALREADY);
>> +             }
>> +     } else {
>> +             /* if !sysfb_dev, set err so no new sysfb is probed later */
>> +             sysfb_dev = ERR_PTR(-EALREADY);
>
> Just a small detail: we can get into this 'else' branch not just with
> NULL, but also with IS_ERR(sysfb_dev). In that case we override
> whatever error code is contained in sysfb_dev and overwrite it with
> ERR_PTR(-EALREADY).
>
> (Probably not a big deal, because we don't actually ever seem to
> extract the error code from the pointer, but wanted to mention it.)

Yepp, I know, but that's just fine.

>> +#ifdef CONFIG_X86_SYSFB
>> +#include <asm/sysfb.h>
>> +#endif
>
> Pet peeve, this looks sexier:
>
> #ifdef CONFIG_X86_SYSFB
> # include <asm/sysfb.h>
> #endif
>
>> @@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>>       }
>>  }
>>
>> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
>> +                                  bool primary)
>> +{
>> +     if (!apert)
>> +             return;
>> +
>> +#ifdef CONFIG_X86_SYSFB
>> +     sysfb_unregister(apert, primary);
>> +#endif
>> +}
>
> So why not make sysfb_unregister() accept a !apert parameter (it would
> simply return), at which point remove_conflicting_sysfb() could be
> eliminated and just be replaced with a direct sysfb_unregister() call
> - with no #ifdefs.
>
> We only need #ifdefs for the sysfb_unregister() declaration in the .h
> file.

I can do that but we still need the #ifdef. sysfb is an x86-asm header
so it's not available on other archs. Even with #ifdef in the header I
still need it around the actual function call.

Thanks
David

> At first sight this looks simpler and cleaner for the fix itself - no
> need for extra cleanups for this detail.
>
> Thanks,
>
>         Ingo