| 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
| ||
|
Date: Thu, 29 Mar 2007 00:30:30 -0400
From: Len Brown <lenb@...nel.org>
To: Ingo Molnar <mingo@...e.hu>, luming.yu@...el.com
Cc: Adrian Bunk <bunk@...sta.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Thomas Meyer <thomas@...3r.de>,
Frederic Riss <frederic.riss@...il.com>,
Marcus Better <marcus@...ter.se>
Subject: Re: [patch] MSI-X: fix resume crash
> Tony, Len the way pci_disable_device is being used in a suspend/resume
> path by a few drivers is completely incompatible with the way irqs are
> allocated on ia64. In particular people the following sequence occurs
> in several drivers.
>
> probe:
> pci_enable_device(pdev);
> request_irq(pdev->irq);
> suspend:
> pci_disable_device(pdev);
> resume:
> pci_enable_device(pdev);
> remove:
> free_irq(pdev->irq);
> pci_disable_device(pdev);
There are no IA64 machines that support system suspend/resume today --
so you have 0 chance of breaking the IA64 suspend/resume installed base.
My understanding is that Luming Yu has cobbled IA64 S4 support
together for a future release though.
> What I'm proposing we do is move the irq allocation code out of
> pci_enable_device and the irq freeing code out of pci_disable_device in
> the future. If we move ia64 to a model where the irq number equal the
> gsi like we have for x86_64 and are in the middle of for i386 that
> should be pretty straight forward. It would even be relatively simple
> to delay vector allocation in that context until request_irq, if we
> needed the delayed allocation benefit. Do you two have any problems
> with moving in that direction?
I think consistency here would be _wonderful_.
Of course the beauty of having identity GSI=IRQ and a /proc/interrupts
that tells you what IOAPIC pin you are using become moot with MSI --
but hey, showing the IRQ number rather than the vector number
is consistent and makes sense.
> If fixing the arch code is unacceptable for some reason I'm not aware of
> we need to audit the 10-20 drivers that call pci_disable_device in their
> suspend/resume processing and ensure that they have freed all of the
> irqs before that point. Given that I have bug reports on the msi path I
> know that isn't true.
I think the suspend/resume interrupt logic needs some serious attention.
We've had several schemes for suspend/resume of interrupts, several
changes in strategy, and right now I think we are inconsistent,
and frankly, I'm amazed it works at all.
-Len
> From: Eric W. Biederman <ebiederm@...ssion.com>
> Signed-off-by: Ingo Molnar <mingo@...e.hu>
> ---
> arch/cris/arch-v32/drivers/pci/bios.c | 4 +++-
> arch/frv/mb93090-mb00/pci-vdk.c | 3 ++-
> arch/i386/pci/common.c | 6 ++++--
> arch/ia64/pci/pci.c | 8 ++++++--
> 4 files changed, 15 insertions(+), 6 deletions(-)
>
> Index: linux/arch/cris/arch-v32/drivers/pci/bios.c
> ===================================================================
> --- linux.orig/arch/cris/arch-v32/drivers/pci/bios.c
> +++ linux/arch/cris/arch-v32/drivers/pci/bios.c
> @@ -100,7 +100,9 @@ int pcibios_enable_device(struct pci_dev
> if ((err = pcibios_enable_resources(dev, mask)) < 0)
> return err;
>
> - return pcibios_enable_irq(dev);
> + if (!dev->msi_enabled)
> + pcibios_enable_irq(dev);
> + return 0;
> }
>
> int pcibios_assign_resources(void)
> Index: linux/arch/frv/mb93090-mb00/pci-vdk.c
> ===================================================================
> --- linux.orig/arch/frv/mb93090-mb00/pci-vdk.c
> +++ linux/arch/frv/mb93090-mb00/pci-vdk.c
> @@ -466,6 +466,7 @@ int pcibios_enable_device(struct pci_dev
>
> if ((err = pcibios_enable_resources(dev, mask)) < 0)
> return err;
> - pcibios_enable_irq(dev);
> + if (!dev->msi_enabled)
> + pcibios_enable_irq(dev);
> return 0;
> }
> Index: linux/arch/i386/pci/common.c
> ===================================================================
> --- linux.orig/arch/i386/pci/common.c
> +++ linux/arch/i386/pci/common.c
> @@ -434,11 +434,13 @@ int pcibios_enable_device(struct pci_dev
> if ((err = pcibios_enable_resources(dev, mask)) < 0)
> return err;
>
> - return pcibios_enable_irq(dev);
> + if (!dev->msi_enabled)
> + return pcibios_enable_irq(dev);
> + return 0;
> }
>
> void pcibios_disable_device (struct pci_dev *dev)
> {
> - if (pcibios_disable_irq)
> + if (!dev->msi_enabled && pcibios_disable_irq)
> pcibios_disable_irq(dev);
> }
> Index: linux/arch/ia64/pci/pci.c
> ===================================================================
> --- linux.orig/arch/ia64/pci/pci.c
> +++ linux/arch/ia64/pci/pci.c
> @@ -557,14 +557,18 @@ pcibios_enable_device (struct pci_dev *d
> if (ret < 0)
> return ret;
>
> - return acpi_pci_irq_enable(dev);
> + if (!dev->msi_enabled)
> + return acpi_pci_irq_enable(dev);
> + return 0;
> }
>
> void
> pcibios_disable_device (struct pci_dev *dev)
> {
> BUG_ON(atomic_read(&dev->enable_cnt));
> - acpi_pci_irq_disable(dev);
> + if (!dev->msi_enabled)
> + acpi_pci_irq_disable(dev);
> + return 0;
> }
>
> void
> -
> 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/
>
-
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