cirrusdrmfb broken with simplefb

cirrusdrmfb broken with simplefb

[Takashi Iwai]

Hi,

with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
gets broken now, as reported at:
    https://bugzilla.novell.com/show_bug.cgi?id…5821

The cirrus VGA resource is reserved at first as "BOOTFB" in
arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
device.  This resource is, however, never released until the platform
device is destroyed, and the framebuffer switching doesn't trigger
it.  It calls fb's destroy callback, at most.  Then, cirrus driver
tries to assign the resource, fails and gives up, resulting in a
complete blank screen.

The same problem should exist on other KMS drivers like mgag200 or
ast, not only cirrus.  Intel graphics doesn't hit this problem just
because the reserved iomem by BOOTFB isn't required by i915 driver.

The patch below is a quick attempt to solve the issue.  It adds a new
API function for releasing resources of platform_device, and call it
in destroy op of simplefb.  But, forcibly releasing resources of a
parent device doesn't sound like a correct design.  We may take such
as a band aid, but definitely need a more fundamental fix.

Any thoughts?


thanks,

Takashi

---
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..f939236 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
 }
 EXPORT_SYMBOL_GPL(platform_device_add_data);
 
+static void do_release_resources(struct platform_device *pdev, int nums)
+{
+	int i;
+
+	for (i = 0; i < nums; i++) {
+		struct resource *r = &pdev->resource[i];
+		unsigned long type = resource_type(r);
+
+		if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
+			release_resource(r);
+	}
+
+	kfree(pdev->resource);
+	pdev->resource = NULL;
+	pdev->num_resources = 0;
+}
+
 /**
  * platform_device_add - add a platform device to device hierarchy
  * @pdev: platform device we're adding
@@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
 		pdev->id = PLATFORM_DEVID_AUTO;
 	}
 
-	while (--i >= 0) {
-		struct resource *r = &pdev->resource[i];
-		unsigned long type = resource_type(r);
-
-		if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
-			release_resource(r);
-	}
+	do_release_resources(pdev, i - 1);
 
  err_out:
 	return ret;
@@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
  */
 void platform_device_del(struct platform_device *pdev)
 {
-	int i;
-
 	if (pdev) {
 		device_del(&pdev->dev);
 
@@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
 			pdev->id = PLATFORM_DEVID_AUTO;
 		}
 
-		for (i = 0; i < pdev->num_resources; i++) {
-			struct resource *r = &pdev->resource[i];
-			unsigned long type = resource_type(r);
-
-			if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
-				release_resource(r);
-		}
+		do_release_resources(pdev, pdev->num_resources);
 	}
 }
 EXPORT_SYMBOL_GPL(platform_device_del);
 
+void platform_device_release_resources(struct platform_device *pdev)
+{
+	do_release_resources(pdev, pdev->num_resources);
+}
+EXPORT_SYMBOL_GPL(platform_device_release_resources);
+
 /**
  * platform_device_register - add a platform-level device
  * @pdev: platform device we're adding
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..fbf5e89 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
+	platform_device_release_resources(to_platform_device(info->device));
 }
 
 static struct fb_ops simplefb_ops = {
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..7cc1f54 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -42,6 +42,7 @@ struct platform_device {
 
 extern int platform_device_register(struct platform_device *);
 extern void platform_device_unregister(struct platform_device *);
+extern void platform_device_release_resources(struct platform_device *pdev);
 
 extern struct bus_type platform_bus_type;
 extern struct device platform_bus;

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Hi,
>
> with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
> gets broken now, as reported at:
>     https://bugzilla.novell.com/show_bug.cgi?id…5821
>
> The cirrus VGA resource is reserved at first as "BOOTFB" in
> arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
> device.  This resource is, however, never released until the platform
> device is destroyed, and the framebuffer switching doesn't trigger
> it.  It calls fb's destroy callback, at most.  Then, cirrus driver
> tries to assign the resource, fails and gives up, resulting in a
> complete blank screen.
>
> The same problem should exist on other KMS drivers like mgag200 or
> ast, not only cirrus.  Intel graphics doesn't hit this problem just
> because the reserved iomem by BOOTFB isn't required by i915 driver.
>
> The patch below is a quick attempt to solve the issue.  It adds a new
> API function for releasing resources of platform_device, and call it
> in destroy op of simplefb.  But, forcibly releasing resources of a
> parent device doesn't sound like a correct design.  We may take such
> as a band aid, but definitely need a more fundamental fix.
>
> Any thoughts?

That bug always existed, simplefb is just the first driver to hit it
(vesafb/efifb didn't use resources). I'm aware of the issue but as a
workaround you can simply disable CONFIG_X86_SYSFB. That restores the
old behavior.

As a proper fix, I'd propose something like:
  dev = platform_find_device("platform-framebuffer");
  platform_remove_device(dev);

And we wrap this as:
  sysfb_remove_framebuffers()
in arch/x86/kernel/sysfb.c

This should cause a ->remove() event for the platform-driver and
correctly release the resources. Comments?
I will try to write a patch and send it later.

Thanks
David

>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b79..f939236 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
>  }
>  EXPORT_SYMBOL_GPL(platform_device_add_data);
>
> +static void do_release_resources(struct platform_device *pdev, int nums)
> +{
> +       int i;
> +
> +       for (i = 0; i < nums; i++) {
> +               struct resource *r = &pdev->resource[i];
> +               unsigned long type = resource_type(r);
> +
> +               if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> +                       release_resource(r);
> +       }
> +
> +       kfree(pdev->resource);
> +       pdev->resource = NULL;
> +       pdev->num_resources = 0;
> +}
> +
>  /**
>   * platform_device_add - add a platform device to device hierarchy
>   * @pdev: platform device we're adding
> @@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
>                 pdev->id = PLATFORM_DEVID_AUTO;
>         }
>
> -       while (--i >= 0) {
> -               struct resource *r = &pdev->resource[i];
> -               unsigned long type = resource_type(r);
> -
> -               if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> -                       release_resource(r);
> -       }
> +       do_release_resources(pdev, i - 1);
>
>   err_out:
>         return ret;
> @@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
>   */
>  void platform_device_del(struct platform_device *pdev)
>  {
> -       int i;
> -
>         if (pdev) {
>                 device_del(&pdev->dev);
>
> @@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
>                         pdev->id = PLATFORM_DEVID_AUTO;
>                 }
>
> -               for (i = 0; i < pdev->num_resources; i++) {
> -                       struct resource *r = &pdev->resource[i];
> -                       unsigned long type = resource_type(r);
> -
> -                       if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> -                               release_resource(r);
> -               }
> +               do_release_resources(pdev, pdev->num_resources);
>         }
>  }
>  EXPORT_SYMBOL_GPL(platform_device_del);
>
> +void platform_device_release_resources(struct platform_device *pdev)
> +{
> +       do_release_resources(pdev, pdev->num_resources);
> +}
> +EXPORT_SYMBOL_GPL(platform_device_release_resources);
> +
>  /**
>   * platform_device_register - add a platform-level device
>   * @pdev: platform device we're adding
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a0..fbf5e89 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
>  {
>         if (info->screen_base)
>                 iounmap(info->screen_base);
> +       platform_device_release_resources(to_platform_device(info->device));
>  }
>
>  static struct fb_ops simplefb_ops = {
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 16f6654..7cc1f54 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -42,6 +42,7 @@ struct platform_device {
>
>  extern int platform_device_register(struct platform_device *);
>  extern void platform_device_unregister(struct platform_device *);
> +extern void platform_device_release_resources(struct platform_device *pdev);
>
>  extern struct bus_type platform_bus_type;
>  extern struct device platform_bus;

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Wed, 18 Dec 2013 09:21:28 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi,
> >
> > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
> > gets broken now, as reported at:
> >     https://bugzilla.novell.com/show_bug.cgi?id…5821
> >
> > The cirrus VGA resource is reserved at first as "BOOTFB" in
> > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
> > device.  This resource is, however, never released until the platform
> > device is destroyed, and the framebuffer switching doesn't trigger
> > it.  It calls fb's destroy callback, at most.  Then, cirrus driver
> > tries to assign the resource, fails and gives up, resulting in a
> > complete blank screen.
> >
> > The same problem should exist on other KMS drivers like mgag200 or
> > ast, not only cirrus.  Intel graphics doesn't hit this problem just
> > because the reserved iomem by BOOTFB isn't required by i915 driver.
> >
> > The patch below is a quick attempt to solve the issue.  It adds a new
> > API function for releasing resources of platform_device, and call it
> > in destroy op of simplefb.  But, forcibly releasing resources of a
> > parent device doesn't sound like a correct design.  We may take such
> > as a band aid, but definitely need a more fundamental fix.
> >
> > Any thoughts?
> 
> That bug always existed, simplefb is just the first driver to hit it
> (vesafb/efifb didn't use resources).

Heh, the bug didn't "exist" because no other grabbed the resource
before.  The way the cirrus driver allocates the resource is no bug,
per se.  But the proper resource takeover is missing.

> I'm aware of the issue but as a
> workaround you can simply disable CONFIG_X86_SYSFB. That restores the
> old behavior.

Yes, but CONFIG_X86_SYSFB breaks things as of now.  Shouldn't it be
mentioned or give some kconfig hints instead of silently giving a
broken output, if any quick fix isn't possible?

> As a proper fix, I'd propose something like:
>   dev = platform_find_device("platform-framebuffer");
>   platform_remove_device(dev);
> 
> And we wrap this as:
>   sysfb_remove_framebuffers()
> in arch/x86/kernel/sysfb.c
> 
> This should cause a ->remove() event for the platform-driver and
> correctly release the resources. Comments?

Who will call this?  From destroy callback?
Or can we simply do put_device() the bound platform device instead?

> I will try to write a patch and send it later.

Thanks!


Takashi

> 
> Thanks
> David
> 
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b79..f939236 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
> >  }
> >  EXPORT_SYMBOL_GPL(platform_device_add_data);
> >
> > +static void do_release_resources(struct platform_device *pdev, int nums)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < nums; i++) {
> > +               struct resource *r = &pdev->resource[i];
> > +               unsigned long type = resource_type(r);
> > +
> > +               if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > +                       release_resource(r);
> > +       }
> > +
> > +       kfree(pdev->resource);
> > +       pdev->resource = NULL;
> > +       pdev->num_resources = 0;
> > +}
> > +
> >  /**
> >   * platform_device_add - add a platform device to device hierarchy
> >   * @pdev: platform device we're adding
> > @@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
> >                 pdev->id = PLATFORM_DEVID_AUTO;
> >         }
> >
> > -       while (--i >= 0) {
> > -               struct resource *r = &pdev->resource[i];
> > -               unsigned long type = resource_type(r);
> > -
> > -               if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > -                       release_resource(r);
> > -       }
> > +       do_release_resources(pdev, i - 1);
> >
> >   err_out:
> >         return ret;
> > @@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
> >   */
> >  void platform_device_del(struct platform_device *pdev)
> >  {
> > -       int i;
> > -
> >         if (pdev) {
> >                 device_del(&pdev->dev);
> >
> > @@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
> >                         pdev->id = PLATFORM_DEVID_AUTO;
> >                 }
> >
> > -               for (i = 0; i < pdev->num_resources; i++) {
> > -                       struct resource *r = &pdev->resource[i];
> > -                       unsigned long type = resource_type(r);
> > -
> > -                       if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > -                               release_resource(r);
> > -               }
> > +               do_release_resources(pdev, pdev->num_resources);
> >         }
> >  }
> >  EXPORT_SYMBOL_GPL(platform_device_del);
> >
> > +void platform_device_release_resources(struct platform_device *pdev)
> > +{
> > +       do_release_resources(pdev, pdev->num_resources);
> > +}
> > +EXPORT_SYMBOL_GPL(platform_device_release_resources);
> > +
> >  /**
> >   * platform_device_register - add a platform-level device
> >   * @pdev: platform device we're adding
> > diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> > index 210f3a0..fbf5e89 100644
> > --- a/drivers/video/simplefb.c
> > +++ b/drivers/video/simplefb.c
> > @@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
> >  {
> >         if (info->screen_base)
> >                 iounmap(info->screen_base);
> > +       platform_device_release_resources(to_platform_device(info->device));
> >  }
> >
> >  static struct fb_ops simplefb_ops = {
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index 16f6654..7cc1f54 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -42,6 +42,7 @@ struct platform_device {
> >
> >  extern int platform_device_register(struct platform_device *);
> >  extern void platform_device_unregister(struct platform_device *);
> > +extern void platform_device_release_resources(struct platform_device *pdev);
> >
> >  extern struct bus_type platform_bus_type;
> >  extern struct device platform_bus;
> 

Re: cirrusdrmfb broken with simplefb

[Geert Uytterhoeven]

On Wed, Dec 18, 2013 at 9:44 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> That bug always existed, simplefb is just the first driver to hit it
>> (vesafb/efifb didn't use resources).
>
> Heh, the bug didn't "exist" because no other grabbed the resource
> before.  The way the cirrus driver allocates the resource is no bug,
> per se.  But the proper resource takeover is missing.
>
>> I'm aware of the issue but as a
>> workaround you can simply disable CONFIG_X86_SYSFB. That restores the
>> old behavior.
>
> Yes, but CONFIG_X86_SYSFB breaks things as of now.  Shouldn't it be
> mentioned or give some kconfig hints instead of silently giving a
> broken output, if any quick fix isn't possible?

Indeed. That's called a "regression" in distro/allmodconfig kernels.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: cirrusdrmfb broken with simplefb

[Ingo Molnar]

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

> Hi
> 
> On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi,
> >
> > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
> > gets broken now, as reported at:
> >     https://bugzilla.novell.com/show_bug.cgi?id…5821
> >
> > The cirrus VGA resource is reserved at first as "BOOTFB" in
> > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
> > device.  This resource is, however, never released until the platform
> > device is destroyed, and the framebuffer switching doesn't trigger
> > it.  It calls fb's destroy callback, at most.  Then, cirrus driver
> > tries to assign the resource, fails and gives up, resulting in a
> > complete blank screen.
> >
> > The same problem should exist on other KMS drivers like mgag200 or
> > ast, not only cirrus.  Intel graphics doesn't hit this problem just
> > because the reserved iomem by BOOTFB isn't required by i915 driver.
> >
> > The patch below is a quick attempt to solve the issue.  It adds a new
> > API function for releasing resources of platform_device, and call it
> > in destroy op of simplefb.  But, forcibly releasing resources of a
> > parent device doesn't sound like a correct design.  We may take such
> > as a band aid, but definitely need a more fundamental fix.
> >
> > Any thoughts?
> 
> That bug always existed, simplefb is just the first driver to hit it 
> (vesafb/efifb didn't use resources). I'm aware of the issue but as a 
> workaround you can simply disable CONFIG_X86_SYSFB. That restores 
> the old behavior.

This looks like a regression, so we'll either need a fix or we'll have 
to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.

> As a proper fix, I'd propose something like:
>   dev = platform_find_device("platform-framebuffer");
>   platform_remove_device(dev);
> 
> And we wrap this as:
>   sysfb_remove_framebuffers()
> in arch/x86/kernel/sysfb.c
> 
> This should cause a ->remove() event for the platform-driver and
> correctly release the resources. Comments?
> I will try to write a patch and send it later.

A fix would be awesome.

Thanks,

	Ingo

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Wed, 18 Dec 2013 09:44:35 +0100,
Takashi Iwai wrote:
> 
> At Wed, 18 Dec 2013 09:21:28 +0100,
> David Herrmann wrote:
> > 
> > Hi
> > 
> > On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > > Hi,
> > >
> > > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
> > > gets broken now, as reported at:
> > >     https://bugzilla.novell.com/show_bug.cgi?id…5821
> > >
> > > The cirrus VGA resource is reserved at first as "BOOTFB" in
> > > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
> > > device.  This resource is, however, never released until the platform
> > > device is destroyed, and the framebuffer switching doesn't trigger
> > > it.  It calls fb's destroy callback, at most.  Then, cirrus driver
> > > tries to assign the resource, fails and gives up, resulting in a
> > > complete blank screen.
> > >
> > > The same problem should exist on other KMS drivers like mgag200 or
> > > ast, not only cirrus.  Intel graphics doesn't hit this problem just
> > > because the reserved iomem by BOOTFB isn't required by i915 driver.
> > >
> > > The patch below is a quick attempt to solve the issue.  It adds a new
> > > API function for releasing resources of platform_device, and call it
> > > in destroy op of simplefb.  But, forcibly releasing resources of a
> > > parent device doesn't sound like a correct design.  We may take such
> > > as a band aid, but definitely need a more fundamental fix.
> > >
> > > Any thoughts?
> > 
> > That bug always existed, simplefb is just the first driver to hit it
> > (vesafb/efifb didn't use resources).
> 
> Heh, the bug didn't "exist" because no other grabbed the resource
> before.  The way the cirrus driver allocates the resource is no bug,
> per se.  But the proper resource takeover is missing.
> 
> > I'm aware of the issue but as a
> > workaround you can simply disable CONFIG_X86_SYSFB. That restores the
> > old behavior.
> 
> Yes, but CONFIG_X86_SYSFB breaks things as of now.  Shouldn't it be
> mentioned or give some kconfig hints instead of silently giving a
> broken output, if any quick fix isn't possible?
> 
> > As a proper fix, I'd propose something like:
> >   dev = platform_find_device("platform-framebuffer");
> >   platform_remove_device(dev);
> > 
> > And we wrap this as:
> >   sysfb_remove_framebuffers()
> > in arch/x86/kernel/sysfb.c
> > 
> > This should cause a ->remove() event for the platform-driver and
> > correctly release the resources. Comments?
> 
> Who will call this?  From destroy callback?
> Or can we simply do put_device() the bound platform device instead?

Just tested, put_device() doesn't work since the refcount isn't 1, so
if any, we need to call platform_device_unregister() to release
forcibly.

The patch below seems working at a quick test.  But I still have
some bad feeling for unregistering the parent device from the child's
destroy callback...


thanks,

Takashi

---
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a02121a..196360c54157 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -41,6 +41,11 @@ static struct fb_var_screeninfo simplefb_var = {
 	.vmode		= FB_VMODE_NONINTERLACED,
 };
 
+struct simplefb_priv {
+	u32 pseudo_palette[16];
+	struct platform_device *pdev;
+};
+
 static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 			      u_int transp, struct fb_info *info)
 {
@@ -68,8 +73,19 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 
 static void simplefb_destroy(struct fb_info *info)
 {
+	struct simplefb_priv *priv = info->par;
+	struct platform_device *pdev = priv->pdev;
+
 	if (info->screen_base)
 		iounmap(info->screen_base);
+
+	/* release the bound platform device when called from
+	 * remove_conflicting_framebuffers()
+	 */
+	if (pdev) {
+		priv->pdev = NULL; /* to avoid double-free */
+		platform_device_unregister(pdev);
+	}
 }
 
 static struct fb_ops simplefb_ops = {
@@ -169,6 +185,7 @@ static int simplefb_probe(struct platform_device *pdev)
 	struct simplefb_params params;
 	struct fb_info *info;
 	struct resource *mem;
+	struct simplefb_priv *priv;
 
 	if (fb_get_options("simplefb", NULL))
 		return -ENODEV;
@@ -188,7 +205,7 @@ static int simplefb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+	info = framebuffer_alloc(sizeof(struct simplefb_priv), &pdev->dev);
 	if (!info)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, info);
@@ -225,7 +242,9 @@ static int simplefb_probe(struct platform_device *pdev)
 		framebuffer_release(info);
 		return -ENODEV;
 	}
-	info->pseudo_palette = (void *)(info + 1);
+
+	priv = info->par;
+	info->pseudo_palette = priv->pseudo_palette;
 
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
@@ -243,6 +262,7 @@ static int simplefb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	priv->pdev = pdev;
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
@@ -251,8 +271,13 @@ static int simplefb_probe(struct platform_device *pdev)
 static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
+	struct simplefb_priv *priv = info->par;
+
+	if (priv->pdev) {
+		priv->pdev = NULL; /* to avoid double-free */
+		unregister_framebuffer(info);
+	}
 
-	unregister_framebuffer(info);
 	framebuffer_release(info);
 
 	return 0;

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 11:17 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 18 Dec 2013 09:44:35 +0100,
> Takashi Iwai wrote:
>>
>> At Wed, 18 Dec 2013 09:21:28 +0100,
>> David Herrmann wrote:
>> >
>> > Hi
>> >
>> > On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > > Hi,
>> > >
>> > > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
>> > > gets broken now, as reported at:
>> > >     https://bugzilla.novell.com/show_bug.cgi?id…5821
>> > >
>> > > The cirrus VGA resource is reserved at first as "BOOTFB" in
>> > > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
>> > > device.  This resource is, however, never released until the platform
>> > > device is destroyed, and the framebuffer switching doesn't trigger
>> > > it.  It calls fb's destroy callback, at most.  Then, cirrus driver
>> > > tries to assign the resource, fails and gives up, resulting in a
>> > > complete blank screen.
>> > >
>> > > The same problem should exist on other KMS drivers like mgag200 or
>> > > ast, not only cirrus.  Intel graphics doesn't hit this problem just
>> > > because the reserved iomem by BOOTFB isn't required by i915 driver.
>> > >
>> > > The patch below is a quick attempt to solve the issue.  It adds a new
>> > > API function for releasing resources of platform_device, and call it
>> > > in destroy op of simplefb.  But, forcibly releasing resources of a
>> > > parent device doesn't sound like a correct design.  We may take such
>> > > as a band aid, but definitely need a more fundamental fix.
>> > >
>> > > Any thoughts?
>> >
>> > That bug always existed, simplefb is just the first driver to hit it
>> > (vesafb/efifb didn't use resources).
>>
>> Heh, the bug didn't "exist" because no other grabbed the resource
>> before.  The way the cirrus driver allocates the resource is no bug,
>> per se.  But the proper resource takeover is missing.
>>
>> > I'm aware of the issue but as a
>> > workaround you can simply disable CONFIG_X86_SYSFB. That restores the
>> > old behavior.
>>
>> Yes, but CONFIG_X86_SYSFB breaks things as of now.  Shouldn't it be
>> mentioned or give some kconfig hints instead of silently giving a
>> broken output, if any quick fix isn't possible?
>>
>> > As a proper fix, I'd propose something like:
>> >   dev = platform_find_device("platform-framebuffer");
>> >   platform_remove_device(dev);
>> >
>> > And we wrap this as:
>> >   sysfb_remove_framebuffers()
>> > in arch/x86/kernel/sysfb.c
>> >
>> > This should cause a ->remove() event for the platform-driver and
>> > correctly release the resources. Comments?
>>
>> Who will call this?  From destroy callback?
>> Or can we simply do put_device() the bound platform device instead?
>
> Just tested, put_device() doesn't work since the refcount isn't 1, so
> if any, we need to call platform_device_unregister() to release
> forcibly.
>
> The patch below seems working at a quick test.  But I still have
> some bad feeling for unregistering the parent device from the child's
> destroy callback...

The idea is right, but call it from
"remove_conflicting_framebuffers()". The problem is, if we load a
driver on top of a dummy like vesafb/efifb/simplefb, we never removed
them properly. Instead, we only unregistered their fb driver. But with
the driver-model, that's just removing the driver, not the dummy
device.

So we'd just call platform_device_unregister() in
remove_conflicting_framebuffers() instead of unregister_framebuffer()
on such devices. I'm now at home and will have a look.

Thanks
David

>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a02121a..196360c54157 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -41,6 +41,11 @@ static struct fb_var_screeninfo simplefb_var = {
>         .vmode          = FB_VMODE_NONINTERLACED,
>  };
>
> +struct simplefb_priv {
> +       u32 pseudo_palette[16];
> +       struct platform_device *pdev;
> +};
> +
>  static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>                               u_int transp, struct fb_info *info)
>  {
> @@ -68,8 +73,19 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>
>  static void simplefb_destroy(struct fb_info *info)
>  {
> +       struct simplefb_priv *priv = info->par;
> +       struct platform_device *pdev = priv->pdev;
> +
>         if (info->screen_base)
>                 iounmap(info->screen_base);
> +
> +       /* release the bound platform device when called from
> +        * remove_conflicting_framebuffers()
> +        */
> +       if (pdev) {
> +               priv->pdev = NULL; /* to avoid double-free */
> +               platform_device_unregister(pdev);
> +       }
>  }
>
>  static struct fb_ops simplefb_ops = {
> @@ -169,6 +185,7 @@ static int simplefb_probe(struct platform_device *pdev)
>         struct simplefb_params params;
>         struct fb_info *info;
>         struct resource *mem;
> +       struct simplefb_priv *priv;
>
>         if (fb_get_options("simplefb", NULL))
>                 return -ENODEV;
> @@ -188,7 +205,7 @@ static int simplefb_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
> +       info = framebuffer_alloc(sizeof(struct simplefb_priv), &pdev->dev);
>         if (!info)
>                 return -ENOMEM;
>         platform_set_drvdata(pdev, info);
> @@ -225,7 +242,9 @@ static int simplefb_probe(struct platform_device *pdev)
>                 framebuffer_release(info);
>                 return -ENODEV;
>         }
> -       info->pseudo_palette = (void *)(info + 1);
> +
> +       priv = info->par;
> +       info->pseudo_palette = priv->pseudo_palette;
>
>         dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
>                              info->fix.smem_start, info->fix.smem_len,
> @@ -243,6 +262,7 @@ static int simplefb_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       priv->pdev = pdev;
>         dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>
>         return 0;
> @@ -251,8 +271,13 @@ static int simplefb_probe(struct platform_device *pdev)
>  static int simplefb_remove(struct platform_device *pdev)
>  {
>         struct fb_info *info = platform_get_drvdata(pdev);
> +       struct simplefb_priv *priv = info->par;
> +
> +       if (priv->pdev) {
> +               priv->pdev = NULL; /* to avoid double-free */
> +               unregister_framebuffer(info);
> +       }
>
> -       unregister_framebuffer(info);
>         framebuffer_release(info);
>
>         return 0;

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Wed, 18 Dec 2013 11:21:46 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Wed, Dec 18, 2013 at 11:17 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Wed, 18 Dec 2013 09:44:35 +0100,
> > Takashi Iwai wrote:
> >>
> >> At Wed, 18 Dec 2013 09:21:28 +0100,
> >> David Herrmann wrote:
> >> >
> >> > Hi
> >> >
> >> > On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > > Hi,
> >> > >
> >> > > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM
> >> > > gets broken now, as reported at:
> >> > >     https://bugzilla.novell.com/show_bug.cgi?id…5821
> >> > >
> >> > > The cirrus VGA resource is reserved at first as "BOOTFB" in
> >> > > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform
> >> > > device.  This resource is, however, never released until the platform
> >> > > device is destroyed, and the framebuffer switching doesn't trigger
> >> > > it.  It calls fb's destroy callback, at most.  Then, cirrus driver
> >> > > tries to assign the resource, fails and gives up, resulting in a
> >> > > complete blank screen.
> >> > >
> >> > > The same problem should exist on other KMS drivers like mgag200 or
> >> > > ast, not only cirrus.  Intel graphics doesn't hit this problem just
> >> > > because the reserved iomem by BOOTFB isn't required by i915 driver.
> >> > >
> >> > > The patch below is a quick attempt to solve the issue.  It adds a new
> >> > > API function for releasing resources of platform_device, and call it
> >> > > in destroy op of simplefb.  But, forcibly releasing resources of a
> >> > > parent device doesn't sound like a correct design.  We may take such
> >> > > as a band aid, but definitely need a more fundamental fix.
> >> > >
> >> > > Any thoughts?
> >> >
> >> > That bug always existed, simplefb is just the first driver to hit it
> >> > (vesafb/efifb didn't use resources).
> >>
> >> Heh, the bug didn't "exist" because no other grabbed the resource
> >> before.  The way the cirrus driver allocates the resource is no bug,
> >> per se.  But the proper resource takeover is missing.
> >>
> >> > I'm aware of the issue but as a
> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores the
> >> > old behavior.
> >>
> >> Yes, but CONFIG_X86_SYSFB breaks things as of now.  Shouldn't it be
> >> mentioned or give some kconfig hints instead of silently giving a
> >> broken output, if any quick fix isn't possible?
> >>
> >> > As a proper fix, I'd propose something like:
> >> >   dev = platform_find_device("platform-framebuffer");
> >> >   platform_remove_device(dev);
> >> >
> >> > And we wrap this as:
> >> >   sysfb_remove_framebuffers()
> >> > in arch/x86/kernel/sysfb.c
> >> >
> >> > This should cause a ->remove() event for the platform-driver and
> >> > correctly release the resources. Comments?
> >>
> >> Who will call this?  From destroy callback?
> >> Or can we simply do put_device() the bound platform device instead?
> >
> > Just tested, put_device() doesn't work since the refcount isn't 1, so
> > if any, we need to call platform_device_unregister() to release
> > forcibly.
> >
> > The patch below seems working at a quick test.  But I still have
> > some bad feeling for unregistering the parent device from the child's
> > destroy callback...
> 
> The idea is right, but call it from
> "remove_conflicting_framebuffers()". The problem is, if we load a
> driver on top of a dummy like vesafb/efifb/simplefb, we never removed
> them properly. Instead, we only unregistered their fb driver. But with
> the driver-model, that's just removing the driver, not the dummy
> device.
> 
> So we'd just call platform_device_unregister() in
> remove_conflicting_framebuffers() instead of unregister_framebuffer()
> on such devices. I'm now at home and will have a look.

So, make sysfb_remove_framebuffers() available on all archs and
call it there?  Hmm...  I think we need a better binding between
platform_device and framebuffer device.  Maybe better than killing
a parent device, but IMO, the root cause is rather that we have two 
individual devices (platform and framebuffer) unnecessarily.  Can we
sort this out?

In anyway, the patch below is a quick revisited fix.  It's calling
sysfb_remove_framebuffers().  Instead of finding over a platform name
string as you suggested, the patch just tries to remember the
instance.

The ifdef in fbmem.c looks ugly, but we can clean up such a thing
later.  And, yes, the patch should be split.


thanks,

Takashi

---
diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..94ba0e4 100644
--- a/arch/x86/include/asm/sysfb.h
+++ b/arch/x86/include/asm/sysfb.h
@@ -76,8 +76,9 @@ static inline void sysfb_apply_efi_quirks(void)
 
 bool parse_mode(const struct screen_info *si,
 		struct simplefb_platform_data *mode);
-int create_simplefb(const struct screen_info *si,
+struct platform_device *create_simplefb(const struct screen_info *si,
 		    const struct simplefb_platform_data *mode);
+void sysfb_remove_framebuffers(void);
 
 #else /* CONFIG_X86_SYSFB */
 
@@ -87,10 +88,10 @@ static inline bool parse_mode(const struct screen_info *si,
 	return false;
 }
 
-static inline int create_simplefb(const struct screen_info *si,
+static inline struct platform_device *create_simplefb(const struct screen_info *si,
 				  const struct simplefb_platform_data *mode)
 {
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 #endif /* CONFIG_X86_SYSFB */
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
index 193ec2c..4faa5d0 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -38,23 +38,22 @@
 #include <linux/screen_info.h>
 #include <asm/sysfb.h>
 
+static struct platform_device *pd;
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
 	struct simplefb_platform_data mode;
-	struct platform_device *pd;
 	const char *name;
 	bool compatible;
-	int ret;
 
 	sysfb_apply_efi_quirks();
 
 	/* try to create a simple-framebuffer device */
 	compatible = parse_mode(si, &mode);
 	if (compatible) {
-		ret = create_simplefb(si, &mode);
-		if (!ret)
-			return 0;
+		pd = create_simplefb(si, &mode);
+		return IS_ERR(pd) ? PTR_ERR(pd) : 0;
 	}
 
 	/* if the FB is incompatible, create a legacy framebuffer device */
@@ -70,5 +69,14 @@ static __init int sysfb_init(void)
 	return IS_ERR(pd) ? PTR_ERR(pd) : 0;
 }
 
+void sysfb_remove_framebuffers(void)
+{
+	if (!IS_ERR_OR_NULL(pd)) {
+		platform_device_unregister(pd);
+		pd = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(sysfb_remove_framebuffers);
+
 /* must execute after PCI subsystem for EFI quirks */
 device_initcall(sysfb_init);
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..d90042ac 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -61,10 +61,9 @@ __init bool parse_mode(const struct screen_info *si,
 	return false;
 }
 
-__init int create_simplefb(const struct screen_info *si,
+__init struct platform_device *create_simplefb(const struct screen_info *si,
 			   const struct simplefb_platform_data *mode)
 {
-	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
 
@@ -74,7 +73,7 @@ __init int create_simplefb(const struct screen_info *si,
 	len = PAGE_ALIGN(len);
 	if (len > (u64)si->lfb_size << 16) {
 		printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	/* setup IORESOURCE_MEM as framebuffer memory */
@@ -84,12 +83,8 @@ __init int create_simplefb(const struct screen_info *si,
 	res.start = si->lfb_base;
 	res.end = si->lfb_base + len - 1;
 	if (res.end <= res.start)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
-	pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
+	return platform_device_register_resndata(NULL, "simple-framebuffer", 0,
 					       &res, 1, mode, sizeof(*mode));
-	if (IS_ERR(pd))
-		return PTR_ERR(pd);
-
-	return 0;
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..49b0f89 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -34,6 +34,9 @@
 #include <linux/fb.h>
 
 #include <asm/fb.h>
+#ifdef CONFIG_X86
+#include <asm/sysfb.h>
+#endif
 
 
     /*
@@ -1600,6 +1603,9 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 			       "%s vs %s - removing generic driver\n",
 			       name, registered_fb[i]->fix.id);
 			do_unregister_framebuffer(registered_fb[i]);
+#ifdef CONFIG_X86
+			sysfb_remove_framebuffers();
+#endif
 		}
 	}
 }
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..c825b88 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -41,6 +41,11 @@ static struct fb_var_screeninfo simplefb_var = {
 	.vmode		= FB_VMODE_NONINTERLACED,
 };
 
+struct simplefb_priv {
+	u32 pseudo_palette[16];
+	bool fb_registered;
+};
+
 static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 			      u_int transp, struct fb_info *info)
 {
@@ -68,8 +73,11 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 
 static void simplefb_destroy(struct fb_info *info)
 {
+	struct simplefb_priv *priv = info->par;
+
 	if (info->screen_base)
 		iounmap(info->screen_base);
+	priv->fb_registered = false;
 }
 
 static struct fb_ops simplefb_ops = {
@@ -169,6 +177,7 @@ static int simplefb_probe(struct platform_device *pdev)
 	struct simplefb_params params;
 	struct fb_info *info;
 	struct resource *mem;
+	struct simplefb_priv *priv;
 
 	if (fb_get_options("simplefb", NULL))
 		return -ENODEV;
@@ -188,7 +197,7 @@ static int simplefb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+	info = framebuffer_alloc(sizeof(struct simplefb_priv), &pdev->dev);
 	if (!info)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, info);
@@ -225,7 +234,9 @@ static int simplefb_probe(struct platform_device *pdev)
 		framebuffer_release(info);
 		return -ENODEV;
 	}
-	info->pseudo_palette = (void *)(info + 1);
+
+	priv = info->par;
+	info->pseudo_palette = priv->pseudo_palette;
 
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
@@ -243,6 +254,7 @@ static int simplefb_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	priv->fb_registered = true;
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
@@ -251,8 +263,11 @@ static int simplefb_probe(struct platform_device *pdev)
 static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
+	struct simplefb_priv *priv = info->par;
+
+	if (priv->fb_registered)
+		unregister_framebuffer(info);
 
-	unregister_framebuffer(info);
 	framebuffer_release(info);
 
 	return 0;

[RFC] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

If we probe a real hw driver for graphics devices, we need to unload any
generic fallback driver like efifb/vesafb/simplefb on the system
framebuffer. This is currently done via remove_conflicting_framebuffers()
in fbmem.c. However, this only removes the fbdev driver, not the fake
platform devices underneath. This hasn't been a problem so far, as efifb
and vesafb didn't store any resources there. However, with simplefb this
changed.

To correctly release the IORESOURCE_MEM resources of simple-framebuffer
platform devices, we need to unregister the underlying platform device
*before* probing any new hw driver. This patch adds sysfb_unregister() for
that purpose. It can be called from any context (except from the
platform-device ->remove callback path) and synchronously unloads any
global sysfb and prevents new sysfbs from getting registered. Thus, you
can call it even before any sysfb has been loaded.

This also changes remove_conflicting_framebuffer() to call this helper
*before* trying it's fbdev heuristic to remove conflicting drivers.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi

This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
this in for 3.14. Your call..

This patch basically simulates an unplug event for system-framebuffers when
loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
optionally pass an aperture-struct and primary-flag similar to
remove_conflicting_framebuffers(). If they're not passed, we remove it
unconditionally.

Untested, but my kernel compiles are already running. If my tests succeed and
nobody has objections, I can resend it as proper PATCH and marked for stable.
And maybe split the fbmem and sysfb changes into two patches..

Thanks
David

 arch/x86/include/asm/sysfb.h     |  10 ++++
 arch/x86/kernel/sysfb.c          | 103 +++++++++++++++++++++++++++++++++++++--
 arch/x86/kernel/sysfb_simplefb.c |   5 +-
 drivers/video/fbmem.c            |  16 ++++++
 4 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..713bc17 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,15 @@ struct efifb_dmi_info {
 	int flags;
 };
 
+__init struct platform_device *sysfb_alloc(const char *name,
+					   int id,
+					   const struct resource *res,
+					   unsigned int res_num,
+					   const void *data,
+					   size_t data_size);
+__init int sysfb_register(struct platform_device *dev);
+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..3d4554e 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,106 @@
 #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;
+
+/* register @dev as sysfb; takes ownership over @dev */
+__init int sysfb_register(struct platform_device *dev)
+{
+	int r = 0;
+
+	mutex_lock(&sysfb_lock);
+	if (!sysfb_dev) {
+		r = platform_device_add(dev);
+		if (r < 0)
+			put_device(&dev->dev);
+		else
+			sysfb_dev = dev;
+	} else {
+		/* if there already is/was a sysfb, destroy @pd but return 0 */
+		platform_device_put(dev);
+	}
+	mutex_unlock(&sysfb_lock);
+
+	return r;
+}
+
+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 */
+void sysfb_unregister(const struct apertures_struct *apert, bool primary)
+{
+
+	mutex_lock(&sysfb_lock);
+	if (!IS_ERR(sysfb_dev)) {
+		if (sysfb_dev) {
+			if (sysfb_match(apert, primary)) {
+				platform_device_del(sysfb_dev);
+				platform_device_put(sysfb_dev);
+				sysfb_dev = ERR_PTR(-EALREADY);
+			}
+		} else {
+			sysfb_dev = ERR_PTR(-EALREADY);
+		}
+	}
+	mutex_unlock(&sysfb_lock);
+}
+
+__init struct platform_device *sysfb_alloc(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;
+
+	pd = platform_device_alloc(name, id);
+	if (!pd)
+		return ERR_PTR(-ENOMEM);
+
+	ret = platform_device_add_resources(pd, res, res_num);
+	if (ret)
+		goto err;
+
+	ret = platform_device_add_data(pd, data, data_size);
+	if (ret)
+		goto err;
+
+	return pd;
+
+err:
+	platform_device_put(pd);
+	return ERR_PTR(ret);
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
@@ -65,9 +160,11 @@ static __init int sysfb_init(void)
 	else
 		name = "platform-framebuffer";
 
-	pd = platform_device_register_resndata(NULL, name, 0,
-					       NULL, 0, si, sizeof(*si));
-	return IS_ERR(pd) ? PTR_ERR(pd) : 0;
+	pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
+	if (IS_ERR(pd))
+		return PTR_ERR(pd);
+
+	return sysfb_register(pd);
 }
 
 /* must execute after PCI subsystem for EFI quirks */
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..8e7bd23 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -86,10 +86,9 @@ __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));
+	pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
 	if (IS_ERR(pd))
 		return PTR_ERR(pd);
 
-	return 0;
+	return sysfb_register(pd);
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..53e3894 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
 
 #include <asm/fb.h>
 
+#if IS_ENABLED(CONFIG_X86_SYSFB)
+#include <asm/sysfb.h>
+#endif
 
     /*
      *  Frame buffer device initialization and setup routines
@@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	}
 }
 
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+				     bool primary)
+{
+#if IS_ENABLED(CONFIG_X86_SYSFB)
+	sysfb_unregister(apert, primary);
+#endif
+}
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i;
@@ -1742,6 +1753,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 +1775,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);
-- 
1.8.5.1

Re: [RFC] x86: sysfb: remove sysfb when probing real hw

[Ingo Molnar]

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

> If we probe a real hw driver for graphics devices, we need to unload any
> generic fallback driver like efifb/vesafb/simplefb on the system
> framebuffer. This is currently done via remove_conflicting_framebuffers()
> in fbmem.c. However, this only removes the fbdev driver, not the fake
> platform devices underneath. This hasn't been a problem so far, as efifb
> and vesafb didn't store any resources there. However, with simplefb this
> changed.
> 
> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
> platform devices, we need to unregister the underlying platform device
> *before* probing any new hw driver. This patch adds sysfb_unregister() for
> that purpose. It can be called from any context (except from the
> platform-device ->remove callback path) and synchronously unloads any
> global sysfb and prevents new sysfbs from getting registered. Thus, you
> can call it even before any sysfb has been loaded.
> 
> This also changes remove_conflicting_framebuffer() to call this helper
> *before* trying it's fbdev heuristic to remove conflicting drivers.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
> 
> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
> this in for 3.14. Your call..
> 
> This patch basically simulates an unplug event for system-framebuffers when
> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
> optionally pass an aperture-struct and primary-flag similar to
> remove_conflicting_framebuffers(). If they're not passed, we remove it
> unconditionally.
> 
> Untested, but my kernel compiles are already running. If my tests succeed and
> nobody has objections, I can resend it as proper PATCH and marked for stable.
> And maybe split the fbmem and sysfb changes into two patches..

Please fix the changelog to conform to the standard changelog style:

 - first describe the symptoms of the bug - how does a user notice? 

 - then describe how the code behaves today and how that is causing the bug

 - and then only describe how it's fixed.

The first item is the most important one - while developers 
(naturally) tend to concentrate on the least important point, the last 
one.

Thanks,

	Ingo

Re: [RFC] x86: sysfb: remove sysfb when probing real hw

[Takashi Iwai]

At Wed, 18 Dec 2013 12:48:03 +0100,
David Herrmann wrote:
> 
> If we probe a real hw driver for graphics devices, we need to unload any
> generic fallback driver like efifb/vesafb/simplefb on the system
> framebuffer. This is currently done via remove_conflicting_framebuffers()
> in fbmem.c. However, this only removes the fbdev driver, not the fake
> platform devices underneath. This hasn't been a problem so far, as efifb
> and vesafb didn't store any resources there. However, with simplefb this
> changed.
> 
> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
> platform devices, we need to unregister the underlying platform device
> *before* probing any new hw driver. This patch adds sysfb_unregister() for
> that purpose. It can be called from any context (except from the
> platform-device ->remove callback path) and synchronously unloads any
> global sysfb and prevents new sysfbs from getting registered. Thus, you
> can call it even before any sysfb has been loaded.
> 
> This also changes remove_conflicting_framebuffer() to call this helper
> *before* trying it's fbdev heuristic to remove conflicting drivers.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
> 
> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
> this in for 3.14. Your call..
> 
> This patch basically simulates an unplug event for system-framebuffers when
> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
> optionally pass an aperture-struct and primary-flag similar to
> remove_conflicting_framebuffers(). If they're not passed, we remove it
> unconditionally.
> 
> Untested, but my kernel compiles are already running. If my tests succeed and
> nobody has objections, I can resend it as proper PATCH and marked for stable.
> And maybe split the fbmem and sysfb changes into two patches..
> 
> Thanks
> David
> 
>  arch/x86/include/asm/sysfb.h     |  10 ++++
>  arch/x86/kernel/sysfb.c          | 103 +++++++++++++++++++++++++++++++++++++--
>  arch/x86/kernel/sysfb_simplefb.c |   5 +-
>  drivers/video/fbmem.c            |  16 ++++++
>  4 files changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..713bc17 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,15 @@ struct efifb_dmi_info {
>  	int flags;
>  };
>  
> +__init struct platform_device *sysfb_alloc(const char *name,
> +					   int id,
> +					   const struct resource *res,
> +					   unsigned int res_num,
> +					   const void *data,
> +					   size_t data_size);
> +__init int sysfb_register(struct platform_device *dev);
> +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..3d4554e 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,106 @@
>  #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;
> +
> +/* register @dev as sysfb; takes ownership over @dev */
> +__init int sysfb_register(struct platform_device *dev)
> +{
> +	int r = 0;
> +
> +	mutex_lock(&sysfb_lock);
> +	if (!sysfb_dev) {
> +		r = platform_device_add(dev);
> +		if (r < 0)
> +			put_device(&dev->dev);
> +		else
> +			sysfb_dev = dev;
> +	} else {
> +		/* if there already is/was a sysfb, destroy @pd but return 0 */
> +		platform_device_put(dev);
> +	}
> +	mutex_unlock(&sysfb_lock);
> +
> +	return r;
> +}


Since sysfb_alloc() always follows sysfb_register() and they are
always coupled, we can simply combine both to one?

Also, do we really need a mutex?  The functions in fbmem.c are already
in registeration_lock, so if this is called only from there, it should
be fine without an extra lock here.  So, the function can be
simplified like:

int 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 *pdev;
	if (sysfb_dev)
		return ret;
	pdev = platform_device_register_resndata(....);
	if (IS_ERR(pdev))
		return PTR_ERR(pdev);
	sysfb_dev = pdev;
	return 0;
}


> +
> +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 */
> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> +{
> +
> +	mutex_lock(&sysfb_lock);
> +	if (!IS_ERR(sysfb_dev)) {
> +		if (sysfb_dev) {
> +			if (sysfb_match(apert, primary)) {
> +				platform_device_del(sysfb_dev);
> +				platform_device_put(sysfb_dev);
> +				sysfb_dev = ERR_PTR(-EALREADY);
> +			}
> +		} else {
> +			sysfb_dev = ERR_PTR(-EALREADY);
> +		}
> +	}
> +	mutex_unlock(&sysfb_lock);
> +}

Simpler would be like:

void sysfb_unregister(const struct apertures_struct *apert, bool primary)
{
	if (sysfb_dev && sysfb_match(apert, primary)) {
		platform_device_unregister(sysfb_dev);
		sysfb_dev = NULL;		
	}
}

> +
> +__init struct platform_device *sysfb_alloc(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;
> +
> +	pd = platform_device_alloc(name, id);
> +	if (!pd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = platform_device_add_resources(pd, res, res_num);
> +	if (ret)
> +		goto err;
> +
> +	ret = platform_device_add_data(pd, data, data_size);
> +	if (ret)
> +		goto err;
> +
> +	return pd;

I don't think we need to open-code this if we can use
platform_device_register_*() helper.


> +
> +err:
> +	platform_device_put(pd);
> +	return ERR_PTR(ret);
> +}
> +
>  static __init int sysfb_init(void)
>  {
>  	struct screen_info *si = &screen_info;
> @@ -65,9 +160,11 @@ static __init int sysfb_init(void)
>  	else
>  		name = "platform-framebuffer";
>  
> -	pd = platform_device_register_resndata(NULL, name, 0,
> -					       NULL, 0, si, sizeof(*si));
> -	return IS_ERR(pd) ? PTR_ERR(pd) : 0;
> +	pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
> +	if (IS_ERR(pd))
> +		return PTR_ERR(pd);
> +
> +	return sysfb_register(pd);
>  }
>  
>  /* must execute after PCI subsystem for EFI quirks */
> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> index 86179d4..8e7bd23 100644
> --- a/arch/x86/kernel/sysfb_simplefb.c
> +++ b/arch/x86/kernel/sysfb_simplefb.c
> @@ -86,10 +86,9 @@ __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));
> +	pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
>  	if (IS_ERR(pd))
>  		return PTR_ERR(pd);
>  
> -	return 0;
> +	return sysfb_register(pd);
>  }
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 010d191..53e3894 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -35,6 +35,9 @@
>  
>  #include <asm/fb.h>
>  
> +#if IS_ENABLED(CONFIG_X86_SYSFB)
> +#include <asm/sysfb.h>
> +#endif
>  
>      /*
>       *  Frame buffer device initialization and setup routines
> @@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>  	}
>  }
>  
> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> +				     bool primary)
> +{
> +#if IS_ENABLED(CONFIG_X86_SYSFB)
> +	sysfb_unregister(apert, primary);
> +#endif

I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
So this can be called even for non-sysfb case (which is also good to
release the unused platform_device).

That is, this (and the above) can be #ifdef CONFIG_X86 instead.


thanks,

Takashi

Re: [RFC] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 18 Dec 2013 12:48:03 +0100,
> David Herrmann wrote:
>>
>> If we probe a real hw driver for graphics devices, we need to unload any
>> generic fallback driver like efifb/vesafb/simplefb on the system
>> framebuffer. This is currently done via remove_conflicting_framebuffers()
>> in fbmem.c. However, this only removes the fbdev driver, not the fake
>> platform devices underneath. This hasn't been a problem so far, as efifb
>> and vesafb didn't store any resources there. However, with simplefb this
>> changed.
>>
>> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
>> platform devices, we need to unregister the underlying platform device
>> *before* probing any new hw driver. This patch adds sysfb_unregister() for
>> that purpose. It can be called from any context (except from the
>> platform-device ->remove callback path) and synchronously unloads any
>> global sysfb and prevents new sysfbs from getting registered. Thus, you
>> can call it even before any sysfb has been loaded.
>>
>> This also changes remove_conflicting_framebuffer() to call this helper
>> *before* trying it's fbdev heuristic to remove conflicting drivers.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> Hi
>>
>> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
>> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
>> this in for 3.14. Your call..
>>
>> This patch basically simulates an unplug event for system-framebuffers when
>> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
>> optionally pass an aperture-struct and primary-flag similar to
>> remove_conflicting_framebuffers(). If they're not passed, we remove it
>> unconditionally.
>>
>> Untested, but my kernel compiles are already running. If my tests succeed and
>> nobody has objections, I can resend it as proper PATCH and marked for stable.
>> And maybe split the fbmem and sysfb changes into two patches..
>>
>> Thanks
>> David
>>
>>  arch/x86/include/asm/sysfb.h     |  10 ++++
>>  arch/x86/kernel/sysfb.c          | 103 +++++++++++++++++++++++++++++++++++++--
>>  arch/x86/kernel/sysfb_simplefb.c |   5 +-
>>  drivers/video/fbmem.c            |  16 ++++++
>>  4 files changed, 128 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
>> index 2aeb3e2..713bc17 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,15 @@ struct efifb_dmi_info {
>>       int flags;
>>  };
>>
>> +__init struct platform_device *sysfb_alloc(const char *name,
>> +                                        int id,
>> +                                        const struct resource *res,
>> +                                        unsigned int res_num,
>> +                                        const void *data,
>> +                                        size_t data_size);
>> +__init int sysfb_register(struct platform_device *dev);
>> +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..3d4554e 100644
>> --- a/arch/x86/kernel/sysfb.c
>> +++ b/arch/x86/kernel/sysfb.c
>> @@ -33,11 +33,106 @@
>>  #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;
>> +
>> +/* register @dev as sysfb; takes ownership over @dev */
>> +__init int sysfb_register(struct platform_device *dev)
>> +{
>> +     int r = 0;
>> +
>> +     mutex_lock(&sysfb_lock);
>> +     if (!sysfb_dev) {
>> +             r = platform_device_add(dev);
>> +             if (r < 0)
>> +                     put_device(&dev->dev);
>> +             else
>> +                     sysfb_dev = dev;
>> +     } else {
>> +             /* if there already is/was a sysfb, destroy @pd but return 0 */
>> +             platform_device_put(dev);
>> +     }
>> +     mutex_unlock(&sysfb_lock);
>> +
>> +     return r;
>> +}
>
>
> Since sysfb_alloc() always follows sysfb_register() and they are
> always coupled, we can simply combine both to one?

We actually cannot call sysfb_register() for efi/vesa-framebuffer as
they lack a ->remove() callback. I fixed that in v2. I will send a
patch which adds ->remove() callbacks for vesafb and efifb later, but
this shouldn't go into this fix.
Furthermore, I think splitting them makes them easier to read.

> Also, do we really need a mutex?  The functions in fbmem.c are already
> in registeration_lock, so if this is called only from there, it should
> be fine without an extra lock here.  So, the function can be
> simplified like:

They have to be called outside of the fb-mutex. We trigger the
->remove() callback, which will call unregister_framebuffer() which
will lock the fb-mutex => deadlock. And as
remove_conflicting_framebuffers() can be called in parallel by many
drivers, we need the lock in sysfb. We could use an
atomic_test_and_dec(), but that might cause one removal to overtake
the previous unregistration. Thus I added the mutex.

Also note that with CONFIG_FB=n and the pending SimpleDRM driver, we
will call sysfb_unregister() from outside of fbmem.c. If we depend on
fbmem.c internal locks, we need to change it for 3.14/15 again.

> int 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 *pdev;
>         if (sysfb_dev)
>                 return ret;
>         pdev = platform_device_register_resndata(....);
>         if (IS_ERR(pdev))
>                 return PTR_ERR(pdev);
>         sysfb_dev = pdev;
>         return 0;
> }
>
>
>> +
>> +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 */
>> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
>> +{
>> +
>> +     mutex_lock(&sysfb_lock);
>> +     if (!IS_ERR(sysfb_dev)) {
>> +             if (sysfb_dev) {
>> +                     if (sysfb_match(apert, primary)) {
>> +                             platform_device_del(sysfb_dev);
>> +                             platform_device_put(sysfb_dev);
>> +                             sysfb_dev = ERR_PTR(-EALREADY);
>> +                     }
>> +             } else {
>> +                     sysfb_dev = ERR_PTR(-EALREADY);
>> +             }
>> +     }
>> +     mutex_unlock(&sysfb_lock);
>> +}
>
> Simpler would be like:
>
> void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> {
>         if (sysfb_dev && sysfb_match(apert, primary)) {
>                 platform_device_unregister(sysfb_dev);
>                 sysfb_dev = NULL;
>         }
> }

Nope, if sysfb_dev is NULL, I need to set it to ERR_PTR(-sth).
Otherwise, imagine i915 getting loaded before sysfb_init(). It calls
sysfb_unregister() without a registered sysfb and continues. A later
sysfb_init() will then load sysfb anyway. With my code, this is
prevented by setting sysfb_dev to ERR_PTR(-EALREADY).

Maybe the device-init ordering prevents this, but I'm not entirely
sure about this so lets be safe and have a strict ordering.

>
>> +
>> +__init struct platform_device *sysfb_alloc(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;
>> +
>> +     pd = platform_device_alloc(name, id);
>> +     if (!pd)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     ret = platform_device_add_resources(pd, res, res_num);
>> +     if (ret)
>> +             goto err;
>> +
>> +     ret = platform_device_add_data(pd, data, data_size);
>> +     if (ret)
>> +             goto err;
>> +
>> +     return pd;
>
> I don't think we need to open-code this if we can use
> platform_device_register_*() helper.
>
>
>> +
>> +err:
>> +     platform_device_put(pd);
>> +     return ERR_PTR(ret);
>> +}
>> +
>>  static __init int sysfb_init(void)
>>  {
>>       struct screen_info *si = &screen_info;
>> @@ -65,9 +160,11 @@ static __init int sysfb_init(void)
>>       else
>>               name = "platform-framebuffer";
>>
>> -     pd = platform_device_register_resndata(NULL, name, 0,
>> -                                            NULL, 0, si, sizeof(*si));
>> -     return IS_ERR(pd) ? PTR_ERR(pd) : 0;
>> +     pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
>> +     if (IS_ERR(pd))
>> +             return PTR_ERR(pd);
>> +
>> +     return sysfb_register(pd);
>>  }
>>
>>  /* must execute after PCI subsystem for EFI quirks */
>> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
>> index 86179d4..8e7bd23 100644
>> --- a/arch/x86/kernel/sysfb_simplefb.c
>> +++ b/arch/x86/kernel/sysfb_simplefb.c
>> @@ -86,10 +86,9 @@ __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));
>> +     pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
>>       if (IS_ERR(pd))
>>               return PTR_ERR(pd);
>>
>> -     return 0;
>> +     return sysfb_register(pd);
>>  }
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index 010d191..53e3894 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -35,6 +35,9 @@
>>
>>  #include <asm/fb.h>
>>
>> +#if IS_ENABLED(CONFIG_X86_SYSFB)
>> +#include <asm/sysfb.h>
>> +#endif
>>
>>      /*
>>       *  Frame buffer device initialization and setup routines
>> @@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>>       }
>>  }
>>
>> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
>> +                                  bool primary)
>> +{
>> +#if IS_ENABLED(CONFIG_X86_SYSFB)
>> +     sysfb_unregister(apert, primary);
>> +#endif
>
> I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
> So this can be called even for non-sysfb case (which is also good to
> release the unused platform_device).
>
> That is, this (and the above) can be #ifdef CONFIG_X86 instead.

Yepp, fixed.

I will send v2 in a minute. I tested it on x86 with efifb+no-sysfb and
simplefb+sysfb, both handovers worked fine.

Thanks
David

[PATCH v2] 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)

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
 - remove simplefb apertures to avoid dead-lock
 - skip sysfb_unregister() if no apertures are set
 - merge sysfb_alloc() into sysfb_register()
 - simplify sysfb_unregister() logic
 - call sysfb_unregister() even if SYSFB=n but X86=y

Tested with efifb and simplefb + i915 handover on x86.

Thanks
David

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

diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..10d719d 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;
 };
 
+__init int 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..9d8da8d 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,74 @@
 #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;
+
+__init int 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 */
+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 {
+		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: [RFC] x86: sysfb: remove sysfb when probing real hw

[Takashi Iwai]

At Wed, 18 Dec 2013 14:34:53 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Wed, 18 Dec 2013 12:48:03 +0100,
> > David Herrmann wrote:
> >>
> >> If we probe a real hw driver for graphics devices, we need to unload any
> >> generic fallback driver like efifb/vesafb/simplefb on the system
> >> framebuffer. This is currently done via remove_conflicting_framebuffers()
> >> in fbmem.c. However, this only removes the fbdev driver, not the fake
> >> platform devices underneath. This hasn't been a problem so far, as efifb
> >> and vesafb didn't store any resources there. However, with simplefb this
> >> changed.
> >>
> >> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
> >> platform devices, we need to unregister the underlying platform device
> >> *before* probing any new hw driver. This patch adds sysfb_unregister() for
> >> that purpose. It can be called from any context (except from the
> >> platform-device ->remove callback path) and synchronously unloads any
> >> global sysfb and prevents new sysfbs from getting registered. Thus, you
> >> can call it even before any sysfb has been loaded.
> >>
> >> This also changes remove_conflicting_framebuffer() to call this helper
> >> *before* trying it's fbdev heuristic to remove conflicting drivers.
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >> Hi
> >>
> >> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
> >> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
> >> this in for 3.14. Your call..
> >>
> >> This patch basically simulates an unplug event for system-framebuffers when
> >> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
> >> optionally pass an aperture-struct and primary-flag similar to
> >> remove_conflicting_framebuffers(). If they're not passed, we remove it
> >> unconditionally.
> >>
> >> Untested, but my kernel compiles are already running. If my tests succeed and
> >> nobody has objections, I can resend it as proper PATCH and marked for stable.
> >> And maybe split the fbmem and sysfb changes into two patches..
> >>
> >> Thanks
> >> David
> >>
> >>  arch/x86/include/asm/sysfb.h     |  10 ++++
> >>  arch/x86/kernel/sysfb.c          | 103 +++++++++++++++++++++++++++++++++++++--
> >>  arch/x86/kernel/sysfb_simplefb.c |   5 +-
> >>  drivers/video/fbmem.c            |  16 ++++++
> >>  4 files changed, 128 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> >> index 2aeb3e2..713bc17 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,15 @@ struct efifb_dmi_info {
> >>       int flags;
> >>  };
> >>
> >> +__init struct platform_device *sysfb_alloc(const char *name,
> >> +                                        int id,
> >> +                                        const struct resource *res,
> >> +                                        unsigned int res_num,
> >> +                                        const void *data,
> >> +                                        size_t data_size);
> >> +__init int sysfb_register(struct platform_device *dev);
> >> +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..3d4554e 100644
> >> --- a/arch/x86/kernel/sysfb.c
> >> +++ b/arch/x86/kernel/sysfb.c
> >> @@ -33,11 +33,106 @@
> >>  #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;
> >> +
> >> +/* register @dev as sysfb; takes ownership over @dev */
> >> +__init int sysfb_register(struct platform_device *dev)
> >> +{
> >> +     int r = 0;
> >> +
> >> +     mutex_lock(&sysfb_lock);
> >> +     if (!sysfb_dev) {
> >> +             r = platform_device_add(dev);
> >> +             if (r < 0)
> >> +                     put_device(&dev->dev);
> >> +             else
> >> +                     sysfb_dev = dev;
> >> +     } else {
> >> +             /* if there already is/was a sysfb, destroy @pd but return 0 */
> >> +             platform_device_put(dev);
> >> +     }
> >> +     mutex_unlock(&sysfb_lock);
> >> +
> >> +     return r;
> >> +}
> >
> >
> > Since sysfb_alloc() always follows sysfb_register() and they are
> > always coupled, we can simply combine both to one?
> 
> We actually cannot call sysfb_register() for efi/vesa-framebuffer as
> they lack a ->remove() callback. I fixed that in v2. I will send a
> patch which adds ->remove() callbacks for vesafb and efifb later, but
> this shouldn't go into this fix.
> Furthermore, I think splitting them makes them easier to read.
>
> > Also, do we really need a mutex?  The functions in fbmem.c are already
> > in registeration_lock, so if this is called only from there, it should
> > be fine without an extra lock here.  So, the function can be
> > simplified like:
> 
> They have to be called outside of the fb-mutex. We trigger the
> ->remove() callback, which will call unregister_framebuffer() which
> will lock the fb-mutex => deadlock. And as
> remove_conflicting_framebuffers() can be called in parallel by many
> drivers, we need the lock in sysfb. We could use an
> atomic_test_and_dec(), but that might cause one removal to overtake
> the previous unregistration. Thus I added the mutex.
> 
> Also note that with CONFIG_FB=n and the pending SimpleDRM driver, we
> will call sysfb_unregister() from outside of fbmem.c. If we depend on
> fbmem.c internal locks, we need to change it for 3.14/15 again.

OK, but please add a comment on it (that it can be called outside fb
lock).


> > int 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 *pdev;
> >         if (sysfb_dev)
> >                 return ret;
> >         pdev = platform_device_register_resndata(....);
> >         if (IS_ERR(pdev))
> >                 return PTR_ERR(pdev);
> >         sysfb_dev = pdev;
> >         return 0;
> > }
> >
> >
> >> +
> >> +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 */
> >> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> >> +{
> >> +
> >> +     mutex_lock(&sysfb_lock);
> >> +     if (!IS_ERR(sysfb_dev)) {
> >> +             if (sysfb_dev) {
> >> +                     if (sysfb_match(apert, primary)) {
> >> +                             platform_device_del(sysfb_dev);
> >> +                             platform_device_put(sysfb_dev);
> >> +                             sysfb_dev = ERR_PTR(-EALREADY);
> >> +                     }
> >> +             } else {
> >> +                     sysfb_dev = ERR_PTR(-EALREADY);
> >> +             }
> >> +     }
> >> +     mutex_unlock(&sysfb_lock);
> >> +}
> >
> > Simpler would be like:
> >
> > void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> > {
> >         if (sysfb_dev && sysfb_match(apert, primary)) {
> >                 platform_device_unregister(sysfb_dev);
> >                 sysfb_dev = NULL;
> >         }
> > }
> 
> Nope, if sysfb_dev is NULL, I need to set it to ERR_PTR(-sth).
> Otherwise, imagine i915 getting loaded before sysfb_init(). It calls
> sysfb_unregister() without a registered sysfb and continues. A later
> sysfb_init() will then load sysfb anyway. With my code, this is
> prevented by setting sysfb_dev to ERR_PTR(-EALREADY).

Fair enough.  But this would be better to be commented there, too, for
avoiding further stray sheep :)


> Maybe the device-init ordering prevents this, but I'm not entirely
> sure about this so lets be safe and have a strict ordering.
> >> +
> >> +__init struct platform_device *sysfb_alloc(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;
> >> +
> >> +     pd = platform_device_alloc(name, id);
> >> +     if (!pd)
> >> +             return ERR_PTR(-ENOMEM);
> >> +
> >> +     ret = platform_device_add_resources(pd, res, res_num);
> >> +     if (ret)
> >> +             goto err;
> >> +
> >> +     ret = platform_device_add_data(pd, data, data_size);
> >> +     if (ret)
> >> +             goto err;
> >> +
> >> +     return pd;
> >
> > I don't think we need to open-code this if we can use
> > platform_device_register_*() helper.
> >
> >
> >> +
> >> +err:
> >> +     platform_device_put(pd);
> >> +     return ERR_PTR(ret);
> >> +}
> >> +
> >>  static __init int sysfb_init(void)
> >>  {
> >>       struct screen_info *si = &screen_info;
> >> @@ -65,9 +160,11 @@ static __init int sysfb_init(void)
> >>       else
> >>               name = "platform-framebuffer";
> >>
> >> -     pd = platform_device_register_resndata(NULL, name, 0,
> >> -                                            NULL, 0, si, sizeof(*si));
> >> -     return IS_ERR(pd) ? PTR_ERR(pd) : 0;
> >> +     pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
> >> +     if (IS_ERR(pd))
> >> +             return PTR_ERR(pd);
> >> +
> >> +     return sysfb_register(pd);
> >>  }
> >>
> >>  /* must execute after PCI subsystem for EFI quirks */
> >> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> >> index 86179d4..8e7bd23 100644
> >> --- a/arch/x86/kernel/sysfb_simplefb.c
> >> +++ b/arch/x86/kernel/sysfb_simplefb.c
> >> @@ -86,10 +86,9 @@ __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));
> >> +     pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
> >>       if (IS_ERR(pd))
> >>               return PTR_ERR(pd);
> >>
> >> -     return 0;
> >> +     return sysfb_register(pd);
> >>  }
> >> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> >> index 010d191..53e3894 100644
> >> --- a/drivers/video/fbmem.c
> >> +++ b/drivers/video/fbmem.c
> >> @@ -35,6 +35,9 @@
> >>
> >>  #include <asm/fb.h>
> >>
> >> +#if IS_ENABLED(CONFIG_X86_SYSFB)
> >> +#include <asm/sysfb.h>
> >> +#endif
> >>
> >>      /*
> >>       *  Frame buffer device initialization and setup routines
> >> @@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
> >>       }
> >>  }
> >>
> >> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> >> +                                  bool primary)
> >> +{
> >> +#if IS_ENABLED(CONFIG_X86_SYSFB)
> >> +     sysfb_unregister(apert, primary);
> >> +#endif
> >
> > I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
> > So this can be called even for non-sysfb case (which is also good to
> > release the unused platform_device).
> >
> > That is, this (and the above) can be #ifdef CONFIG_X86 instead.
> 
> Yepp, fixed.
> 
> I will send v2 in a minute. I tested it on x86 with efifb+no-sysfb and
> simplefb+sysfb, both handovers worked fine.

OK, I'll test it soon.


thanks!

Takashi

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

[David Herrmann]

Hi Pavel

On Wed, Dec 18, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com> 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, ..).
>
> 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)

As you reported issues regarding nvidiafb, can you give this a try, too?

Thanks
David

> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
>  - remove simplefb apertures to avoid dead-lock
>  - skip sysfb_unregister() if no apertures are set
>  - merge sysfb_alloc() into sysfb_register()
>  - simplify sysfb_unregister() logic
>  - call sysfb_unregister() even if SYSFB=n but X86=y
>
> Tested with efifb and simplefb + i915 handover on x86.
>
> Thanks
> David
>
>  arch/x86/include/asm/sysfb.h     |  6 ++++
>  arch/x86/kernel/sysfb.c          | 63 ++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/sysfb_simplefb.c |  9 ++----
>  drivers/video/fbmem.c            | 19 ++++++++++++
>  drivers/video/simplefb.c         |  8 -----
>  5 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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;
>  };
>
> +__init int 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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
>  #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;
> +
> +__init int 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 */
> +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 {
> +               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 v2] x86: sysfb: remove sysfb when probing real hw

[Takashi Iwai]

At Wed, 18 Dec 2013 14:50:11 +0100,
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, ..).
> 
> 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)
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
>  - remove simplefb apertures to avoid dead-lock
>  - skip sysfb_unregister() if no apertures are set
>  - merge sysfb_alloc() into sysfb_register()
>  - simplify sysfb_unregister() logic
>  - call sysfb_unregister() even if SYSFB=n but X86=y
> 
> Tested with efifb and simplefb + i915 handover on x86.

The patch worked fine on QEMU with cirrus driver.

I'd like to see comments, as suggested in my previous mail, in a
couple of places.  Other than that, the patch looks good to me.

So, feel free to my tested-by and reviewed-by tags:

	Reviewed-by: Takashi Iwai <tiwai@suse.de>
	Tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> 
> Thanks
> David
> 
>  arch/x86/include/asm/sysfb.h     |  6 ++++
>  arch/x86/kernel/sysfb.c          | 63 ++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/sysfb_simplefb.c |  9 ++----
>  drivers/video/fbmem.c            | 19 ++++++++++++
>  drivers/video/simplefb.c         |  8 -----
>  5 files changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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;
>  };
>  
> +__init int sysfb_register(const char *name, int id,
> +			  const struct resource *res, unsigned int res_num,
> +			  const void *data, size_t data_size);

In most cases, we declare like
	int __init sysfb_register(...

> +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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
>  #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;
> +
> +__init int 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 */
> +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 {
> +		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: cirrusdrmfb broken with simplefb

[One Thousand Gnomes]

> > That bug always existed, simplefb is just the first driver to hit it 
> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a 
> > workaround you can simply disable CONFIG_X86_SYSFB. That restores 
> > the old behavior.
> 
> This looks like a regression, so we'll either need a fix or we'll have 
> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.

Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
mga.

[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: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
>> > That bug always existed, simplefb is just the first driver to hit it
>> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> > the old behavior.
>>
>> This looks like a regression, so we'll either need a fix or we'll have
>> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>
> Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> mga.

Thanks for the hints. I've read through all I could find and tried to
provide some help.

I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
'n' by default) but don't read the help text. I did my best to tell
people that this option requires CONFIG_FB_SIMPLE, but if you don't
read the help-text you won't notice that. Don't know what to do about
that..

Thanks
David

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Thu, 19 Dec 2013 11:46:51 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> > That bug always existed, simplefb is just the first driver to hit it
> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> > the old behavior.
> >>
> >> This looks like a regression, so we'll either need a fix or we'll have
> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >
> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> > mga.
> 
> Thanks for the hints. I've read through all I could find and tried to
> provide some help.
> 
> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> 'n' by default) but don't read the help text. I did my best to tell
> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> read the help-text you won't notice that. Don't know what to do about
> that..

You can set FB_SIMPLE default value depending on X86_SYSFB, something
like:

--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2455,6 +2455,7 @@ config FB_HYPERV
 config FB_SIMPLE
 	bool "Simple framebuffer support"
 	depends on (FB = y)
+	default y if X86_SYSFB
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT


Takashi

Re: cirrusdrmfb broken with simplefb

[Ingo Molnar]

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

> Hi
> 
> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> > That bug always existed, simplefb is just the first driver to hit it
> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> > the old behavior.
> >>
> >> This looks like a regression, so we'll either need a fix or we'll have
> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >
> > Kernel bugzilla has entries for simplefb breaking both vesafb and 
> > matrox mga.
> 
> Thanks for the hints. I've read through all I could find and tried 
> to provide some help.
> 
> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is 
> 'n' by default) but don't read the help text. I did my best to tell 
> people that this option requires CONFIG_FB_SIMPLE, but if you don't 
> read the help-text you won't notice that. Don't know what to do 
> about that..

People generally don't read the help text - still the kernel should 
not break. So please the Kconfig angle (and the bootup logic, etc.) 
fool-proof, graphics failures are not fun to debug!

Thanks,

	Ingo

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 19 Dec 2013 11:46:51 +0100,
> David Herrmann wrote:
>>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
>> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> > That bug always existed, simplefb is just the first driver to hit it
>> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> >> > the old behavior.
>> >>
>> >> This looks like a regression, so we'll either need a fix or we'll have
>> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>> >
>> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
>> > mga.
>>
>> Thanks for the hints. I've read through all I could find and tried to
>> provide some help.
>>
>> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
>> 'n' by default) but don't read the help text. I did my best to tell
>> people that this option requires CONFIG_FB_SIMPLE, but if you don't
>> read the help-text you won't notice that. Don't know what to do about
>> that..
>
> You can set FB_SIMPLE default value depending on X86_SYSFB, something
> like:
>
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2455,6 +2455,7 @@ config FB_HYPERV
>  config FB_SIMPLE
>         bool "Simple framebuffer support"
>         depends on (FB = y)
> +       default y if X86_SYSFB
>         select FB_CFB_FILLRECT
>         select FB_CFB_COPYAREA
>         select FB_CFB_IMAGEBLIT

The "default <wx> if <yz>" only works if the config hasn't been set at
all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
be ignored.

How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
will try that and send a patch.

Thanks
David

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 1:31 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
>> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> > That bug always existed, simplefb is just the first driver to hit it
>> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> >> > the old behavior.
>> >>
>> >> This looks like a regression, so we'll either need a fix or we'll have
>> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>> >
>> > Kernel bugzilla has entries for simplefb breaking both vesafb and
>> > matrox mga.
>>
>> Thanks for the hints. I've read through all I could find and tried
>> to provide some help.
>>
>> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
>> 'n' by default) but don't read the help text. I did my best to tell
>> people that this option requires CONFIG_FB_SIMPLE, but if you don't
>> read the help-text you won't notice that. Don't know what to do
>> about that..
>
> People generally don't read the help text - still the kernel should
> not break. So please the Kconfig angle (and the bootup logic, etc.)
> fool-proof, graphics failures are not fun to debug!

There're dozens of combinations that break gfx-boot. Even non-obvious
things like disabling VT_HW_CONSOLE_BINDING will break gfx-handover.
Anyhow, bad examples are no excuse.. I will send patches to fool-proof
sysfb.

David

Re: cirrusdrmfb broken with simplefb

[Ingo Molnar]

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

> Hi
> 
> On Thu, Dec 19, 2013 at 1:31 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> > the old behavior.
> >> >>
> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >
> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and
> >> > matrox mga.
> >>
> >> Thanks for the hints. I've read through all I could find and tried
> >> to provide some help.
> >>
> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> 'n' by default) but don't read the help text. I did my best to tell
> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> read the help-text you won't notice that. Don't know what to do
> >> about that..
> >
> > People generally don't read the help text - still the kernel should
> > not break. So please the Kconfig angle (and the bootup logic, etc.)
> > fool-proof, graphics failures are not fun to debug!
> 
> There're dozens of combinations that break gfx-boot. [...]

Then that's a bug too.

> [...] Even non-obvious things like disabling VT_HW_CONSOLE_BINDING 
> will break gfx-handover. Anyhow, bad examples are no excuse.. I will 
> send patches to fool-proof sysfb.

Cool, thanks!

Arguably there's a lot of broken gfx legacy in Linux.

<SoapBox>: 15 years ago we should have merged GGI that really got the 
integrated gfx principles right from the get go IMHO - and we've been 
struggling with fragmented, disjunct gfx concepts since then, but IMHO 
it's getting better gradually, so please don't give up! ;-)

	Ingo

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Thu, 19 Dec 2013 13:36:38 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 19 Dec 2013 11:46:51 +0100,
> > David Herrmann wrote:
> >>
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> > the old behavior.
> >> >>
> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >
> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> >> > mga.
> >>
> >> Thanks for the hints. I've read through all I could find and tried to
> >> provide some help.
> >>
> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> 'n' by default) but don't read the help text. I did my best to tell
> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> read the help-text you won't notice that. Don't know what to do about
> >> that..
> >
> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
> > like:
> >
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
> >  config FB_SIMPLE
> >         bool "Simple framebuffer support"
> >         depends on (FB = y)
> > +       default y if X86_SYSFB
> >         select FB_CFB_FILLRECT
> >         select FB_CFB_COPYAREA
> >         select FB_CFB_IMAGEBLIT
> 
> The "default <wx> if <yz>" only works if the config hasn't been set at
> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> be ignored.

Yes.  I didn't suggest the strict dependency like below, just because
I assumed you didn't add it intentionally.

> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> will try that and send a patch.

That's in general a bad approach.  If the strict dependency is
allowed, a more intuitive way is to give a reverse selection.  But
then you'd need to correct the help text, too, as it's implemented
already as a dependency.

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2299,6 +2299,7 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	select SIMPLE_FB
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for


Takashi

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 2:22 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 19 Dec 2013 13:36:38 +0100,
> David Herrmann wrote:
>>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Thu, 19 Dec 2013 11:46:51 +0100,
>> > David Herrmann wrote:
>> >>
>> >> Hi
>> >>
>> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
>> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> >> > That bug always existed, simplefb is just the first driver to hit it
>> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> >> >> > the old behavior.
>> >> >>
>> >> >> This looks like a regression, so we'll either need a fix or we'll have
>> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>> >> >
>> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
>> >> > mga.
>> >>
>> >> Thanks for the hints. I've read through all I could find and tried to
>> >> provide some help.
>> >>
>> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
>> >> 'n' by default) but don't read the help text. I did my best to tell
>> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
>> >> read the help-text you won't notice that. Don't know what to do about
>> >> that..
>> >
>> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
>> > like:
>> >
>> > --- a/drivers/video/Kconfig
>> > +++ b/drivers/video/Kconfig
>> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
>> >  config FB_SIMPLE
>> >         bool "Simple framebuffer support"
>> >         depends on (FB = y)
>> > +       default y if X86_SYSFB
>> >         select FB_CFB_FILLRECT
>> >         select FB_CFB_COPYAREA
>> >         select FB_CFB_IMAGEBLIT
>>
>> The "default <wx> if <yz>" only works if the config hasn't been set at
>> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
>> be ignored.
>
> Yes.  I didn't suggest the strict dependency like below, just because
> I assumed you didn't add it intentionally.

It doesn't work. If CONFIG_FB=n this breaks.

>> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
>> will try that and send a patch.
>
> That's in general a bad approach.  If the strict dependency is
> allowed, a more intuitive way is to give a reverse selection.  But
> then you'd need to correct the help text, too, as it's implemented
> already as a dependency.

Well, X86_SYSFB just changes the way x86-bootup code handles
firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
CONFIG_DRM=n. However, currently the only driver that can profit from
this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
no-one should ever bother (except for people who *really* want this).

Unfortunately, people still enable it without understanding it.
Changing FB_SIMPLE or other config options doesn't protect against
that, instead I need to make X86_SYSFB fool-proof so it's only
activated if people seriously *want* it.

Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
behavior of "select". It will only enable FB_SIMPLE but not the
dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
(FB_SIMPLE = y)". And I actually like this, because there's no gain in
using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
X86_SYSFB from all non-developers who currently shouldn't care anyway.
Most sysfb code is enabled regardless of X86_SYSFB, the option really
just controls the compatibility mode. And unless SimpleDRM is merged,
there's little gain by setting it for non-developers.

So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
a patch for that. If you're still not convinced, I'd be glad to hear
your concerns. The only other idea I had is this:

config X86_SYSFB
         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
         depends on (FB_SIMPLE = y)
         default y

But I'm not sure I want the "default y" if FB_SIMPLE is active right
now. We can add it later without any problems.

Thanks
David

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2299,6 +2299,7 @@ source "drivers/rapidio/Kconfig"
>
>  config X86_SYSFB
>         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +       select SIMPLE_FB
>         help
>           Firmwares often provide initial graphics framebuffers so the BIOS,
>           bootloader or kernel can show basic video-output during boot for
>
>
> Takashi

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Thu, 19 Dec 2013 14:37:35 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 2:22 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 19 Dec 2013 13:36:38 +0100,
> > David Herrmann wrote:
> >>
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > At Thu, 19 Dec 2013 11:46:51 +0100,
> >> > David Herrmann wrote:
> >> >>
> >> >> Hi
> >> >>
> >> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> >> > the old behavior.
> >> >> >>
> >> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >> >
> >> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> >> >> > mga.
> >> >>
> >> >> Thanks for the hints. I've read through all I could find and tried to
> >> >> provide some help.
> >> >>
> >> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> >> 'n' by default) but don't read the help text. I did my best to tell
> >> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> >> read the help-text you won't notice that. Don't know what to do about
> >> >> that..
> >> >
> >> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
> >> > like:
> >> >
> >> > --- a/drivers/video/Kconfig
> >> > +++ b/drivers/video/Kconfig
> >> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
> >> >  config FB_SIMPLE
> >> >         bool "Simple framebuffer support"
> >> >         depends on (FB = y)
> >> > +       default y if X86_SYSFB
> >> >         select FB_CFB_FILLRECT
> >> >         select FB_CFB_COPYAREA
> >> >         select FB_CFB_IMAGEBLIT
> >>
> >> The "default <wx> if <yz>" only works if the config hasn't been set at
> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> >> be ignored.
> >
> > Yes.  I didn't suggest the strict dependency like below, just because
> > I assumed you didn't add it intentionally.
> 
> It doesn't work. If CONFIG_FB=n this breaks.
> 
> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> >> will try that and send a patch.
> >
> > That's in general a bad approach.  If the strict dependency is
> > allowed, a more intuitive way is to give a reverse selection.  But
> > then you'd need to correct the help text, too, as it's implemented
> > already as a dependency.
> 
> Well, X86_SYSFB just changes the way x86-bootup code handles
> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
> CONFIG_DRM=n. However, currently the only driver that can profit from
> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
> no-one should ever bother (except for people who *really* want this).
>
> Unfortunately, people still enable it without understanding it.
> Changing FB_SIMPLE or other config options doesn't protect against
> that, instead I need to make X86_SYSFB fool-proof so it's only
> activated if people seriously *want* it.

It's not N as default.  It's just unset.  People don't take it
seriously as default N unless you explicitly write it.  And, more
importantly, your help text even suggests to choose Y as default (in
the end of text).  And it doesn't mention how to enable simplefb
although it's recommended.  So you can't expect that people do the
right thing only from that.

> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
> behavior of "select".

There is one single dependency in FB_SIMPLE, so it's relatively easy
in this case.

> It will only enable FB_SIMPLE but not the
> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
> X86_SYSFB from all non-developers who currently shouldn't care anyway.
> Most sysfb code is enabled regardless of X86_SYSFB, the option really
> just controls the compatibility mode. And unless SimpleDRM is merged,
> there's little gain by setting it for non-developers.

Then better to have mentioned in the text or changelog.  Or make it
depends on CONFIG_EXPERT (it doesn't help much, though, but can be
some excuse :)

> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
> a patch for that. If you're still not convinced, I'd be glad to hear
> your concerns.

Why not just select both FB and FB_SIMPLE from X86_SYSFB?

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	select FB
+	select SIMPLE_FB
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for


The "depends on" hides the option until the dependency is satisfied,
so especially when the dependency go over different sections, it
should be avoided.  The reverse selection can be problematic
sometimes, but as long as the dependency chain is short, it works more
easily.


Takashi

Re: cirrusdrmfb broken with simplefb

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 3:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> The "default <wx> if <yz>" only works if the config hasn't been set at
>> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
>> >> be ignored.
>> >
>> > Yes.  I didn't suggest the strict dependency like below, just because
>> > I assumed you didn't add it intentionally.
>>
>> It doesn't work. If CONFIG_FB=n this breaks.
>>
>> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
>> >> will try that and send a patch.
>> >
>> > That's in general a bad approach.  If the strict dependency is
>> > allowed, a more intuitive way is to give a reverse selection.  But
>> > then you'd need to correct the help text, too, as it's implemented
>> > already as a dependency.
>>
>> Well, X86_SYSFB just changes the way x86-bootup code handles
>> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
>> CONFIG_DRM=n. However, currently the only driver that can profit from
>> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
>> no-one should ever bother (except for people who *really* want this).
>>
>> Unfortunately, people still enable it without understanding it.
>> Changing FB_SIMPLE or other config options doesn't protect against
>> that, instead I need to make X86_SYSFB fool-proof so it's only
>> activated if people seriously *want* it.
>
> It's not N as default.  It's just unset.  People don't take it
> seriously as default N unless you explicitly write it.  And, more
> importantly, your help text even suggests to choose Y as default (in
> the end of text).  And it doesn't mention how to enable simplefb
> although it's recommended.  So you can't expect that people do the
> right thing only from that.

Yepp, that was my fault.. Already changed to 'N' in the new patch.

>> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
>> behavior of "select".
>
> There is one single dependency in FB_SIMPLE, so it's relatively easy
> in this case.
>
>> It will only enable FB_SIMPLE but not the
>> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
>> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
>> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
>> X86_SYSFB from all non-developers who currently shouldn't care anyway.
>> Most sysfb code is enabled regardless of X86_SYSFB, the option really
>> just controls the compatibility mode. And unless SimpleDRM is merged,
>> there's little gain by setting it for non-developers.
>
> Then better to have mentioned in the text or changelog.  Or make it
> depends on CONFIG_EXPERT (it doesn't help much, though, but can be
> some excuse :)

I didn't expect it to blow up in my face this way.. probably should
have. But that's why the changelog didn't mention it. Regarding
CONFIG_EXPERT, I doubt that helps much and it's a little bit late for
an excuse ;)

>> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
>> a patch for that. If you're still not convinced, I'd be glad to hear
>> your concerns.
>
> Why not just select both FB and FB_SIMPLE from X86_SYSFB?
>
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
>
>  config X86_SYSFB
>         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +       select FB
> +       select SIMPLE_FB
>         help
>           Firmwares often provide initial graphics framebuffers so the BIOS,
>           bootloader or kernel can show basic video-output during boot for
>
>
> The "depends on" hides the option until the dependency is satisfied,
> so especially when the dependency go over different sections, it
> should be avoided.  The reverse selection can be problematic
> sometimes, but as long as the dependency chain is short, it works more
> easily.

Is the select-chain recursive? So are FB_SIMPLE-select lines recognized?
If yes, this could work. Although given the complaints about bad
simplefb performance compared to vesafb, I'm still not sure. Yeah,
this time people are to blame as they *explicitly* selected X86_SYSFB
instead of plain old vesafb, but it's still annoying to hear the
complaints.

I will think about it some more time and send a v2-patch later today
if nothing else comes to mind.

Thanks for the reviews!
David

Re: cirrusdrmfb broken with simplefb

[Takashi Iwai]

At Thu, 19 Dec 2013 15:13:59 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 3:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> The "default <wx> if <yz>" only works if the config hasn't been set at
> >> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> >> >> be ignored.
> >> >
> >> > Yes.  I didn't suggest the strict dependency like below, just because
> >> > I assumed you didn't add it intentionally.
> >>
> >> It doesn't work. If CONFIG_FB=n this breaks.
> >>
> >> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> >> >> will try that and send a patch.
> >> >
> >> > That's in general a bad approach.  If the strict dependency is
> >> > allowed, a more intuitive way is to give a reverse selection.  But
> >> > then you'd need to correct the help text, too, as it's implemented
> >> > already as a dependency.
> >>
> >> Well, X86_SYSFB just changes the way x86-bootup code handles
> >> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
> >> CONFIG_DRM=n. However, currently the only driver that can profit from
> >> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
> >> no-one should ever bother (except for people who *really* want this).
> >>
> >> Unfortunately, people still enable it without understanding it.
> >> Changing FB_SIMPLE or other config options doesn't protect against
> >> that, instead I need to make X86_SYSFB fool-proof so it's only
> >> activated if people seriously *want* it.
> >
> > It's not N as default.  It's just unset.  People don't take it
> > seriously as default N unless you explicitly write it.  And, more
> > importantly, your help text even suggests to choose Y as default (in
> > the end of text).  And it doesn't mention how to enable simplefb
> > although it's recommended.  So you can't expect that people do the
> > right thing only from that.
> 
> Yepp, that was my fault.. Already changed to 'N' in the new patch.
> 
> >> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
> >> behavior of "select".
> >
> > There is one single dependency in FB_SIMPLE, so it's relatively easy
> > in this case.
> >
> >> It will only enable FB_SIMPLE but not the
> >> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
> >> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
> >> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
> >> X86_SYSFB from all non-developers who currently shouldn't care anyway.
> >> Most sysfb code is enabled regardless of X86_SYSFB, the option really
> >> just controls the compatibility mode. And unless SimpleDRM is merged,
> >> there's little gain by setting it for non-developers.
> >
> > Then better to have mentioned in the text or changelog.  Or make it
> > depends on CONFIG_EXPERT (it doesn't help much, though, but can be
> > some excuse :)
> 
> I didn't expect it to blow up in my face this way.. probably should
> have. But that's why the changelog didn't mention it. Regarding
> CONFIG_EXPERT, I doubt that helps much and it's a little bit late for
> an excuse ;)
> 
> >> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
> >> a patch for that. If you're still not convinced, I'd be glad to hear
> >> your concerns.
> >
> > Why not just select both FB and FB_SIMPLE from X86_SYSFB?
> >
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
> >
> >  config X86_SYSFB
> >         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> > +       select FB
> > +       select SIMPLE_FB
> >         help
> >           Firmwares often provide initial graphics framebuffers so the BIOS,
> >           bootloader or kernel can show basic video-output during boot for
> >
> >
> > The "depends on" hides the option until the dependency is satisfied,
> > so especially when the dependency go over different sections, it
> > should be avoided.  The reverse selection can be problematic
> > sometimes, but as long as the dependency chain is short, it works more
> > easily.
> 
> Is the select-chain recursive?

Yes, the select-chain is recursive.  But select doesn't care about
"depends on", so if a selected item depends on anything else, all
these have to be selected explicitly as well.  In your case, luckily
it's a trivial one-level selection.

> So are FB_SIMPLE-select lines recognized?
> If yes, this could work. Although given the complaints about bad
> simplefb performance compared to vesafb, I'm still not sure. Yeah,
> this time people are to blame as they *explicitly* selected X86_SYSFB
> instead of plain old vesafb, but it's still annoying to hear the
> complaints.

Right.  The performance is a different level of problem, and it's
their choice now.

> I will think about it some more time and send a v2-patch later today
> if nothing else comes to mind.

Great, thanks!


Takashi

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