| Message ID | 1416531745-24661-1-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 20 Nov 2014, suravee.suthikulpanit@amd.com wrote: > This patch checks if the parent domain is NULL before recursively freeing > irqs in the parent domains. Which is nonsense, because if the thing has not been allocated in the first place, then it cannot explode in the free path magically, except there is a missing check in the allocation path error handling. And that's obviously not the case simply because this originates from: > [<fffffe0000449278>] pci_disable_msix+0x40/0x50 Suravee, this is the last warning. I'm tired of your half baken patches which lack any explanation. Read back on my previous replies to your mails for further explanation. This is not a 'try and error and hack enough nonsensical checks into the code' commercial project. This is core kernel code and requires proper explanation. Thanks, tglx
On 11/20/2014 07:32 PM, Thomas Gleixner wrote: > On Thu, 20 Nov 2014, suravee.suthikulpanit@amd.com wrote: >> This patch checks if the parent domain is NULL before recursively freeing >> irqs in the parent domains. > > Which is nonsense, because if the thing has not been allocated in the > first place, then it cannot explode in the free path magically, except > there is a missing check in the allocation path error handling. > > And that's obviously not the case simply because this originates from: >> [<fffffe0000449278>] pci_disable_msix+0x40/0x50 > Thomas, In this case, I have the following irq domain hierarchy: [GIC] -- [GICv2m] -- [MSI] which recursively calling the freeing function: In GIC domain, it currently defines the struct irq_domain_ops.free() with : --> irq_domain_free_irqs_top() |--> irq_domain_free_irqs_common() |--> irq_domain_free_irq_parent() |--> irq_domain_free_irqs_recursive() and there is no check before passing the NULL domain->parent into the irq_domain_free_irqs_recursive(), which causes the error. Since the GIC is the top most domain, it does not have parent domain. So, I'm not sure what is missing from the allocation path error handling, as you mentioned. Thanks, Suravee
On 2014/11/21 10:08, Suravee Suthikulpanit wrote: > On 11/20/2014 07:32 PM, Thomas Gleixner wrote: >> On Thu, 20 Nov 2014, suravee.suthikulpanit@amd.com wrote: >>> This patch checks if the parent domain is NULL before recursively >>> freeing >>> irqs in the parent domains. >> >> Which is nonsense, because if the thing has not been allocated in the >> first place, then it cannot explode in the free path magically, except >> there is a missing check in the allocation path error handling. >> >> And that's obviously not the case simply because this originates from: >>> [<fffffe0000449278>] pci_disable_msix+0x40/0x50 >> > > Thomas, > > In this case, I have the following irq domain hierarchy: > > [GIC] -- [GICv2m] -- [MSI] > > which recursively calling the freeing function: > > In GIC domain, it currently defines the struct irq_domain_ops.free() with : > --> irq_domain_free_irqs_top() > |--> irq_domain_free_irqs_common() > |--> irq_domain_free_irq_parent() > |--> irq_domain_free_irqs_recursive() > > and there is no check before passing the NULL domain->parent into the > irq_domain_free_irqs_recursive(), which causes the error. > > Since the GIC is the top most domain, it does not have parent domain. > So, I'm not sure what is missing from the allocation path error > handling, as you mentioned. Hi Thomas, We have had a discussion about this issue in another thread. Originally irq_domain_free_irqs_common() is designed to be used by irqdomains with parent. But there are desires to reuse it to support irqdomains without parent too for code reduction. So I suggest to change irq_domain_free_irqs_common() instead of irq_domain_free_irqs_parent() because caller of irq_domain_free_irqs_parent() should guarantee current domain do have a parent. I'm preparing a patch for this:) Regards! Gerry > > Thanks, > > Suravee
On 11/20/2014 08:49 PM, Jiang Liu wrote: > > > On 2014/11/21 10:08, Suravee Suthikulpanit wrote: >> On 11/20/2014 07:32 PM, Thomas Gleixner wrote: >>> On Thu, 20 Nov 2014, suravee.suthikulpanit@amd.com wrote: >>>> This patch checks if the parent domain is NULL before recursively >>>> freeing >>>> irqs in the parent domains. >>> >>> Which is nonsense, because if the thing has not been allocated in the >>> first place, then it cannot explode in the free path magically, except >>> there is a missing check in the allocation path error handling. >>> >>> And that's obviously not the case simply because this originates from: >>>> [<fffffe0000449278>] pci_disable_msix+0x40/0x50 >>> >> >> Thomas, >> >> In this case, I have the following irq domain hierarchy: >> >> [GIC] -- [GICv2m] -- [MSI] >> >> which recursively calling the freeing function: >> >> In GIC domain, it currently defines the struct irq_domain_ops.free() with : >> --> irq_domain_free_irqs_top() >> |--> irq_domain_free_irqs_common() >> |--> irq_domain_free_irq_parent() >> |--> irq_domain_free_irqs_recursive() >> >> and there is no check before passing the NULL domain->parent into the >> irq_domain_free_irqs_recursive(), which causes the error. >> >> Since the GIC is the top most domain, it does not have parent domain. >> So, I'm not sure what is missing from the allocation path error >> handling, as you mentioned. > Hi Thomas, > We have had a discussion about this issue in another thread. > Originally irq_domain_free_irqs_common() is designed to be used by > irqdomains with parent. But there are desires to reuse it to support > irqdomains without parent too for code reduction. > So I suggest to change irq_domain_free_irqs_common() instead of > irq_domain_free_irqs_parent() because caller of > irq_domain_free_irqs_parent() should guarantee current domain do have > a parent. > I'm preparing a patch for this:) > Regards! > Gerry Thanks Gerry and Thomas. Suravee >> >> Thanks, >> >> Suravee
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 029acf1..4390eb8 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -1166,6 +1166,9 @@ int irq_domain_alloc_irqs_parent(struct irq_domain *domain, void irq_domain_free_irqs_parent(struct irq_domain *domain, unsigned int irq_base, unsigned int nr_irqs) { + if (!domain->parent) + return; + /* irq_domain_free_irqs_recursive() will call parent's free */ if (!irq_domain_is_auto_recursive(domain)) irq_domain_free_irqs_recursive(domain->parent, irq_base,