diff mbox

cirrusdrmfb broken with simplefb

Message ID s5hlhzik3kk.wl%tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Dec. 18, 2013, 7:42 a.m. UTC 
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=855821

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

---
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Herrmann Dec. 18, 2013, 8:21 a.m. UTC | #1 
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=855821
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Takashi Iwai Dec. 18, 2013, 8:44 a.m. UTC | #2 
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=855821
> >
> > 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;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Dec. 18, 2013, 8:48 a.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar Dec. 18, 2013, 9:29 a.m. UTC | #4 
* 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=855821
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox Dec. 19, 2013, 12:03 a.m. UTC | #5 
> > 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.


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann Dec. 19, 2013, 10:46 a.m. UTC | #6 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar Dec. 19, 2013, 12:31 p.m. UTC | #7 
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann Dec. 19, 2013, 12:39 p.m. UTC | #8 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar Dec. 19, 2013, 12:44 p.m. UTC | #9 
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;