> 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 <[EMAIL PROTECTED]>
> Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]>
> ---
> 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 [EMAIL PROTECTED]
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
