Re: [patch] MSI-X: fix resume crash

[Len Brown]

> 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@xxxxxxxxxxxx>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/