Just to confirm, are you saying that:
>+
>+ pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>+ pdev_assign_irq(dev, hbrg->swizzle_irq, hbrg->map_irq);
>+
Should become:
>+
>+ pdev_assign_irq(dev, hbrg->swizzle_irq, hbrg->map_irq);
>+ pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>+
So that we read the config byte after running the assignment and fixup?
I had not thought about the case where the fixup changes the
PCI_INTERRUPT_PIN value.
It looks to me like you are correct there and this should indeed be fixed.
I can certainly swap those around for the next patch revision if that
is what you are suggesting?
I will give that a quick test but it sounds like you are correct.
Many thanks,
Matthew
On 7 August 2014 04:43, Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Aug 05, 2014 at 05:11:02PM +0100, matthew_minter@xxxxxxxxxxx wrote:
>>From: matthew_minter <matthew_minter@xxxxxxxxxxx>
>>
>>---
>> 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
>
--
------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com
------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
