Re: [PATCH 01/22] Added some infrastructure to allow assigning PCI device IRQs at device enable time and removed pci_fixup_irqs as the other change obsolites it.

[Wei Yang <weiyang@linux.vnet.ibm.com>]

On Tue, Aug 05, 2014 at 05:11:02PM +0100, matthew_minter@xyratex.com wrote:
>From: matthew_minter <matthew_minter@xyratex.com>
>
>---
> drivers/pci/Makefile      | 15 ++-------------
> drivers/pci/host-bridge.c |  2 +-
> drivers/pci/pci.c         |  6 +++++-
> drivers/pci/pci.h         |  1 +
> drivers/pci/probe.c       | 12 ------------
> drivers/pci/setup-irq.c   | 25 +++++++++----------------
> include/linux/pci.h       |  8 ++++----
> 7 files changed, 22 insertions(+), 47 deletions(-)
>
>diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>index e04fe2d..38c4cb0 100644
>--- a/drivers/pci/Makefile
>+++ b/drivers/pci/Makefile
>@@ -4,7 +4,8 @@
>
> obj-y		+= access.o bus.o probe.o host-bridge.o remove.o pci.o \
> 			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
>-			irq.o vpd.o setup-bus.o vc.o
>+			irq.o vpd.o setup-bus.o vc.o setup-irq.o
>+
> obj-$(CONFIG_PROC_FS) += proc.o
> obj-$(CONFIG_SYSFS) += slot.o
>
>@@ -31,18 +32,6 @@ obj-$(CONFIG_PCI_ATS) += ats.o
> obj-$(CONFIG_PCI_IOV) += iov.o
>
> #
>-# Some architectures use the generic PCI setup functions
>-#
>-obj-$(CONFIG_ALPHA) += setup-irq.o
>-obj-$(CONFIG_ARM) += setup-irq.o
>-obj-$(CONFIG_UNICORE32) += setup-irq.o
>-obj-$(CONFIG_SUPERH) += setup-irq.o
>-obj-$(CONFIG_MIPS) += setup-irq.o
>-obj-$(CONFIG_TILE) += setup-irq.o
>-obj-$(CONFIG_SPARC_LEON) += setup-irq.o
>-obj-$(CONFIG_M68K) += setup-irq.o
>-
>-#
> # ACPI Related PCI FW Functions
> # ACPI _DSM provided firmware instance and string name
> #
>diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
>index 0e5f3c9..8ed186f 100644
>--- a/drivers/pci/host-bridge.c
>+++ b/drivers/pci/host-bridge.c
>@@ -16,7 +16,7 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> 	return bus;
> }
>
>-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
>+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
> {
> 	struct pci_bus *root_bus = find_pci_root_bus(bus);
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 63a54a3..d51d076 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -1197,10 +1197,15 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars)
> 	int err;
> 	u16 cmd;
> 	u8 pin;
>+	struct pci_host_bridge *hbrg = find_pci_host_bridge(dev->bus);
>
> 	err = pci_set_power_state(dev, PCI_D0);
> 	if (err < 0 && err != -EIO)
> 		return err;
>+
>+	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>+	pdev_assign_irq(dev, hbrg->swizzle_irq, hbrg->map_irq);
>+
> 	err = pcibios_enable_device(dev, bars);
> 	if (err < 0)
> 		return err;
>@@ -1209,7 +1214,6 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars)
> 	if (dev->msi_enabled || dev->msix_enabled)
> 		return 0;
>
>-	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);

Not sure why you move this up.

Before your change, the call flow would be like this:

| scan_bus
|   + pci_fixup_irqs
|      +    pci_dev->irq and PCI_INTERRUPT_PIN assigned with correct number
|
| pci_enable_device, called from driver
|   + do_pci_enable_device, read the pin from pci_dev

As my understanding, this ensures the pin read from hardware is fixuped. While
after your change, the pci_fixup_irqs is removed and the pin will be assigned
in pdev_assign_irq() after you fix it properly.

Do I missed something?

> 	if (pin) {
> 		pci_read_config_word(dev, PCI_COMMAND, &cmd);
> 		if (cmd & PCI_COMMAND_INTX_DISABLE)

-- 
Richard Yang
Help you, Help me