| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANq1E4TFKDduy71jCXy+8CMS1_OFxWEpsHY3AA0RUJPW4VE8hg@mail.gmail.com>
Date: Wed, 18 Dec 2013 09:21:28 +0100
From: David Herrmann <dh.herrmann@...il.com>
To: Takashi Iwai <tiwai@...e.de>
Cc: Stephen Warren <swarren@...dotorg.org>,
"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
"the arch/x86 maintainers" <x86@...nel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: cirrusdrmfb broken with simplefb
Hi
On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@...e.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists