| Message ID | 1409192561-19744-15-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 <jiang.liu@linux.intel.com> > --- > 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. > + 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. The such is missing in some other patches as well. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jiang.liu@linux.intel.com> >> --- >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 10 Sep 2014, Jiang Liu wrote: > On 2014/9/9 20:37, Thomas Gleixner wrote: > >> + 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 > */ That looks asymetric. Why is the hotplug side not taking ioapic_mutex? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/9/11 4:08, Thomas Gleixner wrote: > On Wed, 10 Sep 2014, Jiang Liu wrote: >> On 2014/9/9 20:37, Thomas Gleixner wrote: >>>> + 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 >> */ > > That looks asymetric. Why is the hotplug side not taking ioapic_mutex? Hi Thomas, We decide to protect system from IOAPIC hotplug in the ACPI layer. For ACPI enumerated IOAPICs, ioapic_mutex is redundant and it's used to protect MPPARSE, SFI, OF enumerated IOAPICs. Regards! Gerry > > Thanks, > > tglx > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + ret = mp_ioapic_registered(gsi_base); + up_write(&acpi_ioapic_rwsem); + + return ret; +} + static int __init acpi_parse_sbf(struct acpi_table_header *table) { struct acpi_table_boot *sb; diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index fc1e1f9567bc..e104993a2394 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -4010,6 +4010,17 @@ int mp_unregister_ioapic(u32 gsi_base) return 0; } +int mp_ioapic_registered(u32 gsi_base) +{ + int ioapic; + + for_each_ioapic(ioapic) + if (ioapics[ioapic].gsi_config.gsi_base == gsi_base) + return 1; + + return 0; +} + int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq, irq_hw_number_t hwirq) { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index b41c5aef5336..754d99d5f258 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -155,6 +155,7 @@ int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr); int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base); int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base); +int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base); void acpi_irq_stats_init(void); extern u32 acpi_irq_handled; extern u32 acpi_irq_not_handled;
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 <jiang.liu@linux.intel.com> --- 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(+)