* [PATCH] x86, irq: Check if irq is remapped before freeing irte @ 2010-10-18 20:47 Yinghai Lu 2010-10-18 21:09 ` Thomas Gleixner 2010-10-19 7:28 ` [tip:irq/core] x86: ioapic: Call free_irte only if interrupt remapping enabled tip-bot for Yinghai Lu 0 siblings, 2 replies; 8+ messages in thread From: Yinghai Lu @ 2010-10-18 20:47 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin; +Cc: linux-kernel@vger.kernel.org On one system that support intr-rempping when boot with "intremap=off" got: [ 177.824202] calling alsa_card_azx_init+0x0/0x1b @ 1 [ 177.843968] HDA Intel 0000:00:1b.0: PCI INT A -> GSI 22 (level, low) -> IRQ 22 [ 177.848210] HDA Intel 0000:00:1b.0: irq 1435 for MSI/MSI-X [ 177.863797] HDA Intel 0000:00:1b.0: setting latency timer to 64 [ 177.895084] hda-intel: no codecs found! [ 177.895501] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8 [ 177.913316] IP: [<ffffffff8145fc18>] free_irte+0x47/0xc0 [ 177.913859] PGD 0 [ 177.914037] Oops: 0000 [#1] SMP [ 177.933240] last sysfs file: [ 177.933501] CPU 0 [ 177.933655] Modules linked in: [ 177.933937] [ 177.934078] Pid: 15044, comm: work_for_cpu Not tainted 2.6.36-rc8-tip-yh-01994-g95100d0-dirty #198 /Sun Fire X4800 [ 177.953986] RIP: 0010:[<ffffffff8145fc18>] [<ffffffff8145fc18>] free_irte+0x47/0xc0 ... [ 178.173326] Call Trace: [ 178.173574] [<ffffffff810515b4>] destroy_irq+0x3a/0x75 [ 178.192934] [<ffffffff81051834>] arch_teardown_msi_irq+0xe/0x10 [ 178.193418] [<ffffffff81458dc3>] arch_teardown_msi_irqs+0x56/0x7f [ 178.213021] [<ffffffff81458e79>] free_msi_irqs+0x8d/0xeb [ 178.213490] [<ffffffff81459673>] pci_disable_msi+0x35/0x3a [ 178.232956] [<ffffffff81b68917>] azx_free+0x83/0x11c [ 178.233301] [<ffffffff81cb1ec7>] azx_probe+0x7b1/0xab4 [ 178.252885] [<ffffffff810a59ef>] ? trace_hardirqs_on+0xd/0xf [ 178.253303] [<ffffffff81442a50>] local_pci_probe+0x4d/0x96 [ 178.272801] [<ffffffff8108e72c>] ? do_work_for_cpu+0x0/0x2b [ 178.273270] [<ffffffff8108e744>] do_work_for_cpu+0x18/0x2b [ 178.292785] [<ffffffff8108e72c>] ? do_work_for_cpu+0x0/0x2b [ 178.293220] [<ffffffff81094135>] kthread+0x9d/0xa5 [ 178.312742] [<ffffffff81034954>] kernel_thread_helper+0x4/0x10 [ 178.313222] [<ffffffff81cc96fc>] ? restore_args+0x0/0x30 [ 178.332746] [<ffffffff81094098>] ? kthread+0x0/0xa5 [ 178.333207] [<ffffffff81034950>] ? kernel_thread_helper+0x0/0x10 Root cause is that irq_2_iommu is embedded into irq_cfg now... Need to check if that irq is really mapped, before free irte. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/intr_remapping.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux-2.6/drivers/pci/intr_remapping.c =================================================================== --- linux-2.6.orig/drivers/pci/intr_remapping.c +++ linux-2.6/drivers/pci/intr_remapping.c @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry unsigned long flags; int index; - if (!entry || !irq_iommu) + if (!entry || !irq_iommu || !irq_iommu->iommu) return -1; spin_lock_irqsave(&irq_2_ir_lock, flags); @@ -268,7 +268,7 @@ int free_irte(int irq) unsigned long flags; int rc; - if (!irq_iommu) + if (!irq_iommu || !irq_iommu->iommu) return -1; spin_lock_irqsave(&irq_2_ir_lock, flags); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 20:47 [PATCH] x86, irq: Check if irq is remapped before freeing irte Yinghai Lu @ 2010-10-18 21:09 ` Thomas Gleixner 2010-10-18 21:17 ` Thomas Gleixner 2010-10-19 7:28 ` [tip:irq/core] x86: ioapic: Call free_irte only if interrupt remapping enabled tip-bot for Yinghai Lu 1 sibling, 1 reply; 8+ messages in thread From: Thomas Gleixner @ 2010-10-18 21:09 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On Mon, 18 Oct 2010, Yinghai Lu wrote: > > Index: linux-2.6/drivers/pci/intr_remapping.c > =================================================================== > --- linux-2.6.orig/drivers/pci/intr_remapping.c > +++ linux-2.6/drivers/pci/intr_remapping.c > @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry > unsigned long flags; > int index; > > - if (!entry || !irq_iommu) > + if (!entry || !irq_iommu || !irq_iommu->iommu) > return -1; Hmm, why do we need this? This is only called from ir_ioapic_set_affinity() and ir_msi_set_affinity(). We should never end up there when intr_remapping=off, right ? > spin_lock_irqsave(&irq_2_ir_lock, flags); > @@ -268,7 +268,7 @@ int free_irte(int irq) > unsigned long flags; > int rc; > > - if (!irq_iommu) > + if (!irq_iommu || !irq_iommu->iommu) > return -1; That one makes sense Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 21:09 ` Thomas Gleixner @ 2010-10-18 21:17 ` Thomas Gleixner 2010-10-18 21:28 ` Yinghai 2010-10-18 22:22 ` Yinghai Lu 0 siblings, 2 replies; 8+ messages in thread From: Thomas Gleixner @ 2010-10-18 21:17 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On Mon, 18 Oct 2010, Thomas Gleixner wrote: > On Mon, 18 Oct 2010, Yinghai Lu wrote: > > > > Index: linux-2.6/drivers/pci/intr_remapping.c > > =================================================================== > > --- linux-2.6.orig/drivers/pci/intr_remapping.c > > +++ linux-2.6/drivers/pci/intr_remapping.c > > @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry > > unsigned long flags; > > int index; > > > > - if (!entry || !irq_iommu) > > + if (!entry || !irq_iommu || !irq_iommu->iommu) > > return -1; > > Hmm, why do we need this? This is only called from > ir_ioapic_set_affinity() and ir_msi_set_affinity(). > > We should never end up there when intr_remapping=off, right ? Thinking more about it, this check is actively bogus. The call sites do: struct irte irte; if (get_irte(irq, &irte)) return -1; So entry _CANNOT_ be NULL. And in fact we should change get_irte() to get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry) The call site already knows about it. No need to lookup irq_iommu based on the irq number. Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 21:17 ` Thomas Gleixner @ 2010-10-18 21:28 ` Yinghai 2010-10-18 22:22 ` Yinghai Lu 1 sibling, 0 replies; 8+ messages in thread From: Yinghai @ 2010-10-18 21:28 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On Oct 18, 2010, at 2:17 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > > On Mon, 18 Oct 2010, Thomas Gleixner wrote: > >> On Mon, 18 Oct 2010, Yinghai Lu wrote: >>> >>> Index: linux-2.6/drivers/pci/intr_remapping.c >>> =================================================================== >>> --- linux-2.6.orig/drivers/pci/intr_remapping.c >>> +++ linux-2.6/drivers/pci/intr_remapping.c >>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry >>> unsigned long flags; >>> int index; >>> >>> - if (!entry || !irq_iommu) >>> + if (!entry || !irq_iommu || !irq_iommu->iommu) >>> return -1; >> >> Hmm, why do we need this? This is only called from >> ir_ioapic_set_affinity() and ir_msi_set_affinity(). >> >> We should never end up there when intr_remapping=off, right ? > > Thinking more about it, this check is actively bogus. The call sites do: > > struct irte irte; > > if (get_irte(irq, &irte)) > return -1; > > So entry _CANNOT_ be NULL. > > And in fact we should change get_irte() to > > get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry) > > The call site already knows about it. No need to lookup irq_iommu > based on the irq number. Yes ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 21:17 ` Thomas Gleixner 2010-10-18 21:28 ` Yinghai @ 2010-10-18 22:22 ` Yinghai Lu 2010-10-18 22:31 ` Thomas Gleixner 1 sibling, 1 reply; 8+ messages in thread From: Yinghai Lu @ 2010-10-18 22:22 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On 10/18/2010 02:17 PM, Thomas Gleixner wrote: > > > On Mon, 18 Oct 2010, Thomas Gleixner wrote: > >> On Mon, 18 Oct 2010, Yinghai Lu wrote: >>> >>> Index: linux-2.6/drivers/pci/intr_remapping.c >>> =================================================================== >>> --- linux-2.6.orig/drivers/pci/intr_remapping.c >>> +++ linux-2.6/drivers/pci/intr_remapping.c >>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry >>> unsigned long flags; >>> int index; >>> >>> - if (!entry || !irq_iommu) >>> + if (!entry || !irq_iommu || !irq_iommu->iommu) >>> return -1; >> >> Hmm, why do we need this? This is only called from >> ir_ioapic_set_affinity() and ir_msi_set_affinity(). >> >> We should never end up there when intr_remapping=off, right ? > > Thinking more about it, this check is actively bogus. The call sites do: > > struct irte irte; > > if (get_irte(irq, &irte)) > return -1; > > So entry _CANNOT_ be NULL. > > And in fact we should change get_irte() to > > get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry) > > The call site already knows about it. No need to lookup irq_iommu > based on the irq number. looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu" extern int get_irte(int irq, struct irte *entry); extern int modify_irte(int irq, struct irte *irte_modified); extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count); extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, u16 sub_handle); extern int map_irq_to_irte_handle(int irq, u16 *sub_handle); extern int free_irte(int irq); Thanks Yinghai ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 22:22 ` Yinghai Lu @ 2010-10-18 22:31 ` Thomas Gleixner 2010-10-18 22:47 ` Yinghai Lu 0 siblings, 1 reply; 8+ messages in thread From: Thomas Gleixner @ 2010-10-18 22:31 UTC (permalink / raw) To: Yinghai Lu; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On Mon, 18 Oct 2010, Yinghai Lu wrote: > On 10/18/2010 02:17 PM, Thomas Gleixner wrote: > > > > > > On Mon, 18 Oct 2010, Thomas Gleixner wrote: > > > >> On Mon, 18 Oct 2010, Yinghai Lu wrote: > >>> > >>> Index: linux-2.6/drivers/pci/intr_remapping.c > >>> =================================================================== > >>> --- linux-2.6.orig/drivers/pci/intr_remapping.c > >>> +++ linux-2.6/drivers/pci/intr_remapping.c > >>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry > >>> unsigned long flags; > >>> int index; > >>> > >>> - if (!entry || !irq_iommu) > >>> + if (!entry || !irq_iommu || !irq_iommu->iommu) > >>> return -1; > >> > >> Hmm, why do we need this? This is only called from > >> ir_ioapic_set_affinity() and ir_msi_set_affinity(). That does not answer that question ! > >> We should never end up there when intr_remapping=off, right ? > > > > Thinking more about it, this check is actively bogus. The call sites do: > > > > struct irte irte; > > > > if (get_irte(irq, &irte)) > > return -1; > > > > So entry _CANNOT_ be NULL. > > > > And in fact we should change get_irte() to > > > > get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry) > > > > The call site already knows about it. No need to lookup irq_iommu > > based on the irq number. > > looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu" > > extern int get_irte(int irq, struct irte *entry); > extern int modify_irte(int irq, struct irte *irte_modified); > extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count); > extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, > u16 sub_handle); > extern int map_irq_to_irte_handle(int irq, u16 *sub_handle); > extern int free_irte(int irq); Probably, but we need to figure out which functions need which checks instead of having either redundant or superflous ones there. Thanks, tglx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte 2010-10-18 22:31 ` Thomas Gleixner @ 2010-10-18 22:47 ` Yinghai Lu 0 siblings, 0 replies; 8+ messages in thread From: Yinghai Lu @ 2010-10-18 22:47 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Ingo Molnar, H. Peter Anvin, linux-kernel@vger.kernel.org On 10/18/2010 03:31 PM, Thomas Gleixner wrote: > On Mon, 18 Oct 2010, Yinghai Lu wrote: > >> On 10/18/2010 02:17 PM, Thomas Gleixner wrote: >>> >>> >>> On Mon, 18 Oct 2010, Thomas Gleixner wrote: >>> >>>> On Mon, 18 Oct 2010, Yinghai Lu wrote: >>>>> >>>>> Index: linux-2.6/drivers/pci/intr_remapping.c >>>>> =================================================================== >>>>> --- linux-2.6.orig/drivers/pci/intr_remapping.c >>>>> +++ linux-2.6/drivers/pci/intr_remapping.c >>>>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry >>>>> unsigned long flags; >>>>> int index; >>>>> >>>>> - if (!entry || !irq_iommu) >>>>> + if (!entry || !irq_iommu || !irq_iommu->iommu) >>>>> return -1; >>>> >>>> Hmm, why do we need this? This is only called from >>>> ir_ioapic_set_affinity() and ir_msi_set_affinity(). > > That does not answer that question ! we don't need that checking there. > >>>> We should never end up there when intr_remapping=off, right ? >>> >>> Thinking more about it, this check is actively bogus. The call sites do: >>> >>> struct irte irte; >>> >>> if (get_irte(irq, &irte)) >>> return -1; >>> >>> So entry _CANNOT_ be NULL. >>> >>> And in fact we should change get_irte() to >>> >>> get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry) >>> >>> The call site already knows about it. No need to lookup irq_iommu >>> based on the irq number. >> >> looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu" >> >> extern int get_irte(int irq, struct irte *entry); >> extern int modify_irte(int irq, struct irte *irte_modified); >> extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count); >> extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, >> u16 sub_handle); >> extern int map_irq_to_irte_handle(int irq, u16 *sub_handle); >> extern int free_irte(int irq); > > Probably, but we need to figure out which functions need which checks > instead of having either redundant or superflous ones there. > only free_irte(). because other have have protection from intr_remapping_enabled or irq_remapped(get_irq_chip_data(irq)) or with ir related chip. this one works too. --- arch/x86/kernel/apic/io_apic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-2.6/arch/x86/kernel/apic/io_apic.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c +++ linux-2.6/arch/x86/kernel/apic/io_apic.c @@ -3109,7 +3109,8 @@ void destroy_irq(unsigned int irq) irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE); - free_irte(irq); + if (intr_remapping_enabled) + free_irte(irq); raw_spin_lock_irqsave(&vector_lock, flags); __clear_irq_vector(irq, cfg); raw_spin_unlock_irqrestore(&vector_lock, flags); ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip:irq/core] x86: ioapic: Call free_irte only if interrupt remapping enabled 2010-10-18 20:47 [PATCH] x86, irq: Check if irq is remapped before freeing irte Yinghai Lu 2010-10-18 21:09 ` Thomas Gleixner @ 2010-10-19 7:28 ` tip-bot for Yinghai Lu 1 sibling, 0 replies; 8+ messages in thread From: tip-bot for Yinghai Lu @ 2010-10-19 7:28 UTC (permalink / raw) To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, yinghai, tglx Commit-ID: 9717967c4b704ce344c954afb5bb160aa9c01c34 Gitweb: http://git.kernel.org/tip/9717967c4b704ce344c954afb5bb160aa9c01c34 Author: Yinghai Lu <yinghai@kernel.org> AuthorDate: Mon, 18 Oct 2010 13:47:48 -0700 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Tue, 19 Oct 2010 09:25:33 +0200 x86: ioapic: Call free_irte only if interrupt remapping enabled On a system that support intr-rempping when booting with "intremap=off" [ 177.895501] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8 [ 177.913316] IP: [<ffffffff8145fc18>] free_irte+0x47/0xc0 ... [ 178.173326] Call Trace: [ 178.173574] [<ffffffff810515b4>] destroy_irq+0x3a/0x75 [ 178.192934] [<ffffffff81051834>] arch_teardown_msi_irq+0xe/0x10 [ 178.193418] [<ffffffff81458dc3>] arch_teardown_msi_irqs+0x56/0x7f [ 178.213021] [<ffffffff81458e79>] free_msi_irqs+0x8d/0xeb Call free_irte only when interrupt remapping is enabled. Signed-off-by: Yinghai Lu <yinghai@kernel.org> LKML-Reference: <4CBCB274.7010108@kernel.org> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/apic/io_apic.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index 20e47e0..8ae808d 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -3109,7 +3109,8 @@ void destroy_irq(unsigned int irq) irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE); - free_irte(irq); + if (intr_remapping_enabled) + free_irte(irq); raw_spin_lock_irqsave(&vector_lock, flags); __clear_irq_vector(irq, cfg); raw_spin_unlock_irqrestore(&vector_lock, flags); ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-10-19 7:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-18 20:47 [PATCH] x86, irq: Check if irq is remapped before freeing irte Yinghai Lu 2010-10-18 21:09 ` Thomas Gleixner 2010-10-18 21:17 ` Thomas Gleixner 2010-10-18 21:28 ` Yinghai 2010-10-18 22:22 ` Yinghai Lu 2010-10-18 22:31 ` Thomas Gleixner 2010-10-18 22:47 ` Yinghai Lu 2010-10-19 7:28 ` [tip:irq/core] x86: ioapic: Call free_irte only if interrupt remapping enabled tip-bot for Yinghai Lu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.