From: Bjorn Helgaas <helgaas@kernel.org>
To: Matthew Minter <matt@masarand.com>
Cc: linux-pci@vger.kernel.org, bhelgaas@google.com,
Liviu.Dudau@arm.com, ddaney@caviumnetworks.com,
lorenzo.pieralisi@arm.com, Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH V4 08/29] x86/pci: Defer IRQ assignment to device enable time
Date: Wed, 23 Dec 2015 17:04:33 -0600 [thread overview]
Message-ID: <20151223230433.GA27708@localhost> (raw)
In-Reply-To: <1445576642-29624-9-git-send-email-matt@masarand.com>
[+cc Thomas]
Hi Matthew,
On Fri, Oct 23, 2015 at 06:03:41AM +0100, Matthew Minter wrote:
> The x86 architecture boot code currently traverses the PCI buses with
> an extra pass in order to initialise the PCI device IRQs at boot, this
> patch avoids this pass and defers the IRQ assignment untill device
> enable time which also has the benefit that hot-plugged devices are
> assigned IRQs without additional code.
I think the outline of this patch (doing the IRQ init at device
enable-time instead of at boot-time) is good, but I am concerned about
two things:
1) pcibios_lookup_irq() contains a for_each_pci_dev() loop that
updates dev->irq for all devices with the same pirq value. Previously
we ran that loop at boot-time via this path:
pci_subsys_init # subsys_initcall
x86_init.pci.init_irq # .init_irq = x86_default_pci_init_irq
x86_default_pci_init_irq # x86_default_pci_init_irq = pcibios_irq_init
pcibios_irq_init
x86_init.pci.fixup_irqs # .fixup_irqs = x86_default_pci_fixup_irqs
x86_default_pci_fixup_irqs # x86_default_pci_fixup_irqs = pcibios_fixup_irqs
pcibios_fixup_irqs
for_each_pci_dev
pcibios_lookup_irq
Now we'll run it later, in this path:
pci_enable_device
pci_enable_device_flags
do_pci_enable_device
pci_assign_irq
pci_map_irq # bridge->map_irq
pcibios_fixup_irq
pcibios_lookup_irq
for_each_pci_dev(dev2)
dev2->irq = irq # <-- potential problem
The problem is that dev2 is an unrelated device and may already have a
driver bound to it, and we shouldn't change dev->irq after a driver
binds to a device.
I don't know enough about interrupts and PIRQ to know whether this is
really a problem in practice or not. I added Thomas in case he knows.
2) I'm also worried about the fact that we don't do the IRQ init until
a driver calls pci_enable_device(). We've been doing some IRQ init in
pcibios_alloc_irq() in this path for a long time:
pci_subsys_init
...
pcibios_fixup_irqs # <-- moved from here ...
for_each_pci_dev
...
pci_device_probe
pcibios_alloc_irq
pcibios_enable_irq # pcibios_enable_irq = acpi_pci_irq_enable
# (or pirq_enable_irq, x86_of_pci_irq_enable, lguest_enable_irq, etc)
acpi_pci_irq_enable
__pci_device_probe
...
pci_drv->probe # driver .probe routine
...
pci_enable_device
...
pci_fixup_irq # <-- ... to here
I think there are two problems here:
2.1) We changed the order of pcibios_enable_irq() and pci_fixup_irq().
Both update dev->irq, and I doubt it's safe to reverse the order.
2.2) It's conceivable that a driver may use interrupts without ever
calling pci_enable_device(). That driver would be broken by this
change.
I think we could probably fix both of these worries by calling
pci_assign_irq() from pci_device_probe() instead of from
do_pci_enable_device().
I'm not so sure about the pcibios_lookup_irq() problem. That's a
little farther outside my area, so I'll have to look at that some
more.
I really want to merge this stuff because it's a major cleanup, so I'm
going to push on this more myself. I'm just writing this as a
heads-up in case anybody else has ideas.
Bjorn
> Signed-off-by: Matthew Minter <matt@masarand.com>
> ---
> arch/x86/include/asm/pci_x86.h | 7 +++--
> arch/x86/kernel/x86_init.c | 1 -
> arch/x86/pci/irq.c | 70 +++++++++++++++++++++---------------------
> 3 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
> index fa1195d..16fd8e9 100644
> --- a/arch/x86/include/asm/pci_x86.h
> +++ b/arch/x86/include/asm/pci_x86.h
> @@ -90,6 +90,7 @@ extern unsigned int pcibios_irq_mask;
>
> extern raw_spinlock_t pci_config_lock;
>
> +extern int pci_map_irq(struct pci_dev *dev, u8 slot, u8 pin);
> extern int (*pcibios_enable_irq)(struct pci_dev *dev);
> extern void (*pcibios_disable_irq)(struct pci_dev *dev);
>
> @@ -119,7 +120,7 @@ extern int __init pci_acpi_init(void);
> extern void __init pcibios_irq_init(void);
> extern int __init pcibios_init(void);
> extern int pci_legacy_init(void);
> -extern void pcibios_fixup_irqs(void);
> +extern int pcibios_fixup_irq(struct pci_dev *dev, u8 pin);
>
> /* pci-mmconfig.c */
>
> @@ -200,9 +201,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
> # define x86_default_pci_init pci_legacy_init
> # endif
> # define x86_default_pci_init_irq pcibios_irq_init
> -# define x86_default_pci_fixup_irqs pcibios_fixup_irqs
> +# define x86_default_pci_fixup_irq pcibios_fixup_irq
> #else
> # define x86_default_pci_init NULL
> # define x86_default_pci_init_irq NULL
> -# define x86_default_pci_fixup_irqs NULL
> +# define x86_default_pci_fixup_irq NULL
> #endif
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 3839628..810f1dd 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -80,7 +80,6 @@ struct x86_init_ops x86_init __initdata = {
> .pci = {
> .init = x86_default_pci_init,
> .init_irq = x86_default_pci_init_irq,
> - .fixup_irqs = x86_default_pci_fixup_irqs,
> },
> };
>
> diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
> index 350e785..15c507b 100644
> --- a/arch/x86/pci/irq.c
> +++ b/arch/x86/pci/irq.c
> @@ -1021,47 +1021,38 @@ static int pcibios_lookup_irq(struct pci_dev *dev, int assign)
> return 1;
> }
>
> -void __init pcibios_fixup_irqs(void)
> +int pcibios_fixup_irq(struct pci_dev *dev, u8 pin)
> {
> - struct pci_dev *dev = NULL;
> - u8 pin;
> -
> + int irq = dev->irq;
> DBG(KERN_DEBUG "PCI: IRQ fixup\n");
> - for_each_pci_dev(dev) {
> - /*
> - * If the BIOS has set an out of range IRQ number, just
> - * ignore it. Also keep track of which IRQ's are
> - * already in use.
> - */
> - if (dev->irq >= 16) {
> - dev_dbg(&dev->dev, "ignoring bogus IRQ %d\n", dev->irq);
> - dev->irq = 0;
> - }
> - /*
> - * If the IRQ is already assigned to a PCI device,
> - * ignore its ISA use penalty
> - */
> - if (pirq_penalty[dev->irq] >= 100 &&
> - pirq_penalty[dev->irq] < 100000)
> - pirq_penalty[dev->irq] = 0;
> - pirq_penalty[dev->irq]++;
> + /*
> + * If the BIOS has set an out of range IRQ number, just
> + * ignore it. Also keep track of which IRQ's are
> + * already in use.
> + */
> + if (irq >= 16) {
> + dev_dbg(&dev->dev, "ignoring bogus IRQ %d\n", irq);
> + irq = 0;
> }
> + /*
> + * If the IRQ is already assigned to a PCI device,
> + * ignore its ISA use penalty
> + */
> + if (pirq_penalty[irq] >= 100 &&
> + pirq_penalty[irq] < 100000)
> + pirq_penalty[irq] = 0;
> + pirq_penalty[irq]++;
>
> - if (io_apic_assign_pci_irqs)
> - return;
> + if (io_apic_assign_pci_irqs || !pin)
> + return irq;
>
> - dev = NULL;
> - for_each_pci_dev(dev) {
> - pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
> - if (!pin)
> - continue;
> + /*
> + * Still no IRQ? Try to lookup one...
> + */
> + if (!irq && pcibios_lookup_irq(dev, 0))
> + irq = dev->irq;
>
> - /*
> - * Still no IRQ? Try to lookup one...
> - */
> - if (!dev->irq)
> - pcibios_lookup_irq(dev, 0);
> - }
> + return irq;
> }
>
> /*
> @@ -1174,6 +1165,7 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> struct pci_sysdata *sd = bridge->bus->sysdata;
> ACPI_COMPANION_SET(&bridge->dev, sd->companion);
> }
> + bridge->map_irq = pci_map_irq;
> return 0;
> }
>
> @@ -1201,6 +1193,14 @@ void pcibios_penalize_isa_irq(int irq, int active)
> pirq_penalize_isa_irq(irq, active);
> }
>
> +int pci_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
> +{
> + dev->irq = pcibios_fixup_irq(dev, pin);
> + if (pcibios_enable_irq(dev))
> + return -1;
> + return dev->irq;
> +}
> +
> static int pirq_enable_irq(struct pci_dev *dev)
> {
> u8 pin = 0;
> --
> 2.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-12-23 23:04 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-23 5:03 [PATCH V4] Delay allocation of PCI device IRQs from boot time until bus scan time to fix PCI hotplugging Matthew Minter
2015-10-23 5:03 ` [PATCH V4 01/29] PCI: Build setup-irq.o on all arches Matthew Minter
2015-10-23 5:03 ` [PATCH V4 02/29] PCI: Add a prototype for pci_find_host_bridge to pci.h Matthew Minter
2015-10-23 5:03 ` [PATCH V4 03/29] PCI: Add IRQ mapping function pointers to pci_host_bridge struct Matthew Minter
2015-10-23 5:03 ` [PATCH V4 04/29] PCI: Remove const from the pci_dev struct passed to pci_fixup_irqs Matthew Minter
2015-10-23 5:03 ` [PATCH V4 05/29] PCI: Add pci_assign_irq function and have pci_fixup_irqs use it Matthew Minter
2015-10-23 5:03 ` [PATCH V4 06/29] PCI: Add a call to pci_assign_irq in do_pci_enable_device Matthew Minter
2015-10-23 5:03 ` [PATCH V4 07/29] x86/pci: Move pcibios_root_bridge_prepare from acpi.c to irq.c Matthew Minter
2015-10-23 5:03 ` [PATCH V4 08/29] x86/pci: Defer IRQ assignment to device enable time Matthew Minter
2015-12-23 23:04 ` Bjorn Helgaas [this message]
2016-01-12 11:25 ` Lorenzo Pieralisi
2015-10-23 5:03 ` [PATCH V4 09/29] x86/pci: Pass pin into pcibios_lookup_irq rather than look it up Matthew Minter
2015-10-23 5:03 ` [PATCH V4 10/29] ARM/PCI: Defer IRQ assignment to device enable time Matthew Minter
2015-10-23 5:03 ` [PATCH V4 11/29] powerpc/pci: " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 12/29] sh/PCI: Remove __init optimisations from IRQ mapping functions/data Matthew Minter
2015-10-23 5:03 ` [PATCH V4 13/29] sh/PCI: Defer IRQ assignment to device enable time Matthew Minter
2015-10-23 5:03 ` [PATCH V4 14/29] alpha/PCI: " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 15/29] cris/PCI: " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 16/29] frv/PCI: " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 17/29] m68k/pci: " Matthew Minter
2015-10-23 5:44 ` kbuild test robot
2015-10-23 5:03 ` [PATCH V4 18/29] microblaze/PCI: " Matthew Minter
2015-10-23 5:40 ` kbuild test robot
2015-10-23 5:03 ` [PATCH V4 19/29] MIPS/PCI: " Matthew Minter
2015-10-23 6:02 ` kbuild test robot
2015-10-23 5:03 ` [PATCH V4 20/29] mn10300: Defer PCI " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 21/29] mn10300: Remove pcibios_enable_device function Matthew Minter
2015-10-23 5:03 ` [PATCH V4 22/29] sparc/PCI: Defer IRQ assignment to device enable time Matthew Minter
2015-10-23 5:03 ` [PATCH V4 23/29] tile: pci: " Matthew Minter
2015-10-23 5:34 ` kbuild test robot
2015-10-23 5:03 ` [PATCH V4 24/29] unicore32/PCI: " Matthew Minter
2015-10-23 5:03 ` [PATCH V4 25/29] PCI: Update pci-versatile to use IRQ deferred assignment Matthew Minter
2015-10-23 5:03 ` [PATCH V4 26/29] PCI: Update pcie-iproc to use IRQ deffered assignment Matthew Minter
2015-10-23 5:04 ` [PATCH V4 27/29] PCI: Update pci-host-generic " Matthew Minter
2015-10-23 9:40 ` kbuild test robot
2015-10-23 5:04 ` [PATCH V4 28/29] of/pci: Fix comment for pci_fixup_irqs changing to pci_assign_irq Matthew Minter
2016-01-13 11:25 ` Lorenzo Pieralisi
2015-10-23 5:04 ` [PATCH V4 29/29] PCI: Remove pci_fixup_irqs function Matthew Minter
2015-12-07 17:32 ` [PATCH V4] Delay allocation of PCI device IRQs from boot time until bus scan time to fix PCI hotplugging Bjorn Helgaas
2015-12-18 21:54 ` Christopher Covington
2015-12-19 11:50 ` Matthew Minter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151223230433.GA27708@localhost \
--to=helgaas@kernel.org \
--cc=Liviu.Dudau@arm.com \
--cc=bhelgaas@google.com \
--cc=ddaney@caviumnetworks.com \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=matt@masarand.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).