From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:25017 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751501AbaIJCqv (ORCPT ); Tue, 9 Sep 2014 22:46:51 -0400 Message-ID: <540FBB93.4020509@linux.intel.com> Date: Wed, 10 Sep 2014 10:46:43 +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 , x86@kernel.org, Len Brown , Pavel Machek , 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 14/16] x86, irq: Introduce helper to check whether an IOAPIC has been registered References: <1409192561-19744-1-git-send-email-jiang.liu@linux.intel.com> <1409192561-19744-15-git-send-email-jiang.liu@linux.intel.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-pci-owner@vger.kernel.org List-ID: On 2014/9/9 20:37, Thomas Gleixner wrote: > On Thu, 28 Aug 2014, Jiang Liu wrote: > >> Introduce acpi_ioapic_registered() to check whether an IOAPIC has already >> been registered, it will be used when enabling IOAPIC hotplug. >> >> Signed-off-by: Jiang Liu >> --- >> arch/x86/include/asm/io_apic.h | 1 + >> arch/x86/kernel/acpi/boot.c | 11 +++++++++++ >> arch/x86/kernel/apic/io_apic.c | 11 +++++++++++ >> include/linux/acpi.h | 1 + >> 4 files changed, 24 insertions(+) >> >> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h >> index ce63cf34c93c..0db2b7037e4b 100644 >> --- a/arch/x86/include/asm/io_apic.h >> +++ b/arch/x86/include/asm/io_apic.h >> @@ -191,6 +191,7 @@ extern void mp_unmap_irq(int irq); >> extern int mp_register_ioapic(int id, u32 address, u32 gsi_base, >> struct ioapic_domain_cfg *cfg); >> extern int mp_unregister_ioapic(u32 gsi_base); >> +extern int mp_ioapic_registered(u32 gsi_base); >> extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq, >> irq_hw_number_t hwirq); >> extern void mp_irqdomain_unmap(struct irq_domain *domain, unsigned int virq); >> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c >> index 560a6d1cb0f4..6aa796f77b71 100644 >> --- a/arch/x86/kernel/acpi/boot.c >> +++ b/arch/x86/kernel/acpi/boot.c >> @@ -810,6 +810,17 @@ int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) >> } >> EXPORT_SYMBOL(acpi_unregister_ioapic); >> >> +int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base) >> +{ >> + int ret; >> + >> + down_write(&acpi_ioapic_rwsem); > > Why down_write? You are merily checking whether the thing is > registered already. Yeah, a down_read() should be enough:) >> + ret = mp_ioapic_registered(gsi_base); >> + up_write(&acpi_ioapic_rwsem); >> + >> + return ret; >> +} > > So I assume that this is for a particular caller in the apci ioapic > hotplug code and that call site has its own serialization. Otherwise > the return value is not protected at all. > > Please add a Docbook comment to that function, and document the > locking rules as thats pretty non obvious. How about this comments about locking rules? /* * Locks related to IOAPIC hotplug * Hotplug side: * ->lock_device_hotplug() //device_hotplug_lock * ->acpi_ioapic_rwsem * ->ioapic_lock * Interrupt mapping side: * ->acpi_ioapic_rwsem * ->ioapic_mutex * ->ioapic_lock */ Regards! Gerry > > The such is missing in some other patches as well. > > Thanks, > > tglx >