From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <54113BAC.10505@linux.intel.com> Date: Thu, 11 Sep 2014 14:05:32 +0800 From: Jiang Liu MIME-Version: 1.0 To: Thomas Gleixner CC: Benjamin Herrenschmidt , Ingo Molnar , "H. Peter Anvin" , "Rafael J. Wysocki" , Bjorn Helgaas , Randy Dunlap , Yinghai Lu , Borislav Petkov , Grant Likely , Len Brown , Pavel Machek , x86@kernel.org, Prarit Bhargava , Konrad Rzeszutek Wilk , Andrew Morton , Tony Luck , Joerg Roedel , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org, Ingo Molnar , linux-pm@vger.kernel.org Subject: Re: [Patch v4 12/16] x86, irq, ACPI: Implement interface to support ACPI based IOAPIC hot-addition References: <1409192561-19744-1-git-send-email-jiang.liu@linux.intel.com> <1409192561-19744-13-git-send-email-jiang.liu@linux.intel.com> <540FC1C2.40100@linux.intel.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-acpi-owner@vger.kernel.org List-ID: On 2014/9/11 4:06, Thomas Gleixner wrote: > On Wed, 10 Sep 2014, Jiang Liu wrote: >>>> int mp_register_ioapic(int id, u32 address, u32 gsi_base, >>>> @@ -3867,8 +3873,15 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base, >>>> } >>>> for_each_ioapic(ioapic) >>>> if (ioapics[ioapic].mp_config.apicaddr == address) { >>>> - pr_warn("address 0x%x conflicts with IOAPIC%d\n", >>>> - address, ioapic); >>>> + /* >>>> + * IOAPIC unit may also be visible in PCI scope. >>>> + * When ioapic PCI driver's probe() is called, >>>> + * the IOAPIC unit may have already been initialized >>>> + * at boot time. >>>> + */ >>>> + if (!ioapic_initialized) >>>> + pr_warn("address 0x%x conflicts with IOAPIC%d\n", >>>> + address, ioapic); >>> >>> Hmm. This smells fishy. Why do we allow multiple initializations of >>> the same IOAPIC in the first place. Either it's done via ACPI or via >>> PCI, but not both. >> The ACPI subsystem will register and initialize all IOAPICs when walking >> ACPI MADT table during boot, before initializing PCI subsystem. >> Later when binding ioapic PCI driver to IOAPIC PCI device, it will try >> to register the IOAPIC device again. >> >> After this patchset is applied, we could remove the !ioapic_initialized >> check. We check acpi_ioapic_register() before calling >> acpi_register_ioapic(). So the check becomes redundant. >> Or we could remove the temporary code from this patch. > > How about removing the disfunctional ioapic PCI driver first and then > implementing the whole thing cleanly? Good suggestion:) > >>> >>>> return -EEXIST; >>>> } >>>> >>>> @@ -3918,6 +3931,14 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base, >>>> ioapics[idx].irqdomain = NULL; >>>> ioapics[idx].irqdomain_cfg = *cfg; >>>> >>>> + if (ioapic_initialized) { >>> >>> I have a hard time to understand this conditional. Why can't we do >>> that unconditionally? >> How about following comments? >> /* >> * If mp_register_ioapic() is called during early boot stage when >> * walking ACPI/SFI/DT tables, it's too early to create irqdomain, >> * we are still using bootmem allocator. So delay it to setup_IO_APIC(). >> */ > > Fine, but then the "if (ioapic_initialized)" conditional still does > not make sense. We surely have some global non ioapic specific > indicator that bootmem is done and the proper memory allocator is > available, right? Flag ioapic_initialized will be used to check whether we have created irqdomains for IOAPICs. Currently function arch_dynirq_lower_bound() uses that flag, and alloc_irq_from_domain() will use it too later. > > Aside of that is there a point to walk those tables before we actually > can make any use of their content? At least we depend on walking those tables to detect whether system has IOAPICs available:) Regards! Gerry > > Thanks, > > tglx >