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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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