It seems that by coincidence, my testing boxes did not have any
devices which needed the PCI_INTERRUPT_PIN fixed up. Therefore I had
not noticed the fact the fixed value was not used in the enable path.
I would assume that someone with a different setup would have had a
problem there however so thank you for pointing out this bug. I have
integrated your fix into version 2 of the patch set.
Many thanks.
On 8 August 2014 02:20, Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Aug 07, 2014 at 12:35:41PM +0100, Matthew Minter wrote:
>>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?
>>
>
> Yes, pci_read_config_byte() should be called after pdev_assign_irq().
>
>>I will give that a quick test but it sounds like you are correct.
>
> I am willing to know the result, especially in the original one, what pin
> value you retrieved from pci_dev.
>
>>
>>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
>>
>>------------------------------
>
> --
> 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
